diff mbox series

[for-9.0,v3,2/2] qtest/virtio-9p-test.c: remove g_test_slow() gate

Message ID 20240327142011.805728-3-dbarboza@ventanamicro.com
State New
Headers show
Series [for-9.0,v3,1/2] qtest/virtio-9p-test.c: create/remove temp dirs after each test | expand

Commit Message

Daniel Henrique Barboza March 27, 2024, 2:20 p.m. UTC
Commit 558f5c42ef gated the local tests with g_test_slow() to skip them
in 'make check'. The reported issue back then was this following CI
problem:

https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05510.html

This problem ended up being fixed after it was detected with the
recently added risc-v machine nodes [1]. virtio-9p-test.c is now
creating and removing temporary dirs for each test run, instead of
creating a single dir for the entire qos-test scope.

We're now able to run these tests with 'make check' in the CI, so let's
go ahead and re-enable them.

This reverts commit 558f5c42efded3e0d0b20a90bce2a9a14580d824.

[1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 tests/qtest/virtio-9p-test.c | 9 ---------
 1 file changed, 9 deletions(-)

Comments

Michael Tokarev April 16, 2024, 7:54 p.m. UTC | #1
27.03.2024 17:20, Daniel Henrique Barboza :
> Commit 558f5c42ef gated the local tests with g_test_slow() to skip them
> in 'make check'. The reported issue back then was this following CI
> problem:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05510.html
> 
> This problem ended up being fixed after it was detected with the
> recently added risc-v machine nodes [1]. virtio-9p-test.c is now
> creating and removing temporary dirs for each test run, instead of
> creating a single dir for the entire qos-test scope.
> 
> We're now able to run these tests with 'make check' in the CI, so let's
> go ahead and re-enable them.
> 
> This reverts commit 558f5c42efded3e0d0b20a90bce2a9a14580d824.
> 
> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html

This makes tests being unable to complete on a tmpfs.  It looks like
9pfs tests needs another tweak here.

# starting QEMU: exec ./qemu-system-x86_64 -qtest unix:/tmp/qtest-798502.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-798502.qmp,id=char0 
-mon chardev=char0,mode=control -display none -audio none -M pc  -fsdev 
local,id=fsdev0,path='/tmp/q/master/qtest-9p-local-9LHRL2',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest 
-accel qtest
Received response 7 (RLERROR) instead of 73 (RMKDIR)
Rlerror has errno 95 (Operation not supported)
**
ERROR:../../../build/qemu/master/tests/qtest/libqos/virtio-9p-client.c:275:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73)

This is when I build it on /tmp/ which is a tmpfs.  When I build
it on a real filesystem, it works fine.

Apparently xattrs aren't supported on a tmpfs.

/mjt
Daniel Henrique Barboza April 16, 2024, 11:16 p.m. UTC | #2
On 4/16/24 16:54, Michael Tokarev wrote:
> 27.03.2024 17:20, Daniel Henrique Barboza :
>> Commit 558f5c42ef gated the local tests with g_test_slow() to skip them
>> in 'make check'. The reported issue back then was this following CI
>> problem:
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05510.html
>>
>> This problem ended up being fixed after it was detected with the
>> recently added risc-v machine nodes [1]. virtio-9p-test.c is now
>> creating and removing temporary dirs for each test run, instead of
>> creating a single dir for the entire qos-test scope.
>>
>> We're now able to run these tests with 'make check' in the CI, so let's
>> go ahead and re-enable them.
>>
>> This reverts commit 558f5c42efded3e0d0b20a90bce2a9a14580d824.
>>
>> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html
> 
> This makes tests being unable to complete on a tmpfs.  It looks like
> 9pfs tests needs another tweak here.
> 
> # starting QEMU: exec ./qemu-system-x86_64 -qtest unix:/tmp/qtest-798502.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-798502.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -M pc  -fsdev local,id=fsdev0,path='/tmp/q/master/qtest-9p-local-9LHRL2',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
> Received response 7 (RLERROR) instead of 73 (RMKDIR)
> Rlerror has errno 95 (Operation not supported)
> **
> ERROR:../../../build/qemu/master/tests/qtest/libqos/virtio-9p-client.c:275:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73)
> 
> This is when I build it on /tmp/ which is a tmpfs.  When I build
> it on a real filesystem, it works fine.
> 
> Apparently xattrs aren't supported on a tmpfs.

Hmmm not sure how to proceed here since I'm not a 9p expert by any means. I'll
let Christian decide what to do.

If we can't figure it out we might need to re-introduce the gate again. Thanks,


Daniel

> 
> /mjt
Christian Schoenebeck April 17, 2024, 11:52 a.m. UTC | #3
On Wednesday, April 17, 2024 1:16:02 AM CEST Daniel Henrique Barboza wrote:
> 
> On 4/16/24 16:54, Michael Tokarev wrote:
> > 27.03.2024 17:20, Daniel Henrique Barboza :
> >> Commit 558f5c42ef gated the local tests with g_test_slow() to skip them
> >> in 'make check'. The reported issue back then was this following CI
> >> problem:
> >>
> >> https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05510.html
> >>
> >> This problem ended up being fixed after it was detected with the
> >> recently added risc-v machine nodes [1]. virtio-9p-test.c is now
> >> creating and removing temporary dirs for each test run, instead of
> >> creating a single dir for the entire qos-test scope.
> >>
> >> We're now able to run these tests with 'make check' in the CI, so let's
> >> go ahead and re-enable them.
> >>
> >> This reverts commit 558f5c42efded3e0d0b20a90bce2a9a14580d824.
> >>
> >> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html
> > 
> > This makes tests being unable to complete on a tmpfs.  It looks like
> > 9pfs tests needs another tweak here.
> > 
> > # starting QEMU: exec ./qemu-system-x86_64 -qtest unix:/tmp/qtest-798502.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-798502.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -M pc  -fsdev local,id=fsdev0,path='/tmp/q/master/qtest-9p-local-9LHRL2',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
> > Received response 7 (RLERROR) instead of 73 (RMKDIR)
> > Rlerror has errno 95 (Operation not supported)
> > **
> > ERROR:../../../build/qemu/master/tests/qtest/libqos/virtio-9p-client.c:275:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73)
> > 
> > This is when I build it on /tmp/ which is a tmpfs.  When I build
> > it on a real filesystem, it works fine.
> > 
> > Apparently xattrs aren't supported on a tmpfs.
> 
> Hmmm not sure how to proceed here since I'm not a 9p expert by any means. I'll
> let Christian decide what to do.
> 
> If we can't figure it out we might need to re-introduce the gate again. Thanks,

