Re: [PATCH for-9.0 0/3] qtest/virtio-9p-test.c: fix slow tests

[Christian Schoenebeck]

On Tuesday, March 26, 2024 5:07:16 PM CET Daniel Henrique Barboza wrote:
> 
> On 3/26/24 12:55, Greg Kurz wrote:
> > Bom dia Daniel !
> 
> Bonne après-midi !
> 
> > 
> > On Tue, 26 Mar 2024 10:26:03 -0300
> > Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> > 
> >> Hi,
> >>
> >> Thomas reported in [1] a problem that happened with the RISC-V machine
> >> where some tests from virtio-9p-test.c were failing with '-m slow', i.e.
> >> enabling slow tests.
> >>
> >> In the end it wasn't a RISC-V specific problem. It just so happens that
> >> the recently added riscv machine nodes runs the tests from
> >> virtio-9p-test two times for each qos-test run: one with the
> >> virtio-9p-device device and another with the virtio-9p-pci. The temp dir
> >> for these tests is being created at the start of qos-test and removed
> >> only at the end of qos-test, and the tests are leaving dirs and files
> >> behind. virtio-9-device tests run first, creates stuff in the temp dir,
> >> then when virtio-9p-pci tests runs again it'll fail because the previous
> >> run left created dirs and files in the same temp dir. Here's a run that
> >> exemplifies the problem:
> >>
> >> $ MALLOC_PERTURB_=21 V=2 QTEST_QEMU_BINARY=./qemu-system-riscv64 
> >> ./tests/qtest/qos-test -m slow
> >> (...)
> >> # starting QEMU: exec ./qemu-system-riscv64 -qtest 
> >> unix:/tmp/qtest-621710.sock -qtest-log /dev/null -chardev 
> >> socket,path=/tmp/qtest-621710.qmp,id=char0 -mon chardev=char0,mode=control 
> >> -display none -audio none -M virt,aclint=on,aia=aplic-imsic -fsdev 
> >> local,id=fsdev0,path='/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2',security_model=mapped-xattr
> >>  -device virtio-9p-device,fsdev=fsdev0,mount_tag=qtest -accel qtest
> >> ( goes ok ...)
> >> # starting QEMU: exec ./qemu-system-riscv64 -qtest 
> >> unix:/tmp/qtest-621710.sock -qtest-log /dev/null -chardev 
> >> socket,path=/tmp/qtest-621710.qmp,id=char0 -mon chardev=char0,mode=control 
> >> -display none -audio none -M virt,aclint=on,aia=aplic-imsic -fsdev 
> >> local,id=fsdev0,path='/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2',security_model=mapped-xattr
> >>  -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
> >> ok 168 
> >> /riscv64/virt/generic-pcihost/pci-bus-generic/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/local/config
> >> Received response 7 (RLERROR) instead of 73 (RMKDIR)
> >> Rlerror has errno 17 (File exists)
> >> **
> >> ERROR:../tests/qtest/libqos/virtio-9p-client.c:275:v9fs_req_recv: 
> >> assertion failed (hdr.id == id): (7 == 73)
> >>
> >> As we can see we're running both 'virtio-9p-device' tests and 
> >> 'virtio-9p-pci'
> >> tests using the same '/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2'
> >> temp dir.
> >>
> > 
> > 
> > Good catch ! I'll try to find some time to review.
> > 
> >> The quick fix I came up with was to make each test clean themselves up
> >> after each run. The tests were also consolidated, i.e. fewer tests with the
> >> same coverage, because the 'unlikat' tests were doing the same thing the
> >> 'create' tests were doing but removing stuff after. Might as well keep just
> >> the 'unlikat' tests.
> >>
> > 
> > As long as coverage is preserved, I'm fine with consolidation of the
> > checks. In any case, last call goes to Christian.
> > 
> >> I also went ahead and reverted 558f5c42efd ("tests/9pfs: Mark "local"
> >> tests as "slow"") after realizing that the problem I was fixing is also
> >> the same problem that this patch was trying to working around with the
> >> skip [2]. I validated this change in this Gitlab pipeline:
> >>
> > 
> > Are you sure with that ? Issues look very similar indeed but not
> > exactly the same.
> 
> We can skip this revert if we're not sure about it. Gitlab passed with it but
> perhaps this isn't evidence enough. I'll let you guys decide.

I am a bit surprised because errnos were different (file exists vs. not
supported), but indeed, it did pass in your Gitlab pipeline. So I am fine with
bringing those tests back in on Gitlab.

/Christian