Re: [pnfs] [PATCH] nfsd41: renew_client must be called under the state lock

["J. Bruce Fields" <bfields@fieldses.org>]

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@panasas.com>
> > 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@panasas.com>
> >     [nfsd41: Do not renew state on error]
> >     Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
> >     Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> >     Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> > 
> > 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;
 }