On 9/20/2015 13:05, Neil Brown wrote:
> Kinglong Mee <kinglongmee@xxxxxxxxx> 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/
Missing a step,
# touch /mnt/test1 /mnt/test2
>> # ./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@xxxxxxxxx>
>> ---
>> 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.
There are three places calling nfs4_open_recover(), those open_claim_type
are, NFS4_OPEN_CLAIM_PREVIOUS, NFS4_OPEN_CLAIM_DELEG_CUR_FH and
NFS4_OPEN_CLAIM_FH.
Only set delegation_type with NFS4_OPEN_CLAIM_PREVIOUS.
So the checking seems wrong here.
>
> 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 :-(
Yes, it's strange.
But it does not affect the problem I reported.
You can send it as a separate patch.
>
> 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?
My default nfs mount is 4.2,
nfs-server:/ /mnt nfs4 rw,seclabel,relatime,vers=4.2,rsize=262144,wsize=262144,namlen=255,hard,proto=tcp,timeo=600,retrans=2,sec=sys,clientaddr=192.168.8.128,local_lock=none,addr=192.168.8.128 0 0
The nfs server is a linux with latest kernel 4.3.0-rc1.
thanks,
Kinglong Mee
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
