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.
--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
