diff mbox series

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

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

Commit Message

Benjamin Coddington Oct. 17, 2023, 9:30 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           |  2 +
 fs/nfs/nfs4client.c       |  4 ++
 fs/nfs/nfs4proc.c         |  1 +
 fs/nfs/nfs4xdr.c          |  7 ++--
 fs/nfs/sysfs.c            | 81 +++++++++++++++++++++++++++++++++++++++
 include/linux/nfs_fs_sb.h |  1 +
 include/linux/nfs_xdr.h   |  1 +
 7 files changed, 93 insertions(+), 4 deletions(-)

Comments

Chuck Lever Oct. 18, 2023, 12:56 p.m. UTC | #1
On Tue, Oct 17, 2023 at 05:30:44PM -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           |  2 +
>  fs/nfs/nfs4client.c       |  4 ++
>  fs/nfs/nfs4proc.c         |  1 +
>  fs/nfs/nfs4xdr.c          |  7 ++--
>  fs/nfs/sysfs.c            | 81 +++++++++++++++++++++++++++++++++++++++
>  include/linux/nfs_fs_sb.h |  1 +
>  include/linux/nfs_xdr.h   |  1 +
>  7 files changed, 93 insertions(+), 4 deletions(-)

Admittedly, it would be much easier for humans to use if the API was
based on the symbolic names of the bits rather than a triplet of raw
hexadecimal values.


> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 44eca51b2808..e9aa1fd4335f 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -922,6 +922,8 @@ 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;
> +	memcpy(target->readdir_attrs, source->readdir_attrs,
> +			sizeof(target->readdir_attrs));
>  }
>  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..6d4f52bf47b3 100644
> --- a/fs/nfs/sysfs.c
> +++ b/fs/nfs/sysfs.c
> @@ -270,7 +270,82 @@ shutdown_store(struct kobject *kobj, struct kobj_attribute *attr,
>  	return count;
>  }
>  
> +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_shutdown = __ATTR_RW(shutdown);
> +static struct kobj_attribute nfs_sysfs_attr_v4_readdir_attrs = __ATTR_RW(v4_readdir_attrs);
>  
>  #define RPC_CLIENT_NAME_SIZE 64
>  
> @@ -325,6 +400,12 @@ 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);
> +
> +	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);
>  }
>  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;
>  };
>  
> -- 
> 2.41.0
>
Jeff Layton Oct. 18, 2023, 1:33 p.m. UTC | #2
On Wed, 2023-10-18 at 08:56 -0400, Chuck Lever wrote:
> On Tue, Oct 17, 2023 at 05:30:44PM -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           |  2 +
> >  fs/nfs/nfs4client.c       |  4 ++
> >  fs/nfs/nfs4proc.c         |  1 +
> >  fs/nfs/nfs4xdr.c          |  7 ++--
> >  fs/nfs/sysfs.c            | 81 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/nfs_fs_sb.h |  1 +
> >  include/linux/nfs_xdr.h   |  1 +
> >  7 files changed, 93 insertions(+), 4 deletions(-)
> 
> Admittedly, it would be much easier for humans to use if the API was
> based on the symbolic names of the bits rather than a triplet of raw
> hexadecimal values.
> 

I think there are some significant footguns with this interface. It'd be
very easy to set this wrong and get weird behavior.  OTOH, we could push
that complexity into userland and provide some sort of script in nfs-
utils for tuning this.

That said...

When we look at interfaces like this, we have to consider that they may
be around for a long, long time (decades, even), and people will come to
rely on them to do strange things that are difficult for us to support.
If we have someone saying that their READDIR performance slowed down, we
now have to grab those settings from this sysfs file and validate them
when trying to help them.

Personally, I'd prefer a simple binary "make it work the old way"
switch, if we're concerned about performance regressions here. I think
that's the sort of thing that is simple to explain to admins that are
suffering from this problem and (more importantly) the sort of setting
we can later remove when it's no longer needed.

Adding this sort of fine-grained knob will create more problems than it
solves, as people will (inevitably) use it incorrectly.

