Re: [PATCH 4/6] NFS: Have nfs41_proc_reclaim_complete() call nfs4_call_sync_custom()

[Trond Myklebust <trondmy@hammerspace.com>]

On Mon, 2019-09-02 at 11:11 +1000, NeilBrown wrote:
> On Mon, Aug 19 2019, Trond Myklebust wrote:
> 
> > On Mon, 2019-08-19 at 15:28 -0400, schumaker.anna@gmail.com wrote:
> > > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > > 
> > > An async call followed by an rpc_wait_for_completion() is
> > > basically
> > > the
> > > same as a synchronous call, so we can use nfs4_call_sync_custom()
> > > to
> > > keep our custom callback ops and the RPC_TASK_NO_ROUND_ROBIN
> > > flag.
> > > 
> > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > > ---
> > >  fs/nfs/nfs4proc.c | 13 ++-----------
> > >  1 file changed, 2 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > > index de2b3fd806ef..1b7863ec12d3 100644
> > > --- a/fs/nfs/nfs4proc.c
> > > +++ b/fs/nfs/nfs4proc.c
> > > @@ -8857,7 +8857,6 @@ static int
> > > nfs41_proc_reclaim_complete(struct
> > > nfs_client *clp,
> > >  		const struct cred *cred)
> > >  {
> > >  	struct nfs4_reclaim_complete_data *calldata;
> > > -	struct rpc_task *task;
> > >  	struct rpc_message msg = {
> > >  		.rpc_proc =
> > > &nfs4_procedures[NFSPROC4_CLNT_RECLAIM_COMPLETE],
> > >  		.rpc_cred = cred,
> > > @@ -8866,7 +8865,7 @@ static int
> > > nfs41_proc_reclaim_complete(struct
> > > nfs_client *clp,
> > >  		.rpc_client = clp->cl_rpcclient,
> > >  		.rpc_message = &msg,
> > >  		.callback_ops = &nfs4_reclaim_complete_call_ops,
> > > -		.flags = RPC_TASK_ASYNC | RPC_TASK_NO_ROUND_ROBIN,
> > > +		.flags = RPC_TASK_NO_ROUND_ROBIN,
> > >  	};
> > >  	int status = -ENOMEM;
> > >  
> > > @@ -8881,15 +8880,7 @@ static int
> > > nfs41_proc_reclaim_complete(struct
> > > nfs_client *clp,
> > >  	msg.rpc_argp = &calldata->arg;
> > >  	msg.rpc_resp = &calldata->res;
> > >  	task_setup_data.callback_data = calldata;
> > > -	task = rpc_run_task(&task_setup_data);
> > > -	if (IS_ERR(task)) {
> > > -		status = PTR_ERR(task);
> > > -		goto out;
> > > -	}
> > > -	status = rpc_wait_for_completion_task(task);
> > > -	if (status == 0)
> > > -		status = task->tk_status;
> > > -	rpc_put_task(task);
> > > +	status = nfs4_call_sync_custom(&task_setup_data);
> > >  out:
> > >  	dprintk("<-- %s status=%d\n", __func__, status);
> > >  	return status;
> > 
> > Hmm... I'm a little confused. Why does RECLAIM_COMPLETE need
> > RPC_TASK_NO_ROUND_ROBIN? It should be ordered so it is called after
> > BIND_CONN_TO_SESSION in nfs4_state_manager(), so in principle it is
> > supposed to be able to recover from an error like
> > NFS4ERR_CONN_NOT_BOUND_TO_SESSION. Are there other situations where
> > we
> > need RPC_TASK_NO_ROUND_ROBIN?
> 
> I thought it was conceptually simpler to keep *all* state management
> commands on the one connection.  It probably isn't strictly necessary
> as
> you say, but equally there is no need to distribute them over
> multiple
> connections.
> Having them all on the one connection might make analysing a packet
> trace easier...
> 

We do want BIND_CONN_TO_SESSION to be a part of the recovery process
where and when it is needed. If not, we end up having to catch a load
of NFS4ERR_CONN_NOT_BOUND_TO_SESSION errors once the recovery thread is
done, and having to then kick off a second recovery just to handle
those errors.

IOW: Deliberately suppressing those errors by trying to route all the
recovery through a single connection is actually not helpful.
Right now, we will catch those errors in the case where there is state
recovery to be performed (since those calls are allowed to be routed
through all connections) but it might be nice to also use
RECLAIM_COMPLETE as a canary for connection binding.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com