diff mbox series

[v2,2/2] NFSv4: Allow per-mount tuning of READDIR attrs

Message ID fbd3cd01c28e8c132058ae16b22eeae5ddaa8178.1697722160.git.bcodding@redhat.com (mailing list archive)
State New, archived
Headers show
Series NFSv4 READDIR d_type fixup | expand

Commit Message

Benjamin Coddington Oct. 19, 2023, 1:34 p.m. UTC 
Expose a per-mount knob in sysfs to set the READDIR requested attributes
for a non-plus READDIR request.

For example:

  echo 0x800 0x800000 0x0 > /sys/fs/nfs/0\:57/v4_readdir_attrs

.. will revert the client to only request rdattr_error and
mounted_on_fileid for any non "plus" READDIR, as before the patch
preceeding this one in this series.  This provides existing installations
an option to fix a potential performance regression that may occur after
NFS clients update to request additional default READDIR attributes.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfs/client.c           |  4 ++
 fs/nfs/nfs4client.c       |  4 ++
 fs/nfs/nfs4proc.c         |  1 +
 fs/nfs/nfs4xdr.c          |  7 ++--
 fs/nfs/sysfs.c            | 86 +++++++++++++++++++++++++++++++++++++++
 include/linux/nfs_fs_sb.h |  1 +
 include/linux/nfs_xdr.h   |  1 +
 7 files changed, 100 insertions(+), 4 deletions(-)

Comments

