Re: [PATCH 2/8] NFSv4: don't let hanging mounts block other mounts

[NeilBrown <neilb@suse.com>]

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

On Fri, Aug 18 2017, Trond Myklebust wrote:

> On Fri, 2017-08-18 at 17:12 +1000, NeilBrown wrote:
>> If you try an NFSv4 mount from an inaccessible server, it will hang
>> as
>> you would expect.
>> If you then try an NFSv4 mount from a different accessible server,
>> it will also hang.  This is not expected.
>> 
>> The second mount is blocked in
>>   nfs4_init_client()
>>   -> nfs4_discover_server_trunking()
>>   -> nfs40_discover_server_trunking()
>>   -> nfs40_walk_client_list()
>>   -> nfs4_match_client()
>>   -> nfs_wait_client_init_complete()
>> It is waiting for the first mount to complete so that it can then
>> see if the two servers are really one and the same.
>> 
>> It is not necessary to wait here when an nfs_client cl_cons_state is
>> NFS_CS_INITING.  Such a client will, after changing cl_cons_state,
>> call
>> nfs4_discover_server_trunking() itself.  So if the current client
>> just
>> skips those clients, trunking will still be discovered if necessary.
>> 
>> I am unsure of situation with NFS_CS_SESSION_INITING, but I suspect
>> that the comment "Wait for CREATE_SESSION to finish" implies that
>> it is only clients in NFS_CS_SESSION_INITING that need to be waited
>> for.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>  fs/nfs/nfs4client.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
>> index e9bea90dc017..d8b9b7ff19a9 100644
>> --- a/fs/nfs/nfs4client.c
>> +++ b/fs/nfs/nfs4client.c
>> @@ -482,7 +482,7 @@ static int nfs4_match_client(struct
>> nfs_client  *pos,  struct nfs_client *new,
>>  	 * remaining fields in "pos", especially the client
>>  	 * ID and serverowner fields.  Wait for CREATE_SESSION
>>  	 * to finish. */
>> -	if (pos->cl_cons_state > NFS_CS_READY) {
>> +	if (pos->cl_cons_state == NFS_CS_SESSION_INITING) {
>>  		atomic_inc(&pos->cl_count);
>>  		spin_unlock(&nn->nfs_client_lock);
>
> This could cause us to declare a false positive match with a client
> that is uninitialised.

Thanks for the review.

A positive match is reported by returning zero.
If cl_cons_state is not NFS_CS_READY, nfs4_match_client() will not
return zero.

So I don't see how a false positive is possible.

A false negative might be possible with an uninitialized client, but
once that client is initialised, it will call walk_client_list and find
the match.  Won't it?

Thanks,
NeilBrown

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