> 
> > diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> > index 44eca51b2808..e9aa1fd4335f 100644
> > --- a/fs/nfs/client.c
> > +++ b/fs/nfs/client.c
> > @@ -922,6 +922,8 @@ 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;
> > +	memcpy(target->readdir_attrs, source->readdir_attrs,
> > +			sizeof(target->readdir_attrs));
> >  }
> >  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..6d4f52bf47b3 100644
> > --- a/fs/nfs/sysfs.c
> > +++ b/fs/nfs/sysfs.c
> > @@ -270,7 +270,82 @@ shutdown_store(struct kobject *kobj, struct kobj_attribute *attr,
> >  	return count;
> >  }
> >  
> > +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_shutdown = __ATTR_RW(shutdown);
> > +static struct kobj_attribute nfs_sysfs_attr_v4_readdir_attrs = __ATTR_RW(v4_readdir_attrs);
> >  
> >  #define RPC_CLIENT_NAME_SIZE 64
> >  
> > @@ -325,6 +400,12 @@ 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);
> > +
> > +	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);
> >  }
> >  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;
> >  };
> >  
> > -- 
> > 2.41.0
> > 
>
Benjamin Coddington Oct. 18, 2023, 2:24 p.m. UTC | #3
On 18 Oct 2023, at 9:33, Jeff Layton wrote:

> On Wed, 2023-10-18 at 08:56 -0400, Chuck Lever wrote:
>> On Tue, Oct 17, 2023 at 05:30:44PM -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           |  2 +
>>>  fs/nfs/nfs4client.c       |  4 ++
>>>  fs/nfs/nfs4proc.c         |  1 +
>>>  fs/nfs/nfs4xdr.c          |  7 ++--
>>>  fs/nfs/sysfs.c            | 81 +++++++++++++++++++++++++++++++++++++++
>>>  include/linux/nfs_fs_sb.h |  1 +
>>>  include/linux/nfs_xdr.h   |  1 +
>>>  7 files changed, 93 insertions(+), 4 deletions(-)
>>
>> Admittedly, it would be much easier for humans to use if the API was
>> based on the symbolic names of the bits rather than a triplet of raw
>> hexadecimal values.

This isn't aiming to be an ease-of-use interface.  This is tinkering with
the innards of the client.  If you're doing this, you better know how to
convert between bases, because you're going to need that and more.

If we want to make it nice, patches to nfsctl can follow.

> I think there are some significant footguns with this interface. It'd be
> very easy to set this wrong and get weird behavior.  OTOH, we could push
> that complexity into userland and provide some sort of script in nfs-
> utils for tuning this.
>
> That said...
>
> When we look at interfaces like this, we have to consider that they may
> be around for a long, long time (decades, even), and people will come to
> rely on them to do strange things that are difficult for us to support.
> If we have someone saying that their READDIR performance slowed down, we
> now have to grab those settings from this sysfs file and validate them
> when trying to help them.
>
> Personally, I'd prefer a simple binary "make it work the old way"
> switch, if we're concerned about performance regressions here. I think
> that's the sort of thing that is simple to explain to admins that are
> suffering from this problem and (more importantly) the sort of setting
> we can later remove when it's no longer needed.
>
> Adding this sort of fine-grained knob will create more problems than it
> solves, as people will (inevitably) use it incorrectly.

I disagree that it will create more problems than it solves.

Also, sysfs isn't there for you to experiment with in production, and
sysadmins know this.  Sysfs is "_The_ filesystem for exporting kernel
objects".   There are plenty of ways to hose a system and corrupt data by
playing around with sysfs.

If we take the position that everything in NFS' sysfs must have a higher
standard of safety than even module parameters (see recover_lost_locks),
that means we better look at making every sysfs interface non-shoot-footy,
which is just insane.  Just take a look at a sampling of writeable files,
here's a couple:

/sys/block/sda/device/delete
/sys/kernel/sunrpc/xprt-switches/switch-1/xprt-0-local/dstaddr

