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

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

[Benny Halevy]

nfsd4_sequence() should not renew the client state if the session was not
found or if there was a bad slot.  This will also avoid dereferencing a
null session pointer.

FIXME: 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>
---

Bruce, I'm not whether it is too late but this patch fixes
a bug in 2.6.31-rc that we've hit in the last Bakeathon.

Benny

 fs/nfsd/nfs4state.c |   30 +++++++++++++++++++-----------
 1 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 1dedde9..3e7b20a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1436,12 +1436,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;
+	}
 
 	slot = &session->se_slots[seq->slotid];
 	dprintk("%s: slotid %d\n", __func__, seq->slotid);
@@ -1454,10 +1458,12 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 		 * for nfsd4_proc_compound processing */
 		status = nfsd4_replay_cache_entry(resp, seq);
 		cstate->status = nfserr_replay_cache;
-		goto replay_cache;
-	}
-	if (status)
 		goto out;
+	}
+	if (status) {
+		spin_unlock(&sessionid_lock);
+		goto err;
+	}
 
 	/* Success! bump slot seqid */
 	slot->sl_inuse = true;
@@ -1470,15 +1476,17 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 	cstate->slot = slot;
 	cstate->session = session;
 
-replay_cache:
-	/* Renew the clientid on success and on replay.
-	 * Hold a session reference until done processing the compound:
+	/* Hold a session reference until done processing the compound:
 	 * nfsd4_put_session called only if the cstate slot is set.
 	 */
-	renew_client(session->se_client);
 	nfsd4_get_session(session);
 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:
 	dprintk("%s: return %d\n", __func__, ntohl(status));
 	return status;
 }
-- 
1.6.4

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

[J. Bruce Fields]

On Thu, Aug 20, 2009 at 03:21:56AM +0300, Benny Halevy wrote:
> nfsd4_sequence() should not renew the client state if the session was not
> found or if there was a bad slot.  This will also avoid dereferencing a
> null session pointer.
> 
> FIXME: 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.

Is that first paragraph left over from some previous iteration of this
patch?

> 
> 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>
> ---
> 
> Bruce, I'm not whether it is too late but this patch fixes
> a bug in 2.6.31-rc that we've hit in the last Bakeathon.

I think we should wait for 2.6.32.  (And was the bug you hit due to the
lack of state locking or to the NULL dereference?)

I don't have any fix for the locking problem queued up for 2.6.32.

--b.

> 
> Benny
> 
>  fs/nfsd/nfs4state.c |   30 +++++++++++++++++++-----------
>  1 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 1dedde9..3e7b20a 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1436,12 +1436,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;
> +	}
>  
>  	slot = &session->se_slots[seq->slotid];
>  	dprintk("%s: slotid %d\n", __func__, seq->slotid);
> @@ -1454,10 +1458,12 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>  		 * for nfsd4_proc_compound processing */
>  		status = nfsd4_replay_cache_entry(resp, seq);
>  		cstate->status = nfserr_replay_cache;
> -		goto replay_cache;
> -	}
> -	if (status)
>  		goto out;
> +	}
> +	if (status) {
> +		spin_unlock(&sessionid_lock);
> +		goto err;
> +	}
>  
>  	/* Success! bump slot seqid */
>  	slot->sl_inuse = true;
> @@ -1470,15 +1476,17 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>  	cstate->slot = slot;
>  	cstate->session = session;
>  
> -replay_cache:
> -	/* Renew the clientid on success and on replay.
> -	 * Hold a session reference until done processing the compound:
> +	/* Hold a session reference until done processing the compound:
>  	 * nfsd4_put_session called only if the cstate slot is set.
>  	 */
> -	renew_client(session->se_client);
>  	nfsd4_get_session(session);
>  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:
>  	dprintk("%s: return %d\n", __func__, ntohl(status));
>  	return status;
>  }
> -- 
> 1.6.4
> 

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

[Benny Halevy]

