[LTP] [PATCH 1/1] nfs/nfs08.sh: Add test for NFS cache invalidation

[LTP] [PATCH 1/1] nfs/nfs08.sh: Add test for NFS cache invalidation

[Petr Vorel]

v4 [1] of not yet upstreamed patch accidentally broke cache invalidation
for directories by clearing NFS_INO_INVALID_DATA inappropriately.
Although it was fixed in v5 [2] thus kernel was not actually broken,
it's better to prevent this in the future.

[1] https://lore.kernel.org/linux-nfs/167649314509.15170.15885497881041431304@noble.neil.brown.name/
[2] https://lore.kernel.org/linux-nfs/167943762461.8008.3152357340238024342@noble.neil.brown.name/

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
NOTE: although Neil suggested to test on ext2 to make the problems more
obvious (ext2 has low-resolution time stamps) and to use sleep, with
nfs_lib.sh LTP code it works without sleep and on all filesystems.

It'd require more changes in nfs_lib.sh to force ext2 only for nfs08.sh
(other tests are using the library). Not only it's needed, but I also
plan to switch nfs_lib.sh to use all available filesystems via
TST_ALL_FILESYSTEMS=1, therefore ext2 will be among tested filesystems
anyway.

Test fails also on all available NFS versions, thus using the same
approach as the other tests - test all versions, although not sure if
it's really needed. But test runs very quickly (unlike other nfs tests
it's not a stress test), thus I'd keep it.

NOTE: anybody trying to test this patch with TMPDIR (to test different
filesystems) test with this fix [1].

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/20230412073953.1983857-1-pvorel@suse.cz/

 runtest/net.nfs                           | 11 +++++++++++
 testcases/network/nfs/nfs_stress/Makefile |  9 +--------
 testcases/network/nfs/nfs_stress/nfs08.sh | 20 ++++++++++++++++++++
 3 files changed, 32 insertions(+), 8 deletions(-)
 create mode 100755 testcases/network/nfs/nfs_stress/nfs08.sh

diff --git a/runtest/net.nfs b/runtest/net.nfs
index 3249c35cb..72cf4b307 100644
--- a/runtest/net.nfs
+++ b/runtest/net.nfs
@@ -72,6 +72,17 @@ nfs4_ipv6_07 nfs07.sh -6 -v 4 -t tcp
 nfs41_ipv6_07 nfs07.sh -6 -v 4.1 -t tcp
 nfs42_ipv6_07 nfs07.sh -6 -v 4.2 -t tcp
 
+nfs3_08 nfs08.sh -v 3 -t udp
+nfs3t_08 nfs08.sh -v 3 -t tcp
+nfs4_08 nfs08.sh -v 4 -t tcp
+nfs41_08 nfs08.sh -v 4.1 -t tcp
+nfs42_08 nfs08.sh -v 4.2 -t tcp
+nfs3_ipv6_08 nfs08.sh -6 -v 3 -t udp
+nfs3t_ipv6_08 nfs08.sh -6 -v 3 -t tcp
+nfs4_ipv6_08 nfs08.sh -6 -v 4 -t tcp
+nfs41_ipv6_08 nfs08.sh -6 -v 4.1 -t tcp
+nfs42_ipv6_08 nfs08.sh -6 -v 4.2 -t tcp
+
 nfslock3_01 nfslock01.sh -v 3 -t udp
 nfslock3t_01 nfslock01.sh -v 3 -t tcp
 nfslock4_01 nfslock01.sh -v 4 -t tcp
diff --git a/testcases/network/nfs/nfs_stress/Makefile b/testcases/network/nfs/nfs_stress/Makefile
index 8cd095867..5b396dede 100644
--- a/testcases/network/nfs/nfs_stress/Makefile
+++ b/testcases/network/nfs/nfs_stress/Makefile
@@ -9,13 +9,6 @@ include $(top_srcdir)/include/mk/testcases.mk
 nfs04_create_file: CPPFLAGS += -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
 nfs05_make_tree: LDLIBS += -lpthread
 
-INSTALL_TARGETS		:= nfs_lib.sh \
-			   nfs01.sh \
-			   nfs02.sh \
-			   nfs03.sh \
-			   nfs04.sh \
-			   nfs05.sh \
-			   nfs06.sh \
-			   nfs07.sh
+INSTALL_TARGETS	:= *.sh
 
 include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/network/nfs/nfs_stress/nfs08.sh b/testcases/network/nfs/nfs_stress/nfs08.sh
