Re: [PATCH RFC v17 06/11] NFSD: Update find_clp_in_name_tree() to handle courtesy client

[dai.ngo@oracle.com]

On 3/22/22 2:50 PM, J. Bruce Fields wrote:
> On Thu, Mar 17, 2022 at 12:43:43AM -0700, Dai Ngo wrote:
>> Update find_clp_in_name_tree to check and expire courtesy client.
>>
>> Update find_confirmed_client_by_name to discard the courtesy
>> client by setting CLIENT_EXPIRED.
>>
>> Update nfsd4_setclientid to expire client with CLIENT_EXPIRED
>> state to prevent multiple confirmed clients with the same name
>> on the conf_id_hashtbl list.

This should be nn->conf_name_tree instead of conf_id_hashtbl list.

> I could use a little more "why" here.
>
> I'll give it another read, but right now I'm just not understanding how
> this is meant to work.

Here is the problem description when running pynfs/nfs4.0 CID1 (need to
run CID5 first to create ton of clients) which requires the change in
nfsd4_setclientid to get the test to pass:

1st setclientid(name=clientA,verf=X)
   . find confirmed courtesy client with same name clientA (clientid:1) and
     mark it DESTROY and leave it on the confirmed list.
   . assign new clientA with clientid:2
   . put clientid:2 on unconfirmed list
   . return clientid:2

1st setclientid_confirm(clientid:2)
   . does not find any confirmed for clientid:2
   . find unconfirmed clientid:2
   . does not find any confirmed clientA (find_confirmed_client_by_name)
     since courtesy clientA was marked DESTROY
   . put clientid:2: to conf_name_tree list
   
     PROBLEM: new clientA (clientid:2) and courtesy clientA (clientid:1)
     are both now on the conf_name_tree list.

clientid:2 does OPEN -> stidX
   
2nd setclientid(name=clientA, verf=Y)
   . does not find any confirmed clientA: skip courtesy clientA since it
     was marked DESTROY and does not find confirmed  client:2 due to the
     PROBLEM noted above; duplicate entry in binary tree.
   . create new clientid:3 and put it on unconfirmed list
   . return clientid:3

2nd setclientid_confirm(clientid:3)
   . does not find any confirmed clientid:3
   . find unconfirmed clientid:3
   . does not find any confirmed clientA: skip courtesy clientA since it
     was marked DESTROY and does not find confirmed client:2 due to the
     PROBLEM noted above; duplicate entry in binary tree.
   . put clientid:3 on conf_name_tree list

clientid:3 does CLOSE(stidX) -> NFS4_OK since clientid:2 still valid  <ERROR!>


-Dai

>
> --b.
>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>   fs/nfsd/nfs4state.c | 27 ++++++++++++++++++++++++---
>>   fs/nfsd/state.h     | 22 ++++++++++++++++++++++
>>   2 files changed, 46 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index c6b5e05c9c34..dc0e60bf694c 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -2834,8 +2834,11 @@ find_clp_in_name_tree(struct xdr_netobj *name, struct rb_root *root)
>>   			node = node->rb_left;
>>   		else if (cmp < 0)
>>   			node = node->rb_right;
>> -		else
>> +		else {
>> +			if (nfsd4_courtesy_clnt_expired(clp))
>> +				return NULL;
>>   			return clp;
>> +		}
>>   	}
>>   	return NULL;
>>   }
>> @@ -2914,8 +2917,15 @@ static bool clp_used_exchangeid(struct nfs4_client *clp)
>>   static struct nfs4_client *
>>   find_confirmed_client_by_name(struct xdr_netobj *name, struct nfsd_net *nn)
>>   {
>> +	struct nfs4_client *clp;
>> +
>>   	lockdep_assert_held(&nn->client_lock);
>> -	return find_clp_in_name_tree(name, &nn->conf_name_tree);
>> +	clp = find_clp_in_name_tree(name, &nn->conf_name_tree);
>> +	if (clp && clp->cl_cs_client_state == NFSD4_CLIENT_RECONNECTED) {
>> +		nfsd4_discard_courtesy_clnt(clp);
>> +		clp = NULL;
>> +	}
>> +	return clp;
>>   }
>>   
>>   static struct nfs4_client *
>> @@ -4032,12 +4042,19 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>   	struct nfs4_client	*unconf = NULL;
>>   	__be32 			status;
>>   	struct nfsd_net		*nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>> +	struct nfs4_client	*cclient = NULL;
>>   
>>   	new = create_client(clname, rqstp, &clverifier);
>>   	if (new == NULL)
>>   		return nfserr_jukebox;
>>   	spin_lock(&nn->client_lock);
>> -	conf = find_confirmed_client_by_name(&clname, nn);
>> +	/* find confirmed client by name */
>> +	conf = find_clp_in_name_tree(&clname, &nn->conf_name_tree);
>> +	if (conf && conf->cl_cs_client_state == NFSD4_CLIENT_RECONNECTED) {
>> +		cclient = conf;
>> +		conf = NULL;
>> +	}
>> +
>>   	if (conf && client_has_state(conf)) {
>>   		status = nfserr_clid_inuse;
>>   		if (clp_used_exchangeid(conf))
>> @@ -4068,7 +4085,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>   	new = NULL;
>>   	status = nfs_ok;
>>   out:
>> +	if (cclient)
>> +		unhash_client_locked(cclient);
>>   	spin_unlock(&nn->client_lock);
>> +	if (cclient)
>> +		expire_client(cclient);
>>   	if (new)
>>   		free_client(new);
>>   	if (unconf) {
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index d156ae3ab46c..14b2c158ccca 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -735,6 +735,7 @@ extern void nfsd4_client_record_remove(struct nfs4_client *clp);
>>   extern int nfsd4_client_record_check(struct nfs4_client *clp);
>>   extern void nfsd4_record_grace_done(struct nfsd_net *nn);
>>   
>> +/* courteous server */
>>   static inline bool
>>   nfsd4_expire_courtesy_clnt(struct nfs4_client *clp)
>>   {
>> @@ -749,4 +750,25 @@ nfsd4_expire_courtesy_clnt(struct nfs4_client *clp)
>>   	return rc;
>>   }
>>   
>> +static inline void
>> +nfsd4_discard_courtesy_clnt(struct nfs4_client *clp)
>> +{
>> +	spin_lock(&clp->cl_cs_lock);
>> +	clp->cl_cs_client_state = NFSD4_CLIENT_EXPIRED;
>> +	spin_unlock(&clp->cl_cs_lock);
>> +}
>> +
>> +static inline bool
>> +nfsd4_courtesy_clnt_expired(struct nfs4_client *clp)
>> +{
>> +	bool rc = false;
>> +
>> +	spin_lock(&clp->cl_cs_lock);
>> +	if (clp->cl_cs_client_state == NFSD4_CLIENT_EXPIRED)
>> +		rc = true;
>> +	if (clp->cl_cs_client_state == NFSD4_CLIENT_COURTESY)
>> +		clp->cl_cs_client_state = NFSD4_CLIENT_RECONNECTED;
>> +	spin_unlock(&clp->cl_cs_lock);
>> +	return rc;
>> +}
>>   #endif   /* NFSD4_STATE_H */
>> -- 
>> 2.9.5