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@xxxxxxxxx wrote:
>> > > From: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
>> > >
>> > > 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@xxxxxxxxxx>
>> > > ---
>> > > 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:
signature.asc
Description: PGP signature