new file mode 100755
index 000000000..8a0f40242
--- /dev/null
+++ b/testcases/network/nfs/nfs_stress/nfs08.sh
@@ -0,0 +1,20 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2023 Petr Vorel <pvorel@suse.cz>
+# Test reproducer for broken NFS cache invalidation for directories.
+# Based on reproducer from Neil Brown <neilb@suse.de>
+
+TST_TESTFUNC="do_test"
+
+do_test()
+{
+	tst_res TINFO "testing NFS cache invalidation for directories"
+
+	touch 1
+	EXPECT_PASS 'ls | grep 1'
+	touch 2
+	EXPECT_PASS 'ls | grep 2'
+}
+
+. nfs_lib.sh
+tst_run
-- 
2.40.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/1] nfs/nfs08.sh: Add test for NFS cache invalidation

[Petr Vorel]

Hi all,

[ Cc linux-nfs ML, sorry for the noise ]

Kind regards,
Petr

> v4 [1] of not yet upstreamed patch accidentally broke cache invalidation
> for directories by clearing NFS_INO_INVALID_DATA inappropriately.
> Although it was fixed in v5 [2] thus kernel was not actually broken,
> it's better to prevent this in the future.

> [1] https://lore.kernel.org/linux-nfs/167649314509.15170.15885497881041431304@noble.neil.brown.name/
> [2] https://lore.kernel.org/linux-nfs/167943762461.8008.3152357340238024342@noble.neil.brown.name/

> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> NOTE: although Neil suggested to test on ext2 to make the problems more
> obvious (ext2 has low-resolution time stamps) and to use sleep, with
> nfs_lib.sh LTP code it works without sleep and on all filesystems.

> It'd require more changes in nfs_lib.sh to force ext2 only for nfs08.sh
> (other tests are using the library). Not only it's needed, but I also
> plan to switch nfs_lib.sh to use all available filesystems via
> TST_ALL_FILESYSTEMS=1, therefore ext2 will be among tested filesystems
> anyway.

> Test fails also on all available NFS versions, thus using the same
> approach as the other tests - test all versions, although not sure if
> it's really needed. But test runs very quickly (unlike other nfs tests
> it's not a stress test), thus I'd keep it.

> NOTE: anybody trying to test this patch with TMPDIR (to test different
> filesystems) test with this fix [1].

> Kind regards,
> Petr

> [1] https://lore.kernel.org/ltp/20230412073953.1983857-1-pvorel@suse.cz/

>  runtest/net.nfs                           | 11 +++++++++++
>  testcases/network/nfs/nfs_stress/Makefile |  9 +--------
>  testcases/network/nfs/nfs_stress/nfs08.sh | 20 ++++++++++++++++++++
>  3 files changed, 32 insertions(+), 8 deletions(-)
>  create mode 100755 testcases/network/nfs/nfs_stress/nfs08.sh

> diff --git a/runtest/net.nfs b/runtest/net.nfs
> index 3249c35cb..72cf4b307 100644
> --- a/runtest/net.nfs
> +++ b/runtest/net.nfs
> @@ -72,6 +72,17 @@ nfs4_ipv6_07 nfs07.sh -6 -v 4 -t tcp
>  nfs41_ipv6_07 nfs07.sh -6 -v 4.1 -t tcp
>  nfs42_ipv6_07 nfs07.sh -6 -v 4.2 -t tcp

