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

[Anna Schumaker <anna.schumaker@oracle.com>]

Hi Ben,

On 1/9/25 9:33 AM, Benjamin Coddington wrote:
> 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.

Sure! I've made that change in my branch.

> 
>> +		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.  :)

Hah! No, I did not. I've removed it for v2.

Thanks for looking at this!

Anna

> 
> 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
>