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

[dai.ngo@oracle.com]

On 4/1/22 8:57 AM, Chuck Lever III wrote:
>
>> On Apr 1, 2022, at 11:21 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
>>
>> On Thu, Mar 31, 2022 at 09:02:04AM -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_name_tree.
>>>
>>> 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 fe8969ba94b3..eadce5d19473 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -2893,8 +2893,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;
>>> }
>>> @@ -2973,8 +2976,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 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);
>>> +}
>> This is a red flag to me.... What guarantees that the condition we just
>> checked (cl_cs_client_state == NFSD4_CLIENT_RECONNECTED) is still true
>> here?  Why couldn't another thread have raced in and called
>> reactivate_courtesy_client?
>>
>> Should we be holding cl_cs_lock across both the cl_cs_client_state and
>> the assignment?
> Holding cl_cs_lock while checking cl_cs_client_state and then
> updating it sounds OK to me.

Thanks Bruce and Chuck!

I replaced nfsd4_discard_courtesy_clnt with nfsd4_discard_reconnect_clnt
which checks and sets the cl_cs_client_state under the cl_cs_lock:

static inline bool
nfsd4_discard_reconnect_clnt(struct nfs4_client *clp)
{
         bool ret = false;

         spin_lock(&clp->cl_cs_lock);
         if (clp->cl_cs_client_state == NFSD4_CLIENT_RECONNECTED) {
                 clp->cl_cs_client_state = NFSD4_CLIENT_EXPIRED;
                 ret = true;
         }
         spin_unlock(&clp->cl_cs_lock);
         return ret;
}

>
>
>> Or should reactivate_courtesy_client be taking the
>> client_lock?
>>
>> I'm still not clear on the need for the CLIENT_RECONNECTED state.
>>
>> I think analysis would be a bit simpler if the only states were ACTIVE,
>> COURTESY, and EXPIRED, the only transitions possible were
>>
>> 	ACTIVE->COURTESY->{EXPIRED or ACTIVE}
>>
>> and the same lock ensured that those were the only possible transitions.
> Yes, that would be easier, but I don't think it's realistic. There
> are lock ordering issues between nn->client_lock and the locks on
> the individual files and state that make it onerous.
>
>
>> (And to be honest I'd still prefer the original approach where we expire
>> clients from the posix locking code and then retry.  It handles an
>> additional case (the one where reboot happens after a long network
>> partition), and I don't think it requires adding these new client
>> states....)
> The locking of the earlier approach was unworkable.
>
> But, I'm happy to consider that again if you can come up with a way
> of handling it properly and simply.

I will wait for feedback from Bruce before sending v20 with the
above change.

-Dai

>
>
> --
> Chuck Lever
>
>
>