[PATCH] generic/732: fix mount option munging

[PATCH] generic/732: fix mount option munging

[Jeff Layton]

With NFS in particular, we are usually testing with some mount options.
Ensure that we preserve those and just add "nosharecache" onto the end
of the string.

Cc: Yongcheng Yang <yoyang@redhat.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 tests/generic/732 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/generic/732 b/tests/generic/732
index 785aac58f361..ae49152e42dc 100755
--- a/tests/generic/732
+++ b/tests/generic/732
@@ -40,7 +40,7 @@ mkdir -p $testdir1 $testdir2
 # Don't share the data and attribute caches among mount points for NFS.
 # This caching behavior is necessary to reproduce this issue as we're
 # checking the alignment of each mount point's own unique cache.
-[ "$FSTYP" = "nfs" ] && MOUNT_OPTIONS="-o nosharecache"
+[ "$FSTYP" = "nfs" ] && MOUNT_OPTIONS="$MOUNT_OPTIONS -o nosharecache"
 
 SCRATCH_MNT=$testdir1 _scratch_mount
 SCRATCH_MNT=$testdir2 _scratch_mount

---
base-commit: 59299b65ac8f15935ab45e7920cbfda8a6beffd1
change-id: 20231101-master-328ff30ce66f

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] generic/732: fix mount option munging

[Zorro Lang]

On Wed, Nov 01, 2023 at 02:17:13PM -0400, Jeff Layton wrote:
> With NFS in particular, we are usually testing with some mount options.
> Ensure that we preserve those and just add "nosharecache" onto the end
> of the string.
> 
> Cc: Yongcheng Yang <yoyang@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  tests/generic/732 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/generic/732 b/tests/generic/732
> index 785aac58f361..ae49152e42dc 100755
> --- a/tests/generic/732
> +++ b/tests/generic/732
> @@ -40,7 +40,7 @@ mkdir -p $testdir1 $testdir2
>  # Don't share the data and attribute caches among mount points for NFS.
>  # This caching behavior is necessary to reproduce this issue as we're
>  # checking the alignment of each mount point's own unique cache.
> -[ "$FSTYP" = "nfs" ] && MOUNT_OPTIONS="-o nosharecache"
> +[ "$FSTYP" = "nfs" ] && MOUNT_OPTIONS="$MOUNT_OPTIONS -o nosharecache"

Good to me, and looks like the later option replaces the former one, if
there're same options (e.g. -o sharecache -o nosharecache).

Reviewed-by: Zorro Lang <zlang@redhat.com>

>  
>  SCRATCH_MNT=$testdir1 _scratch_mount
>  SCRATCH_MNT=$testdir2 _scratch_mount
> 
> ---
> base-commit: 59299b65ac8f15935ab45e7920cbfda8a6beffd1
> change-id: 20231101-master-328ff30ce66f
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@kernel.org>
> 
> 

Re: [PATCH] generic/732: fix mount option munging

[Jeff Layton]

On Thu, 2023-11-02 at 16:06 +0800, Zorro Lang wrote:
> On Wed, Nov 01, 2023 at 02:17:13PM -0400, Jeff Layton wrote:
> > With NFS in particular, we are usually testing with some mount options.
> > Ensure that we preserve those and just add "nosharecache" onto the end
> > of the string.
> > 
> > Cc: Yongcheng Yang <yoyang@redhat.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  tests/generic/732 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/generic/732 b/tests/generic/732
> > index 785aac58f361..ae49152e42dc 100755
> > --- a/tests/generic/732
> > +++ b/tests/generic/732
> > @@ -40,7 +40,7 @@ mkdir -p $testdir1 $testdir2
> >  # Don't share the data and attribute caches among mount points for NFS.
> >  # This caching behavior is necessary to reproduce this issue as we're
> >  # checking the alignment of each mount point's own unique cache.
> > -[ "$FSTYP" = "nfs" ] && MOUNT_OPTIONS="-o nosharecache"
> > +[ "$FSTYP" = "nfs" ] && MOUNT_OPTIONS="$MOUNT_OPTIONS -o nosharecache"
> 
> Good to me, and looks like the later option replaces the former one, if
> there're same options (e.g. -o sharecache -o nosharecache).
> 
> Reviewed-by: Zorro Lang <zlang@redhat.com>
> 
> 