Ben
Chuck Lever Oct. 18, 2023, 2:25 p.m. UTC | #4
On Wed, Oct 18, 2023 at 09:33:40AM -0400, Jeff Layton wrote:
> On Wed, 2023-10-18 at 08:56 -0400, Chuck Lever wrote:
> > On Tue, Oct 17, 2023 at 05:30:44PM -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           |  2 +
> > >  fs/nfs/nfs4client.c       |  4 ++
> > >  fs/nfs/nfs4proc.c         |  1 +
> > >  fs/nfs/nfs4xdr.c          |  7 ++--
> > >  fs/nfs/sysfs.c            | 81 +++++++++++++++++++++++++++++++++++++++
> > >  include/linux/nfs_fs_sb.h |  1 +
> > >  include/linux/nfs_xdr.h   |  1 +
> > >  7 files changed, 93 insertions(+), 4 deletions(-)
> > 
> > Admittedly, it would be much easier for humans to use if the API was
> > based on the symbolic names of the bits rather than a triplet of raw
> > hexadecimal values.
> > 
> 
> I think there are some significant footguns with this interface. It'd be
> very easy to set this wrong and get weird behavior.  OTOH, we could push
> that complexity into userland and provide some sort of script in nfs-
> utils for tuning this.
> 
> That said...
> 
> When we look at interfaces like this, we have to consider that they may
> be around for a long, long time (decades, even), and people will come to
> rely on them to do strange things that are difficult for us to support.
> If we have someone saying that their READDIR performance slowed down, we
> now have to grab those settings from this sysfs file and validate them
> when trying to help them.
> 
> Personally, I'd prefer a simple binary "make it work the old way"
> switch, if we're concerned about performance regressions here. I think
> that's the sort of thing that is simple to explain to admins that are
> suffering from this problem and (more importantly) the sort of setting
> we can later remove when it's no longer needed.
> 
> Adding this sort of fine-grained knob will create more problems than it
> solves, as people will (inevitably) use it incorrectly.

Totally agree with this assessment. It will get baked into wacky
knowledge base articles for decades. Voice of experience here ;-)

It's not yet clear sysadmins will even need a switch like this, so I
would go further and say you should hold off on merging anything
like it until there is an actual reported need for it.

Now, full control over that bitmap is still very neat thing for
experimentation by NFS developers. Hiding this behind a Kconfig
option would let you merge it but then turn it off in production
kernels.


> > > diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> > > index 44eca51b2808..e9aa1fd4335f 100644
> > > --- a/fs/nfs/client.c
> > > +++ b/fs/nfs/client.c
> > > @@ -922,6 +922,8 @@ 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;
> > > +	memcpy(target->readdir_attrs, source->readdir_attrs,
> > > +			sizeof(target->readdir_attrs));
> > >  }
> > >  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..6d4f52bf47b3 100644
> > > --- a/fs/nfs/sysfs.c
> > > +++ b/fs/nfs/sysfs.c
> > > @@ -270,7 +270,82 @@ shutdown_store(struct kobject *kobj, struct kobj_attribute *attr,
> > >  	return count;
> > >  }
> > >  
> > > +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_shutdown = __ATTR_RW(shutdown);
> > > +static struct kobj_attribute nfs_sysfs_attr_v4_readdir_attrs = __ATTR_RW(v4_readdir_attrs);
> > >  
> > >  #define RPC_CLIENT_NAME_SIZE 64
> > >  
> > > @@ -325,6 +400,12 @@ 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);
> > > +
> > > +	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);
> > >  }
> > >  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;
> > >  };
> > >  
> > > -- 
> > > 2.41.0
> > > 
> > 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
Chuck Lever Oct. 18, 2023, 2:33 p.m. UTC | #5
On Wed, Oct 18, 2023 at 10:24:18AM -0400, Benjamin Coddington wrote:
> On 18 Oct 2023, at 9:33, Jeff Layton wrote:
> 
> > On Wed, 2023-10-18 at 08:56 -0400, Chuck Lever wrote:
> >> On Tue, Oct 17, 2023 at 05:30:44PM -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           |  2 +
> >>>  fs/nfs/nfs4client.c       |  4 ++
> >>>  fs/nfs/nfs4proc.c         |  1 +
> >>>  fs/nfs/nfs4xdr.c          |  7 ++--
> >>>  fs/nfs/sysfs.c            | 81 +++++++++++++++++++++++++++++++++++++++
> >>>  include/linux/nfs_fs_sb.h |  1 +
> >>>  include/linux/nfs_xdr.h   |  1 +
> >>>  7 files changed, 93 insertions(+), 4 deletions(-)
> >>
> >> Admittedly, it would be much easier for humans to use if the API was
> >> based on the symbolic names of the bits rather than a triplet of raw
> >> hexadecimal values.
> 
> This isn't aiming to be an ease-of-use interface.  This is tinkering with
> the innards of the client.  If you're doing this, you better know how to
> convert between bases, because you're going to need that and more.
> 
> If we want to make it nice, patches to nfsctl can follow.

I don't see a reason this shouldn't be easier to use, especially
since mistakes in setting these bits have consequences. There are
currently 82 of them, after all.

