LinuxLists.cc - [PATCH 2/2] NFSv4: Allow per-mount tuning of READDIR attrs

2023-10-17 21:31:34

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

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 <[email protected]>
---
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(-)

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

2023-10-18 12:56:48

by Chuck Lever III

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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
>

--
Chuck Lever

2023-10-18 13:33:52

by Jeff Layton

[permalink] [raw]

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

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 <[email protected]>
> > ---
> > 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
> >
>

--
Jeff Layton <[email protected]>

2023-10-18 14:27:18

by Chuck Lever III

[permalink] [raw]

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

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 <[email protected]>
> > > ---
> > > 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 <[email protected]>

--
Chuck Lever

2023-10-18 14:27:26

by Benjamin Coddington

[permalink] [raw]

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

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 <[email protected]>
>>> ---
>>> 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

2023-10-18 14:36:03

by Chuck Lever III

[permalink] [raw]

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

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 <[email protected]>
> >>> ---
> >>> 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.

--
Chuck Lever

2023-10-18 18:39:20

by Anna Schumaker

[permalink] [raw]

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

On Wed, Oct 18, 2023 at 10:25 AM Chuck Lever <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
>
> --
> Chuck Lever

2023-10-18 19:09:58

by Benjamin Coddington

[permalink] [raw]

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

On 18 Oct 2023, at 14:38, Anna Schumaker wrote:

> On Wed, Oct 18, 2023 at 10:25 AM Chuck Lever <[email protected]> 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

2023-10-18 19:18:29

by Benjamin Coddington

[permalink] [raw]

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

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

2023-10-19 12:10:17

by kernel test robot

[permalink] [raw]

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

2023-10-19 12:18:24

by kernel test robot

[permalink] [raw]

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

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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

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

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-10-19 12:55:05

by Benjamin Coddington

[permalink] [raw]

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

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