[PATCH 0/6] NFS: Add an nfs4_call_sync_custom() function

[PATCH 0/6] NFS: Add an nfs4_call_sync_custom() function

[schumaker.anna]

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

The nfs4_call_sync() function creates a default rpc_task_setup structure
that works for most, but not all, cases. We have some code duplication
in functions that can't use nfs4_call_sync(), so these patches aim to
help with that and make it easier to customize synchronous RPC calls in
the future

Anna.


Anna Schumaker (6):
  NFS: Add an nfs4_call_sync_custom() function
  NFS: Have nfs4_proc_setclientid() call nfs4_call_sync_custom()
  NFS: Have _nfs4_proc_secinfo() call nfs4_call_sync_custom()
  NFS: Have nfs41_proc_reclaim_complete() call nfs4_call_sync_custom()
  NFS: Have nfs41_proc_secinfo_no_name() call nfs4_call_sync_custom()
  NFS: Have nfs4_proc_get_lease_time() call nfs4_call_sync_custom()

 fs/nfs/nfs4proc.c | 106 +++++++++++++++++++++++++---------------------
 1 file changed, 57 insertions(+), 49 deletions(-)

-- 
2.22.1

[PATCH 1/6] NFS: Add an nfs4_call_sync_custom() function

[schumaker.anna]

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

There are a few cases where we need to manually configure the
rpc_task_setup structure to get the behavior we want.

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
 fs/nfs/nfs4proc.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 1406858bae6c..e5b6499c0b8b 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1073,14 +1073,26 @@ static const struct rpc_call_ops nfs40_call_sync_ops = {
 	.rpc_call_done = nfs40_call_sync_done,
 };
 
