Re: [PATCH 1/1] NFSv4.0 encode nconnect-enabled client into clientid

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

Hi Olga-

> On Jan 31, 2020, at 11:57 AM, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote:
> 
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> It helps some servers to be able to identify if the incoming client is
> doing nconnect mount or not. While creating the unique client id for
> the SETCLIENTID operation add nconnect=X to it.

This makes me itch uncomfortably.

The long-form client ID string is not supposed to be used to communicate
client implementation details. In fact, this string is supposed to be
opaque to the server -- it can only compare these strings for equality.

IMO you would also need to write something akin to a standard that describes
this convention so that servers can depend on the form of the string.

How would a server use this information?


> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
> fs/nfs/nfs4proc.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 402410c..a90ea28 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5950,7 +5950,7 @@ static void nfs4_init_boot_verifier(const struct nfs_client *clp,
> 		return 0;
> 
> 	rcu_read_lock();
> -	len = 14 +
> +	len = 14 + 12 +
> 		strlen(clp->cl_rpcclient->cl_nodename) +
> 		1 +
> 		strlen(rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_ADDR)) +
> @@ -5972,13 +5972,15 @@ static void nfs4_init_boot_verifier(const struct nfs_client *clp,
> 
> 	rcu_read_lock();
> 	if (nfs4_client_id_uniquifier[0] != '\0')
> -		scnprintf(str, len, "Linux NFSv4.0 %s/%s/%s",
> +		scnprintf(str, len, "Linux NFSv4.0 nconnect=%d %s/%s/%s",
> +			  clp->cl_nconnect,
> 			  clp->cl_rpcclient->cl_nodename,
> 			  nfs4_client_id_uniquifier,
> 			  rpc_peeraddr2str(clp->cl_rpcclient,
> 					   RPC_DISPLAY_ADDR));
> 	else
> -		scnprintf(str, len, "Linux NFSv4.0 %s/%s",
> +		scnprintf(str, len, "Linux NFSv4.0 nconnect=%d %s/%s",
> +			  clp->cl_nconnect,
> 			  clp->cl_rpcclient->cl_nodename,
> 			  rpc_peeraddr2str(clp->cl_rpcclient,
> 					   RPC_DISPLAY_ADDR));
> @@ -5994,7 +5996,7 @@ static void nfs4_init_boot_verifier(const struct nfs_client *clp,
> 	size_t len;
> 	char *str;
> 
> -	len = 10 + 10 + 1 + 10 + 1 +
> +	len = 10 + 10 + 1 + 10 + 1 + 12 +
> 		strlen(nfs4_client_id_uniquifier) + 1 +
> 		strlen(clp->cl_rpcclient->cl_nodename) + 1;
> 
> @@ -6010,9 +6012,9 @@ static void nfs4_init_boot_verifier(const struct nfs_client *clp,
> 	if (!str)
> 		return -ENOMEM;
> 
> -	scnprintf(str, len, "Linux NFSv%u.%u %s/%s",
> +	scnprintf(str, len, "Linux NFSv%u.%u nconnect=%d %s/%s",
> 			clp->rpc_ops->version, clp->cl_minorversion,
> -			nfs4_client_id_uniquifier,
> +			clp->cl_nconnect, nfs4_client_id_uniquifier,
> 			clp->cl_rpcclient->cl_nodename);

Doesn't this also change the client ID string used for EXCHANGE_ID ?


> 	clp->cl_owner_id = str;
> 	return 0;
> @@ -6030,7 +6032,7 @@ static void nfs4_init_boot_verifier(const struct nfs_client *clp,
> 	if (nfs4_client_id_uniquifier[0] != '\0')
> 		return nfs4_init_uniquifier_client_string(clp);
> 
> -	len = 10 + 10 + 1 + 10 + 1 +
> +	len = 10 + 10 + 1 + 10 + 1 + 12 +
> 		strlen(clp->cl_rpcclient->cl_nodename) + 1;
> 
> 	if (len > NFS4_OPAQUE_LIMIT + 1)
> @@ -6045,9 +6047,9 @@ static void nfs4_init_boot_verifier(const struct nfs_client *clp,
> 	if (!str)
> 		return -ENOMEM;
> 
> -	scnprintf(str, len, "Linux NFSv%u.%u %s",
> +	scnprintf(str, len, "Linux NFSv%u.%u nconnect=%d %s",
> 			clp->rpc_ops->version, clp->cl_minorversion,
> -			clp->cl_rpcclient->cl_nodename);
> +			clp->cl_nconnect, clp->cl_rpcclient->cl_nodename);
> 	clp->cl_owner_id = str;
> 	return 0;
> }
> -- 
> 1.8.3.1
> 

--
Chuck Lever