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: 2871 bytes --]

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

Thanks,
NeilBrown


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

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