> +nfs3_08 nfs08.sh -v 3 -t udp
> +nfs3t_08 nfs08.sh -v 3 -t tcp
> +nfs4_08 nfs08.sh -v 4 -t tcp
> +nfs41_08 nfs08.sh -v 4.1 -t tcp
> +nfs42_08 nfs08.sh -v 4.2 -t tcp
> +nfs3_ipv6_08 nfs08.sh -6 -v 3 -t udp
> +nfs3t_ipv6_08 nfs08.sh -6 -v 3 -t tcp
> +nfs4_ipv6_08 nfs08.sh -6 -v 4 -t tcp
> +nfs41_ipv6_08 nfs08.sh -6 -v 4.1 -t tcp
> +nfs42_ipv6_08 nfs08.sh -6 -v 4.2 -t tcp
> +
>  nfslock3_01 nfslock01.sh -v 3 -t udp
>  nfslock3t_01 nfslock01.sh -v 3 -t tcp
>  nfslock4_01 nfslock01.sh -v 4 -t tcp
> diff --git a/testcases/network/nfs/nfs_stress/Makefile b/testcases/network/nfs/nfs_stress/Makefile
> index 8cd095867..5b396dede 100644
> --- a/testcases/network/nfs/nfs_stress/Makefile
> +++ b/testcases/network/nfs/nfs_stress/Makefile
> @@ -9,13 +9,6 @@ include $(top_srcdir)/include/mk/testcases.mk
>  nfs04_create_file: CPPFLAGS += -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
>  nfs05_make_tree: LDLIBS += -lpthread

> -INSTALL_TARGETS		:= nfs_lib.sh \
> -			   nfs01.sh \
> -			   nfs02.sh \
> -			   nfs03.sh \
> -			   nfs04.sh \
> -			   nfs05.sh \
> -			   nfs06.sh \
> -			   nfs07.sh
> +INSTALL_TARGETS	:= *.sh

>  include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/network/nfs/nfs_stress/nfs08.sh b/testcases/network/nfs/nfs_stress/nfs08.sh
> new file mode 100755
> index 000000000..8a0f40242
> --- /dev/null
> +++ b/testcases/network/nfs/nfs_stress/nfs08.sh
> @@ -0,0 +1,20 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (c) 2023 Petr Vorel <pvorel@suse.cz>
> +# Test reproducer for broken NFS cache invalidation for directories.
> +# Based on reproducer from Neil Brown <neilb@suse.de>
> +
> +TST_TESTFUNC="do_test"
> +
> +do_test()
> +{
> +	tst_res TINFO "testing NFS cache invalidation for directories"
> +
> +	touch 1
> +	EXPECT_PASS 'ls | grep 1'
> +	touch 2
> +	EXPECT_PASS 'ls | grep 2'
> +}
> +
> +. nfs_lib.sh
> +tst_run

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/1] nfs/nfs08.sh: Add test for NFS cache invalidation

[Cyril Hrubis]