It's not that tmpfs exactly doesn't support xattrs. It supports the trusted.*
and security.* namespaces since 2011, so tmpfs was limited to those two. For
the 9p 'local' backend however we also need the user.* namespace which was
just added in Linux 6.6 last year (commit 2daf18a).

Unfortunately the respective kernel option TMPFS_XATTR is still off by default
(linux/fs/Kconfig).

Back then, when we added that 'slow' gate for the 9p 'local' tests, things
were a bit different. They simply did not run in the gitlab pipeline (for
reasons described above). Now they do.

So obviously it would make sense to preserve these tests for the gitlab
pipeline this time, e.g. by skipping these tests only if the underlying test
directory does not support *.user xattrs. I'm just not sure yet where exactly
such kind of *active* check would fit best into the glib test layout, as this
can be a bit tricky with glib.

/Christian
Daniel P. Berrangé April 17, 2024, 12:20 p.m. UTC | #4
On Wed, Apr 17, 2024 at 01:52:24PM +0200, Christian Schoenebeck wrote:
> On Wednesday, April 17, 2024 1:16:02 AM CEST Daniel Henrique Barboza wrote:
> > 
> > On 4/16/24 16:54, Michael Tokarev wrote:
> > > 27.03.2024 17:20, Daniel Henrique Barboza :
> > >> Commit 558f5c42ef gated the local tests with g_test_slow() to skip them
> > >> in 'make check'. The reported issue back then was this following CI
> > >> problem:
> > >>
> > >> https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05510.html
> > >>
> > >> This problem ended up being fixed after it was detected with the
> > >> recently added risc-v machine nodes [1]. virtio-9p-test.c is now
> > >> creating and removing temporary dirs for each test run, instead of
> > >> creating a single dir for the entire qos-test scope.
> > >>
> > >> We're now able to run these tests with 'make check' in the CI, so let's
> > >> go ahead and re-enable them.
> > >>
> > >> This reverts commit 558f5c42efded3e0d0b20a90bce2a9a14580d824.
> > >>
> > >> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html
> > > 
> > > This makes tests being unable to complete on a tmpfs.  It looks like
> > > 9pfs tests needs another tweak here.
> > > 
> > > # starting QEMU: exec ./qemu-system-x86_64 -qtest unix:/tmp/qtest-798502.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-798502.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -M pc  -fsdev local,id=fsdev0,path='/tmp/q/master/qtest-9p-local-9LHRL2',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
> > > Received response 7 (RLERROR) instead of 73 (RMKDIR)
> > > Rlerror has errno 95 (Operation not supported)
> > > **
> > > ERROR:../../../build/qemu/master/tests/qtest/libqos/virtio-9p-client.c:275:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73)
> > > 
> > > This is when I build it on /tmp/ which is a tmpfs.  When I build
> > > it on a real filesystem, it works fine.
> > > 
> > > Apparently xattrs aren't supported on a tmpfs.
> > 
> > Hmmm not sure how to proceed here since I'm not a 9p expert by any means. I'll
> > let Christian decide what to do.
> > 
> > If we can't figure it out we might need to re-introduce the gate again. Thanks,
> 
> It's not that tmpfs exactly doesn't support xattrs. It supports the trusted.*
> and security.* namespaces since 2011, so tmpfs was limited to those two. For
> the 9p 'local' backend however we also need the user.* namespace which was
> just added in Linux 6.6 last year (commit 2daf18a).
> 
> Unfortunately the respective kernel option TMPFS_XATTR is still off by default
> (linux/fs/Kconfig).
> 
> Back then, when we added that 'slow' gate for the 9p 'local' tests, things
> were a bit different. They simply did not run in the gitlab pipeline (for
> reasons described above). Now they do.
> 
> So obviously it would make sense to preserve these tests for the gitlab
> pipeline this time, e.g. by skipping these tests only if the underlying test
> directory does not support *.user xattrs. I'm just not sure yet where exactly
> such kind of *active* check would fit best into the glib test layout, as this
> can be a bit tricky with glib

You should run a method which checks ability to use '.user' xattrs, and
if it reports failure, then skip calling g_test_add. IOW, you can put
the xattr test in the same place as the old g_test_slow() check was.


With regards,
Daniel
diff mbox series

Patch

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 0179b3a394..3c8cd235cf 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -746,15 +746,6 @@  static void register_virtio_9p_test(void)
 
 
     /* 9pfs test cases using the 'local' filesystem driver */
-
-    /*
-     * XXX: Until we are sure that these tests can run everywhere,
-     * keep them as "slow" so that they aren't run with "make check".
-     */
-    if (!g_test_slow()) {
-        return;
-    }
-
     opts.before = assign_9p_local_driver;
     qos_add_test("local/config", "virtio-9p", pci_config,  &opts);
     qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, &opts);