[PATCH] nfsd: fix nfsd4_delegreturn to return correct error codes

[PATCH] nfsd: fix nfsd4_delegreturn to return correct error codes

[Andrew Elble]

When delegations are revoked:

In the NFSv4 case, NFS4ERR_BAD_STATEID
In the NFSv4.1 case, NFS4ERR_DELEG_REVOKED

Signed-off-by: Andrew Elble <aweits@rit.edu>
---
 fs/nfsd/nfs4state.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 66df2903ab8e..0f0634ca4158 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5147,15 +5147,21 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if ((status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, 0)))
 		return status;
 
-	status = nfsd4_lookup_stateid(cstate, stateid, NFS4_DELEG_STID, &s, nn);
+	status = nfsd4_lookup_stateid(cstate, stateid,
+				NFS4_DELEG_STID|NFS4_REVOKED_DELEG_STID,
+				&s, nn);
 	if (status)
 		goto out;
 	dp = delegstateid(s);
 	status = check_stateid_generation(stateid, &dp->dl_stid.sc_stateid, nfsd4_has_session(cstate));
 	if (status)
 		goto put_stateid;
-
-	destroy_delegation(dp);
+	if (dp->dl_stid.sc_type == NFS4_DELEG_STID)
+		destroy_delegation(dp);
+	if (dp->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID)
+		status = nfserr_bad_stateid;
+		if (cstate->minorversion)
+			status = nfserr_deleg_revoked;
 put_stateid:
 	nfs4_put_stid(&dp->dl_stid);
 out:
-- 
2.4.6

Re: [PATCH] nfsd: fix nfsd4_delegreturn to return correct error codes

[J. Bruce Fields]

On Wed, Nov 04, 2015 at 05:07:17PM -0500, Andrew Elble wrote:
> When delegations are revoked:
> 
> In the NFSv4 case, NFS4ERR_BAD_STATEID
> In the NFSv4.1 case, NFS4ERR_DELEG_REVOKED

Thanks.  That looks right, but this isn't unique to delegreturn--looks
like it's probably applicable to anything that might take a delegation
stateid--maybe preprocess_stateid_op needs a similar change?

--b.

> 
> Signed-off-by: Andrew Elble <aweits@rit.edu>
> ---
>  fs/nfsd/nfs4state.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 66df2903ab8e..0f0634ca4158 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5147,15 +5147,21 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	if ((status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, 0)))
>  		return status;
>  
> -	status = nfsd4_lookup_stateid(cstate, stateid, NFS4_DELEG_STID, &s, nn);
> +	status = nfsd4_lookup_stateid(cstate, stateid,
> +				NFS4_DELEG_STID|NFS4_REVOKED_DELEG_STID,
> +				&s, nn);
>  	if (status)
>  		goto out;
>  	dp = delegstateid(s);
>  	status = check_stateid_generation(stateid, &dp->dl_stid.sc_stateid, nfsd4_has_session(cstate));
>  	if (status)
>  		goto put_stateid;
> -
> -	destroy_delegation(dp);
> +	if (dp->dl_stid.sc_type == NFS4_DELEG_STID)
> +		destroy_delegation(dp);
> +	if (dp->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID)
> +		status = nfserr_bad_stateid;
> +		if (cstate->minorversion)
> +			status = nfserr_deleg_revoked;
>  put_stateid:
>  	nfs4_put_stid(&dp->dl_stid);
>  out:
> -- 
> 2.4.6

Re: [PATCH] nfsd: fix nfsd4_delegreturn to return correct error codes

[Trond Myklebust]

On Wed, Nov 4, 2015 at 5:07 PM, Andrew Elble <aweits@rit.edu> wrote:
> When delegations are revoked:
>
> In the NFSv4 case, NFS4ERR_BAD_STATEID
> In the NFSv4.1 case, NFS4ERR_DELEG_REVOKED
>
> Signed-off-by: Andrew Elble <aweits@rit.edu>
> ---
>  fs/nfsd/nfs4state.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 66df2903ab8e..0f0634ca4158 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5147,15 +5147,21 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>         if ((status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, 0)))
>                 return status;
>
> -       status = nfsd4_lookup_stateid(cstate, stateid, NFS4_DELEG_STID, &s, nn);
> +       status = nfsd4_lookup_stateid(cstate, stateid,
> +                               NFS4_DELEG_STID|NFS4_REVOKED_DELEG_STID,
> +                               &s, nn);
>         if (status)
>                 goto out;
>         dp = delegstateid(s);
>         status = check_stateid_generation(stateid, &dp->dl_stid.sc_stateid, nfsd4_has_session(cstate));
>         if (status)
>                 goto put_stateid;
> -
> -       destroy_delegation(dp);
> +       if (dp->dl_stid.sc_type == NFS4_DELEG_STID)
> +               destroy_delegation(dp);
> +       if (dp->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID)
> +               status = nfserr_bad_stateid;
> +               if (cstate->minorversion)
> +                       status = nfserr_deleg_revoked;