Thanks, Zorro.

Now that I look more closely, I'm not sure this test will ever pass
properly on NFS. It's basically doing this:

client1: A/f -> B/f
client2: B/f -> A/f
client1: A/f -> B/f

...and the two clients aren't aware of the changes the other is making
(because they were mounted using -o nosharecache). The 3rd rename ends
up thinking that the B/f dentry is still positive, and the rename
syscall fails with EEXIST.

The really confusing bit is that this test passes against servers
running older kernels, because the rename response had the wrong change
info in it and that tricks the client into invalidating the directory
caches when it shouldn't need to do that.

We fixed that in fdd2630a7398 (nfsd: fix change_info in NFSv4 RENAME
replies), and now this test pretty reliably fails when testing against
modern nfsd.

We have some longer term plans to add support for directory delegations
eventually, which may make it easier to keep the caches more coherent,
in this situation, but until then we might want to skip this test.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] generic/732: fix mount option munging

[Zorro Lang]

On Thu, Nov 02, 2023 at 02:59:07PM -0400, Jeff Layton wrote:
> On Thu, 2023-11-02 at 16:06 +0800, Zorro Lang wrote:
> > On Wed, Nov 01, 2023 at 02:17:13PM -0400, Jeff Layton wrote:
> > > With NFS in particular, we are usually testing with some mount options.
> > > Ensure that we preserve those and just add "nosharecache" onto the end
> > > of the string.
> > > 
> > > Cc: Yongcheng Yang <yoyang@redhat.com>
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  tests/generic/732 | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/generic/732 b/tests/generic/732
> > > index 785aac58f361..ae49152e42dc 100755
> > > --- a/tests/generic/732
> > > +++ b/tests/generic/732
> > > @@ -40,7 +40,7 @@ mkdir -p $testdir1 $testdir2
> > >  # Don't share the data and attribute caches among mount points for NFS.
> > >  # This caching behavior is necessary to reproduce this issue as we're
> > >  # checking the alignment of each mount point's own unique cache.
> > > -[ "$FSTYP" = "nfs" ] && MOUNT_OPTIONS="-o nosharecache"
> > > +[ "$FSTYP" = "nfs" ] && MOUNT_OPTIONS="$MOUNT_OPTIONS -o nosharecache"
> > 
> > Good to me, and looks like the later option replaces the former one, if
> > there're same options (e.g. -o sharecache -o nosharecache).
> > 
> > Reviewed-by: Zorro Lang <zlang@redhat.com>
> > 
> > 
> 
> Thanks, Zorro.
> 
> Now that I look more closely, I'm not sure this test will ever pass
> properly on NFS. It's basically doing this:
> 
> client1: A/f -> B/f
> client2: B/f -> A/f
> client1: A/f -> B/f
> 
> ...and the two clients aren't aware of the changes the other is making
> (because they were mounted using -o nosharecache). The 3rd rename ends
> up thinking that the B/f dentry is still positive, and the rename
> syscall fails with EEXIST.
> 
> The really confusing bit is that this test passes against servers
> running older kernels, because the rename response had the wrong change
> info in it and that tricks the client into invalidating the directory
> caches when it shouldn't need to do that.

Hi Jeff,

Thanks for this information.

Do you mean this case tests passed on nfs due to a bug, and that bug
will be fixed soon?

I remember yoyang@ wrote this case for a known nfs issue. And this case
tests passed on most filesystems. If it won't be suitable for newer nfsd,
can we change it for new nfs? Or _notrun it by someone condition?

> 
> We fixed that in fdd2630a7398 (nfsd: fix change_info in NFSv4 RENAME
> replies), and now this test pretty reliably fails when testing against
> modern nfsd.
> 
> We have some longer term plans to add support for directory delegations
> eventually, which may make it easier to keep the caches more coherent,
> in this situation, but until then we might want to skip this test.

Is there any chance (change it a bit) to make this case still works for nfs?

Thanks,
Zorro

> 
> -- 
> Jeff Layton <jlayton@kernel.org>
> 

Re: [PATCH] generic/732: fix mount option munging

[Jeff Layton]

