| Message ID | 1446674837-4980-1-git-send-email-aweits@rit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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
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? -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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...
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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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:
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(-)