Umm... If the client is sending delegreturn, then why not destroy the
delegation? What is the point of forcing the client to send
FREE_STATEID, when it is telling you that it is no longer caching any
locks or associated state and is no longer interested in keeping the
delegation?

Re: [PATCH] nfsd: fix nfsd4_delegreturn to return correct error codes

[Jeff Layton]

On Wed,  4 Nov 2015 17:07:17 -0500
Andrew Elble <aweits@rit.edu> wrote:

> When delegations are revoked:
> 
> In the NFSv4 case, NFS4ERR_BAD_STATEID
> In the NFSv4.1 case, NFS4ERR_DELEG_REVOKED
> 
> Signed-off-by: Andrew Elble <aweits@rit.edu>
> ---
>  fs/nfsd/nfs4state.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 66df2903ab8e..0f0634ca4158 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5147,15 +5147,21 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	if ((status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, 0)))
>  		return status;
>  
> -	status = nfsd4_lookup_stateid(cstate, stateid, NFS4_DELEG_STID, &s, nn);
> +	status = nfsd4_lookup_stateid(cstate, stateid,
> +				NFS4_DELEG_STID|NFS4_REVOKED_DELEG_STID,
> +				&s, nn);
>  	if (status)
>  		goto out;
>  	dp = delegstateid(s);
>  	status = check_stateid_generation(stateid, &dp->dl_stid.sc_stateid, nfsd4_has_session(cstate));
>  	if (status)
>  		goto put_stateid;
> -
> -	destroy_delegation(dp);
> +	if (dp->dl_stid.sc_type == NFS4_DELEG_STID)
> +		destroy_delegation(dp);
> +	if (dp->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID)
> +		status = nfserr_bad_stateid;
> +		if (cstate->minorversion)
> +			status = nfserr_deleg_revoked;
>  put_stateid:
>  	nfs4_put_stid(&dp->dl_stid);
>  out:

Looks correct to me.

Reviewed-by: Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH] nfsd: fix nfsd4_delegreturn to return correct error codes

[Andrew W Elble]

> Umm... If the client is sending delegreturn, then why not destroy the
> delegation?

ObDisclaimer: My "internal" version of this patch does just that.

If the DELEGRETURN is the first time that the client hears of the
revocation, I'm guessing that there isn't anything that can be done to
rewind time and do anything differently. But as Bruce points out, it
seems like there are other places where NFS4ERR_DELEG_REVOKED should be
being returned.

> What is the point of forcing the client to send
> FREE_STATEID, when it is telling you that it is no longer caching any
> locks or associated state and is no longer interested in keeping the
> delegation?

But - I keep re-reading RFC5661, and I can't figure out how this is
what was intended.

It seems like the "correct" thing to do is inform the client (via probably
too many different methods) that the/a delegation is revoked and let it
acknowledge via FREE_STATEID. The allowed error returns in 15.2 seem to
confirm this view.

Thanks,

Andy

-- 
Andrew W. Elble
aweits@discipline.rit.edu
Infrastructure Engineer, Communications Technical Lead
Rochester Institute of Technology
PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912

Re: [PATCH] nfsd: fix nfsd4_delegreturn to return correct error codes

[Trond Myklebust]

On Fri, Nov 6, 2015 at 8:08 AM, Andrew W Elble <aweits@rit.edu> wrote:
>
>> Umm... If the client is sending delegreturn, then why not destroy the
>> delegation?
>
> ObDisclaimer: My "internal" version of this patch does just that.
>
> If the DELEGRETURN is the first time that the client hears of the
> revocation, I'm guessing that there isn't anything that can be done to
> rewind time and do anything differently. But as Bruce points out, it
> seems like there are other places where NFS4ERR_DELEG_REVOKED should be
> being returned.
>
>> What is the point of forcing the client to send
>> FREE_STATEID, when it is telling you that it is no longer caching any
>> locks or associated state and is no longer interested in keeping the
>> delegation?
>
> But - I keep re-reading RFC5661, and I can't figure out how this is
> what was intended.
>
> It seems like the "correct" thing to do is inform the client (via probably
> too many different methods) that the/a delegation is revoked and let it
> acknowledge via FREE_STATEID. The allowed error returns in 15.2 seem to
> confirm this view.

