Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Subject: Re: [PATCH 1/1] NFSv4.0 encode nconnect-enabled client into clientid
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20200131165702.1751-1-olga.kornievskaia@gmail.com>
Date:   Fri, 31 Jan 2020 12:09:58 -0500
Cc:     trond.myklebust@hammerspace.com,
        Anna Schumaker <anna.schumaker@netapp.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <45AE6B67-4B01-412B-8ABF-94E06BFB77D8@oracle.com>
References: <20200131165702.1751-1-olga.kornievskaia@gmail.com>
To:     Olga Kornievskaia <olga.kornievskaia@gmail.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

Hi Olga-

> On Jan 31, 2020, at 11:57 AM, Olga Kornievskaia =
<olga.kornievskaia@gmail.com> wrote:
>=20
> From: Olga Kornievskaia <kolga@netapp.com>
>=20
> 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=3DX 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(-)
>=20
> 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;
>=20
> 	rcu_read_lock();
> -	len =3D 14 +
> +	len =3D 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,
>=20
> 	rcu_read_lock();
> 	if (nfs4_client_id_uniquifier[0] !=3D '\0')
> -		scnprintf(str, len, "Linux NFSv4.0 %s/%s/%s",
> +		scnprintf(str, len, "Linux NFSv4.0 nconnect=3D%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=3D%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;
>=20
> -	len =3D 10 + 10 + 1 + 10 + 1 +
> +	len =3D 10 + 10 + 1 + 10 + 1 + 12 +
> 		strlen(nfs4_client_id_uniquifier) + 1 +
> 		strlen(clp->cl_rpcclient->cl_nodename) + 1;
>=20
> @@ -6010,9 +6012,9 @@ static void nfs4_init_boot_verifier(const struct =
nfs_client *clp,
> 	if (!str)
> 		return -ENOMEM;
>=20
> -	scnprintf(str, len, "Linux NFSv%u.%u %s/%s",
> +	scnprintf(str, len, "Linux NFSv%u.%u nconnect=3D%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 =3D str;
> 	return 0;
> @@ -6030,7 +6032,7 @@ static void nfs4_init_boot_verifier(const struct =
nfs_client *clp,
> 	if (nfs4_client_id_uniquifier[0] !=3D '\0')
> 		return nfs4_init_uniquifier_client_string(clp);
>=20
> -	len =3D 10 + 10 + 1 + 10 + 1 +
> +	len =3D 10 + 10 + 1 + 10 + 1 + 12 +
> 		strlen(clp->cl_rpcclient->cl_nodename) + 1;
>=20
> 	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;
>=20
> -	scnprintf(str, len, "Linux NFSv%u.%u %s",
> +	scnprintf(str, len, "Linux NFSv%u.%u nconnect=3D%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 =3D str;
> 	return 0;
> }
> --=20
> 1.8.3.1
>=20

--
Chuck Lever