Re: [PATCH 05/29] nfsd41: create_session cache hold client reference

["J. Bruce Fields" <bfields@fieldses.org>]

On Fri, Apr 24, 2009 at 09:52:56AM -0400, Andy Adamson wrote:
>
> On Apr 23, 2009, at 7:28 PM, J. Bruce Fields wrote:
>
>> On Thu, Apr 23, 2009 at 12:42:44PM -0400, andros@netapp.com wrote:
>>> From: Andy Adamson <andros@netapp.com>
>>>
>>> expire_client can be called on a confirmed or unconfirmed client  
>>> while
>>> processing the create session operation and accessing the clientid  
>>> slot.
>>
>> I don't understand--isn't all that under the state lock for now?
>
> Yes it is. I now see that expire_client is also always called under the 
> state lock. Fair enough, this patch can be dropped.

OK, thanks.  Note this wouldn't be quite right even in the absence of a
lock, because there'd be nothing to guarantee that conf is still good at
the time we do the atomic_inc(&conf->cl_count) (what happens if someone
frees conf before we even get there?

The way this kind of thing often works is:

	acquire some lock
		search for an object in some common data structure
		bump the reference count on the found object
	drop the lock

So the lock protects the object from destruction until we get the
reference count, and then the reference count protects it after we've
dropped the lock.

In this case we'll probably eventually transition to a spinlock
protecting the client hash tables, and then do the above in the
functions that look up clients, so that the object returned from those
functions already comes with its own reference taken.

--b.

>
> -->Andy
>
>>
>>
>> --b.
>>
>>>
>>> Signed-off-by: Andy Adamson <andros@netapp.com>
>>> ---
>>> fs/nfsd/nfs4state.c |   14 ++++++++++----
>>> 1 files changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index a585a58..accad58 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -1355,6 +1355,7 @@ nfsd4_create_session(struct svc_rqst *rqstp,
>>> 	conf = find_confirmed_client(&cr_ses->clientid);
>>>
>>> 	if (conf) {
>>> +		atomic_inc(&conf->cl_count);
>>> 		slot = &conf->cl_slot;
>>> 		status = check_slot_seqid(cr_ses->seqid, slot);
>>> 		if (status == nfserr_replay_cache) {
>>> @@ -1363,27 +1364,30 @@ nfsd4_create_session(struct svc_rqst *rqstp,
>>> 			cstate->status = nfserr_replay_clientid_cache;
>>> 			/* Return the cached reply status */
>>> 			status = nfsd4_replay_create_session(resp, slot);
>>> -			goto out;
>>> +			goto out_put;
>>> 		} else if (cr_ses->seqid != conf->cl_slot.sl_seqid + 1) {
>>> 			status = nfserr_seq_misordered;
>>> 			dprintk("Sequence misordered!\n");
>>> 			dprintk("Expected seqid= %d but got seqid= %d\n",
>>> 				slot->sl_seqid, cr_ses->seqid);
>>> -			goto out;
>>> +			goto out_put;
>>> 		}
>>> 		conf->cl_slot.sl_seqid++;
>>> 	} else if (unconf) {
>>> +		atomic_inc(&unconf->cl_count);
>>> 		slot = &unconf->cl_slot;
>>> 		status = check_slot_seqid(cr_ses->seqid, slot);
>>> 		if (status) {
>>> 			/* an unconfirmed replay returns misordered */
>>> 			status = nfserr_seq_misordered;
>>> -			goto out;
>>> +			conf = unconf; /* for put_nfs4_client */
>>> +			goto out_put;
>>> 		}
>>>
>>> 		if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
>>> 		    (ip_addr != unconf->cl_addr)) {
>>> 			status = nfserr_clid_inuse;
>>> +			conf = unconf; /* for put_nfs4_client */
>>> 			goto out_cache;
>>> 		}
>>>
>>> @@ -1413,7 +1417,7 @@ nfsd4_create_session(struct svc_rqst *rqstp,
>>>
>>> 	status = alloc_init_session(rqstp, conf, cr_ses);
>>> 	if (status)
>>> -		goto out;
>>> +		goto out_put;
>>>
>>> 	memcpy(cr_ses->sessionid.data, conf->cl_sessionid.data,
>>> 	       NFS4_MAX_SESSIONID_LEN);
>>> @@ -1423,6 +1427,8 @@ out_cache:
>>> 	/* cache solo and embedded create sessions under the state lock */
>>> 	nfsd4_cache_create_session(cr_ses, slot, status);
>>>
>>> +out_put:
>>> +	put_nfs4_client(conf);
>>> out:
>>> 	nfs4_unlock_state();
>>> 	dprintk("%s returns %d\n", __func__, ntohl(status));
>>> -- 
>>> 1.5.4.3
>>>
>