Just because the protocol _allows_ you to do this, it doesn't mean
that it is the right thing to do.
Section 8.2.4. reads:

   Stateids must remain valid until either a client restart or a server
   restart or until the client returns all of the locks associated with
   the stateid by means of an operation such as CLOSE or DELEGRETURN.
   If the locks are lost due to revocation, as long as the client ID is
   valid, the stateid remains a valid designation of that revoked state
   until the client frees it by using FREE_STATEID.

In this case, there are no lost locks due to revocation.

The client has presumably not received NFS4ERR_DELEG_REVOKED on any of
its previous uses of the delegation stateid and it has presumably not
seen any SEQ4_STATUS_* notifications when recovering locks, since both
of those situations should cause the client to switch to using
TEST_STATEID/FREE_STATEID instead of a DELEGRETURN.

So, what exactly is the client supposed to do differently when the
server replies NFS4ERR_DELEG_REVOKED to the DELEGRETURN other than
send a completely redundant FREE_STATEID? Why couldn't the server just
return NFS4ERR_OK?

Re: [PATCH] nfsd: fix nfsd4_delegreturn to return correct error codes

[Trond Myklebust]

On Fri, Nov 6, 2015 at 8:48 AM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> On Fri, Nov 6, 2015 at 8:08 AM, Andrew W Elble <aweits@rit.edu> wrote:
>>
>>> Umm... If the client is sending delegreturn, then why not destroy the
>>> delegation?
>>
>> ObDisclaimer: My "internal" version of this patch does just that.
>>
>> If the DELEGRETURN is the first time that the client hears of the
>> revocation, I'm guessing that there isn't anything that can be done to
>> rewind time and do anything differently. But as Bruce points out, it
>> seems like there are other places where NFS4ERR_DELEG_REVOKED should be
>> being returned.
>>
>>> What is the point of forcing the client to send
>>> FREE_STATEID, when it is telling you that it is no longer caching any
>>> locks or associated state and is no longer interested in keeping the
>>> delegation?
>>
>> But - I keep re-reading RFC5661, and I can't figure out how this is
>> what was intended.
>>
>> It seems like the "correct" thing to do is inform the client (via probably
>> too many different methods) that the/a delegation is revoked and let it
>> acknowledge via FREE_STATEID. The allowed error returns in 15.2 seem to
>> confirm this view.
>
> Just because the protocol _allows_ you to do this, it doesn't mean
> that it is the right thing to do.
> Section 8.2.4. reads:
>
>    Stateids must remain valid until either a client restart or a server
>    restart or until the client returns all of the locks associated with
>    the stateid by means of an operation such as CLOSE or DELEGRETURN.
>    If the locks are lost due to revocation, as long as the client ID is
>    valid, the stateid remains a valid designation of that revoked state
>    until the client frees it by using FREE_STATEID.
>
> In this case, there are no lost locks due to revocation.
>
> The client has presumably not received NFS4ERR_DELEG_REVOKED on any of
> its previous uses of the delegation stateid and it has presumably not
> seen any SEQ4_STATUS_* notifications when recovering locks, since both
> of those situations should cause the client to switch to using
> TEST_STATEID/FREE_STATEID instead of a DELEGRETURN.
>
> So, what exactly is the client supposed to do differently when the
> server replies NFS4ERR_DELEG_REVOKED to the DELEGRETURN other than
> send a completely redundant FREE_STATEID? Why couldn't the server just
> return NFS4ERR_OK?

BTW: even if you do return NFS4ERR_DELEG_REVOKED, there is precisely
zero value in keeping the stateid around. It is perfectly legitimate
to reply NFS4ERR_BAD_STATEID in reply to FREE_STATEID.

Re: [PATCH] nfsd: fix nfsd4_delegreturn to return correct error codes

[Jeff Layton]

On Fri, 6 Nov 2015 09:03:50 -0500
Trond Myklebust <trond.myklebust@primarydata.com> wrote:

