Re: [PATCH 2/2] NFS: add a clientid mount option

[Chuck Lever <chuck.lever@oracle.com>]

On 7/14/25 2:30 AM, Christoph Hellwig wrote:
> Add a mount option to set a clientid, similarly to how it can be
> configured through the per-netfs sysfs file.  This allows for easy
> testing of behavior that relies on the client ID likes locks or
> delegations with having to resort to separate VMs or containers.

The problem with approaches like this is that it becomes difficult
to manage multiple mounts of the same server. Each of those mounts
really cannot have a different clientid.

For testing, why can't you use the per-container clientid setting?


> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfs/client.c           | 12 ++++++++++++
>  fs/nfs/fs_context.c       | 12 ++++++++++++
>  fs/nfs/internal.h         |  2 ++
>  fs/nfs/nfs4client.c       |  1 +
>  fs/nfs/nfs4proc.c         |  7 ++++++-
>  include/linux/nfs_fs_sb.h |  1 +
>  6 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 47258dc3af70..1a55debab6e5 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -181,6 +181,12 @@ struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_init)
>  	clp->cl_nconnect = cl_init->nconnect;
>  	clp->cl_max_connect = cl_init->max_connect ? cl_init->max_connect : 1;
>  	clp->cl_net = get_net_track(cl_init->net, &clp->cl_ns_tracker, GFP_KERNEL);
> +	if (cl_init->clientid) {
> +		err = -ENOMEM;
> +		clp->clientid = kstrdup(cl_init->clientid, GFP_KERNEL);
> +		if (!clp->clientid)
> +			goto error_free_host;
> +	}
>  
>  #if IS_ENABLED(CONFIG_NFS_LOCALIO)
>  	seqlock_init(&clp->cl_boot_lock);
> @@ -193,6 +199,8 @@ struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_init)
>  	clp->cl_xprtsec = cl_init->xprtsec;
>  	return clp;
>  
> +error_free_host:
> +	kfree(clp->cl_hostname);
>  error_cleanup:
>  	put_nfs_version(clp->cl_nfs_mod);
>  error_dealloc:
> @@ -254,6 +262,7 @@ void nfs_free_client(struct nfs_client *clp)
>  	put_nfs_version(clp->cl_nfs_mod);
>  	kfree(clp->cl_hostname);
>  	kfree(clp->cl_acceptor);
> +	kfree(clp->clientid);
>  	kfree_rcu(clp, rcu);
>  }
>  EXPORT_SYMBOL_GPL(nfs_free_client);
> @@ -339,6 +348,9 @@ static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *dat
>  		if (clp->cl_xprtsec.policy != data->xprtsec.policy)
>  			continue;
>  
> +		if (data->clientid && data->clientid != clp->clientid)
> +			continue;
> +
>  		refcount_inc(&clp->cl_count);
>  		return clp;
>  	}
> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> index 9e94d18448ff..fe9ecdc8db3c 100644
> --- a/fs/nfs/fs_context.c
> +++ b/fs/nfs/fs_context.c
> @@ -98,6 +98,7 @@ enum nfs_param {
>  	Opt_xprtsec,
>  	Opt_cert_serial,
>  	Opt_privkey_serial,
> +	Opt_clientid,
>  };
>  
>  enum {
> @@ -225,6 +226,7 @@ static const struct fs_parameter_spec nfs_fs_parameters[] = {
>  	fsparam_string("xprtsec",	Opt_xprtsec),
>  	fsparam_s32("cert_serial",	Opt_cert_serial),
>  	fsparam_s32("privkey_serial",	Opt_privkey_serial),
> +	fsparam_string("clientid",	Opt_clientid),
>  	{}
>  };
>  
> @@ -1031,6 +1033,14 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>  			goto out_invalid_value;
>  		}
>  		break;
> +	case Opt_clientid:
> +		if (!param->string || strlen(param->string) == 0 ||
> +		    strlen(param->string) > NFS4_CLIENT_ID_UNIQ_LEN - 1)
> +			goto out_of_bounds;
> +		kfree(ctx->clientid);
> +		ctx->clientid = param->string;
> +		param->string = NULL;
> +		break;
>  
>  		/*
>  		 * Special options
> @@ -1650,6 +1660,7 @@ static int nfs_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc)
>  	ctx->nfs_server.hostname	= NULL;
>  	ctx->fscache_uniq		= NULL;
>  	ctx->clone_data.fattr		= NULL;
> +	ctx->clientid			= NULL;
>  	fc->fs_private = ctx;
>  	return 0;
>  }
> @@ -1670,6 +1681,7 @@ static void nfs_fs_context_free(struct fs_context *fc)
>  		kfree(ctx->fscache_uniq);
>  		nfs_free_fhandle(ctx->mntfh);
>  		nfs_free_fattr(ctx->clone_data.fattr);
> +		kfree(ctx->clientid);
>  		kfree(ctx);
>  	}
>  }
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index 69c2c10ee658..1a392676d27c 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -86,6 +86,7 @@ struct nfs_client_initdata {
>  	struct xprtsec_parms xprtsec;
>  	unsigned long connect_timeout;
>  	unsigned long reconnect_timeout;
> +	const char *clientid;
>  };
>  
>  /*
> @@ -115,6 +116,7 @@ struct nfs_fs_context {
>  	unsigned short		mountfamily;
>  	bool			has_sec_mnt_opts;
>  	int			lock_status;
> +	char			*clientid;
>  
>  	struct {
>  		union {
> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
> index 2e623da1a787..3ab5cc985224 100644
> --- a/fs/nfs/nfs4client.c
> +++ b/fs/nfs/nfs4client.c
> @@ -1153,6 +1153,7 @@ static int nfs4_init_server(struct nfs_server *server, struct fs_context *fc)
>  		.xprtsec = ctx->xprtsec,
>  		.nconnect = ctx->nfs_server.nconnect,
>  		.max_connect = ctx->nfs_server.max_connect,
> +		.clientid = ctx->clientid,
>  	};
>  	int error;
>  
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index ef2077e185b6..ad53bc4ef50c 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -6487,6 +6487,11 @@ nfs4_get_uniquifier(struct nfs_client *clp, char *buf, size_t buflen)
>  
>  	buf[0] = '\0';
>  
> +	if (clp->clientid) {
> +		strscpy(buf, clp->clientid, buflen);
> +		goto out;
> +	}
> +
>  	if (nn_clp) {
>  		rcu_read_lock();
>  		id = rcu_dereference(nn_clp->identifier);
> @@ -6497,7 +6502,7 @@ nfs4_get_uniquifier(struct nfs_client *clp, char *buf, size_t buflen)
>  
>  	if (nfs4_client_id_uniquifier[0] != '\0' && buf[0] == '\0')
>  		strscpy(buf, nfs4_client_id_uniquifier, buflen);
> -
> +out:
>  	return strlen(buf);
>  }
>  
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index d2d36711a119..73bed04529a7 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -128,6 +128,7 @@ struct nfs_client {
>  	netns_tracker		cl_ns_tracker;
>  	struct list_head	pending_cb_stateids;
>  	struct rcu_head		rcu;
> +	const char		*clientid;
>  
>  #if IS_ENABLED(CONFIG_NFS_LOCALIO)
>  	struct timespec64	cl_nfssvc_boot;


-- 
Chuck Lever