Re: [PATCH RFC v22 1/7] NFSD: add courteous server support for thread with only delegation

[dai.ngo@oracle.com]

On 4/27/22 9:12 AM, J. Bruce Fields wrote:
> On Wed, Apr 27, 2022 at 01:52:47AM -0700, Dai Ngo wrote:
>> +	if (!try_to_expire_client(clp)) {
>> +		nn = net_generic(clp->net, nfsd_net_id);
>> +		mod_delayed_work(laundry_wq, &nn->laundromat_work, 0);
>> +	}
> ...
>> +static inline bool try_to_expire_client(struct nfs4_client *clp)
>> +{
>> +	bool ret;
>> +
>> +	ret = NFSD4_ACTIVE ==
>> +		cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
>> +	return ret;
>> +}
> Hm, this feels a little backwards to me.

yes, I should have renamed the function to something like 'is_client_active()'.

>    I'd expect
> "try_to_expire_client" to return true on success.  Maybe call it
> "client_is_expirable()" to make that clear?  Or stick with
> "try_to_expire_client and make it return 0/-EAGAIN.
>
> If "NFSD4_ACTIVE != cmpxchg(...)" is too confusing, I don't think we
> really even need the atomic return of the previous value, I think it
> would be OK just to do:
>
> 	cmpschg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
> 	return clp->cl_state == NFSD4_EXPIRABLE;

I'll change to this:

static inline bool try_to_expire_client(struct nfs4_client *clp)
{
         bool ret;
	
	cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
	return clp->cl_state == NFSD4_EXPIRABLE;

}

and its caller:

	if (try_to_expire_client(clp)) {
		nn = net_generic(clp->net, nfsd_net_id);
		mod_delayed_work(laundry_wq, &nn->laundromat_work, 0);
	}

-Dai

>
> --b.