Re: [PATCH 1/3] NFS: Add implid to sysfs

Benjamin Coddington
2025-01-09T14:33:15Z

On 8 Jan 2025, at 16:36, Anna Schumaker wrote:

> From: Anna Schumaker <anna.schumaker@oracle.com>
>
> The Linux NFS server added support for returning this information during
> an EXCHANGE_ID in Linux v6.13. This is something and admin might want to
> query, so let's add it to sysfs.

I agree that admins will want to know (and they might not yet know they want
to know), good idea!

A couple comments..

> Signed-off-by: Anna Schumaker <anna.schumaker@oracle.com>
> ---
> fs/nfs/sysfs.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 79 insertions(+)
>
> diff --git a/fs/nfs/sysfs.c b/fs/nfs/sysfs.c
> index 7b59a40d40c0..6b82c92c45bf 100644
> --- a/fs/nfs/sysfs.c
> +++ b/fs/nfs/sysfs.c
> @@ -272,6 +272,32 @@ shutdown_store(struct kobject *kobj, struct kobj_attribute *attr,
>
> static struct kobj_attribute nfs_sysfs_attr_shutdown = __ATTR_RW(shutdown);
>
> +#if IS_ENABLED(CONFIG_NFS_V4_1)
> +static ssize_t
> +implid_domain_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + struct nfs_server *server = container_of(kobj, struct nfs_server, kobj);
> + struct nfs41_impl_id *impl_id = server->nfs_client->cl_implid;
> + return sysfs_emit(buf, "%s\n", impl_id->domain);
> +}
> +
> +static struct kobj_attribute nfs_sysfs_attr_implid_domain = __ATTR_RO(implid_domain);
> +
> +
> +static ssize_t
> +implid_name_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + struct nfs_server *server = container_of(kobj, struct nfs_server, kobj);
> + struct nfs41_impl_id *impl_id = server->nfs_client->cl_implid;
> + return sysfs_emit(buf, "%s\n", impl_id->name);
> +}
> +
> +static struct kobj_attribute nfs_sysfs_attr_implid_name = __ATTR_RO(implid_name);
> +
> +#endif /* IS_ENABLED(CONFIG_NFS_V4_1) */
> +
> #define RPC_CLIENT_NAME_SIZE 64
>
> void nfs_sysfs_link_rpc_client(struct nfs_server *server,
> @@ -309,6 +335,33 @@ static struct kobj_type nfs_sb_ktype = {
> .child_ns_type = nfs_netns_object_child_ns_type,
> };
>
> +#if IS_ENABLED(CONFIG_NFS_V4_1)
> +static void nfs_sysfs_add_nfsv41_server(struct nfs_server *server)
> +{
> + struct nfs_client *clp = server->nfs_client;
> + int ret;
> +
> + if (clp->cl_implid && strlen(clp->cl_implid->domain) > 0) {

Can we create the files and leave them empty if the strings are not set?
Having an empty file might be a nice prompt for an implementation to start
sending values. I also have a slight preference for a less dynamic sysfs.

> + ret = sysfs_create_file_ns(&server->kobj, &nfs_sysfs_attr_implid_domain.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);
> +
> + ret = sysfs_create_file_ns(&server->kobj, &nfs_sysfs_attr_implid_name.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);
> +
> + }
> +}
> +#else /* CONFIG_NFS_V4_1 */
> +static inline void nfs_sysfs_add_nfsv41_server(struct nfs_server *server)
> +{
> +}
> +#endif /* CONFIG_NFS_V4_1 */
> +
> void nfs_sysfs_add_server(struct nfs_server *server)
> {
> int ret;
> @@ -325,6 +378,32 @@ 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);
> +
> + nfs_sysfs_add_nfsv41_server(server);

I think you didn't mean to send the hunk below. :)

Ben

> +
> +/* if (server->nfs_client->cl_serverowner) {
> + ret = sysfs_create_file_ns(&server->kobj, &nfs_sysfs_attr_serverowner.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);
> + }
> +
> + if (server->nfs_client->cl_serverscope) {
> + ret = sysfs_create_file_ns(&server->kobj, &nfs_sysfs_attr_serverscope.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);
> + }
> +
> + if (server->nfs_client->cl_implid) {
> + ret = sysfs_create_file_ns(&server->kobj, &nfs_sysfs_attr_implid.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);
>
> --
> 2.47.1