Re: [PATCH] nfs: Fix open state losing after delegation return

[Neil Brown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3572 bytes --]

Kinglong Mee <kinglongmee@gmail.com> writes:

> After delegation return caused by setting file mode, 
> client lost the open state,  DESTROY_CLIENTID will 
> get error NFS4ERR_CLIENTID_BUSY from server.
>
> The delegation_type passed to nfs4_open_recover_helper
> with NFS4_OPEN_CLAIM_DELEG_CUR_FH is never set.
>
> Reproduce steps,
> # mount -t nfs nfs-server:/ /mnt/
> # ./delegation_test /mnt/
> # umount /mnt    <--- costs 10 seconds
>
> The delegation_test.c is,
> #include <stdio.h>
> #include <stdlib.h>
> #include <sys/types.h>
> #include <unistd.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <error.h>
>
> int main(int argc, char **argv)
> {
>         int fd1, fd2;
>         char fname1[1024], fname2[1024];
>         struct stat sb;
>
>         if (argc < 2)
>                 return -1;
>
>         sprintf(fname1, "%s/test1", argv[1]);
>         sprintf(fname2, "%s/test2", argv[1]);
>
>         fd1 = open(fname1, O_RDONLY);
>         fd2 = open(fname2, O_RDONLY);
>
>         fstat(fd1, &sb);
>         fchmod(fd1, sb.st_mode + 1);
>
>         close(fd1);
>         close(fd2);
>
>         return 0;
> }
>
> Fixes: 39f897fdbd ("NFSv4: When returning a delegation, don't reclaim an incompatible open mode.")
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfs/nfs4proc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 693b903..472a52f 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1576,8 +1576,7 @@ static int nfs4_open_recover_helper(struct nfs4_opendata *opendata, fmode_t fmod
>  	struct nfs4_state *newstate;
>  	int ret;
>  
> -	if ((opendata->o_arg.claim == NFS4_OPEN_CLAIM_DELEGATE_CUR ||
> -	     opendata->o_arg.claim == NFS4_OPEN_CLAIM_DELEG_CUR_FH) &&
> +	if (opendata->o_arg.claim == NFS4_OPEN_CLAIM_DELEGATE_CUR &&
>  	    (opendata->o_arg.u.delegation_type & fmode) != fmode)
>  		/* This mode can't have been delegated, so we must have
>  		 * a valid open_stateid to cover it - not need to reclaim.
> -- 
> 2.5.0

This patches doesn't look right to me.  It isn't at all clear why the
change given addresses the symptoms described.

However looking back at my patch (39f897fdbd) it looks really wrong and
I cannot imagine how I missed the problem when I submitted it.

nfs4_open_recover assumes that if nfs4_open_recover_helper returns zero,
the 'newstate' has been given a value and if that doesn't match 'state'
it returns -ESTALE.

However with my patch, nfs4_open_recovery_helper can return zero and
leave 'newstate' uniitialised.  I cannot even see how that passed
testing :-(

You could maybe fix with

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 693b903b48bd..6899197ff1c3 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1604,7 +1604,7 @@ static int nfs4_open_recover_helper(struct nfs4_opendata *opendata, fmode_t fmod
 
 static int nfs4_open_recover(struct nfs4_opendata *opendata, struct nfs4_state *state)
 {
-	struct nfs4_state *newstate;
+	struct nfs4_state *newstate = state;
 	int ret;
 
 	/* Don't trigger recovery in nfs_test_and_clear_all_open_stateid */

but I'm not at all sure I like that.  I'll try to find time to look at
the code properly and make a more concrete proposal - and to test with
your test case too.  However you didn't give details on the mount: I
assume it was a 4.1 mount?  Was it against the Linux nfs server or
something else?

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]