On Fri, 2018-10-05 at 13:44 +0000, Trond Myklebust wrote:
> On Fri, 2018-10-05 at 09:34 -0400, Andrew Elble wrote:
> > It's possible for DELEGRETURN to run into corner cases where a
> > delegation has been revoked by the server, but that fact is being
> > hidden by an error on the PUTFH/DELEGRETURN. So, in all error
> > cases where revoked status isn't immediately obvious, do a
> > TEST_STATEID to see if it is indeed revoked.
> >
> > Signed-off-by: Andrew Elble <aweits@xxxxxxx>
> > ---
> > fs/nfs/nfs4proc.c | 20 +++++++++++++++++++-
> > 1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 8220a168282e..3f38d281a814 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -386,6 +386,16 @@ static void nfs4_free_revoked_stateid(struct
> > nfs_server *server,
> > __nfs4_free_revoked_stateid(server, &tmp, cred);
> > }
> >
> > +static void nfs4_free_possibly_revoked_stateid(struct nfs_server
> > *server,
> > + const nfs4_stateid *stateid,
> > + struct rpc_cred *cred)
> > +{
> > + nfs4_stateid tmp;
> > +
> > + nfs4_stateid_copy(&tmp, stateid);
> > + nfs4_test_and_free_stateid(server, &tmp, cred);
> > +}
> > +
> > static long nfs4_update_delay(long *timeout)
> > {
> > long ret;
> > @@ -6035,9 +6045,13 @@ static void nfs4_delegreturn_done(struct
> > rpc_task *task, void *calldata)
> > nfs4_free_revoked_stateid(data->res.server,
> > data->args.stateid,
> > task->tk_msg.rpc_cred);
> > - /* Fallthrough */
> > + task->tk_status = 0;
> > + break;
> > case -NFS4ERR_BAD_STATEID:
> > case -NFS4ERR_STALE_STATEID:
> > + nfs4_free_possibly_revoked_stateid(data->res.server,
> > + data->args.stateid,
> > + task->tk_msg.rpc_cred);
> > task->tk_status = 0;
> > break;
> > case -NFS4ERR_OLD_STATEID:
> > @@ -6058,6 +6072,10 @@ static void nfs4_delegreturn_done(struct
> > rpc_task *task, void *calldata)
> > &exception);
> > if (exception.retry)
> > goto out_restart;
> > +
> > + nfs4_free_possibly_revoked_stateid(data->res.server,
> > + data->args.stateid,
> > + task->tk_msg.rpc_cred);
> > }
> > data->rpc_status = task->tk_status;
> > return;
>
> NACK. We don't ever want to run synchronous RPC calls from inside an
> rpciod context. There be deadlocks...
So, my question is why would we need to change nfs4_delegreturn_done at
all? It should already be sending a FREE_STATEID when the server
returns NFS4ERR_EXPIRED or NFS4ERR_DELEG_REVOKED thanks to the call to
nfs4_free_revoked_stateid().
If the server is returning anything other than those 2 errors for a
stateid that is pending a FREE_STATEID from the client, then that
server is broken.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