Hi!
> v4 [1] of not yet upstreamed patch accidentally broke cache invalidation
> for directories by clearing NFS_INO_INVALID_DATA inappropriately.
> Although it was fixed in v5 [2] thus kernel was not actually broken,
> it's better to prevent this in the future.
> 
> [1] https://lore.kernel.org/linux-nfs/167649314509.15170.15885497881041431304@noble.neil.brown.name/
> [2] https://lore.kernel.org/linux-nfs/167943762461.8008.3152357340238024342@noble.neil.brown.name/
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> NOTE: although Neil suggested to test on ext2 to make the problems more
> obvious (ext2 has low-resolution time stamps) and to use sleep, with
> nfs_lib.sh LTP code it works without sleep and on all filesystems.
> 
> It'd require more changes in nfs_lib.sh to force ext2 only for nfs08.sh
> (other tests are using the library). Not only it's needed, but I also
> plan to switch nfs_lib.sh to use all available filesystems via
> TST_ALL_FILESYSTEMS=1, therefore ext2 will be among tested filesystems
> anyway.
> 
> Test fails also on all available NFS versions, thus using the same
> approach as the other tests - test all versions, although not sure if
> it's really needed. But test runs very quickly (unlike other nfs tests
> it's not a stress test), thus I'd keep it.
> 
> NOTE: anybody trying to test this patch with TMPDIR (to test different
> filesystems) test with this fix [1].
> 
> Kind regards,
> Petr
> 
> [1] https://lore.kernel.org/ltp/20230412073953.1983857-1-pvorel@suse.cz/
> 
>  runtest/net.nfs                           | 11 +++++++++++
>  testcases/network/nfs/nfs_stress/Makefile |  9 +--------
>  testcases/network/nfs/nfs_stress/nfs08.sh | 20 ++++++++++++++++++++
>  3 files changed, 32 insertions(+), 8 deletions(-)
>  create mode 100755 testcases/network/nfs/nfs_stress/nfs08.sh
> 
> diff --git a/runtest/net.nfs b/runtest/net.nfs
> index 3249c35cb..72cf4b307 100644
> --- a/runtest/net.nfs
> +++ b/runtest/net.nfs
> @@ -72,6 +72,17 @@ nfs4_ipv6_07 nfs07.sh -6 -v 4 -t tcp
>  nfs41_ipv6_07 nfs07.sh -6 -v 4.1 -t tcp
>  nfs42_ipv6_07 nfs07.sh -6 -v 4.2 -t tcp
>  
> +nfs3_08 nfs08.sh -v 3 -t udp
> +nfs3t_08 nfs08.sh -v 3 -t tcp
> +nfs4_08 nfs08.sh -v 4 -t tcp
> +nfs41_08 nfs08.sh -v 4.1 -t tcp
> +nfs42_08 nfs08.sh -v 4.2 -t tcp
> +nfs3_ipv6_08 nfs08.sh -6 -v 3 -t udp
> +nfs3t_ipv6_08 nfs08.sh -6 -v 3 -t tcp
> +nfs4_ipv6_08 nfs08.sh -6 -v 4 -t tcp
> +nfs41_ipv6_08 nfs08.sh -6 -v 4.1 -t tcp
> +nfs42_ipv6_08 nfs08.sh -6 -v 4.2 -t tcp
> +
>  nfslock3_01 nfslock01.sh -v 3 -t udp
>  nfslock3t_01 nfslock01.sh -v 3 -t tcp
>  nfslock4_01 nfslock01.sh -v 4 -t tcp
> diff --git a/testcases/network/nfs/nfs_stress/Makefile b/testcases/network/nfs/nfs_stress/Makefile
> index 8cd095867..5b396dede 100644
> --- a/testcases/network/nfs/nfs_stress/Makefile
> +++ b/testcases/network/nfs/nfs_stress/Makefile
> @@ -9,13 +9,6 @@ include $(top_srcdir)/include/mk/testcases.mk
>  nfs04_create_file: CPPFLAGS += -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
>  nfs05_make_tree: LDLIBS += -lpthread
>  
> -INSTALL_TARGETS		:= nfs_lib.sh \
> -			   nfs01.sh \
> -			   nfs02.sh \
> -			   nfs03.sh \
> -			   nfs04.sh \
> -			   nfs05.sh \
> -			   nfs06.sh \
> -			   nfs07.sh
> +INSTALL_TARGETS	:= *.sh
>  
>  include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/network/nfs/nfs_stress/nfs08.sh b/testcases/network/nfs/nfs_stress/nfs08.sh
> new file mode 100755
> index 000000000..8a0f40242
> --- /dev/null
> +++ b/testcases/network/nfs/nfs_stress/nfs08.sh
> @@ -0,0 +1,20 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (c) 2023 Petr Vorel <pvorel@suse.cz>
> +# Test reproducer for broken NFS cache invalidation for directories.
> +# Based on reproducer from Neil Brown <neilb@suse.de>
> +
> +TST_TESTFUNC="do_test"
> +
> +do_test()
> +{
> +	tst_res TINFO "testing NFS cache invalidation for directories"
> +
> +	touch 1
> +	EXPECT_PASS 'ls | grep 1'
> +
> +	touch 2
> +	EXPECT_PASS 'ls | grep 2'
> +}

I do not get how this actually detects case invalidation, it probably
does, but slightly better description how this actually excercises the
case would help.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/1] nfs/nfs08.sh: Add test for NFS cache invalidation

[Petr Vorel]

> Hi!
> > v4 [1] of not yet upstreamed patch accidentally broke cache invalidation
> > for directories by clearing NFS_INO_INVALID_DATA inappropriately.
> > Although it was fixed in v5 [2] thus kernel was not actually broken,
> > it's better to prevent this in the future.

> > [1] https://lore.kernel.org/linux-nfs/167649314509.15170.15885497881041431304@noble.neil.brown.name/
> > [2] https://lore.kernel.org/linux-nfs/167943762461.8008.3152357340238024342@noble.neil.brown.name/

