Re: [PATCH 3/9] NFS: send state management on a single connection.

[NeilBrown <neilb@suse.com>]

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

On Tue, Jul 23 2019,  Schumaker, Anna  wrote:

> Hi Neil,
>
> On Thu, 2019-05-30 at 10:41 +1000, NeilBrown wrote:
>> With NFSv4.1, different network connections need to be explicitly
>> bound to a session.  During session startup, this is not possible
>> so only a single connection must be used for session startup.
>> 
>> So add a task flag to disable the default round-robin choice of
>> connections (when nconnect > 1) and force the use of a single
>> connection.
>> Then use that flag on all requests for session management - for
>> consistence, include NFSv4.0 management (SETCLIENTID) and session
>> destruction
>> 
>> Reported-by: Chuck Lever <chuck.lever@oracle.com>
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>  fs/nfs/nfs4proc.c            |   22 +++++++++++++---------
>>  include/linux/sunrpc/sched.h |    1 +
>>  net/sunrpc/clnt.c            |   24 +++++++++++++++++++++++-
>>  3 files changed, 37 insertions(+), 10 deletions(-)
>> 
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index c29cbef6b53f..22b3dbfc4fa1 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -5978,7 +5978,7 @@ int nfs4_proc_setclientid(struct nfs_client
>> *clp, u32 program,
>>  		.rpc_message = &msg,
>>  		.callback_ops = &nfs4_setclientid_ops,
>>  		.callback_data = &setclientid,
>> -		.flags = RPC_TASK_TIMEOUT,
>> +		.flags = RPC_TASK_TIMEOUT | RPC_TASK_NO_ROUND_ROBIN,
>>  	};
>>  	int status;
>>  
>> @@ -6044,7 +6044,8 @@ int nfs4_proc_setclientid_confirm(struct
>> nfs_client *clp,
>>  	dprintk("NFS call  setclientid_confirm auth=%s, (client ID
>> %llx)\n",
>>  		clp->cl_rpcclient->cl_auth->au_ops->au_name,
>>  		clp->cl_clientid);
>> -	status = rpc_call_sync(clp->cl_rpcclient, &msg,
>> RPC_TASK_TIMEOUT);
>> +	status = rpc_call_sync(clp->cl_rpcclient, &msg,
>> +			       RPC_TASK_TIMEOUT |
>> RPC_TASK_NO_ROUND_ROBIN);
>>  	trace_nfs4_setclientid_confirm(clp, status);
>>  	dprintk("NFS reply setclientid_confirm: %d\n", status);
>>  	return status;
>> @@ -7633,7 +7634,7 @@ static int _nfs4_proc_secinfo(struct inode
>> *dir, const struct qstr *name, struct
>>  		NFS_SP4_MACH_CRED_SECINFO, &clnt, &msg);
>>  
>>  	status = nfs4_call_sync(clnt, NFS_SERVER(dir), &msg,
>> &args.seq_args,
>> -				&res.seq_res, 0);
>> +				&res.seq_res, RPC_TASK_NO_ROUND_ROBIN);
>
> I'm confused about what setting RPC_TASK_NO_ROUND_ROBIN as the
> "cache_reply" argument to nfs4_call_sync() actually does. As far as I
> can tell, it's passed to nfs4_init_sequence() which sets it to the
> nfs4_sequence_args "sa_cache_this" field, which is a one bit boolean
> (defined in include/linux/nfs_xdr.h). So why pass the flag?

Thanks for reviewing my patch!  Yes, that is an error.
I think I had confused nfs4_call_sync() with rpc_call_sync().
Similar names, different args.

For those calls, I want to get RPC_TASK_NO_ROUND_ROBIN set in the .flags
field of 'task_setup' in nfs4_call_sync_sequence().

Maybe we could add a 'flags' arg to nfs4_call_sync_sequence() and
add a new nfs4_call_sync_state() which sets RPC_TASK_NO_ROUND_ROBIN.

If you are happy with this approach, let me know and I send a proper
patch.

