Re: [RFC 04/11] nfsd: mark client for renew

[Benny Halevy <bhalevy@panasas.com>]

On Dec. 17, 2009, 0:06 +0200, "J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> On Wed, Dec 16, 2009 at 11:56:05PM +0200, Benny Halevy wrote:
>> On Dec. 16, 2009, 22:53 +0200, " J. Bruce Fields" <bfields@citi.umich.edu> wrote:
>>> On Wed, Dec 16, 2009 at 03:48:39PM -0500,  J. Bruce Fields wrote:
>>>> On Wed, Dec 16, 2009 at 07:41:12PM +0200, Benny Halevy wrote:
>>>>> Mark the nfs4_client for renew on operations holding a stateid (or nfs41
>>>>> OP_SEQUENCE).
>>>>> Do the actual lru list update when the compound processing is complete.
>>>>> This will prevents the client from being expired by the laundromat while
>>>>> the compound is in progress.
>>>>>
>>>>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>>>>> ---
>>>>>  fs/nfsd/nfs4proc.c  |    5 +++++
>>>>>  fs/nfsd/nfs4state.c |   39 +++++++++++++++++++++++++++++++++++----
>>>>>  fs/nfsd/state.h     |    1 +
>>>>>  fs/nfsd/xdr4.h      |    1 +
>>>>>  4 files changed, 42 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>> index 37514c4..a8e9731 100644
>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>> @@ -1010,6 +1010,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
>>>>>  	resp->opcnt = 0;
>>>>>  	resp->rqstp = rqstp;
>>>>>  	resp->cstate.minorversion = args->minorversion;
>>>>> +	resp->cstate.renew_client = NULL;
>>>>>  	resp->cstate.replay_owner = NULL;
>>>>>  	fh_init(&resp->cstate.current_fh, NFS4_FHSIZE);
>>>>>  	fh_init(&resp->cstate.save_fh, NFS4_FHSIZE);
>>>>> @@ -1114,6 +1115,10 @@ encode_op:
>>>>>  	fh_put(&resp->cstate.current_fh);
>>>>>  	fh_put(&resp->cstate.save_fh);
>>>>>  	BUG_ON(resp->cstate.replay_owner);
>>>>> +	if (resp->cstate.renew_client) {
>>>>> +		renew_client(resp->cstate.renew_client);
>>>>> +		put_nfs4_client(resp->cstate.renew_client);
>>>>> +	}
>>>>>  out:
>>>>>  	nfsd4_release_compoundargs(args);
>>>>>  	/* Reset deferral mechanism for RPC deferrals */
>>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>>> index c456551..16ccab3 100644
>>>>> --- a/fs/nfsd/nfs4state.c
>>>>> +++ b/fs/nfsd/nfs4state.c
>>>>> @@ -634,8 +634,39 @@ free_session(struct kref *kref)
>>>>>  }
>>>>>  
>>>>>  static inline void
>>>>> +mark_client_for_renew(struct nfsd4_compound_state *cstate,
>>>>> +		      struct nfs4_client *clp)
>>>>> +{
>>>>> +	int old_state = atomic_cmpxchg(&clp->cl_state, CL_STATE_NORMAL,
>>>>> +				       CL_STATE_RENEW);
>>>>> +
>>>>> +	if (old_state != CL_STATE_EXPIRED) {
>>>>> +		cstate->renew_client = clp;
>>>>> +		get_nfs4_client(clp);
>>>> If cstate->renew_client is already set, then we may leak a reference
>>>> count here, right?
>>>>
>>>> Also: at least in the v4 case, I don't think there's anything preventing
>>>> cstate->renew_client being already set to a different client.  (With
>>>> sessions hopefully that's not possible.)
>>>>
>> Yeah.  That's a valid point.  So if it is already set then
>> if it is the same client we can do nothing, else we can renew
>> the previous one here put it and replace cstate->renew_client.
> 
> I'm not sure if that can only happen if someone's intentionally messing
> with us, or if maybe there's a legitimate case where it can happen
> (maybe when a new client is created by some op in the compound?) But,
> yes, that sounds like a safe way to handle it.
> 

Can the client send a compound with multiple I/Os for different
stateids (and even filehandles) that lead back to different clientids?

Benny

> --b.