On Mon, 2023-11-06 at 02:43 +0800, Zorro Lang wrote:
> On Thu, Nov 02, 2023 at 02:59:07PM -0400, Jeff Layton wrote:
> > On Thu, 2023-11-02 at 16:06 +0800, Zorro Lang wrote:
> > > On Wed, Nov 01, 2023 at 02:17:13PM -0400, Jeff Layton wrote:
> > > > With NFS in particular, we are usually testing with some mount options.
> > > > Ensure that we preserve those and just add "nosharecache" onto the end
> > > > of the string.
> > > > 
> > > > Cc: Yongcheng Yang <yoyang@redhat.com>
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  tests/generic/732 | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tests/generic/732 b/tests/generic/732
> > > > index 785aac58f361..ae49152e42dc 100755
> > > > --- a/tests/generic/732
> > > > +++ b/tests/generic/732
> > > > @@ -40,7 +40,7 @@ mkdir -p $testdir1 $testdir2
> > > >  # Don't share the data and attribute caches among mount points for NFS.
> > > >  # This caching behavior is necessary to reproduce this issue as we're
> > > >  # checking the alignment of each mount point's own unique cache.
> > > > -[ "$FSTYP" = "nfs" ] && MOUNT_OPTIONS="-o nosharecache"
> > > > +[ "$FSTYP" = "nfs" ] && MOUNT_OPTIONS="$MOUNT_OPTIONS -o nosharecache"
> > > 
> > > Good to me, and looks like the later option replaces the former one, if
> > > there're same options (e.g. -o sharecache -o nosharecache).
> > > 
> > > Reviewed-by: Zorro Lang <zlang@redhat.com>
> > > 
> > > 
> > 
> > Thanks, Zorro.
> > 
> > Now that I look more closely, I'm not sure this test will ever pass
> > properly on NFS. It's basically doing this:
> > 
> > client1: A/f -> B/f
> > client2: B/f -> A/f
> > client1: A/f -> B/f
> > 
> > ...and the two clients aren't aware of the changes the other is making
> > (because they were mounted using -o nosharecache). The 3rd rename ends
> > up thinking that the B/f dentry is still positive, and the rename
> > syscall fails with EEXIST.
> > 
> > The really confusing bit is that this test passes against servers
> > running older kernels, because the rename response had the wrong change
> > info in it and that tricks the client into invalidating the directory
> > caches when it shouldn't need to do that.
> 
> Hi Jeff,
> 
> Thanks for this information.
> 
> Do you mean this case tests passed on nfs due to a bug, and that bug
> will be fixed soon?
> 

It has already been fixed (fdd2630a7398).

This is a bit of a confusing bug. It turns out that in one specific
case, that patch actually helps a test somewhat similar to this to pass,
but in most cases, having that bug fixed will make this test fail on NFS
(and that behavior is expected, if not ideal).

The real problem is that this is trying to test the behavior of the
NFSv4 change attribute, but that value isn't directly observable from
userland today. All you can do is try to infer its effect on the cache,
which is also impossible to observe directly.

> I remember yoyang@ wrote this case for a known nfs issue. And this case
> tests passed on most filesystems. If it won't be suitable for newer nfsd,
> can we change it for new nfs? Or _notrun it by someone condition?
> 
> > 
> > We fixed that in fdd2630a7398 (nfsd: fix change_info in NFSv4 RENAME
> > replies), and now this test pretty reliably fails when testing against
> > modern nfsd.
> > 
> > We have some longer term plans to add support for directory delegations
> > eventually, which may make it easier to keep the caches more coherent,
> > in this situation, but until then we might want to skip this test.
> 
> Is there any chance (change it a bit) to make this case still works for nfs?
> 

Sure: You could wait out the dircache timeout (60s) between rename
calls. See acdirmin/acdirmax mount options too, which could be used to
lower those timeouts to make the test run more quickly. But...if you're
using those mount options, you wouldn't hit the original bug that this
is was trying to test in the first place.

It's probably better to just skip this on NFS. This would probably be a
better test done via pynfs rather than xfstests, since that would allow
you to directly observe that the change attribute changes when you
expect it to.
-- 
Jeff Layton <jlayton@kernel.org>