But, OK, the polish can be applied by a user space tool.
Anna Schumaker Oct. 18, 2023, 6:38 p.m. UTC | #6
On Wed, Oct 18, 2023 at 10:25 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On Wed, Oct 18, 2023 at 09:33:40AM -0400, Jeff Layton wrote:
> > On Wed, 2023-10-18 at 08:56 -0400, Chuck Lever wrote:
> > > On Tue, Oct 17, 2023 at 05:30:44PM -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
> > > >

I understand why you're not adding a keyword for each attribute
requested in a readdir, but would it be possible to have a single
magic keyword like "reset" or "default" that is an alias for reverting
to the default attributes?

> > > > .. 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           |  2 +
> > > >  fs/nfs/nfs4client.c       |  4 ++
> > > >  fs/nfs/nfs4proc.c         |  1 +
> > > >  fs/nfs/nfs4xdr.c          |  7 ++--
> > > >  fs/nfs/sysfs.c            | 81 +++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/nfs_fs_sb.h |  1 +
> > > >  include/linux/nfs_xdr.h   |  1 +
> > > >  7 files changed, 93 insertions(+), 4 deletions(-)
> > >
> > > Admittedly, it would be much easier for humans to use if the API was
> > > based on the symbolic names of the bits rather than a triplet of raw
> > > hexadecimal values.
> > >
> >
> > I think there are some significant footguns with this interface. It'd be
> > very easy to set this wrong and get weird behavior.  OTOH, we could push
> > that complexity into userland and provide some sort of script in nfs-
> > utils for tuning this.
> >
> > That said...
> >
> > When we look at interfaces like this, we have to consider that they may
> > be around for a long, long time (decades, even), and people will come to
> > rely on them to do strange things that are difficult for us to support.
> > If we have someone saying that their READDIR performance slowed down, we
> > now have to grab those settings from this sysfs file and validate them
> > when trying to help them.
> >
> > Personally, I'd prefer a simple binary "make it work the old way"
> > switch, if we're concerned about performance regressions here. I think
> > that's the sort of thing that is simple to explain to admins that are
> > suffering from this problem and (more importantly) the sort of setting
> > we can later remove when it's no longer needed.
> >
> > Adding this sort of fine-grained knob will create more problems than it
> > solves, as people will (inevitably) use it incorrectly.
>
> Totally agree with this assessment. It will get baked into wacky
> knowledge base articles for decades. Voice of experience here ;-)
>
> It's not yet clear sysadmins will even need a switch like this, so I
> would go further and say you should hold off on merging anything
> like it until there is an actual reported need for it.
>
> Now, full control over that bitmap is still very neat thing for
> experimentation by NFS developers. Hiding this behind a Kconfig
> option would let you merge it but then turn it off in production
> kernels.

Definitely a neat thing to have, but I'm also in favor of hiding it
behind a kconfig option to start.

Anna

>
>
> > > > diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> > > > index 44eca51b2808..e9aa1fd4335f 100644
> > > > --- a/fs/nfs/client.c
> > > > +++ b/fs/nfs/client.c
> > > > @@ -922,6 +922,8 @@ 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;
> > > > + memcpy(target->readdir_attrs, source->readdir_attrs,
> > > > +                 sizeof(target->readdir_attrs));
> > > >  }
> > > >  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..6d4f52bf47b3 100644
> > > > --- a/fs/nfs/sysfs.c
> > > > +++ b/fs/nfs/sysfs.c
> > > > @@ -270,7 +270,82 @@ shutdown_store(struct kobject *kobj, struct kobj_attribute *attr,
> > > >   return count;
> > > >  }
> > > >
> > > > +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_shutdown = __ATTR_RW(shutdown);
> > > > +static struct kobj_attribute nfs_sysfs_attr_v4_readdir_attrs = __ATTR_RW(v4_readdir_attrs);
> > > >
> > > >  #define RPC_CLIENT_NAME_SIZE 64
> > > >
> > > > @@ -325,6 +400,12 @@ 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);
> > > > +
> > > > + 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);
> > > >  }
> > > >  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;
> > > >  };
> > > >
> > > > --
> > > > 2.41.0
> > > >
> > >
> >
> > --
> > Jeff Layton <jlayton@kernel.org>
>
> --
> Chuck Lever
Benjamin Coddington Oct. 18, 2023, 7:08 p.m. UTC | #7
On 18 Oct 2023, at 14:38, Anna Schumaker wrote:

