From: "J. Bruce Fields" Subject: Re: [PATCH 05/29] nfsd41: create_session cache hold client reference Date: Fri, 24 Apr 2009 10:02:45 -0400 Message-ID: <20090424140245.GB15038@fieldses.org> References: <1240504988-9572-1-git-send-email-andros@netapp.com> <1240504988-9572-2-git-send-email-andros@netapp.com> <1240504988-9572-3-git-send-email-andros@netapp.com> <1240504988-9572-4-git-send-email-andros@netapp.com> <1240504988-9572-5-git-send-email-andros@netapp.com> <1240504988-9572-6-git-send-email-andros@netapp.com> <20090423232825.GK1906@fieldses.org> <1D363F53-7E7A-472C-8176-BB8F33A94BDB@netapp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-nfs@vger.kernel.org, pnfs@linux-nfs.org To: Andy Adamson Return-path: Received: from mail.fieldses.org ([141.211.133.115]:39342 "EHLO pickle.fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759398AbZDXOCq (ORCPT ); Fri, 24 Apr 2009 10:02:46 -0400 In-Reply-To: <1D363F53-7E7A-472C-8176-BB8F33A94BDB@netapp.com> Sender: linux-nfs-owner@vger.kernel.org List-ID: 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 >>> >>> 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 >>> --- >>> 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 >>> >