On Aug. 24, 2009, 19:29 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> On Thu, Aug 20, 2009 at 03:21:56AM +0300, Benny Halevy wrote:
>> nfsd4_sequence() should not renew the client state if the session was not
>> found or if there was a bad slot.  This will also avoid dereferencing a
>> null session pointer.
>>
>> FIXME: 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.
> 
> Is that first paragraph left over from some previous iteration of this
> patch?

Not really.
It sort of sets up the stage for the state lock elimination project.
That said, feel free to remove these lines as they are not particularly
important for understanding what this patch does or how to do it better.

Benny

> 
>> 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>
>> ---
>>
>> Bruce, I'm not whether it is too late but this patch fixes
>> a bug in 2.6.31-rc that we've hit in the last Bakeathon.
> 
> I think we should wait for 2.6.32.  (And was the bug you hit due to the
> lack of state locking or to the NULL dereference?)
> 
> I don't have any fix for the locking problem queued up for 2.6.32.
> 
> --b.
> 
>> Benny
>>
>>  fs/nfsd/nfs4state.c |   30 +++++++++++++++++++-----------
>>  1 files changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 1dedde9..3e7b20a 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -1436,12 +1436,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;
>> +	}
>>  
>>  	slot = &session->se_slots[seq->slotid];
>>  	dprintk("%s: slotid %d\n", __func__, seq->slotid);
>> @@ -1454,10 +1458,12 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>>  		 * for nfsd4_proc_compound processing */
>>  		status = nfsd4_replay_cache_entry(resp, seq);
>>  		cstate->status = nfserr_replay_cache;
>> -		goto replay_cache;
>> -	}
>> -	if (status)
>>  		goto out;
>> +	}
>> +	if (status) {
>> +		spin_unlock(&sessionid_lock);
>> +		goto err;
>> +	}
>>  
>>  	/* Success! bump slot seqid */
>>  	slot->sl_inuse = true;
>> @@ -1470,15 +1476,17 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>>  	cstate->slot = slot;
>>  	cstate->session = session;
>>  
>> -replay_cache:
>> -	/* Renew the clientid on success and on replay.
>> -	 * Hold a session reference until done processing the compound:
>> +	/* Hold a session reference until done processing the compound:
>>  	 * nfsd4_put_session called only if the cstate slot is set.
>>  	 */
>> -	renew_client(session->se_client);
>>  	nfsd4_get_session(session);
>>  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:
>>  	dprintk("%s: return %d\n", __func__, ntohl(status));
>>  	return status;
>>  }
>> -- 
>> 1.6.4
>>

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

[J. Bruce Fields]

On Tue, Aug 25, 2009 at 10:06:34AM +0300, Benny Halevy wrote:
> On Aug. 24, 2009, 19:29 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > On Thu, Aug 20, 2009 at 03:21:56AM +0300, Benny Halevy wrote:
> >> nfsd4_sequence() should not renew the client state if the session was not
> >> found or if there was a bad slot.  This will also avoid dereferencing a
> >> null session pointer.
> >>
> >> FIXME: 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.
> > 
> > Is that first paragraph left over from some previous iteration of this
> > patch?
> 
> Not really.
> It sort of sets up the stage for the state lock elimination project.
> That said, feel free to remove these lines as they are not particularly
> important for understanding what this patch does or how to do it better.

Note I was referring to "should not renew..."  That seems to describe a
problem that has already been fixed.

--b.