Thanks,
NeilBrown

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 39896afc6edf..dd2725fe7a74 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1077,7 +1077,8 @@ 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)
+				   struct nfs4_sequence_res *res,
+				   int flags)
 {
 	int ret;
 	struct rpc_task *task;
@@ -1091,7 +1092,8 @@ static int nfs4_call_sync_sequence(struct rpc_clnt *clnt,
 		.rpc_client = clnt,
 		.rpc_message = msg,
 		.callback_ops = clp->cl_mvops->call_sync_ops,
-		.callback_data = &data
+		.callback_data = &data,
+		.flags = flags,
 	};
 
 	task = rpc_run_task(&task_setup);
@@ -1112,7 +1114,20 @@ int nfs4_call_sync(struct rpc_clnt *clnt,
 		   int cache_reply)
 {
 	nfs4_init_sequence(args, res, cache_reply, 0);
-	return nfs4_call_sync_sequence(clnt, server, msg, args, res);
+	return nfs4_call_sync_sequence(clnt, server, msg, args, res, 0);
+}
+
+int nfs4_call_sync_state(struct rpc_clnt *clnt,
+			 struct nfs_server *server,
+			 struct rpc_message *msg,
+			 struct nfs4_sequence_args *args,
+			 struct nfs4_sequence_res *res,
+			 int cache_reply)
+{
+	/* State management commands are never round-robined */
+	nfs4_init_sequence(args, res, cache_reply, 0);
+	return nfs4_call_sync_sequence(clnt, server, msg, args, res,
+				       RPC_TASK_NO_ROUND_ROBIN);
 }
 
 static void
@@ -7387,7 +7402,7 @@ static int _nfs40_proc_get_locations(struct inode *inode,
 
 	nfs4_init_sequence(&args.seq_args, &res.seq_res, 0, 1);
 	status = nfs4_call_sync_sequence(clnt, server, &msg,
-					&args.seq_args, &res.seq_res);
+					 &args.seq_args, &res.seq_res, 0);
 	if (status)
 		return status;
 
@@ -7440,7 +7455,7 @@ static int _nfs41_proc_get_locations(struct inode *inode,
 
 	nfs4_init_sequence(&args.seq_args, &res.seq_res, 0, 1);
 	status = nfs4_call_sync_sequence(clnt, server, &msg,
-					&args.seq_args, &res.seq_res);
+					 &args.seq_args, &res.seq_res, 0);
 	if (status == NFS4_OK &&
 	    res.seq_res.sr_status_flags & SEQ4_STATUS_LEASE_MOVED)
 		status = -NFS4ERR_LEASE_MOVED;
@@ -7529,7 +7544,7 @@ static int _nfs40_proc_fsid_present(struct inode *inode, const struct cred *cred
 
 	nfs4_init_sequence(&args.seq_args, &res.seq_res, 0, 1);
 	status = nfs4_call_sync_sequence(clnt, server, &msg,
-						&args.seq_args, &res.seq_res);
+					 &args.seq_args, &res.seq_res, 0);
 	nfs_free_fhandle(res.fh);
 	if (status)
 		return status;
@@ -7570,7 +7585,7 @@ static int _nfs41_proc_fsid_present(struct inode *inode, const struct cred *cred
 
 	nfs4_init_sequence(&args.seq_args, &res.seq_res, 0, 1);
 	status = nfs4_call_sync_sequence(clnt, server, &msg,
-						&args.seq_args, &res.seq_res);
+					 &args.seq_args, &res.seq_res, 0);
 	nfs_free_fhandle(res.fh);
 	if (status == NFS4_OK &&
 	    res.seq_res.sr_status_flags & SEQ4_STATUS_LEASE_MOVED)
@@ -7656,8 +7671,8 @@ static int _nfs4_proc_secinfo(struct inode *dir, const struct qstr *name, struct
 	nfs4_state_protect(NFS_SERVER(dir)->nfs_client,
 		NFS_SP4_MACH_CRED_SECINFO, &clnt, &msg);
 
-	status = nfs4_call_sync(clnt, NFS_SERVER(dir), &msg, &args.seq_args,
-				&res.seq_res, RPC_TASK_NO_ROUND_ROBIN);
+	status = nfs4_call_sync_state(clnt, NFS_SERVER(dir), &msg, &args.seq_args,
+				      &res.seq_res, 0);
 	dprintk("NFS reply  secinfo: %d\n", status);
 
 	put_cred(cred);
@@ -9357,8 +9372,8 @@ _nfs41_proc_secinfo_no_name(struct nfs_server *server, struct nfs_fh *fhandle,
 	}
 
 	dprintk("--> %s\n", __func__);
-	status = nfs4_call_sync(clnt, server, &msg, &args.seq_args,
-				&res.seq_res, RPC_TASK_NO_ROUND_ROBIN);
+	status = nfs4_call_sync_state(clnt, server, &msg, &args.seq_args,
+				      &res.seq_res, 0);
 	dprintk("<-- %s status=%d\n", __func__, status);
 
 	put_cred(cred);
@@ -9497,7 +9512,7 @@ static int _nfs41_test_stateid(struct nfs_server *server,
 	dprintk("NFS call  test_stateid %p\n", stateid);
 	nfs4_init_sequence(&args.seq_args, &res.seq_res, 0, 1);
 	status = nfs4_call_sync_sequence(rpc_client, server, &msg,
-			&args.seq_args, &res.seq_res);
+					 &args.seq_args, &res.seq_res, 0);
 	if (status != NFS_OK) {
 		dprintk("NFS reply test_stateid: failed, %d\n", status);
 		return status;

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