+static int nfs4_call_sync_custom(struct rpc_task_setup *task_setup)
+{
+	int ret;
+	struct rpc_task *task;
+
+	task = rpc_run_task(task_setup);
+	if (IS_ERR(task))
+		return PTR_ERR(task);
+
+	ret = task->tk_status;
+	rpc_put_task(task);
+	return ret;
+}
+
 static int nfs4_call_sync_sequence(struct rpc_clnt *clnt,
 				   struct nfs_server *server,
 				   struct rpc_message *msg,
 				   struct nfs4_sequence_args *args,
 				   struct nfs4_sequence_res *res)
 {
-	int ret;
-	struct rpc_task *task;
 	struct nfs_client *clp = server->nfs_client;
 	struct nfs4_call_sync_data data = {
 		.seq_server = server,
@@ -1094,14 +1106,7 @@ static int nfs4_call_sync_sequence(struct rpc_clnt *clnt,
 		.callback_data = &data
 	};
 
-	task = rpc_run_task(&task_setup);
-	if (IS_ERR(task))
-		ret = PTR_ERR(task);
-	else {
-		ret = task->tk_status;
-		rpc_put_task(task);
-	}
-	return ret;
+	return nfs4_call_sync_custom(&task_setup);
 }
 
 int nfs4_call_sync(struct rpc_clnt *clnt,
-- 
2.22.1

[PATCH 2/6] NFS: Have nfs4_proc_setclientid() call nfs4_call_sync_custom()

[schumaker.anna]

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

Rather than running the task manually

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
 fs/nfs/nfs4proc.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index e5b6499c0b8b..234312240f33 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6023,7 +6023,6 @@ int nfs4_proc_setclientid(struct nfs_client *clp, u32 program,
 		.rpc_resp = res,
 		.rpc_cred = cred,
 	};
-	struct rpc_task *task;
 	struct rpc_task_setup task_setup_data = {
 		.rpc_client = clp->cl_rpcclient,
 		.rpc_message = &msg,
@@ -6056,17 +6055,12 @@ int nfs4_proc_setclientid(struct nfs_client *clp, u32 program,
 	dprintk("NFS call  setclientid auth=%s, '%s'\n",
 		clp->cl_rpcclient->cl_auth->au_ops->au_name,
 		clp->cl_owner_id);
-	task = rpc_run_task(&task_setup_data);
-	if (IS_ERR(task)) {
-		status = PTR_ERR(task);
-		goto out;
-	}
-	status = task->tk_status;
+
+	status = nfs4_call_sync_custom(&task_setup_data);
 	if (setclientid.sc_cred) {
 		clp->cl_acceptor = rpcauth_stringify_acceptor(setclientid.sc_cred);
 		put_rpccred(setclientid.sc_cred);
 	}
-	rpc_put_task(task);
 out:
 	trace_nfs4_setclientid(clp, status);
 	dprintk("NFS reply setclientid: %d\n", status);
-- 
2.22.1

[PATCH 3/6] NFS: Have _nfs4_proc_secinfo() call nfs4_call_sync_custom()

[schumaker.anna]

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

We do this to set the RPC_TASK_NO_ROUND_ROBIN flag in the task_setup
structure

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
 fs/nfs/nfs4proc.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 234312240f33..de2b3fd806ef 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -7644,6 +7644,8 @@ int nfs4_proc_fsid_present(struct inode *inode, const struct cred *cred)
 static int _nfs4_proc_secinfo(struct inode *dir, const struct qstr *name, struct nfs4_secinfo_flavors *flavors, bool use_integrity)
 {
 	int status;
+	struct rpc_clnt *clnt = NFS_SERVER(dir)->client;
+	struct nfs_client *clp = NFS_SERVER(dir)->nfs_client;
 	struct nfs4_secinfo_arg args = {
 		.dir_fh = NFS_FH(dir),
 		.name   = name,
@@ -7656,26 +7658,37 @@ static int _nfs4_proc_secinfo(struct inode *dir, const struct qstr *name, struct
 		.rpc_argp = &args,
 		.rpc_resp = &res,
 	};
-	struct rpc_clnt *clnt = NFS_SERVER(dir)->client;
+	struct nfs4_call_sync_data data = {
+		.seq_server = NFS_SERVER(dir),
+		.seq_args = &args.seq_args,
+		.seq_res = &res.seq_res,
+	};
+	struct rpc_task_setup task_setup = {
+		.rpc_client = clnt,
+		.rpc_message = &msg,
+		.callback_ops = clp->cl_mvops->call_sync_ops,
+		.callback_data = &data,
+		.flags = RPC_TASK_NO_ROUND_ROBIN,
+	};
 	const struct cred *cred = NULL;
 
 	if (use_integrity) {
-		clnt = NFS_SERVER(dir)->nfs_client->cl_rpcclient;
-		cred = nfs4_get_clid_cred(NFS_SERVER(dir)->nfs_client);
+		clnt = clp->cl_rpcclient;
+		task_setup.rpc_client = clnt;
+
+		cred = nfs4_get_clid_cred(clp);
 		msg.rpc_cred = cred;
 	}
 
 	dprintk("NFS call  secinfo %s\n", name->name);
 
-	nfs4_state_protect(NFS_SERVER(dir)->nfs_client,
-		NFS_SP4_MACH_CRED_SECINFO, &clnt, &msg);
+	nfs4_state_protect(clp, NFS_SP4_MACH_CRED_SECINFO, &clnt, &msg);
+	nfs4_init_sequence(&args.seq_args, &res.seq_res, 0, 0);
+	status = nfs4_call_sync_custom(&task_setup);
 
-	status = nfs4_call_sync(clnt, NFS_SERVER(dir), &msg, &args.seq_args,
-				&res.seq_res, RPC_TASK_NO_ROUND_ROBIN);
 	dprintk("NFS reply  secinfo: %d\n", status);
 
 	put_cred(cred);
-
 	return status;
 }
 
-- 
2.22.1

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

[schumaker.anna]

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;
-- 
2.22.1

[PATCH 5/6] NFS: Have nfs41_proc_secinfo_no_name() call nfs4_call_sync_custom()

[schumaker.anna]

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

We need to use the custom rpc_task_setup here to set the
RPC_TASK_NO_ROUND_ROBIN flag on the RPC call.

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
 fs/nfs/nfs4proc.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 1b7863ec12d3..df12af8f6b36 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -9365,18 +9365,32 @@ _nfs41_proc_secinfo_no_name(struct nfs_server *server, struct nfs_fh *fhandle,
 		.rpc_resp = &res,
 	};
 	struct rpc_clnt *clnt = server->client;
+	struct nfs4_call_sync_data data = {
+		.seq_server = server,
+		.seq_args = &args.seq_args,
+		.seq_res = &res.seq_res,
+	};
+	struct rpc_task_setup task_setup = {
+		.rpc_client = server->client,
+		.rpc_message = &msg,
+		.callback_ops = server->nfs_client->cl_mvops->call_sync_ops,
+		.callback_data = &data,
+		.flags = RPC_TASK_NO_ROUND_ROBIN,
+	};
 	const struct cred *cred = NULL;
 	int status;
 
 	if (use_integrity) {
 		clnt = server->nfs_client->cl_rpcclient;
+		task_setup.rpc_client = clnt;
+
 		cred = nfs4_get_clid_cred(server->nfs_client);
 		msg.rpc_cred = cred;
 	}
 
 	dprintk("--> %s\n", __func__);
-	status = nfs4_call_sync(clnt, server, &msg, &args.seq_args,
-				&res.seq_res, RPC_TASK_NO_ROUND_ROBIN);
+	nfs4_init_sequence(&args.seq_args, &res.seq_res, 0, 0);
+	status = nfs4_call_sync_custom(&task_setup);
 	dprintk("<-- %s status=%d\n", __func__, status);
 
 	put_cred(cred);
-- 
2.22.1

[PATCH 6/6] NFS: Have nfs4_proc_get_lease_time() call nfs4_call_sync_custom()

[schumaker.anna]

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

This removes some code duplication, since both functions were doing the
same thing.

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
 fs/nfs/nfs4proc.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index df12af8f6b36..00c7a92e3d6b 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -8356,7 +8356,6 @@ static const struct rpc_call_ops nfs4_get_lease_time_ops = {
 
 int nfs4_proc_get_lease_time(struct nfs_client *clp, struct nfs_fsinfo *fsinfo)
 {
-	struct rpc_task *task;
 	struct nfs4_get_lease_time_args args;
 	struct nfs4_get_lease_time_res res = {
 		.lr_fsinfo = fsinfo,
@@ -8378,17 +8377,9 @@ int nfs4_proc_get_lease_time(struct nfs_client *clp, struct nfs_fsinfo *fsinfo)
 		.callback_data = &data,
 		.flags = RPC_TASK_TIMEOUT,
 	};
-	int status;
 
 	nfs4_init_sequence(&args.la_seq_args, &res.lr_seq_res, 0, 1);
-	task = rpc_run_task(&task_setup);
-
-	if (IS_ERR(task))
-		return PTR_ERR(task);
-
-	status = task->tk_status;
-	rpc_put_task(task);
-	return status;
+	return nfs4_call_sync_custom(&task_setup);
 }
 
 #ifdef CONFIG_NFS_V4_1
-- 
2.22.1

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

[Trond Myklebust]

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

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

[Anna Schumaker]

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

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

[NeilBrown]

[-- 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 --]

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

[Trond Myklebust]

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

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

[NeilBrown]

[-- 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 --]