> On Wed, Oct 18, 2023 at 10:25 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On Wed, Oct 18, 2023 at 09:33:40AM -0400, Jeff Layton wrote:
>>> On Wed, 2023-10-18 at 08:56 -0400, Chuck Lever wrote:
>>>> On Tue, Oct 17, 2023 at 05:30:44PM -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
>>>>>
>
> I understand why you're not adding a keyword for each attribute
> requested in a readdir, but would it be possible to have a single
> magic keyword like "reset" or "default" that is an alias for reverting
> to the default attributes?

Yes, it's possible.  But what happens when we change the defaults again?
"Reset" becomes meaningless after that.  That sort of sysfs addition is not
future-proof.  This file both shows the current and any future default set
of attributes being used on the client as well as allowing them to be
modified.

The only attributes that are allowed to be set are those that the client
would already request and properly decode in the readdir plus path.  The
foot-shooty space is the permutation of every combination of those 20
attributes, save the three cases we've been stomping on already: 1) the
non-plus case, 2) the new non-plus with type, and 3) the plus case with all
20 attributes.

I suppose I could test all those cases for weirdness, but I expect they'd
all "just work" for listing directories. (I have tested quite a few without
surprises.)  Perhaps some cases would expose assumptions in the attribute
cache on the client -- for example the client might expect _SIZE and
_SPACE_USED to always be updated in the same operation.  But, I don't expect
that to create devastating issues, and I really don't think anyone's going
to need to break the client by trying to ask for only _SIZE without
_SPACE_USED (all hypothetical).

Another way forward may be to just allow the addition or removal of _TYPE,
but the client I want to use allows me to request any of those attributes.
If this never ends up helping anyone, I'll eat my hat.

Ben
Benjamin Coddington Oct. 18, 2023, 7:17 p.m. UTC | #8
On 18 Oct 2023, at 14:38, Anna Schumaker wrote:
>> It's not yet clear sysadmins will even need a switch like this, so I
>> would go further and say you should hold off on merging anything
>> like it until there is an actual reported need for it.
>>
>> Now, full control over that bitmap is still very neat thing for
>> experimentation by NFS developers. Hiding this behind a Kconfig
>> option would let you merge it but then turn it off in production
>> kernels.
>
> Definitely a neat thing to have, but I'm also in favor of hiding it
> behind a kconfig option to start.

Ah, missed replying to this part in the last message:

Ok, if my last message isn't convicing enough, are we talking about
something like CONFIG_NFS_EXPERT_SYSFS?

I'm worried that trying to make NFS nicer/safer in sysfs just means we're
gatekeeping what's already a complex and breakable set of technologies in
the one place (sysfs) where we can actually expose some complexity.

Ben
kernel test robot Oct. 19, 2023, 12:06 p.m. UTC | #9
Hi Benjamin,

kernel test robot noticed the following build errors:

[auto build test ERROR on trondmy-nfs/linux-next]
[also build test ERROR on linus/master v6.6-rc6 next-20231019]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Benjamin-Coddington/NFSv4-Always-ask-for-type-with-READDIR/20231018-053217
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
patch link:    https://lore.kernel.org/r/bd900de1d19bc56e6df5b44379f373617acc894e.1697577945.git.bcodding%40redhat.com
patch subject: [PATCH 2/2] NFSv4: Allow per-mount tuning of READDIR attrs
config: nios2-defconfig (https://download.01.org/0day-ci/archive/20231019/202310191902.6BOby9rI-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231019/202310191902.6BOby9rI-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310191902.6BOby9rI-lkp@intel.com/

All errors (new ones prefixed by >>):

   fs/nfs/client.c: In function 'nfs_server_copy_userdata':
>> fs/nfs/client.c:925:22: error: 'struct nfs_server' has no member named 'readdir_attrs'
     925 |         memcpy(target->readdir_attrs, source->readdir_attrs,
         |                      ^~
   fs/nfs/client.c:925:45: error: 'struct nfs_server' has no member named 'readdir_attrs'
     925 |         memcpy(target->readdir_attrs, source->readdir_attrs,
         |                                             ^~
   fs/nfs/client.c:926:38: error: 'struct nfs_server' has no member named 'readdir_attrs'
     926 |                         sizeof(target->readdir_attrs));
         |                                      ^~
--
   fs/nfs/sysfs.c: In function 'v4_readdir_attrs_show':
>> fs/nfs/sysfs.c:281:31: error: 'struct nfs_server' has no member named 'readdir_attrs'
     281 |                         server->readdir_attrs[0],
         |                               ^~
   fs/nfs/sysfs.c:282:31: error: 'struct nfs_server' has no member named 'readdir_attrs'
     282 |                         server->readdir_attrs[1],
         |                               ^~
   fs/nfs/sysfs.c:283:31: error: 'struct nfs_server' has no member named 'readdir_attrs'
     283 |                         server->readdir_attrs[2]);
         |                               ^~
   fs/nfs/sysfs.c: In function 'v4_readdir_attrs_store':
   fs/nfs/sysfs.c:338:23: error: 'struct nfs_server' has no member named 'readdir_attrs'
     338 |                 server->readdir_attrs[0] = attrs[0];
         |                       ^~
   fs/nfs/sysfs.c:340:23: error: 'struct nfs_server' has no member named 'readdir_attrs'
     340 |                 server->readdir_attrs[1] = attrs[1];
         |                       ^~
   fs/nfs/sysfs.c:342:23: error: 'struct nfs_server' has no member named 'readdir_attrs'
     342 |                 server->readdir_attrs[2] = attrs[2];
         |                       ^~
   fs/nfs/sysfs.c: In function 'v4_readdir_attrs_show':
>> fs/nfs/sysfs.c:284:1: error: control reaches end of non-void function [-Werror=return-type]
     284 | }
         | ^
   cc1: some warnings being treated as errors


