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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> On Apr 1, 2022, at 11:21 AM, J. Bruce Fields <bfields@xxxxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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.


> 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.


--
Chuck Lever







[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux