Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Nagendra Tomar <Nagendra.Tomar@microsoft.com>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Trond Myklebust <trond.myklebust@hammerspace.com>
Subject: Re: [RFC] nconnect xprt stickiness for a file
Thread-Topic: [RFC] nconnect xprt stickiness for a file
Thread-Index: AdcbmG3Wy+rLrgPySr2+3WoogOFJygAZiM4A
Date:   Thu, 18 Mar 2021 13:57:01 +0000
Message-ID: <FFBB2134-E8A6-46A7-ADA0-5E222DC11620@oracle.com>
References: <KU1P153MB01977184017092050048033F9E699@KU1P153MB0197.APCP153.PROD.OUTLOOK.COM>
In-Reply-To: <KU1P153MB01977184017092050048033F9E699@KU1P153MB0197.APCP153.PROD.OUTLOOK.COM>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <C70446C44A84234EAADEFED1E3834974@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 3181c432-7cd1-4fa3-c423-08d8ea15b486
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Mar 2021 13:57:01.6460
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: kOY6LJ2Lf7nLeNB/n18woQvfVLY2xzHNtWZ4e/ddP4WrzUU/kDGRA+h2O9sCK6ahzqRoY1k2b4ZzCUWc7nYZxg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB2440
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9927 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 spamscore=0 bulkscore=0
 malwarescore=0 adultscore=0 mlxscore=0 mlxlogscore=999 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000
 definitions=main-2103180103
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9927 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxlogscore=999
 spamscore=0 mlxscore=0 bulkscore=0 suspectscore=0 priorityscore=1501
 lowpriorityscore=0 clxscore=1011 adultscore=0 phishscore=0 impostorscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000
 definitions=main-2103180103
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Mar 17, 2021, at 9:56 PM, Nagendra Tomar <Nagendra.Tomar@microsoft.com=
> wrote:
>=20
> We have a clustered NFS server behind a L4 load-balancer with the followi=
ng
> Characteristics (relevant to this discussion):
>=20
> 1. RPC requests for the same file issued to different cluster nodes are n=
ot efficient.
>    One file one cluster node is efficient. This is particularly true for =
WRITEs.
> 2. Multiple nconnect xprts land on different cluster nodes due to the sou=
rce=20
>    port being different for all.
>=20
> Due to this, the default nconnect roundrobin policy does not work very we=
ll as
> it results in RPCs targeted to the same file to be serviced by different =
cluster nodes.
>=20
> To solve this, we tweaked the nfs multipath code to always choose the sam=
e xprt=20
> for the same file. We do that by adding a new integer field to rpc_messag=
e,
> rpc_xprt_hint, which is set by NFS layer and used by RPC layer to pick a =
xprt.
> NFS layer sets it to the hash of the target file's filehandle, thus ensur=
ing same file
> requests always use the same xprt. This works well.
>=20
> I am interested in knowing your thoughts on this, has anyone else also co=
me across
> similar issue, is there any other way of solving this, etc.

Would a pNFS file layout work? The MDS could direct I/O for
a particular file to a specific DS.


> Following  patch is just to demonstrate the idea. It works but covers onl=
y what I
> needed for the experiment. Based on the feedback, I can follow it up with=
 a formal=20
> patch if needed.
>=20
> Thanks,
> Tomar
>=20
> ---
> diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
> index 5c4e23abc..8f1cf03dc 100644
> --- a/fs/nfs/nfs3proc.c
> +++ b/fs/nfs/nfs3proc.c
> @@ -108,6 +108,7 @@ nfs3_proc_getattr(struct nfs_server *server, struct n=
fs_fh *fhandle,
> 		.rpc_proc	=3D &nfs3_procedures[NFS3PROC_GETATTR],
> 		.rpc_argp	=3D fhandle,
> 		.rpc_resp	=3D fattr,
> +		.rpc_xprt_hint	=3D nfs_fh_hash(fhandle),
> 	};
> 	int	status;
> 	unsigned short task_flags =3D 0;
> @@ -136,6 +137,7 @@ nfs3_proc_setattr(struct dentry *dentry, struct nfs_f=
attr *fattr,
> 		.rpc_proc	=3D &nfs3_procedures[NFS3PROC_SETATTR],
> 		.rpc_argp	=3D &arg,
> 		.rpc_resp	=3D fattr,
> +		.rpc_xprt_hint	=3D nfs_fh_hash(arg.fh),
> 	};
> 	int	status;
>=20
> @@ -171,6 +173,7 @@ __nfs3_proc_lookup(struct inode *dir, const char *nam=
e, size_t len,
> 		.rpc_proc	=3D &nfs3_procedures[NFS3PROC_LOOKUP],
> 		.rpc_argp	=3D &arg,
> 		.rpc_resp	=3D &res,
> +		.rpc_xprt_hint	=3D nfs_fh_hash(arg.fh),
> 	};
> 	int			status;
>=20
> @@ -235,6 +238,7 @@ static int nfs3_proc_access(struct inode *inode, stru=
ct nfs_access_entry *entry)
> 		.rpc_argp	=3D &arg,
> 		.rpc_resp	=3D &res,
> 		.rpc_cred	=3D entry->cred,
> +		.rpc_xprt_hint	=3D nfs_fh_hash(arg.fh),
> 	};
> 	int status =3D -ENOMEM;
>=20
> @@ -266,6 +270,7 @@ static int nfs3_proc_readlink(struct inode *inode, st=
ruct page *page,
> 	struct rpc_message msg =3D {
> 		.rpc_proc	=3D &nfs3_procedures[NFS3PROC_READLINK],
> 		.rpc_argp	=3D &args,
> +		.rpc_xprt_hint	=3D nfs_fh_hash(args.fh),
> 	};
> 	int status =3D -ENOMEM;
>=20
> @@ -355,6 +360,7 @@ nfs3_proc_create(struct inode *dir, struct dentry *de=
ntry, struct iattr *sattr,
> 	data->arg.create.name =3D dentry->d_name.name;
> 	data->arg.create.len =3D dentry->d_name.len;
> 	data->arg.create.sattr =3D sattr;
> +	data->msg.rpc_xprt_hint =3D nfs_fh_hash(data->arg.create.fh);
>=20
> 	data->arg.create.createmode =3D NFS3_CREATE_UNCHECKED;
> 	if (flags & O_EXCL) {
> @@ -442,6 +448,7 @@ nfs3_proc_remove(struct inode *dir, struct dentry *de=
ntry)
> 		.rpc_proc =3D &nfs3_procedures[NFS3PROC_REMOVE],
> 		.rpc_argp =3D &arg,
> 		.rpc_resp =3D &res,
> +		.rpc_xprt_hint =3D nfs_fh_hash(arg.fh),
> 	};
> 	int status =3D -ENOMEM;
>=20
> @@ -524,6 +531,7 @@ nfs3_proc_link(struct inode *inode, struct inode *dir=
, const struct qstr *name)
> 		.rpc_proc	=3D &nfs3_procedures[NFS3PROC_LINK],
> 		.rpc_argp	=3D &arg,
> 		.rpc_resp	=3D &res,
> +		.rpc_xprt_hint	=3D nfs_fh_hash(arg.tofh),
> 	};
> 	int status =3D -ENOMEM;
>=20
> @@ -566,6 +574,7 @@ nfs3_proc_symlink(struct inode *dir, struct dentry *d=
entry, struct page *page,
> 	data->arg.symlink.pages =3D &page;
> 	data->arg.symlink.pathlen =3D len;
> 	data->arg.symlink.sattr =3D sattr;
> +	data->msg.rpc_xprt_hint =3D nfs_fh_hash(data->arg.symlink.fromfh);
>=20
> 	d_alias =3D nfs3_do_create(dir, dentry, data);
> 	status =3D PTR_ERR_OR_ZERO(d_alias);
> @@ -602,6 +611,7 @@ nfs3_proc_mkdir(struct inode *dir, struct dentry *den=
try, struct iattr *sattr)
> 	data->arg.mkdir.name =3D dentry->d_name.name;
> 	data->arg.mkdir.len =3D dentry->d_name.len;
> 	data->arg.mkdir.sattr =3D sattr;
> +	data->msg.rpc_xprt_hint =3D nfs_fh_hash(data->arg.mkdir.fh);
>=20
> 	d_alias =3D nfs3_do_create(dir, dentry, data);
> 	status =3D PTR_ERR_OR_ZERO(d_alias);
> @@ -636,6 +646,7 @@ nfs3_proc_rmdir(struct inode *dir, const struct qstr =
*name)
> 	struct rpc_message msg =3D {
> 		.rpc_proc	=3D &nfs3_procedures[NFS3PROC_RMDIR],
> 		.rpc_argp	=3D &arg,
> +		.rpc_xprt_hint	=3D nfs_fh_hash(arg.fh),
> 	};
> 	int status =3D -ENOMEM;
>=20
> @@ -682,6 +693,7 @@ static int nfs3_proc_readdir(struct nfs_readdir_arg *=
nr_arg,
> 		.rpc_argp	=3D &arg,
> 		.rpc_resp	=3D &res,
> 		.rpc_cred	=3D nr_arg->cred,
> +		.rpc_xprt_hint	=3D nfs_fh_hash(arg.fh),
> 	};
> 	int status =3D -ENOMEM;
>=20
> @@ -735,6 +747,7 @@ nfs3_proc_mknod(struct inode *dir, struct dentry *den=
try, struct iattr *sattr,
> 	data->arg.mknod.len =3D dentry->d_name.len;
> 	data->arg.mknod.sattr =3D sattr;
> 	data->arg.mknod.rdev =3D rdev;
> +	data->msg.rpc_xprt_hint =3D nfs_fh_hash(data->arg.mknod.fh);
>=20
> 	switch (sattr->ia_mode & S_IFMT) {
> 	case S_IFBLK:
> @@ -782,6 +795,7 @@ nfs3_proc_statfs(struct nfs_server *server, struct nf=
s_fh *fhandle,
> 		.rpc_proc	=3D &nfs3_procedures[NFS3PROC_FSSTAT],
> 		.rpc_argp	=3D fhandle,
> 		.rpc_resp	=3D stat,
> +		.rpc_xprt_hint	=3D nfs_fh_hash(fhandle),
> 	};
> 	int	status;
>=20
> @@ -800,6 +814,7 @@ do_proc_fsinfo(struct rpc_clnt *client, struct nfs_fh=
 *fhandle,
> 		.rpc_proc	=3D &nfs3_procedures[NFS3PROC_FSINFO],
> 		.rpc_argp	=3D fhandle,
> 		.rpc_resp	=3D info,
> +		.rpc_xprt_hint	=3D nfs_fh_hash(fhandle),
> 	};
> 	int	status;
>=20
> @@ -834,6 +849,7 @@ nfs3_proc_pathconf(struct nfs_server *server, struct =
nfs_fh *fhandle,
> 		.rpc_proc	=3D &nfs3_procedures[NFS3PROC_PATHCONF],
> 		.rpc_argp	=3D fhandle,
> 		.rpc_resp	=3D info,
> +		.rpc_xprt_hint	=3D nfs_fh_hash(fhandle),
> 	};
> 	int	status;
>=20
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index 78c9c4bde..60578e9fd 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -762,6 +762,7 @@ int nfs_initiate_pgio(struct rpc_clnt *clnt, struct n=
fs_pgio_header *hdr,
> 		.rpc_argp =3D &hdr->args,
> 		.rpc_resp =3D &hdr->res,
> 		.rpc_cred =3D cred,
> +		.rpc_xprt_hint =3D nfs_fh_hash(hdr->args.fh),
> 	};
> 	struct rpc_task_setup task_setup_data =3D {
> 		.rpc_client =3D clnt,
> diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
> index b27ebdcce..b1713d6fb 100644
> --- a/fs/nfs/unlink.c
> +++ b/fs/nfs/unlink.c
> @@ -355,6 +355,12 @@ nfs_async_rename(struct inode *old_dir, struct inode=
 *new_dir,
> 	msg.rpc_resp =3D &data->res;
> 	msg.rpc_cred =3D data->cred;
>=20
> +	if (data->args.new_dir)
> +		msg.rpc_xprt_hint =3D nfs_fh_hash(data->args.new_dir);
> +	else
> +		msg.rpc_xprt_hint =3D nfs_fh_hash(data->args.old_dir);
> +
> +
> 	/* set up nfs_renamedata */
> 	data->old_dir =3D old_dir;
> 	ihold(old_dir);
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index 639c34fec..284b364a6 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -1687,6 +1687,7 @@ int nfs_initiate_commit(struct rpc_clnt *clnt, stru=
ct nfs_commit_data *data,
> 		.rpc_argp =3D &data->args,
> 		.rpc_resp =3D &data->res,
> 		.rpc_cred =3D data->cred,
> +		.rpc_xprt_hint =3D nfs_fh_hash(data->args.fh),
> 	};
> 	struct rpc_task_setup task_setup_data =3D {
> 		.task =3D &data->task,
> diff --git a/include/linux/nfs.h b/include/linux/nfs.h
> index 0dc7ad38a..5d5b2d20b 100644
> --- a/include/linux/nfs.h
> +++ b/include/linux/nfs.h
> @@ -10,6 +10,7 @@
>=20
> #include <linux/sunrpc/msg_prot.h>
> #include <linux/string.h>
> +#include <linux/jhash.h>
> #include <uapi/linux/nfs.h>
>=20
> /*
> @@ -36,6 +37,10 @@ static inline void nfs_copy_fh(struct nfs_fh *target, =
const struct nfs_fh *sourc
> 	memcpy(target->data, source->data, source->size);
> }
>=20
> +static inline u32 nfs_fh_hash(const struct nfs_fh *fh)
> +{
> +	return (fh ? jhash(fh->data, fh->size, 0) : 0);
> +}
>=20
> /*
>  * This is really a general kernel constant, but since nothing like
> diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
> index df696efdd..8f365280c 100644
> --- a/include/linux/sunrpc/sched.h
> +++ b/include/linux/sunrpc/sched.h
> @@ -27,6 +27,7 @@ struct rpc_message {
> 	void *			rpc_argp;	/* Arguments */
> 	void *			rpc_resp;	/* Result */
> 	const struct cred *	rpc_cred;	/* Credentials */
> +	u32			rpc_xprt_hint;
> };
>=20
> struct rpc_call_ops;
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 612f0a641..fcf8e7962 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -1058,6 +1058,53 @@ rpc_task_get_next_xprt(struct rpc_clnt *clnt)
> 	return rpc_task_get_xprt(clnt, xprt_iter_get_next(&clnt->cl_xpi));
> }
>=20
> +static
> +bool xprt_is_active(const struct rpc_xprt *xprt)
> +{
> +	return kref_read(&xprt->kref) !=3D 0;
> +}
> +
> +static struct rpc_xprt *
> +rpc_task_get_hashed_xprt(struct rpc_clnt *clnt, const struct rpc_task *t=
ask)
> +{
> +	const struct rpc_xprt_switch *xps =3D NULL;
> +	struct rpc_xprt *xprt =3D NULL;
> +	const struct rpc_message *rpc_message =3D &task->tk_msg;
> +	const u32 hash =3D rpc_message->rpc_xprt_hint;
> +
> +	if (!hash)
> +		return rpc_task_get_next_xprt(clnt);
> +
> +	rcu_read_lock();
> +	xps =3D rcu_dereference(clnt->cl_xpi.xpi_xpswitch);
> +
> +	if (xps && hash) {
> +		const struct list_head *head =3D &xps->xps_xprt_list;
> +		struct rpc_xprt *pos;
> +		const u32 nactive =3D READ_ONCE(xps->xps_nactive);
> +		const u32 xprt_idx =3D (hash % nactive);
> +		u32 idx =3D 0;
> +
> +		list_for_each_entry_rcu(pos, head, xprt_switch) {
> +			if (xprt_idx > idx++)
> +				continue;
> +			if (xprt_is_active(pos)) {
> +				xprt =3D xprt_get(pos);
> +				break;
> +			}
> +		}
> +	}
> +
> +	/*
> +	 * Use first transport, if not found any.
> +	 */
> +	if (!xprt)
> +		xprt =3D xprt_get(rcu_dereference(clnt->cl_xprt));
> +	rcu_read_unlock();
> +
> +	return rpc_task_get_xprt(clnt, xprt);
> +}
> +
> static
> void rpc_task_set_transport(struct rpc_task *task, struct rpc_clnt *clnt)
> {
> @@ -1066,7 +1113,7 @@ void rpc_task_set_transport(struct rpc_task *task, =
struct rpc_clnt *clnt)
> 	if (task->tk_flags & RPC_TASK_NO_ROUND_ROBIN)
> 		task->tk_xprt =3D rpc_task_get_first_xprt(clnt);
> 	else
> -		task->tk_xprt =3D rpc_task_get_next_xprt(clnt);
> +		task->tk_xprt =3D rpc_task_get_hashed_xprt(clnt, task);
> }
>=20
> static
> @@ -1100,6 +1147,7 @@ rpc_task_set_rpc_message(struct rpc_task *task, con=
st struct rpc_message *msg)
> 		task->tk_msg.rpc_argp =3D msg->rpc_argp;
> 		task->tk_msg.rpc_resp =3D msg->rpc_resp;
> 		task->tk_msg.rpc_cred =3D msg->rpc_cred;
> +		task->tk_msg.rpc_xprt_hint =3D msg->rpc_xprt_hint;
> 		if (!(task->tk_flags & RPC_TASK_CRED_NOREF))
> 			get_cred(task->tk_msg.rpc_cred);
> 	}
> @@ -1130,8 +1178,8 @@ struct rpc_task *rpc_run_task(const struct rpc_task=
_setup *task_setup_data)
> 	if (!RPC_IS_ASYNC(task))
> 		task->tk_flags |=3D RPC_TASK_CRED_NOREF;
>=20
> -	rpc_task_set_client(task, task_setup_data->rpc_client);
> 	rpc_task_set_rpc_message(task, task_setup_data->rpc_message);
> +	rpc_task_set_client(task, task_setup_data->rpc_client);
>=20
> 	if (task->tk_action =3D=3D NULL)
> 		rpc_call_start(task);

--
Chuck Lever