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

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3997 bytes --]

On Mon, Sep 02 2019, Trond Myklebust wrote:

> 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.
>

Sounds reasonable.  Thanks for the explanation.  I certainly have no
objection.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]