> On Fri, Nov 6, 2015 at 8:48 AM, Trond Myklebust
> <trond.myklebust@primarydata.com> wrote:
> > On Fri, Nov 6, 2015 at 8:08 AM, Andrew W Elble <aweits@rit.edu> wrote:
> >>
> >>> Umm... If the client is sending delegreturn, then why not destroy the
> >>> delegation?
> >>
> >> ObDisclaimer: My "internal" version of this patch does just that.
> >>
> >> If the DELEGRETURN is the first time that the client hears of the
> >> revocation, I'm guessing that there isn't anything that can be done to
> >> rewind time and do anything differently. But as Bruce points out, it
> >> seems like there are other places where NFS4ERR_DELEG_REVOKED should be
> >> being returned.
> >>
> >>> What is the point of forcing the client to send
> >>> FREE_STATEID, when it is telling you that it is no longer caching any
> >>> locks or associated state and is no longer interested in keeping the
> >>> delegation?
> >>
> >> But - I keep re-reading RFC5661, and I can't figure out how this is
> >> what was intended.
> >>
> >> It seems like the "correct" thing to do is inform the client (via probably
> >> too many different methods) that the/a delegation is revoked and let it
> >> acknowledge via FREE_STATEID. The allowed error returns in 15.2 seem to
> >> confirm this view.
> >
> > Just because the protocol _allows_ you to do this, it doesn't mean
> > that it is the right thing to do.
> > Section 8.2.4. reads:
> >
> >    Stateids must remain valid until either a client restart or a server
> >    restart or until the client returns all of the locks associated with
> >    the stateid by means of an operation such as CLOSE or DELEGRETURN.
> >    If the locks are lost due to revocation, as long as the client ID is
> >    valid, the stateid remains a valid designation of that revoked state
> >    until the client frees it by using FREE_STATEID.
> >
> > In this case, there are no lost locks due to revocation.
> >
> > The client has presumably not received NFS4ERR_DELEG_REVOKED on any of
> > its previous uses of the delegation stateid and it has presumably not
> > seen any SEQ4_STATUS_* notifications when recovering locks, since both
> > of those situations should cause the client to switch to using
> > TEST_STATEID/FREE_STATEID instead of a DELEGRETURN.
> >
> > So, what exactly is the client supposed to do differently when the
> > server replies NFS4ERR_DELEG_REVOKED to the DELEGRETURN other than
> > send a completely redundant FREE_STATEID? Why couldn't the server just
> > return NFS4ERR_OK?
> 
> BTW: even if you do return NFS4ERR_DELEG_REVOKED, there is precisely
> zero value in keeping the stateid around. It is perfectly legitimate
> to reply NFS4ERR_BAD_STATEID in reply to FREE_STATEID.

Yeah, agreed. Now that I think about it, you should just go ahead and
destroy the delegation as the client is signaling that it's through
with it anyway.

Hmm...so is there any advantage to reporting NFS4ERR_DELEG_REVOKED
there at all? I guess that could be a signal that it may not have held
a delegation that it thought it had, but it's probably too late to do
anything about it if that occurs. Some older clients may also not
handle that error gracefully too, so just returning NFS4_OK might be
best...

-- 
Jeff Layton <jeff.layton@primarydata.com>

Re: [PATCH] nfsd: fix nfsd4_delegreturn to return correct error codes

[Bruce James Fields]

On Fri, Nov 06, 2015 at 09:19:47AM -0500, Jeff Layton wrote:
> On Fri, 6 Nov 2015 09:03:50 -0500
> Trond Myklebust <trond.myklebust@primarydata.com> wrote:
> 
> > On Fri, Nov 6, 2015 at 8:48 AM, Trond Myklebust
> > <trond.myklebust@primarydata.com> wrote:
> > > On Fri, Nov 6, 2015 at 8:08 AM, Andrew W Elble <aweits@rit.edu> wrote:
> > >>
> > >>> Umm... If the client is sending delegreturn, then why not destroy the
> > >>> delegation?

Yeah, good question, I didn't think about that.

The one thing that bothers me:

> Hmm...so is there any advantage to reporting NFS4ERR_DELEG_REVOKED
> there at all? I guess that could be a signal that it may not have held
> a delegation that it thought it had,

Yes, I'd like to think a little about this.  It does worry me that the
loss of a delegation could be completely silent.

Even in the absence of locks, userspace may now have gotten incorrect
lookup or stat results.  (Unless the client is careful not to depend on
delegations for any guarantees that go beyond close-to-open?)

So,

	client1		client2
	-------		-------

			make prog
					(client2 gets delegation on prog.c)

	vi prog.c
					(client2's deleg recalled, then
					revoked)

			wait for ac timeout
			make prog
			make: 'prog' is up to date