> >> 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>
> >> ---
> >>
> >> Bruce, I'm not whether it is too late but this patch fixes
> >> a bug in 2.6.31-rc that we've hit in the last Bakeathon.
> > 
> > I think we should wait for 2.6.32.  (And was the bug you hit due to the
> > lack of state locking or to the NULL dereference?)
> > 
> > I don't have any fix for the locking problem queued up for 2.6.32.
> > 
> > --b.
> > 
> >> Benny
> >>
> >>  fs/nfsd/nfs4state.c |   30 +++++++++++++++++++-----------
> >>  1 files changed, 19 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index 1dedde9..3e7b20a 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -1436,12 +1436,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;
> >> +	}
> >>  
> >>  	slot = &session->se_slots[seq->slotid];
> >>  	dprintk("%s: slotid %d\n", __func__, seq->slotid);
> >> @@ -1454,10 +1458,12 @@ nfsd4_sequence(struct svc_rqst *rqstp,
> >>  		 * for nfsd4_proc_compound processing */
> >>  		status = nfsd4_replay_cache_entry(resp, seq);
> >>  		cstate->status = nfserr_replay_cache;
> >> -		goto replay_cache;
> >> -	}
> >> -	if (status)
> >>  		goto out;
> >> +	}
> >> +	if (status) {
> >> +		spin_unlock(&sessionid_lock);
> >> +		goto err;
> >> +	}
> >>  
> >>  	/* Success! bump slot seqid */
> >>  	slot->sl_inuse = true;
> >> @@ -1470,15 +1476,17 @@ nfsd4_sequence(struct svc_rqst *rqstp,
> >>  	cstate->slot = slot;
> >>  	cstate->session = session;
> >>  
> >> -replay_cache:
> >> -	/* Renew the clientid on success and on replay.
> >> -	 * Hold a session reference until done processing the compound:
> >> +	/* Hold a session reference until done processing the compound:
> >>  	 * nfsd4_put_session called only if the cstate slot is set.
> >>  	 */
> >> -	renew_client(session->se_client);
> >>  	nfsd4_get_session(session);
> >>  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:
> >>  	dprintk("%s: return %d\n", __func__, ntohl(status));
> >>  	return status;
> >>  }
> >> -- 
> >> 1.6.4
> >>

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

[Benny Halevy]

On Aug. 25, 2009, 16:18 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> On Tue, Aug 25, 2009 at 10:06:34AM +0300, Benny Halevy wrote:
>> On Aug. 24, 2009, 19:29 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
>>> On Thu, Aug 20, 2009 at 03:21:56AM +0300, Benny Halevy wrote:
>>>> nfsd4_sequence() should not renew the client state if the session was not
>>>> found or if there was a bad slot.  This will also avoid dereferencing a
>>>> null session pointer.
>>>>
>>>> FIXME: 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.
>>> Is that first paragraph left over from some previous iteration of this
>>> patch?
>> Not really.
>> It sort of sets up the stage for the state lock elimination project.
>> That said, feel free to remove these lines as they are not particularly
>> important for understanding what this patch does or how to do it better.
> 
> Note I was referring to "should not renew..."  That seems to describe a
> problem that has already been fixed.

Hmm, apparently this came from "Do not renew state on error"
which was squashed together with this patch.
The text is still valid though somewhat unrelated to this patch's
title.  Maybe a better title would be the original one:
""Do not renew state on error"

Benny

> 
> --b.
> 
>>>> 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>
>>>> ---
>>>>
>>>> Bruce, I'm not whether it is too late but this patch fixes
>>>> a bug in 2.6.31-rc that we've hit in the last Bakeathon.
>>> I think we should wait for 2.6.32.  (And was the bug you hit due to the
>>> lack of state locking or to the NULL dereference?)
>>>
>>> I don't have any fix for the locking problem queued up for 2.6.32.
>>>
>>> --b.
>>>
>>>> Benny
>>>>
>>>>  fs/nfsd/nfs4state.c |   30 +++++++++++++++++++-----------
>>>>  1 files changed, 19 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index 1dedde9..3e7b20a 100644
>>>> --- a/fs/nfsd/nfs4state.c
>>>> +++ b/fs/nfsd/nfs4state.c
>>>> @@ -1436,12 +1436,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;
>>>> +	}
>>>>  
>>>>  	slot = &session->se_slots[seq->slotid];
>>>>  	dprintk("%s: slotid %d\n", __func__, seq->slotid);
>>>> @@ -1454,10 +1458,12 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>>>>  		 * for nfsd4_proc_compound processing */
>>>>  		status = nfsd4_replay_cache_entry(resp, seq);
>>>>  		cstate->status = nfserr_replay_cache;
>>>> -		goto replay_cache;
>>>> -	}
>>>> -	if (status)
>>>>  		goto out;
>>>> +	}
>>>> +	if (status) {
>>>> +		spin_unlock(&sessionid_lock);
>>>> +		goto err;
>>>> +	}
>>>>  
>>>>  	/* Success! bump slot seqid */
>>>>  	slot->sl_inuse = true;
>>>> @@ -1470,15 +1476,17 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>>>>  	cstate->slot = slot;
>>>>  	cstate->session = session;
>>>>  
>>>> -replay_cache:
>>>> -	/* Renew the clientid on success and on replay.
>>>> -	 * Hold a session reference until done processing the compound:
>>>> +	/* Hold a session reference until done processing the compound:
>>>>  	 * nfsd4_put_session called only if the cstate slot is set.
>>>>  	 */
>>>> -	renew_client(session->se_client);
>>>>  	nfsd4_get_session(session);
>>>>  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:
>>>>  	dprintk("%s: return %d\n", __func__, ntohl(status));
>>>>  	return status;
>>>>  }
>>>> -- 
>>>> 1.6.4
>>>>

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

