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 >> Signed-off-by: NeilBrown >> --- >> 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;