diff mbox series

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

Message ID 20190819192900.19312-5-Anna.Schumaker@Netapp.com (mailing list archive)
State New, archived
Headers show
Series NFS: Add an nfs4_call_sync_custom() function | expand

Commit Message

Anna Schumaker Aug. 19, 2019, 7:28 p.m. UTC 
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(-)

Comments

Trond Myklebust Aug. 19, 2019, 7:56 p.m. UTC | #1 
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?
Anna Schumaker Aug. 19, 2019, 8:02 p.m. UTC | #2 
See 5a0c257f8e0f4c4b3c33dff545317c21a921303e (NFS: send state
management on a single connection)

My understanding is that it forces all the state management calls into
a single connection during session startup, but before the extra
network connections are bound to the session.

Anna

On Mon, 2019-08-19 at 19:56 +0000, 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?
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
> 
>
NeilBrown Sept. 2, 2019, 1:11 a.m. UTC | #3 
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
Trond Myklebust Sept. 2, 2019, 4:54 p.m. UTC | #4 
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.
NeilBrown Sept. 3, 2019, 2:07 a.m. UTC | #5 
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
diff mbox series

Patch

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;