On Thu, 2023-08-31 at 11:15 -0400, Benjamin Coddington wrote:
> Expose a knob in sysfs to set the READDIR requested attributes for a
> non-plus READDIR request. This allows installations another option for
> tuning READDIR on v4. Further work is needed to detect whether enough
> attributes are being returned to also prime the dcache.
>
> Signed-off-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
> ---
> fs/nfs/client.c | 2 ++
> fs/nfs/nfs4client.c | 3 ++
> fs/nfs/nfs4proc.c | 1 +
> fs/nfs/nfs4xdr.c | 7 ++---
> fs/nfs/sysfs.c | 58 +++++++++++++++++++++++++++++++++++++++
> include/linux/nfs_fs_sb.h | 1 +
> include/linux/nfs_xdr.h | 1 +
> 7 files changed, 69 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index e4c5f193ed5e..cf23a7c54bf1 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -920,6 +920,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 d9114a754db7..ba1dffdd25eb 100644
> --- a/fs/nfs/nfs4client.c
> +++ b/fs/nfs/nfs4client.c
> @@ -1108,6 +1108,9 @@ static int nfs4_server_common_setup(struct nfs_server *server,
>
> nfs4_server_set_init_caps(server);
>
> + server->readdir_attrs[0] = 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 832fa226b8f2..12cc9e972f36 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5109,6 +5109,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 deec76cf5afe..1825e3eeb34b 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_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_TYPE|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..6bded395df18 100644
> --- a/fs/nfs/sysfs.c
> +++ b/fs/nfs/sysfs.c
> @@ -270,7 +270,59 @@ 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[24], *v;
> + size_t token = 0;
> + int i;
> +
> + if (count > 24)
> + return -EINVAL;
> +
> + v = strncpy(p, buf, count);
> +
> + for (i = 0; i < 3; i++) {
> + token += strcspn(v, " ") + 1;
> + if (token > 22)
> + return -EINVAL;
> +
> + p[token - 1] = '\0';
> + if (kstrtoint(v, 0, &attrs[i]))
> + return -EINVAL;
> + v = &p[token];
> + }
> +
> + 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 +377,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 20eeba8b009d..f37cc3fe140e 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -218,6 +218,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;
> };
>
This doesn't seem worthwhile to me. We have a clear reason to add
WORD0_TYPE to "basic" READDIR, which is that we want to be able to fill
out the d_type for getdents.
I don't see the same sort of rationale for fetching other attributes. It
would just be priming the inode cache with certain info. That could
useful for some workloads, but that seems like sort of a niche thing.
Adding more info also reduces the number of entries you can pack into a
READDIR reply, which is makes it easier to trigger cookie problems with
creates and deletes in large directories.
--
Jeff Layton <jlayton@xxxxxxxxxx>
