Grace period NEVER ends

Commit Message

Boaz Harrosh Aug. 12, 2011, 1:16 a.m. UTC

On 08/11/2011 05:51 PM, Boaz Harrosh wrote:
> Hi
> 
> I have this weird problem with latest code. It is Benny's latest based
> on 3.1-rc1 + linus/master of today + trond/linux-next.
> 
> For my testing I use a pNFS client mounted on localhost over a pNFS-nfsd on
> the same machine. If I wait the 90 seconds or so and then mount all is well
> but if I mount right away, or stop the server, then start and mount.
>   The Grace period NEVER ends. On prints I see the server returning 100013
>   continuously forever.
> 
> This is my usual test setup. In the passed it would wait the annoying grace
> and continue. Now it never returns.
> 
> Before I start to bisect back to a good point I though I might ask for pointers
> on what might have changed in this respect.
> 
> Thanks for any input
> Boaz
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

I put some prints and I see that the nfserr_grace is returned from here:


If you ask me that if() in nfsd4_open above is totally bogus.
So what if this is not the cl_firststate? Don't we have to
actually ask is this a grace period at all? which we do below.
It looks to me like the complete if (and its comment) is not
needed. We already check for the proper things below.

Boaz




--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Boaz Harrosh Aug. 12, 2011, 1:29 a.m. UTC | #1

With this patch I'm back to the previous behavior. That is
wait your grace period then continue.

---
NFSD: Remove a wrong check in nfs4_open

We are already doing the proper grace period checking
farther down in nfs4_open. This check was just checking
nothing and was totally unrelated to the comment about
"RECLAIM_COMPLETE". It was a bug because if an open was
coming before the grace period end, it would then never
pass the condition of not being cl_firststate.

Boaz

---
@@ -295,15 +295,6 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (open->op_create && open->op_claim_type != NFS4_OPEN_CLAIM_NULL)
 		return nfserr_inval;
 
-	/*
-	 * RFC5661 18.51.3
-	 * Before RECLAIM_COMPLETE done, server should deny new lock
-	 */
-	if (nfsd4_has_session(cstate) &&
-	    !cstate->session->se_client->cl_firststate &&
-	    open->op_claim_type != NFS4_OPEN_CLAIM_PREVIOUS)
-		return nfserr_grace;
-
 	if (nfsd4_has_session(cstate))
 		copy_clientid(&open->op_clientid, cstate->session);
 

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bruce Fields Aug. 12, 2011, 2:15 a.m. UTC | #2

On Thu, Aug 11, 2011 at 06:29:20PM -0700, Boaz Harrosh wrote:
> With this patch I'm back to the previous behavior. That is
> wait your grace period then continue.

Is it true for some reason that the client never sends RECLAIM_COMPLETE?

--b.

> 
> ---
> NFSD: Remove a wrong check in nfs4_open
> 
> We are already doing the proper grace period checking
> farther down in nfs4_open. This check was just checking
> nothing and was totally unrelated to the comment about
> "RECLAIM_COMPLETE". It was a bug because if an open was
> coming before the grace period end, it would then never
> pass the condition of not being cl_firststate.
> 
> Boaz
> 
> ---
> @@ -295,15 +295,6 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	if (open->op_create && open->op_claim_type != NFS4_OPEN_CLAIM_NULL)
>  		return nfserr_inval;
>  
> -	/*
> -	 * RFC5661 18.51.3
> -	 * Before RECLAIM_COMPLETE done, server should deny new lock
> -	 */
> -	if (nfsd4_has_session(cstate) &&
> -	    !cstate->session->se_client->cl_firststate &&
> -	    open->op_claim_type != NFS4_OPEN_CLAIM_PREVIOUS)
> -		return nfserr_grace;
> -
>  	if (nfsd4_has_session(cstate))
>  		copy_clientid(&open->op_clientid, cstate->session);
>  
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Casey Bodley Aug. 12, 2011, 2:08 p.m. UTC | #3

On Thu, Aug 11, 2011 at 10:15 PM, J. Bruce Fields <[e-mail]> wrote:
> On Thu, Aug 11, 2011 at 06:29:20PM -0700, Boaz Harrosh wrote:
>> With this patch I'm back to the previous behavior. That is
>> wait your grace period then continue.
>
> Is it true for some reason that the client never sends RECLAIM_COMPLETE?

I tested this yesterday with the windows client and saw the same
never-ending grace period on OPEN.  We do send RECLAIM_COMPLETE, and
it completes successfully.  Other operations like CREATE and REMOVE
succeed as well.

>
> --b.
>
>>
>> ---
>> NFSD: Remove a wrong check in nfs4_open
>>
>> We are already doing the proper grace period checking
>> farther down in nfs4_open. This check was just checking
>> nothing and was totally unrelated to the comment about
>> "RECLAIM_COMPLETE". It was a bug because if an open was
>> coming before the grace period end, it would then never
>> pass the condition of not being cl_firststate.
>>
>> Boaz
>>
>> ---
>> @@ -295,15 +295,6 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>       if (open->op_create && open->op_claim_type != NFS4_OPEN_CLAIM_NULL)
>>               return nfserr_inval;
>>
>> -     /*
>> -      * RFC5661 18.51.3
>> -      * Before RECLAIM_COMPLETE done, server should deny new lock
>> -      */
>> -     if (nfsd4_has_session(cstate) &&
>> -         !cstate->session->se_client->cl_firststate &&
>> -         open->op_claim_type != NFS4_OPEN_CLAIM_PREVIOUS)
>> -             return nfserr_grace;
>> -
>>       if (nfsd4_has_session(cstate))
>>               copy_clientid(&open->op_clientid, cstate->session);
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Trond Myklebust Aug. 12, 2011, 3:52 p.m. UTC | #4