[J. Bruce Fields]

On Tue, Aug 25, 2009 at 08:02:41PM +0300, Benny Halevy wrote:
> On Aug. 25, 2009, 16:18 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > On Tue, Aug 25, 2009 at 10:06:34AM +0300, Benny Halevy wrote:
> >> On Aug. 24, 2009, 19:29 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> >>> On Thu, Aug 20, 2009 at 03:21:56AM +0300, Benny Halevy wrote:
> >>>> nfsd4_sequence() should not renew the client state if the session was not
> >>>> found or if there was a bad slot.  This will also avoid dereferencing a
> >>>> null session pointer.
> >>>>
> >>>> FIXME: 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.
> >>> Is that first paragraph left over from some previous iteration of this
> >>> patch?
> >> Not really.
> >> It sort of sets up the stage for the state lock elimination project.
> >> That said, feel free to remove these lines as they are not particularly
> >> important for understanding what this patch does or how to do it better.
> > 
> > Note I was referring to "should not renew..."  That seems to describe a
> > problem that has already been fixed.
> 
> Hmm, apparently this came from "Do not renew state on error"
> which was squashed together with this patch.
> The text is still valid though somewhat unrelated to this patch's
> title.  Maybe a better title would be the original one:
> ""Do not renew state on error"

The title's right, it's just the comment that went stale.  I've applied
for 2.6.32 as follows.

--b.

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;
+	}
 
 	slot = &session->se_slots[seq->slotid];
 	dprintk("%s: slotid %d\n", __func__, seq->slotid);
@@ -1481,10 +1485,12 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 		 * for nfsd4_svc_encode_compoundres processing */
 		status = nfsd4_replay_cache_entry(resp, seq);
 		cstate->status = nfserr_replay_cache;
-		goto replay_cache;
-	}
-	if (status)
 		goto out;
+	}
+	if (status) {
+		spin_unlock(&sessionid_lock);
+		goto err;
+	}
 
 	/* Success! bump slot seqid */
 	slot->sl_inuse = true;
@@ -1497,15 +1503,17 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 	cstate->slot = slot;
 	cstate->session = session;
 
-replay_cache:
-	/* Renew the clientid on success and on replay.
-	 * Hold a session reference until done processing the compound:
+	/* Hold a session reference until done processing the compound:
 	 * nfsd4_put_session called only if the cstate slot is set.
 	 */
-	renew_client(session->se_client);
 	nfsd4_get_session(session);
 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:
 	dprintk("%s: return %d\n", __func__, ntohl(status));
 	return status;
 }

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

[Boaz Harrosh]