vim +925 fs/nfs/client.c

   908	
   909	/*
   910	 * Copy useful information when duplicating a server record
   911	 */
   912	void nfs_server_copy_userdata(struct nfs_server *target, struct nfs_server *source)
   913	{
   914		target->flags = source->flags;
   915		target->rsize = source->rsize;
   916		target->wsize = source->wsize;
   917		target->acregmin = source->acregmin;
   918		target->acregmax = source->acregmax;
   919		target->acdirmin = source->acdirmin;
   920		target->acdirmax = source->acdirmax;
   921		target->caps = source->caps;
   922		target->options = source->options;
   923		target->auth_info = source->auth_info;
   924		target->port = source->port;
 > 925		memcpy(target->readdir_attrs, source->readdir_attrs,
   926				sizeof(target->readdir_attrs));
   927	}
   928	EXPORT_SYMBOL_GPL(nfs_server_copy_userdata);
   929
kernel test robot Oct. 19, 2023, 12:18 p.m. UTC | #10
Hi Benjamin,

kernel test robot noticed the following build errors:

[auto build test ERROR on trondmy-nfs/linux-next]
[also build test ERROR on linus/master v6.6-rc6 next-20231019]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Benjamin-Coddington/NFSv4-Always-ask-for-type-with-READDIR/20231018-053217
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
patch link:    https://lore.kernel.org/r/bd900de1d19bc56e6df5b44379f373617acc894e.1697577945.git.bcodding%40redhat.com
patch subject: [PATCH 2/2] NFSv4: Allow per-mount tuning of READDIR attrs
config: powerpc-mpc885_ads_defconfig (https://download.01.org/0day-ci/archive/20231019/202310192058.OzHqCGKn-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231019/202310192058.OzHqCGKn-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310192058.OzHqCGKn-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/hardirq.h:11:
   In file included from arch/powerpc/include/asm/hardirq.h:6:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:672:
   arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
      47 | DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      48 |                  (p, b, c), pio, p)
         |                  ~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:669:3: note: expanded from macro 'DEF_PCI_AC_NORET'
     669 |                 __do_##name al;                                 \
         |                 ^~~~~~~~~~~~~~
   <scratch space>:98:1: note: expanded from here
      98 | __do_insl
         | ^
   arch/powerpc/include/asm/io.h:611:56: note: expanded from macro '__do_insl'
     611 | #define __do_insl(p, b, n)      readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
         |                                        ~~~~~~~~~~~~~~~~~~~~~^
   In file included from fs/nfs/client.c:19:
   In file included from include/linux/sunrpc/addr.h:14:
   In file included from include/net/ipv6.h:12:
   In file included from include/linux/ipv6.h:94:
   In file included from include/linux/tcp.h:17:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/powerpc/include/asm/hardirq.h:6:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:672:
   arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
      49 | DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      50 |                  (p, b, c), pio, p)
         |                  ~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:669:3: note: expanded from macro 'DEF_PCI_AC_NORET'
     669 |                 __do_##name al;                                 \
         |                 ^~~~~~~~~~~~~~
   <scratch space>:100:1: note: expanded from here
     100 | __do_outsb
         | ^
   arch/powerpc/include/asm/io.h:612:58: note: expanded from macro '__do_outsb'
     612 | #define __do_outsb(p, b, n)     writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
         |                                         ~~~~~~~~~~~~~~~~~~~~~^
   In file included from fs/nfs/client.c:19:
   In file included from include/linux/sunrpc/addr.h:14:
   In file included from include/net/ipv6.h:12:
   In file included from include/linux/ipv6.h:94:
   In file included from include/linux/tcp.h:17:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/powerpc/include/asm/hardirq.h:6:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:672:
   arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
      51 | DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      52 |                  (p, b, c), pio, p)
         |                  ~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:669:3: note: expanded from macro 'DEF_PCI_AC_NORET'
     669 |                 __do_##name al;                                 \
         |                 ^~~~~~~~~~~~~~
   <scratch space>:102:1: note: expanded from here
     102 | __do_outsw
         | ^
   arch/powerpc/include/asm/io.h:613:58: note: expanded from macro '__do_outsw'
     613 | #define __do_outsw(p, b, n)     writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
         |                                         ~~~~~~~~~~~~~~~~~~~~~^
   In file included from fs/nfs/client.c:19:
   In file included from include/linux/sunrpc/addr.h:14:
   In file included from include/net/ipv6.h:12:
   In file included from include/linux/ipv6.h:94:
   In file included from include/linux/tcp.h:17:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/powerpc/include/asm/hardirq.h:6:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:672:
   arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
      53 | DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      54 |                  (p, b, c), pio, p)
         |                  ~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:669:3: note: expanded from macro 'DEF_PCI_AC_NORET'
     669 |                 __do_##name al;                                 \
         |                 ^~~~~~~~~~~~~~
   <scratch space>:104:1: note: expanded from here
     104 | __do_outsl
         | ^
   arch/powerpc/include/asm/io.h:614:58: note: expanded from macro '__do_outsl'
     614 | #define __do_outsl(p, b, n)     writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
         |                                         ~~~~~~~~~~~~~~~~~~~~~^
