On Fri, Apr 24, 2009 at 10:02 AM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> 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@xxxxxxxxxx wrote:
>>>> From: Andy Adamson <andros@xxxxxxxxxx>
>>>>
>>>> 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.
Got it - thanks
-->Andy
>
> --b.
>
>>
>> -->Andy
>>
>>>
>>>
>>> --b.
>>>
>>>>
>>>> Signed-off-by: Andy Adamson <andros@xxxxxxxxxx>
>>>> ---
>>>> 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
>>>>
>>
> _______________________________________________
> pNFS mailing list
> pNFS@xxxxxxxxxxxxx
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