On 08/26/2009 12:59 AM, J. Bruce Fields wrote:
> On Tue, Aug 25, 2009 at 08:02:41PM +0300, Benny Halevy wrote:
>> On Aug. 25, 2009, 16:18 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
>>> On Tue, Aug 25, 2009 at 10:06:34AM +0300, Benny Halevy wrote:
>>>> On Aug. 24, 2009, 19:29 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
>>>>> On Thu, Aug 20, 2009 at 03:21:56AM +0300, Benny Halevy wrote:
>>>>>> nfsd4_sequence() should not renew the client state if the session was not
>>>>>> found or if there was a bad slot.  This will also avoid dereferencing a
>>>>>> null session pointer.
>>>>>>
>>>>>> FIXME: 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.
>>>>> Is that first paragraph left over from some previous iteration of this
>>>>> patch?
>>>> Not really.
>>>> It sort of sets up the stage for the state lock elimination project.
>>>> That said, feel free to remove these lines as they are not particularly
>>>> important for understanding what this patch does or how to do it better.
>>>
>>> Note I was referring to "should not renew..."  That seems to describe a
>>> problem that has already been fixed.
>>
>> Hmm, apparently this came from "Do not renew state on error"
>> which was squashed together with this patch.
>> The text is still valid though somewhat unrelated to this patch's
>> title.  Maybe a better title would be the original one:
>> ""Do not renew state on error"
> 
> The title's right, it's just the comment that went stale.  I've applied
> for 2.6.32 as follows.
> 
> --b.
> 
> 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

Boaz

>  	slot = &session->se_slots[seq->slotid];
>  	dprintk("%s: slotid %d\n", __func__, seq->slotid);
> @@ -1481,10 +1485,12 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>  		 * for nfsd4_svc_encode_compoundres processing */
>  		status = nfsd4_replay_cache_entry(resp, seq);
>  		cstate->status = nfserr_replay_cache;
> -		goto replay_cache;
> -	}
> -	if (status)
>  		goto out;
> +	}
> +	if (status) {
> +		spin_unlock(&sessionid_lock);
> +		goto err;
> +	}
>  
>  	/* Success! bump slot seqid */
>  	slot->sl_inuse = true;
> @@ -1497,15 +1503,17 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>  	cstate->slot = slot;
>  	cstate->session = session;
>  
> -replay_cache:
> -	/* Renew the clientid on success and on replay.
> -	 * Hold a session reference until done processing the compound:
> +	/* Hold a session reference until done processing the compound:
>  	 * nfsd4_put_session called only if the cstate slot is set.
>  	 */
> -	renew_client(session->se_client);
>  	nfsd4_get_session(session);
>  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:
>  	dprintk("%s: return %d\n", __func__, ntohl(status));
>  	return status;
>  }
> _______________________________________________
> pNFS mailing list
> pNFS@linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs

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

[J. Bruce Fields]

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;
 }

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

[Benny Halevy]

On Aug. 27, 2009, 0:02 +0300, "J. Bruce Fields" <bfields@fieldses.org> 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@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;
>  }

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

Benny

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

[J. Bruce Fields]

On Thu, Aug 27, 2009 at 07:37:02PM +0300, Benny Halevy wrote:
> On Aug. 27, 2009, 0:02 +0300, "J. Bruce Fields" <bfields@fieldses.org> 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@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;
> >  }
> 
> 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!

--b.

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

[J. Bruce Fields]

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@fieldses.org> 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@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;
> > >  }
> > 
> > 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.

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

[Benny Halevy]

FIXME: 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>
---

Bruce, please add this patch to the nfsd41-for-2.6.31 series.
Also availble on git://linux-nfs.org/~bhalevy/linux-pnfs.git nfsd41-for-2.6.31

 fs/nfsd/nfs4state.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e21da54..c845365 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1443,10 +1443,12 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 		 * for nfsd4_svc_encode_compoundres processing */
 		status = nfsd4_replay_cache_entry(resp, seq);
 		cstate->status = nfserr_replay_cache;
-		goto replay_cache;
-	}
-	if (status)
 		goto out;
+	}
+	if (status) {
+		spin_unlock(&sessionid_lock);
+		goto err;
+	}
 
 	/* Success! bump slot seqid */
 	slot->sl_inuse = true;
@@ -1462,11 +1464,13 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 	/* Hold a session reference until done caching the response */
 	nfsd4_get_session(session);
 
-replay_cache:
-	/* Renew the clientid on success and on replay */
-	renew_client(session->se_client);
 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:
 	dprintk("%s: return %d\n", __func__, ntohl(status));
 	return status;
 }
-- 
1.6.3.1