>> fs/nfs/client.c:925:17: error: no member named 'readdir_attrs' in 'struct nfs_server'
     925 |         memcpy(target->readdir_attrs, source->readdir_attrs,
         |                ~~~~~~  ^
   fs/nfs/client.c:925:40: error: no member named 'readdir_attrs' in 'struct nfs_server'
     925 |         memcpy(target->readdir_attrs, source->readdir_attrs,
         |                                       ~~~~~~  ^
   fs/nfs/client.c:926:19: error: no member named 'readdir_attrs' in 'struct nfs_server'
     926 |                         sizeof(target->readdir_attrs));
         |                                ~~~~~~  ^
   6 warnings and 3 errors generated.
--
   In file included from fs/nfs/sysfs.c:11:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/powerpc/include/asm/hardirq.h:6:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:672:
   arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
      47 | DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      48 |                  (p, b, c), pio, p)
         |                  ~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:669:3: note: expanded from macro 'DEF_PCI_AC_NORET'
     669 |                 __do_##name al;                                 \
         |                 ^~~~~~~~~~~~~~
   <scratch space>:182:1: note: expanded from here
     182 | __do_insl
         | ^
   arch/powerpc/include/asm/io.h:611:56: note: expanded from macro '__do_insl'
     611 | #define __do_insl(p, b, n)      readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
         |                                        ~~~~~~~~~~~~~~~~~~~~~^
   In file included from fs/nfs/sysfs.c:11:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/powerpc/include/asm/hardirq.h:6:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:672:
   arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
      49 | DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      50 |                  (p, b, c), pio, p)
         |                  ~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:669:3: note: expanded from macro 'DEF_PCI_AC_NORET'
     669 |                 __do_##name al;                                 \
         |                 ^~~~~~~~~~~~~~
   <scratch space>:184:1: note: expanded from here
     184 | __do_outsb
         | ^
   arch/powerpc/include/asm/io.h:612:58: note: expanded from macro '__do_outsb'
     612 | #define __do_outsb(p, b, n)     writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
         |                                         ~~~~~~~~~~~~~~~~~~~~~^
   In file included from fs/nfs/sysfs.c:11:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/powerpc/include/asm/hardirq.h:6:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:672:
   arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
      51 | DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      52 |                  (p, b, c), pio, p)
         |                  ~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:669:3: note: expanded from macro 'DEF_PCI_AC_NORET'
     669 |                 __do_##name al;                                 \
         |                 ^~~~~~~~~~~~~~
   <scratch space>:186:1: note: expanded from here
     186 | __do_outsw
         | ^
   arch/powerpc/include/asm/io.h:613:58: note: expanded from macro '__do_outsw'
     613 | #define __do_outsw(p, b, n)     writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
         |                                         ~~~~~~~~~~~~~~~~~~~~~^
   In file included from fs/nfs/sysfs.c:11:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/powerpc/include/asm/hardirq.h:6:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:672:
   arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
      53 | DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      54 |                  (p, b, c), pio, p)
         |                  ~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:669:3: note: expanded from macro 'DEF_PCI_AC_NORET'
     669 |                 __do_##name al;                                 \
         |                 ^~~~~~~~~~~~~~
   <scratch space>:188:1: note: expanded from here
     188 | __do_outsl
         | ^
   arch/powerpc/include/asm/io.h:614:58: note: expanded from macro '__do_outsl'
     614 | #define __do_outsl(p, b, n)     writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
         |                                         ~~~~~~~~~~~~~~~~~~~~~^