On Thu, 2011-08-11 at 18:29 -0700, Boaz Harrosh wrote: 
> With this patch I'm back to the previous behavior. That is
> wait your grace period then continue.
> 
> ---
> NFSD: Remove a wrong check in nfs4_open
> 
> We are already doing the proper grace period checking
> farther down in nfs4_open. This check was just checking
> nothing and was totally unrelated to the comment about
> "RECLAIM_COMPLETE". It was a bug because if an open was
> coming before the grace period end, it would then never
> pass the condition of not being cl_firststate.
> 
> Boaz
> 
> ---
> @@ -295,15 +295,6 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	if (open->op_create && open->op_claim_type != NFS4_OPEN_CLAIM_NULL)
>  		return nfserr_inval;
>  
> -	/*
> -	 * RFC5661 18.51.3
> -	 * Before RECLAIM_COMPLETE done, server should deny new lock
> -	 */
> -	if (nfsd4_has_session(cstate) &&
> -	    !cstate->session->se_client->cl_firststate &&
> -	    open->op_claim_type != NFS4_OPEN_CLAIM_PREVIOUS)
> -		return nfserr_grace;
> -

BTW: restricting opens to CLAIM_PREVIOUS only during the grace period
seems wrong.

If I have already reclaimed my delegation, then why shouldn't I be able
to do a CLAIM_DELEGATE_CUR and/or CLAIM_DELEG_CUR_FH open? It is not as
if those can ever cause a lock that will conflict with some other
client's lock reclaims.

Trond

Bruce Fields Aug. 12, 2011, 4:25 p.m. UTC | #5

On Fri, Aug 12, 2011 at 11:52:05AM -0400, Trond Myklebust wrote:
> On Thu, 2011-08-11 at 18:29 -0700, Boaz Harrosh wrote: 
> > With this patch I'm back to the previous behavior. That is
> > wait your grace period then continue.
> > 
> > ---
> > NFSD: Remove a wrong check in nfs4_open
> > 
> > We are already doing the proper grace period checking
> > farther down in nfs4_open. This check was just checking
> > nothing and was totally unrelated to the comment about
> > "RECLAIM_COMPLETE". It was a bug because if an open was
> > coming before the grace period end, it would then never
> > pass the condition of not being cl_firststate.
> > 
> > Boaz
> > 
> > ---
> > @@ -295,15 +295,6 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	if (open->op_create && open->op_claim_type != NFS4_OPEN_CLAIM_NULL)
> >  		return nfserr_inval;
> >  
> > -	/*
> > -	 * RFC5661 18.51.3
> > -	 * Before RECLAIM_COMPLETE done, server should deny new lock
> > -	 */
> > -	if (nfsd4_has_session(cstate) &&
> > -	    !cstate->session->se_client->cl_firststate &&
> > -	    open->op_claim_type != NFS4_OPEN_CLAIM_PREVIOUS)
> > -		return nfserr_grace;
> > -
> 
> BTW: restricting opens to CLAIM_PREVIOUS only during the grace period
> seems wrong.
> 
> If I have already reclaimed my delegation, then why shouldn't I be able
> to do a CLAIM_DELEGATE_CUR and/or CLAIM_DELEG_CUR_FH open? It is not as
> if those can ever cause a lock that will conflict with some other
> client's lock reclaims.

I was assuming the client had to send a reclaim_complete before it could
do that, but... I think you're right, those must fall under the "reclaim
operations" that a client is allowed to do before the reclaim_complete.

I need to look at rfc 5661 10.2.1--I don't think we have that stuff
right at all.  Argh.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index a68384f..8eae742 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -301,8 +301,10 @@  nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	 */
 	if (nfsd4_has_session(cstate) &&
 	    !cstate->session->se_client->cl_firststate &&
-	    open->op_claim_type != NFS4_OPEN_CLAIM_PREVIOUS)
+	    open->op_claim_type != NFS4_OPEN_CLAIM_PREVIOUS) {
+		printk(KERN_ERR "!!! Before RECLAIM_COMPLETE done\n");
 		return nfserr_grace;
+	}
 
 	if (nfsd4_has_session(cstate))
 		copy_clientid(&open->op_clientid, cstate->session);

Is that new code?
and this print below is never shown:
diff --git a/fs/lockd/grace.c b/fs/lockd/grace.c
index 183cc1f..202c3e7 100644
--- a/fs/lockd/grace.c
+++ b/fs/lockd/grace.c
@@ -54,6 +54,10 @@  EXPORT_SYMBOL_GPL(locks_end_grace);
  */
 int locks_in_grace(void)
 {
-	return !list_empty(&grace_list);
+	int ret = !list_empty(&grace_list);
+
+	if (ret)
+		printk(KERN_ERR "locks_in_grace => true\n");
+	return ret;
 }
 EXPORT_SYMBOL_GPL(locks_in_grace);