...
> > --- /dev/null
> > +++ b/testcases/network/nfs/nfs_stress/nfs08.sh
> > @@ -0,0 +1,20 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +# Copyright (c) 2023 Petr Vorel <pvorel@suse.cz>
> > +# Test reproducer for broken NFS cache invalidation for directories.
> > +# Based on reproducer from Neil Brown <neilb@suse.de>
> > +
> > +TST_TESTFUNC="do_test"
> > +
> > +do_test()
> > +{
> > +	tst_res TINFO "testing NFS cache invalidation for directories"
> > +
> > +	touch 1
> > +	EXPECT_PASS 'ls | grep 1'
> > +
> > +	touch 2
> > +	EXPECT_PASS 'ls | grep 2'
> > +}

> I do not get how this actually detects case invalidation, it probably
> does, but slightly better description how this actually excercises the
> case would help.

The behavior is:

"touch 1" asks for data invalidation (new file created), therefore following ls
(EXPECT_PASS 'ls | grep 1') fills the cache.  "touch 2" should again ask for
data invalidation, but it the unfixed v4 version of the patch it did not
resulted to cache invalidation.  Therefore second ls (EXPECT_PASS 'ls | grep 2')
shows just 1, but not 2. i.e. in the affected kernel only second ls failed,
but obviously both should be checked (nobody knows how another bug on cache
invalidation will behave).

I can add the description above to the commit message and adjust the comment in
the file.

Kind regards,
Petr

diff --git testcases/network/nfs/nfs_stress/nfs08.sh testcases/network/nfs/nfs_stress/nfs08.sh
index 8a0f40242..691e221d4 100755
--- testcases/network/nfs/nfs_stress/nfs08.sh
+++ testcases/network/nfs/nfs_stress/nfs08.sh
@@ -2,6 +2,8 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 # Copyright (c) 2023 Petr Vorel <pvorel@suse.cz>
 # Test reproducer for broken NFS cache invalidation for directories.
+# Kernel patch caused broken cache invalidation, which caused the second 'ls'
+# did not show '2'.
 # Based on reproducer from Neil Brown <neilb@suse.de>
 
 TST_TESTFUNC="do_test"

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/1] nfs/nfs08.sh: Add test for NFS cache invalidation

[Petr Vorel]

Hi all,

> > Hi!
> > > v4 [1] of not yet upstreamed patch accidentally broke cache invalidation
> > > for directories by clearing NFS_INO_INVALID_DATA inappropriately.
> > > Although it was fixed in v5 [2] thus kernel was not actually broken,
> > > it's better to prevent this in the future.

> > > [1] https://lore.kernel.org/linux-nfs/167649314509.15170.15885497881041431304@noble.neil.brown.name/
> > > [2] https://lore.kernel.org/linux-nfs/167943762461.8008.3152357340238024342@noble.neil.brown.name/

> ...
> > > --- /dev/null
> > > +++ b/testcases/network/nfs/nfs_stress/nfs08.sh
> > > @@ -0,0 +1,20 @@
> > > +#!/bin/sh
> > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > +# Copyright (c) 2023 Petr Vorel <pvorel@suse.cz>
> > > +# Test reproducer for broken NFS cache invalidation for directories.
> > > +# Based on reproducer from Neil Brown <neilb@suse.de>
> > > +
> > > +TST_TESTFUNC="do_test"
> > > +
> > > +do_test()
> > > +{
> > > +	tst_res TINFO "testing NFS cache invalidation for directories"
> > > +
> > > +	touch 1
> > > +	EXPECT_PASS 'ls | grep 1'
> > > +
> > > +	touch 2
> > > +	EXPECT_PASS 'ls | grep 2'
> > > +}

> > I do not get how this actually detects case invalidation, it probably
> > does, but slightly better description how this actually excercises the
> > case would help.

> The behavior is:

> "touch 1" asks for data invalidation (new file created), therefore following ls
> (EXPECT_PASS 'ls | grep 1') fills the cache.  "touch 2" should again ask for
> data invalidation, but it the unfixed v4 version of the patch it did not
> resulted to cache invalidation.  Therefore second ls (EXPECT_PASS 'ls | grep 2')
> shows just 1, but not 2. i.e. in the affected kernel only second ls failed,
> but obviously both should be checked (nobody knows how another bug on cache
> invalidation will behave).

> I can add the description above to the commit message and adjust the comment in
> the file.

Merged with the updated description.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp