> -----Original Message----- > From: J. Bruce Fields [mailto:bfields@xxxxxxxxxxxx] > Sent: Thu 2009-06-18 15:37 > To: Halevy, Benny > Cc: pnfs@xxxxxxxxxxxxx; linux-nfs@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 32/44] nfsd41: only reference the session on non-replaysequence > > On Tue, Jun 16, 2009 at 04:20:44AM +0300, Benny Halevy wrote: > > From: Andy Adamson <andros@xxxxxxxxxx> > > > > Solo CREATE_SESSION replays used to share the logic in nfsdsvc_enc_compound_res > > and use nfsd4_store_cache_entry, but not set the cstate session nor take a > > reference on the session. CREATE_SESSION no longer uses nfsd4_store_cache_entry. > > > > The SEQUENCE operation only needs to reference the session to call > > nfsd4_store_cache_entry. > > > > Signed-off-by: Andy Adamson <andros@xxxxxxxxxx> > > Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxx> > > --- > > fs/nfsd/nfs4state.c | 9 ++++----- > > fs/nfsd/nfs4xdr.c | 14 ++++++-------- > > 2 files changed, 10 insertions(+), 13 deletions(-) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index 591879d..c91b333 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -1436,13 +1436,12 @@ nfsd4_sequence(struct svc_rqst *rqstp, > > cstate->slot = slot; > > cstate->session = session; > > > > + /* Hold a session reference until done caching the response */ > > + nfsd4_get_session(session); > > + > > replay_cache: > > - /* Renew the clientid on success and on replay. > > - * Hold a session reference until done processing the compound: > > - * nfsd4_put_session called only if the cstate slot is set. > > - */ > > + /* Renew the clientid on success and on replay */ > > renew_client(session->se_client); > > - nfsd4_get_session(session); > > The result of this seems to be that in the "got replay_cache" case, we > exit this function with cstate->session set, but without a reference > taken. > > That makes me extremely nervous. Any time we're keeping a pointer to a > reference-counted object, and dropping the lock under which the pointer > was acquired, we should be holding a reference. > > If you're sure you won't be needing that session any more, then set > cstate->session to NULL. Or, get a reference on the session just do: if (nfsd4_has_session(cs)) { if (cs->status != nfserr_replay_cache) { nfsd4_store_cache_entry(resp); cs->slot->sl_inuse = 0; } nfsd4_put_session(cs->session); } Benny > > --b. > > > out: > > spin_unlock(&sessionid_lock); > > dprintk("%s: return %d\n", __func__, ntohl(status)); > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > > index cebde87..e1b1397 100644 > > --- a/fs/nfsd/nfs4xdr.c > > +++ b/fs/nfsd/nfs4xdr.c > > @@ -3300,6 +3300,7 @@ nfs4svc_encode_compoundres(struct svc_rqst *rqstp, __be32 *p, struct nfsd4_compo > > /* > > * All that remains is to write the tag and operation count... > > */ > > + struct nfsd4_compound_state *cs = &resp->cstate; > > struct kvec *iov; > > p = resp->tagp; > > *p++ = htonl(resp->taglen); > > @@ -3313,14 +3314,11 @@ nfs4svc_encode_compoundres(struct svc_rqst *rqstp, __be32 *p, struct nfsd4_compo > > iov = &rqstp->rq_res.head[0]; > > iov->iov_len = ((char*)resp->p) - (char*)iov->iov_base; > > BUG_ON(iov->iov_len > PAGE_SIZE); > > - if (nfsd4_has_session(&resp->cstate)) { > > - if (resp->cstate.status != nfserr_replay_cache) { > > - nfsd4_store_cache_entry(resp); > > - dprintk("%s: SET SLOT STATE TO AVAILABLE\n", __func__); > > - resp->cstate.slot->sl_inuse = 0; > > - } > > - if (resp->cstate.session) > > - nfsd4_put_session(resp->cstate.session); > > + if (nfsd4_has_session(cs) && cs->status != nfserr_replay_cache) { > > + nfsd4_store_cache_entry(resp); > > + dprintk("%s: SET SLOT STATE TO AVAILABLE\n", __func__); > > + cs->slot->sl_inuse = 0; > > + nfsd4_put_session(cs->session); > > } > > return 1; > > } > > -- > > 1.6.3 > > > -- 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