>> fs/nfs/sysfs.c:281:12: error: no member named 'readdir_attrs' in 'struct nfs_server'
     281 |                         server->readdir_attrs[0],
         |                         ~~~~~~  ^
   fs/nfs/sysfs.c:282:12: error: no member named 'readdir_attrs' in 'struct nfs_server'
     282 |                         server->readdir_attrs[1],
         |                         ~~~~~~  ^
   fs/nfs/sysfs.c:283:12: error: no member named 'readdir_attrs' in 'struct nfs_server'
     283 |                         server->readdir_attrs[2]);
         |                         ~~~~~~  ^
   fs/nfs/sysfs.c:338:11: error: no member named 'readdir_attrs' in 'struct nfs_server'
     338 |                 server->readdir_attrs[0] = attrs[0];
         |                 ~~~~~~  ^
   fs/nfs/sysfs.c:340:11: error: no member named 'readdir_attrs' in 'struct nfs_server'
     340 |                 server->readdir_attrs[1] = attrs[1];
         |                 ~~~~~~  ^
   fs/nfs/sysfs.c:342:11: error: no member named 'readdir_attrs' in 'struct nfs_server'
     342 |                 server->readdir_attrs[2] = attrs[2];
         |                 ~~~~~~  ^
   6 warnings and 6 errors generated.


vim +925 fs/nfs/client.c

   908	
   909	/*
   910	 * Copy useful information when duplicating a server record
   911	 */
   912	void nfs_server_copy_userdata(struct nfs_server *target, struct nfs_server *source)
   913	{
   914		target->flags = source->flags;
   915		target->rsize = source->rsize;
   916		target->wsize = source->wsize;
   917		target->acregmin = source->acregmin;
   918		target->acregmax = source->acregmax;
   919		target->acdirmin = source->acdirmin;
   920		target->acdirmax = source->acdirmax;
   921		target->caps = source->caps;
   922		target->options = source->options;
   923		target->auth_info = source->auth_info;
   924		target->port = source->port;
 > 925		memcpy(target->readdir_attrs, source->readdir_attrs,
   926				sizeof(target->readdir_attrs));
   927	}
   928	EXPORT_SYMBOL_GPL(nfs_server_copy_userdata);
   929
Benjamin Coddington Oct. 19, 2023, 12:52 p.m. UTC | #11
On 19 Oct 2023, at 8:06, kernel test robot wrote:

> Hi Benjamin,
>
> kernel test robot noticed the following build errors:

Thanks robot.   I need to drop the sysfs code without CONFIG_NFS_V4.

I will send another version.

Ben
diff mbox series

Patch

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 44eca51b2808..e9aa1fd4335f 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -922,6 +922,8 @@  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;
+	memcpy(target->readdir_attrs, source->readdir_attrs,
+			sizeof(target->readdir_attrs));
 }
 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..6d4f52bf47b3 100644
--- a/fs/nfs/sysfs.c
+++ b/fs/nfs/sysfs.c
@@ -270,7 +270,82 @@  shutdown_store(struct kobject *kobj, struct kobj_attribute *attr,
 	return count;
 }
 
+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_shutdown = __ATTR_RW(shutdown);
+static struct kobj_attribute nfs_sysfs_attr_v4_readdir_attrs = __ATTR_RW(v4_readdir_attrs);
 
 #define RPC_CLIENT_NAME_SIZE 64
 
@@ -325,6 +400,12 @@  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);
+
+	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);
 }
 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;
 };