| Message ID | 20171103180631.76071-1-aweits@rit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Thanks for the quick turnaround! On Fri, 2017-11-03 at 14:06 -0400, Andrew Elble wrote: > If a delegation has been revoked by the server, operations using that > delegation should error out with NFS4ERR_DELEG_REVOKED in the >4.1 > case, and NFS4ERR_BAD_STATEID otherwise. > > Signed-off-by: Andrew Elble <aweits@rit.edu> Reviewed-by: Trond Myklebust <trond.myklebust@primarydata.com> > --- > v2: deconflicting with Trond's OPEN/CLOSE locking work > v3: don't return NFS4_OK on DELEGRETURN with revoked delegation > > fs/nfsd/nfs4state.c | 25 ++++++++++++++++++++++++- > 1 file changed, 24 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 0c04f81aa63b..d386d569edbc 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -3966,7 +3966,8 @@ static struct nfs4_delegation > *find_deleg_stateid(struct nfs4_client *cl, statei > { > struct nfs4_stid *ret; > > - ret = find_stateid_by_type(cl, s, NFS4_DELEG_STID); > + ret = find_stateid_by_type(cl, s, > + NFS4_DELEG_STID|NFS4_REVOKED_DELEG_S > TID); > if (!ret) > return NULL; > return delegstateid(ret); > @@ -3989,6 +3990,12 @@ static bool nfsd4_is_deleg_cur(struct > nfsd4_open *open) > deleg = find_deleg_stateid(cl, &open->op_delegate_stateid); > if (deleg == NULL) > goto out; > + if (deleg->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID) { > + nfs4_put_stid(&deleg->dl_stid); > + if (cl->cl_minorversion) > + status = nfserr_deleg_revoked; > + goto out; > + } > flags = share_access_to_flags(open->op_share_access); > status = nfs4_check_delegmode(deleg, flags); > if (status) { > @@ -4858,6 +4865,16 @@ static __be32 nfsd4_validate_stateid(struct > nfs4_client *cl, stateid_t *stateid) > struct nfs4_stid **s, struct nfsd_net *nn) > { > __be32 status; > + bool return_revoked = false; > + > + /* > + * only return revoked delegations if explicitly asked. > + * otherwise we report revoked or bad_stateid status. > + */ > + if (typemask & NFS4_REVOKED_DELEG_STID) > + return_revoked = true; > + else if (typemask & NFS4_DELEG_STID) > + typemask |= NFS4_REVOKED_DELEG_STID; > > if (ZERO_STATEID(stateid) || ONE_STATEID(stateid)) > return nfserr_bad_stateid; > @@ -4872,6 +4889,12 @@ static __be32 nfsd4_validate_stateid(struct > nfs4_client *cl, stateid_t *stateid) > *s = find_stateid_by_type(cstate->clp, stateid, typemask); > if (!*s) > return nfserr_bad_stateid; > + if (((*s)->sc_type == NFS4_REVOKED_DELEG_STID) && > !return_revoked) { > + nfs4_put_stid(*s); > + if (cstate->minorversion) > + return nfserr_deleg_revoked; > + return nfserr_bad_stateid; > + } > return nfs_ok; > } > -- Trond Myklebust Linux NFS client maintainer, PrimaryData trond.myklebust@primarydata.com
On Fri, Nov 03, 2017 at 06:36:55PM +0000, Trond Myklebust wrote: > Thanks for the quick turnaround! > > On Fri, 2017-11-03 at 14:06 -0400, Andrew Elble wrote: > > If a delegation has been revoked by the server, operations using that > > delegation should error out with NFS4ERR_DELEG_REVOKED in the >4.1 > > case, and NFS4ERR_BAD_STATEID otherwise. > > > > Signed-off-by: Andrew Elble <aweits@rit.edu> > > Reviewed-by: Trond Myklebust <trond.myklebust@primarydata.com> I wonder if there's a way to simplify the resulting logic in nfsd4_lookup_stateid--I guess I don't see anything. Could we get some context here in the changelog, though? What actual problem was this causing? --b. > > > --- > > v2: deconflicting with Trond's OPEN/CLOSE locking work > > v3: don't return NFS4_OK on DELEGRETURN with revoked delegation > > > > fs/nfsd/nfs4state.c | 25 ++++++++++++++++++++++++- > > 1 file changed, 24 insertions(+), 1 deletion(-) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index 0c04f81aa63b..d386d569edbc 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -3966,7 +3966,8 @@ static struct nfs4_delegation > > *find_deleg_stateid(struct nfs4_client *cl, statei > > { > > struct nfs4_stid *ret; > > > > - ret = find_stateid_by_type(cl, s, NFS4_DELEG_STID); > > + ret = find_stateid_by_type(cl, s, > > + NFS4_DELEG_STID|NFS4_REVOKED_DELEG_S > > TID); > > if (!ret) > > return NULL; > > return delegstateid(ret); > > @@ -3989,6 +3990,12 @@ static bool nfsd4_is_deleg_cur(struct > > nfsd4_open *open) > > deleg = find_deleg_stateid(cl, &open->op_delegate_stateid); > > if (deleg == NULL) > > goto out; > > + if (deleg->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID) { > > + nfs4_put_stid(&deleg->dl_stid); > > + if (cl->cl_minorversion) > > + status = nfserr_deleg_revoked; > > + goto out; > > + } > > flags = share_access_to_flags(open->op_share_access); > > status = nfs4_check_delegmode(deleg, flags); > > if (status) { > > @@ -4858,6 +4865,16 @@ static __be32 nfsd4_validate_stateid(struct > > nfs4_client *cl, stateid_t *stateid) > > struct nfs4_stid **s, struct nfsd_net *nn) > > { > > __be32 status; > > + bool return_revoked = false; > > + > > + /* > > + * only return revoked delegations if explicitly asked. > > + * otherwise we report revoked or bad_stateid status. > > + */ > > + if (typemask & NFS4_REVOKED_DELEG_STID) > > + return_revoked = true; > > + else if (typemask & NFS4_DELEG_STID) > > + typemask |= NFS4_REVOKED_DELEG_STID; > > > > if (ZERO_STATEID(stateid) || ONE_STATEID(stateid)) > > return nfserr_bad_stateid; > > @@ -4872,6 +4889,12 @@ static __be32 nfsd4_validate_stateid(struct > > nfs4_client *cl, stateid_t *stateid) > > *s = find_stateid_by_type(cstate->clp, stateid, typemask); > > if (!*s) > > return nfserr_bad_stateid; > > + if (((*s)->sc_type == NFS4_REVOKED_DELEG_STID) && > > !return_revoked) { > > + nfs4_put_stid(*s); > > + if (cstate->minorversion) > > + return nfserr_deleg_revoked; > > + return nfserr_bad_stateid; > > + } > > return nfs_ok; > > } > > > -- > Trond Myklebust > Linux NFS client maintainer, PrimaryData > trond.myklebust@primarydata.com -- 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
"bfields@fieldses.org" <bfields@fieldses.org> writes: > On Fri, Nov 03, 2017 at 06:36:55PM +0000, Trond Myklebust wrote: >> Thanks for the quick turnaround! >> >> On Fri, 2017-11-03 at 14:06 -0400, Andrew Elble wrote: >> > If a delegation has been revoked by the server, operations using that >> > delegation should error out with NFS4ERR_DELEG_REVOKED in the >4.1 >> > case, and NFS4ERR_BAD_STATEID otherwise. >> > >> > Signed-off-by: Andrew Elble <aweits@rit.edu> >> >> Reviewed-by: Trond Myklebust <trond.myklebust@primarydata.com> > > I wonder if there's a way to simplify the resulting logic in > nfsd4_lookup_stateid--I guess I don't see anything. > > Could we get some context here in the changelog, though? What actual > problem was this causing? Prior thread (roughly) here: http://www.spinics.net/lists/linux-nfs/msg55260.html This is the one patch I'm still carrying from the lost delegation work a while back. Testing showed that there is a path still open to lost delegations via delegreturn. running with: echo "error != 0" | sudo tee /sys/kernel/debug/tracing/events/nfs4/nfs4_delegreturn_exit/filter gave us this at one point with an interim version of this patch: kworker/0:0H-3990 [000] .... 5899655.609266: nfs4_delegreturn_exit: error=-10087 (DELEG_REVOKED) dev=00:30 fhandle=0xe43d9d3a kworker/0:2H-12665 [000] .... 5900011.719468: nfs4_delegreturn_exit: error=-10087 (DELEG_REVOKED) dev=00:30 fhandle=0x16e37c0a The Linux client prior to 26d36301bd653df6481fd38f3e1435a1f15e56d1 would just drop delegations that suffered from a nfserr_bad_stateid during delegreturn. Now it will do test/free if the return error is nfserr_deleg_revoked. If the client drops a delegation while the server has it on the revoked list, we stay stuck in endless state manager looping for that client. It might be a good idea for a stable backport of aforementioned commit, or some kind of other workaround? Possibly interpreting nfserr_bad_stateid an analogue of nfserr_deleg_revoked clientside when dealing with a >4.0 mount? (also seems like an error on the putfh might need to be caught as well?) Thanks, Andy
On Mon, Nov 06, 2017 at 12:30:23PM -0500, Andrew W Elble wrote: > Prior thread (roughly) here: http://www.spinics.net/lists/linux-nfs/msg55260.html > > This is the one patch I'm still carrying from the lost delegation work a > while back. Testing showed that there is a path still open to lost > delegations via delegreturn. > > running with: > echo "error != 0" | sudo tee /sys/kernel/debug/tracing/events/nfs4/nfs4_delegreturn_exit/filter > > gave us this at one point with an interim version of this patch: > > kworker/0:0H-3990 [000] .... 5899655.609266: nfs4_delegreturn_exit: > error=-10087 (DELEG_REVOKED) dev=00:30 fhandle=0xe43d9d3a > kworker/0:2H-12665 [000] .... 5900011.719468: nfs4_delegreturn_exit: > error=-10087 (DELEG_REVOKED) dev=00:30 fhandle=0x16e37c0a > > The Linux client prior to 26d36301bd653df6481fd38f3e1435a1f15e56d1 would > just drop delegations that suffered from a nfserr_bad_stateid during > delegreturn. Now it will do test/free if the return error is > nfserr_deleg_revoked. > > If the client drops a delegation while the server has it on the revoked > list, we stay stuck in endless state manager looping for that client. > > It might be a good idea for a stable backport of aforementioned commit, > or some kind of other workaround? Possibly interpreting > nfserr_bad_stateid an analogue of nfserr_deleg_revoked clientside > when dealing with a >4.0 mount? (also seems like an error on the putfh > might need to be caught as well?) I'm just looking for a concise explanation of why your patch is important. And I probably haven't dug enough, but I'm still not quite following. If I understand right, the only visible change from your patch will be returning DELEG_REVOKED instead of BAD_STATEID to 4.1 clients in some cases. I'm not clear what the result was (for old or new clients)--a client left believing it held a delegation when it didn't, or a client entering an infinite state manager loop? --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
"bfields@fieldses.org" <bfields@fieldses.org> writes: > I'm just looking for a concise explanation of why your patch is > important. And I probably haven't dug enough, but I'm still not quite > following. > > If I understand right, the only visible change from your patch will be > returning DELEG_REVOKED instead of BAD_STATEID to 4.1 clients in some > cases. I'm not clear what the result was (for old or new clients)--a > client left believing it held a delegation when it didn't, or a client > entering an infinite state manager loop? The current Linux client will "lose" a delegation on DELEGRETURN if it does not see NFS4ERR_DELEG_REVOKED. This is unrecoverable and will result in the client state manager looping unable to satisfy the server's inevitable assertion of SEQ4_STATUS_RECALLABLE_STATE_REVOKED. RFC5661 10.2.1: "A client normally finds out about revocation of a delegation when it uses a stateid associated with a delegation and receives one of the errors NFS4ERR_EXPIRED, NFS4ERR_ADMIN_REVOKED, or NFS4ERR_DELEG_REVOKED." Thanks, Andy
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 0c04f81aa63b..d386d569edbc 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -3966,7 +3966,8 @@ static struct nfs4_delegation *find_deleg_stateid(struct nfs4_client *cl, statei { struct nfs4_stid *ret; - ret = find_stateid_by_type(cl, s, NFS4_DELEG_STID); + ret = find_stateid_by_type(cl, s, + NFS4_DELEG_STID|NFS4_REVOKED_DELEG_STID); if (!ret) return NULL; return delegstateid(ret); @@ -3989,6 +3990,12 @@ static bool nfsd4_is_deleg_cur(struct nfsd4_open *open) deleg = find_deleg_stateid(cl, &open->op_delegate_stateid); if (deleg == NULL) goto out; + if (deleg->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID) { + nfs4_put_stid(&deleg->dl_stid); + if (cl->cl_minorversion) + status = nfserr_deleg_revoked; + goto out; + } flags = share_access_to_flags(open->op_share_access); status = nfs4_check_delegmode(deleg, flags); if (status) { @@ -4858,6 +4865,16 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid) struct nfs4_stid **s, struct nfsd_net *nn) { __be32 status; + bool return_revoked = false; + + /* + * only return revoked delegations if explicitly asked. + * otherwise we report revoked or bad_stateid status. + */ + if (typemask & NFS4_REVOKED_DELEG_STID) + return_revoked = true; + else if (typemask & NFS4_DELEG_STID) + typemask |= NFS4_REVOKED_DELEG_STID; if (ZERO_STATEID(stateid) || ONE_STATEID(stateid)) return nfserr_bad_stateid; @@ -4872,6 +4889,12 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid) *s = find_stateid_by_type(cstate->clp, stateid, typemask); if (!*s) return nfserr_bad_stateid; + if (((*s)->sc_type == NFS4_REVOKED_DELEG_STID) && !return_revoked) { + nfs4_put_stid(*s); + if (cstate->minorversion) + return nfserr_deleg_revoked; + return nfserr_bad_stateid; + } return nfs_ok; }
If a delegation has been revoked by the server, operations using that delegation should error out with NFS4ERR_DELEG_REVOKED in the >4.1 case, and NFS4ERR_BAD_STATEID otherwise. Signed-off-by: Andrew Elble <aweits@rit.edu> --- v2: deconflicting with Trond's OPEN/CLOSE locking work v3: don't return NFS4_OK on DELEGRETURN with revoked delegation fs/nfsd/nfs4state.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)