Hm, I think the client has to see a STATUS_RECALLABLE_STATE_REVOKED at
some point here, though.

In fact, is there any situation this could happen without the client
having seeing that flag on the SEQUENCE preceding this op?  I guess
there could be an extremely narrow window for a race between the two
ops.  Is that race is the only justification for having this error in
the protocol at all?  In the typical case it's going to see the sequence
flag at the same time as the error and have to do some kind of recovery
anyway.

> but it's probably too late to do anything about it if that occurs.

Yeah, I can't think what the client could do beyond log something scary.

> Some older clients may also not handle that error gracefully too,

Is it likely?  (How's the current client?)  Not sure what ungraceful
handling would be, though, it would be an odd client that would do
anything other than either FREE_STATEID or just give up.

I don't see any reason this case needs to be optimized, so if clients
handle the REVOKED error then maybe it's OK.

> so just returning NFS4_OK might be best...

Anyway, after all that, yes, maybe so, if only out of conservatism about
change.

--b.

Re: [PATCH] nfsd: fix nfsd4_delegreturn to return correct error codes

[Jeff Layton]

On Fri, 6 Nov 2015 10:29:14 -0500
Bruce James Fields <bfields@fieldses.org> wrote:

> On Fri, Nov 06, 2015 at 09:19:47AM -0500, Jeff Layton wrote:
> > On Fri, 6 Nov 2015 09:03:50 -0500
> > Trond Myklebust <trond.myklebust@primarydata.com> wrote:
> > 
> > > On Fri, Nov 6, 2015 at 8:48 AM, Trond Myklebust
> > > <trond.myklebust@primarydata.com> wrote:
> > > > On Fri, Nov 6, 2015 at 8:08 AM, Andrew W Elble <aweits@rit.edu> wrote:
> > > >>
> > > >>> Umm... If the client is sending delegreturn, then why not destroy the
> > > >>> delegation?
> 
> Yeah, good question, I didn't think about that.
> 
> The one thing that bothers me:
> 
> > Hmm...so is there any advantage to reporting NFS4ERR_DELEG_REVOKED
> > there at all? I guess that could be a signal that it may not have held
> > a delegation that it thought it had,
> 
> Yes, I'd like to think a little about this.  It does worry me that the
> loss of a delegation could be completely silent.
> 
> Even in the absence of locks, userspace may now have gotten incorrect
> lookup or stat results.  (Unless the client is careful not to depend on
> delegations for any guarantees that go beyond close-to-open?)
> 
> So,
> 
> 	client1		client2
> 	-------		-------
> 
> 			make prog
> 					(client2 gets delegation on prog.c)
> 
> 	vi prog.c
> 					(client2's deleg recalled, then
> 					revoked)
> 
> 			wait for ac timeout
> 			make prog
> 			make: 'prog' is up to date
> 
> Hm, I think the client has to see a STATUS_RECALLABLE_STATE_REVOKED at
> some point here, though.
> 
> In fact, is there any situation this could happen without the client
> having seeing that flag on the SEQUENCE preceding this op?  I guess
> there could be an extremely narrow window for a race between the two
> ops.  Is that race is the only justification for having this error in
> the protocol at all?  In the typical case it's going to see the sequence
> flag at the same time as the error and have to do some kind of recovery
> anyway.
> 

Yes, I think a revoke would have to race in between the SEQUENCE and
DELEGRETURN for it to happen.

> > but it's probably too late to do anything about it if that occurs.
> 
> Yeah, I can't think what the client could do beyond log something
> scary.
> 
> > Some older clients may also not handle that error gracefully too,
> 
> Is it likely?  (How's the current client?)  Not sure what ungraceful
> handling would be, though, it would be an odd client that would do
> anything other than either FREE_STATEID or just give up.
> 

I think the current client will handle it sanely. I believe we detach
the delegation from the inode before issuing a delegreturn, so we
shouldn't end up triggering a new one from nfs4_handle_exception (which
was my main concern).

> I don't see any reason this case needs to be optimized, so if clients
> handle the REVOKED error then maybe it's OK.
> 
> > so just returning NFS4_OK might be best...
> 
> Anyway, after all that, yes, maybe so, if only out of conservatism
> about change.
> 

Yeah, that might be best, at least until we can come up with a reason
to change it.

-- 
Jeff Layton <jeff.layton@primarydata.com>