Jeffrey Layton Oct. 19, 2023, 4:55 p.m. UTC | #1 
On Thu, 2023-10-19 at 09:34 -0400, Benjamin Coddington wrote:
> Expose a per-mount knob in sysfs to set the READDIR requested attributes
> for a non-plus READDIR request.
> 
> For example:
> 
>   echo 0x800 0x800000 0x0 > /sys/fs/nfs/0\:57/v4_readdir_attrs
> 
> .. will revert the client to only request rdattr_error and
> mounted_on_fileid for any non "plus" READDIR, as before the patch
> preceeding this one in this series.  This provides existing installations
> an option to fix a potential performance regression that may occur after
> NFS clients update to request additional default READDIR attributes.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/client.c           |  4 ++
>  fs/nfs/nfs4client.c       |  4 ++
>  fs/nfs/nfs4proc.c         |  1 +
>  fs/nfs/nfs4xdr.c          |  7 ++--
>  fs/nfs/sysfs.c            | 86 +++++++++++++++++++++++++++++++++++++++
>  include/linux/nfs_fs_sb.h |  1 +
>  include/linux/nfs_xdr.h   |  1 +
>  7 files changed, 100 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 44eca51b2808..981a2437b752 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -922,6 +922,10 @@ void nfs_server_copy_userdata(struct nfs_server *target, struct nfs_server *sour
>  	target->options = source->options;
>  	target->auth_info = source->auth_info;
>  	target->port = source->port;
> +#if IS_ENABLED(CONFIG_NFS_V4)
> +	memcpy(target->readdir_attrs, source->readdir_attrs,
> +			sizeof(target->readdir_attrs));
> +#endif
>  }
>  EXPORT_SYMBOL_GPL(nfs_server_copy_userdata);
>  
> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
> index 11e3a285594c..3bbfb4244c14 100644
> --- a/fs/nfs/nfs4client.c
> +++ b/fs/nfs/nfs4client.c
> @@ -1115,6 +1115,10 @@ static int nfs4_server_common_setup(struct nfs_server *server,
>  
>  	nfs4_server_set_init_caps(server);
>  
> +	/* Default (non-plus) v4 readdir attributes */
> +	server->readdir_attrs[0] = FATTR4_WORD0_TYPE|FATTR4_WORD0_RDATTR_ERROR;
> +	server->readdir_attrs[1] = FATTR4_WORD1_MOUNTED_ON_FILEID;
> +
>  	/* Probe the root fh to retrieve its FSID and filehandle */
>  	error = nfs4_get_rootfh(server, mntfh, auth_probe);
>  	if (error < 0)
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 7016eaadf555..0f0028de7941 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5113,6 +5113,7 @@ static int _nfs4_proc_readdir(struct nfs_readdir_arg *nr_arg,
>  		.pgbase = 0,
>  		.count = nr_arg->page_len,
>  		.plus = nr_arg->plus,
> +		.server = server,
>  	};
>  	struct nfs4_readdir_res res;
>  	struct rpc_message msg = {
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 7200d6f7cd7b..45a9b40b801e 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -1601,16 +1601,15 @@ static void encode_read(struct xdr_stream *xdr, const struct nfs_pgio_args *args
>  
>  static void encode_readdir(struct xdr_stream *xdr, const struct nfs4_readdir_arg *readdir, struct rpc_rqst *req, struct compound_hdr *hdr)
>  {
> -	uint32_t attrs[3] = {
> -		FATTR4_WORD0_TYPE|FATTR4_WORD0_RDATTR_ERROR,
> -		FATTR4_WORD1_MOUNTED_ON_FILEID,
> -	};
> +	uint32_t attrs[3];
>  	uint32_t dircount = readdir->count;
>  	uint32_t maxcount = readdir->count;
>  	__be32 *p, verf[2];
>  	uint32_t attrlen = 0;
>  	unsigned int i;
>  
> +	memcpy(attrs, readdir->server->readdir_attrs, sizeof(attrs));
> +
>  	if (readdir->plus) {
>  		attrs[0] |= FATTR4_WORD0_CHANGE|FATTR4_WORD0_SIZE|
>  			FATTR4_WORD0_FSID|FATTR4_WORD0_FILEHANDLE|FATTR4_WORD0_FILEID;
> diff --git a/fs/nfs/sysfs.c b/fs/nfs/sysfs.c
> index bf378ecd5d9f..03ada52aaee0 100644
> --- a/fs/nfs/sysfs.c
> +++ b/fs/nfs/sysfs.c
> @@ -272,6 +272,84 @@ shutdown_store(struct kobject *kobj, struct kobj_attribute *attr,
>  
>  static struct kobj_attribute nfs_sysfs_attr_shutdown = __ATTR_RW(shutdown);
>  
> +#if IS_ENABLED(CONFIG_NFS_V4)
> +static ssize_t
> +v4_readdir_attrs_show(struct kobject *kobj, struct kobj_attribute *attr,
> +				char *buf)
> +{
> +	struct nfs_server *server;
> +	server = container_of(kobj, struct nfs_server, kobj);
> +
> +	return sysfs_emit(buf, "0x%x 0x%x 0x%x\n",
> +			server->readdir_attrs[0],
> +			server->readdir_attrs[1],
> +			server->readdir_attrs[2]);
> +}
> +
> +static ssize_t
> +v4_readdir_attrs_store(struct kobject *kobj, struct kobj_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct nfs_server *server;
> +	u32 attrs[3];
> +	char p[36], *v;
> +	size_t token = 0;
> +	int i;
> +
> +	if (count > 36)
> +		return -EINVAL;
> +
> +	v = strncpy(p, buf, count);
> +
> +	for (i = 0; i < 3; i++) {
> +		token += strcspn(v, " ") + 1;
> +		if (token > 34)
> +			return -EINVAL;
> +
> +		p[token - 1] = '\0';
> +		if (kstrtoint(v, 0, &attrs[i]))
> +			return -EINVAL;
> +		v = &p[token];
> +	}
> +
> +	/* Allow only what we decode - see decode_getfattr_attrs() */
> +	if (attrs[0] & ~(FATTR4_WORD0_TYPE |
> +			FATTR4_WORD0_CHANGE |
> +			FATTR4_WORD0_SIZE |
> +			FATTR4_WORD0_FSID |
> +			FATTR4_WORD0_RDATTR_ERROR |
> +			FATTR4_WORD0_FILEHANDLE |
> +			FATTR4_WORD0_FILEID |
> +			FATTR4_WORD0_FS_LOCATIONS) ||
> +		attrs[1] & ~(FATTR4_WORD1_MODE |
> +			FATTR4_WORD1_NUMLINKS |
> +			FATTR4_WORD1_OWNER |
> +			FATTR4_WORD1_OWNER_GROUP |
> +			FATTR4_WORD1_RAWDEV |
> +			FATTR4_WORD1_SPACE_USED |
> +			FATTR4_WORD1_TIME_ACCESS |
> +			FATTR4_WORD1_TIME_METADATA |
> +			FATTR4_WORD1_TIME_MODIFY |
> +			FATTR4_WORD1_MOUNTED_ON_FILEID) ||
> +		attrs[2] & ~(FATTR4_WORD2_MDSTHRESHOLD |
> +			FATTR4_WORD2_SECURITY_LABEL))
> +		return -EINVAL;
> +
> +	server = container_of(kobj, struct nfs_server, kobj);
> +
> +	if (attrs[0])
> +		server->readdir_attrs[0] = attrs[0];
> +	if (attrs[1])
> +		server->readdir_attrs[1] = attrs[1];
> +	if (attrs[2])
> +		server->readdir_attrs[2] = attrs[2];
> +
> +	return count;
> +}
> +
> +static struct kobj_attribute nfs_sysfs_attr_v4_readdir_attrs = __ATTR_RW(v4_readdir_attrs);
> +#endif /* CONFIG_NFS_V4 */
> +
>  #define RPC_CLIENT_NAME_SIZE 64
>  
>  void nfs_sysfs_link_rpc_client(struct nfs_server *server,
> @@ -325,6 +403,14 @@ void nfs_sysfs_add_server(struct nfs_server *server)
>  	if (ret < 0)
>  		pr_warn("NFS: sysfs_create_file_ns for server-%d failed (%d)\n",
>  			server->s_sysfs_id, ret);
> +
> +#if IS_ENABLED(CONFIG_NFS_V4)
> +	ret = sysfs_create_file_ns(&server->kobj, &nfs_sysfs_attr_v4_readdir_attrs.attr,
> +				nfs_netns_server_namespace(&server->kobj));
> +	if (ret < 0)
> +		pr_warn("NFS: sysfs_create_file_ns for server-%d failed (%d)\n",
> +			server->s_sysfs_id, ret);
> +#endif /* IS_ENABLED(CONFIG_NFS_V4) */
>  }
>  EXPORT_SYMBOL_GPL(nfs_sysfs_add_server);
>  
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index cd628c4b011e..db87261b7cd7 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -219,6 +219,7 @@ struct nfs_server {
>  						   of change attribute, size, ctime
>  						   and mtime attributes supported by
>  						   the server */
> +	u32			readdir_attrs[3]; /* V4 tuneable default readdir attrs */
>  	u32			acl_bitmask;	/* V4 bitmask representing the ACEs
>  						   that are supported on this
>  						   filesystem */
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index 12bbb5c63664..e05d861b1788 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -1142,6 +1142,7 @@ struct nfs4_readdir_arg {
>  	struct page **			pages;	/* zero-copy data */
>  	unsigned int			pgbase;	/* zero-copy data */
>  	const u32 *			bitmask;
> +	const struct nfs_server		*server;
>  	bool				plus;
>  };
>  

If you are going to add this, then I think the consensus from
yesterday's comments was to put it behind a new Kconfig option. Maybe
there could be a new CONFIG_NFS_TWEAKS or something that exposes this
interface (and other stuff we're not sure about making generally
available).
Benjamin Coddington Oct. 19, 2023, 5:31 p.m. UTC | #2 
On 19 Oct 2023, at 12:55, Jeff Layton wrote:

> If you are going to add this, then I think the consensus from
> yesterday's comments was to put it behind a new Kconfig option. Maybe
> there could be a new CONFIG_NFS_TWEAKS or something that exposes this
> interface (and other stuff we're not sure about making generally
> available).

Right.  I was hoping the discussion would continue, and this version was
just to make the robot happy.  I'll send it again with it config-ed out in a
few days if there's no other discussion.

Ben
diff mbox series

Patch

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 44eca51b2808..981a2437b752 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -922,6 +922,10 @@  void nfs_server_copy_userdata(struct nfs_server *target, struct nfs_server *sour
 	target->options = source->options;
 	target->auth_info = source->auth_info;
 	target->port = source->port;
+#if IS_ENABLED(CONFIG_NFS_V4)
+	memcpy(target->readdir_attrs, source->readdir_attrs,
+			sizeof(target->readdir_attrs));
+#endif
 }
 EXPORT_SYMBOL_GPL(nfs_server_copy_userdata);
 
diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 11e3a285594c..3bbfb4244c14 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -1115,6 +1115,10 @@  static int nfs4_server_common_setup(struct nfs_server *server,
 
 	nfs4_server_set_init_caps(server);
 
+	/* Default (non-plus) v4 readdir attributes */
+	server->readdir_attrs[0] = FATTR4_WORD0_TYPE|FATTR4_WORD0_RDATTR_ERROR;
+	server->readdir_attrs[1] = FATTR4_WORD1_MOUNTED_ON_FILEID;
+
 	/* Probe the root fh to retrieve its FSID and filehandle */
 	error = nfs4_get_rootfh(server, mntfh, auth_probe);
 	if (error < 0)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 7016eaadf555..0f0028de7941 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5113,6 +5113,7 @@  static int _nfs4_proc_readdir(struct nfs_readdir_arg *nr_arg,
 		.pgbase = 0,
 		.count = nr_arg->page_len,
 		.plus = nr_arg->plus,
+		.server = server,
 	};
 	struct nfs4_readdir_res res;
 	struct rpc_message msg = {
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 7200d6f7cd7b..45a9b40b801e 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -1601,16 +1601,15 @@  static void encode_read(struct xdr_stream *xdr, const struct nfs_pgio_args *args
 
 static void encode_readdir(struct xdr_stream *xdr, const struct nfs4_readdir_arg *readdir, struct rpc_rqst *req, struct compound_hdr *hdr)
 {
-	uint32_t attrs[3] = {
-		FATTR4_WORD0_TYPE|FATTR4_WORD0_RDATTR_ERROR,
-		FATTR4_WORD1_MOUNTED_ON_FILEID,
-	};
+	uint32_t attrs[3];
 	uint32_t dircount = readdir->count;
 	uint32_t maxcount = readdir->count;
 	__be32 *p, verf[2];
 	uint32_t attrlen = 0;
 	unsigned int i;
 
+	memcpy(attrs, readdir->server->readdir_attrs, sizeof(attrs));
+
 	if (readdir->plus) {
 		attrs[0] |= FATTR4_WORD0_CHANGE|FATTR4_WORD0_SIZE|
 			FATTR4_WORD0_FSID|FATTR4_WORD0_FILEHANDLE|FATTR4_WORD0_FILEID;
diff --git a/fs/nfs/sysfs.c b/fs/nfs/sysfs.c
index bf378ecd5d9f..03ada52aaee0 100644
--- a/fs/nfs/sysfs.c
+++ b/fs/nfs/sysfs.c
@@ -272,6 +272,84 @@  shutdown_store(struct kobject *kobj, struct kobj_attribute *attr,
 
 static struct kobj_attribute nfs_sysfs_attr_shutdown = __ATTR_RW(shutdown);
 
+#if IS_ENABLED(CONFIG_NFS_V4)
+static ssize_t
+v4_readdir_attrs_show(struct kobject *kobj, struct kobj_attribute *attr,
+				char *buf)
+{
+	struct nfs_server *server;
+	server = container_of(kobj, struct nfs_server, kobj);
+
+	return sysfs_emit(buf, "0x%x 0x%x 0x%x\n",
+			server->readdir_attrs[0],
+			server->readdir_attrs[1],
+			server->readdir_attrs[2]);
+}
+
+static ssize_t
+v4_readdir_attrs_store(struct kobject *kobj, struct kobj_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct nfs_server *server;
+	u32 attrs[3];
+	char p[36], *v;
+	size_t token = 0;
+	int i;
+
+	if (count > 36)
+		return -EINVAL;
+
+	v = strncpy(p, buf, count);
+
+	for (i = 0; i < 3; i++) {
+		token += strcspn(v, " ") + 1;
+		if (token > 34)
+			return -EINVAL;
+
+		p[token - 1] = '\0';
+		if (kstrtoint(v, 0, &attrs[i]))
+			return -EINVAL;
+		v = &p[token];
+	}
+
+	/* Allow only what we decode - see decode_getfattr_attrs() */
+	if (attrs[0] & ~(FATTR4_WORD0_TYPE |
+			FATTR4_WORD0_CHANGE |
+			FATTR4_WORD0_SIZE |
+			FATTR4_WORD0_FSID |
+			FATTR4_WORD0_RDATTR_ERROR |
+			FATTR4_WORD0_FILEHANDLE |
+			FATTR4_WORD0_FILEID |
+			FATTR4_WORD0_FS_LOCATIONS) ||
+		attrs[1] & ~(FATTR4_WORD1_MODE |
+			FATTR4_WORD1_NUMLINKS |
+			FATTR4_WORD1_OWNER |
+			FATTR4_WORD1_OWNER_GROUP |
+			FATTR4_WORD1_RAWDEV |
+			FATTR4_WORD1_SPACE_USED |
+			FATTR4_WORD1_TIME_ACCESS |
+			FATTR4_WORD1_TIME_METADATA |
+			FATTR4_WORD1_TIME_MODIFY |
+			FATTR4_WORD1_MOUNTED_ON_FILEID) ||
+		attrs[2] & ~(FATTR4_WORD2_MDSTHRESHOLD |
+			FATTR4_WORD2_SECURITY_LABEL))
+		return -EINVAL;
+
+	server = container_of(kobj, struct nfs_server, kobj);
+
+	if (attrs[0])
+		server->readdir_attrs[0] = attrs[0];
+	if (attrs[1])
+		server->readdir_attrs[1] = attrs[1];
+	if (attrs[2])
+		server->readdir_attrs[2] = attrs[2];
+
+	return count;
+}
+
+static struct kobj_attribute nfs_sysfs_attr_v4_readdir_attrs = __ATTR_RW(v4_readdir_attrs);
+#endif /* CONFIG_NFS_V4 */
+
 #define RPC_CLIENT_NAME_SIZE 64
 
 void nfs_sysfs_link_rpc_client(struct nfs_server *server,
@@ -325,6 +403,14 @@  void nfs_sysfs_add_server(struct nfs_server *server)
 	if (ret < 0)
 		pr_warn("NFS: sysfs_create_file_ns for server-%d failed (%d)\n",
 			server->s_sysfs_id, ret);
+
+#if IS_ENABLED(CONFIG_NFS_V4)
+	ret = sysfs_create_file_ns(&server->kobj, &nfs_sysfs_attr_v4_readdir_attrs.attr,
+				nfs_netns_server_namespace(&server->kobj));
+	if (ret < 0)
+		pr_warn("NFS: sysfs_create_file_ns for server-%d failed (%d)\n",
+			server->s_sysfs_id, ret);
+#endif /* IS_ENABLED(CONFIG_NFS_V4) */
 }
 EXPORT_SYMBOL_GPL(nfs_sysfs_add_server);
 
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index cd628c4b011e..db87261b7cd7 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -219,6 +219,7 @@  struct nfs_server {
 						   of change attribute, size, ctime
 						   and mtime attributes supported by
 						   the server */
+	u32			readdir_attrs[3]; /* V4 tuneable default readdir attrs */
 	u32			acl_bitmask;	/* V4 bitmask representing the ACEs
 						   that are supported on this
 						   filesystem */
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 12bbb5c63664..e05d861b1788 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -1142,6 +1142,7 @@  struct nfs4_readdir_arg {
 	struct page **			pages;	/* zero-copy data */
 	unsigned int			pgbase;	/* zero-copy data */
 	const u32 *			bitmask;
+	const struct nfs_server		*server;
 	bool				plus;
 };