On Thu, Aug 27, 2009 at 05:16:39PM -0400, bfields wrote:
> On Thu, Aug 27, 2009 at 07:37:02PM +0300, Benny Halevy wrote:
> > On Aug. 27, 2009, 0:02 +0300, "J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:
> > > On Wed, Aug 26, 2009 at 11:20:48AM +0300, Boaz Harrosh wrote:
> > >> On 08/26/2009 12:59 AM, J. Bruce Fields wrote:
> > >>> commit fdf7875040506ca7e595dffef56cbd81ae6b384b
> > >>> Author: Benny Halevy <bhalevy@xxxxxxxxxxx>
> > >>> Date: Thu Aug 20 03:21:56 2009 +0300
> > >>>
> > >>> nfsd41: renew_client must be called under the state lock
> > >>>
> > >>> Until we work out the state locking so we can use a spin lock to protect
> > >>> the cl_lru, we need to take the state_lock to renew the client.
> > >>>
> > >>> Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxx>
> > >>> [nfsd41: Do not renew state on error]
> > >>> Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@xxxxxxxxxx>
> > >>> Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxx>
> > >>> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxxxxxx>
> > >>>
> > >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > >>> index d2a0524..8e5ac49 100644
> > >>> --- a/fs/nfsd/nfs4state.c
> > >>> +++ b/fs/nfsd/nfs4state.c
> > >>> @@ -1463,12 +1463,16 @@ nfsd4_sequence(struct svc_rqst *rqstp,
> > >>> spin_lock(&sessionid_lock);
> > >>> status = nfserr_badsession;
> > >>> session = find_in_sessionid_hashtbl(&seq->sessionid);
> > >>> - if (!session)
> > >>> - goto out;
> > >>> + if (!session) {
> > >>> + spin_unlock(&sessionid_lock);
> > >>> + goto err;
> > >>> + }
> > >>>
> > >>> status = nfserr_badslot;
> > >>> - if (seq->slotid >= session->se_fchannel.maxreqs)
> > >>> - goto out;
> > >>> + if (seq->slotid >= session->se_fchannel.maxreqs) {
> > >>> + spin_unlock(&sessionid_lock);
> > >>> + goto err;
> > >>> + }
> > >>>
> > >> A spin_unlock is a big and delicate inlined code site
> > >> Why not do a "goto err_unlock" or something
> > >
> > > Yes, we could add an
> > >
> > > err_unlock:
> > > spin_unlock(&sessionid_lock);
> > > goto err;
> > >
> > > at the end. It still seems little delicate.
> > >
> > > How about the following (on top of Benny's patch), which sends all the
> > > unlock cases to one label? (Disclaimer: untested. And this depends on
> > > the assumption that cstate->session is NULL on entry to this function,
> > > which I haven't checked.)
> > >
> > > --b.
> > >
> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > index 8e5ac49..5f634d2 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -1463,16 +1463,12 @@ nfsd4_sequence(struct svc_rqst *rqstp,
> > > spin_lock(&sessionid_lock);
> > > status = nfserr_badsession;
> > > session = find_in_sessionid_hashtbl(&seq->sessionid);
> > > - if (!session) {
> > > - spin_unlock(&sessionid_lock);
> > > - goto err;
> > > - }
> > > + if (!session)
> > > + goto out;
> > >
> > > status = nfserr_badslot;
> > > - if (seq->slotid >= session->se_fchannel.maxreqs) {
> > > - spin_unlock(&sessionid_lock);
> > > - goto err;
> > > - }
> > > + if (seq->slotid >= session->se_fchannel.maxreqs)
> > > + goto out;
> > >
> > > slot = &session->se_slots[seq->slotid];
> > > dprintk("%s: slotid %d\n", __func__, seq->slotid);
> > > @@ -1487,10 +1483,8 @@ nfsd4_sequence(struct svc_rqst *rqstp,
> > > cstate->status = nfserr_replay_cache;
> > > goto out;
> > > }
> > > - if (status) {
> > > - spin_unlock(&sessionid_lock);
> > > - goto err;
> > > - }
> > > + if (status)
> > > + goto out;
> > >
> > > /* Success! bump slot seqid */
> > > slot->sl_inuse = true;
> > > @@ -1510,10 +1504,11 @@ nfsd4_sequence(struct svc_rqst *rqstp,
> > > out:
> > > spin_unlock(&sessionid_lock);
> > > /* Renew the clientid on success and on replay */
> > > - nfs4_lock_state();
> > > - renew_client(session->se_client);
> > > - nfs4_unlock_state();
> > > -err:
> > > + if (cstate->session) {
> > > + nfs4_lock_state();
> > > + renew_client(session->se_client);
> > > + nfs4_unlock_state();
> > > + }
> > > dprintk("%s: return %d\n", __func__, ntohl(status));
> > > return status;
> > > }
> >
> > The code looks correct.
> > Coming to think about it, I hope that cstate is initialized to zeroes
> > since we're not explicitly clearing it. Should we?
> > cstate comes from the struct nfsd4_compoundres * nfsd4_proc_compound
> > is being called with...
>
> net/sunrpc/svc.c:svc_process_common() has this:
>
> /* Initialize storage for argp and resp */
> memset(rqstp->rq_argp, 0, procp->pc_argsize);
> memset(rqstp->rq_resp, 0, procp->pc_ressize);
>
> OK!
(Applied).--b.
--
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
