Linux-NFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/1] NFSv4.0 encode nconnect-enabled client into clientid
@ 2020-01-31 16:57 Olga Kornievskaia
  2020-01-31 17:09 ` Chuck Lever
  0 siblings, 1 reply; 7+ messages in thread
From: Olga Kornievskaia @ 2020-01-31 16:57 UTC (permalink / raw)
  To: trond.myklebust, anna.schumaker; +Cc: linux-nfs

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.

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] NFSv4.0 encode nconnect-enabled client into clientid
  2020-01-31 16:57 [PATCH 1/1] NFSv4.0 encode nconnect-enabled client into clientid Olga Kornievskaia
@ 2020-01-31 17:09 ` Chuck Lever
  2020-01-31 17:46   ` Olga Kornievskaia
  0 siblings, 1 reply; 7+ messages in thread
From: Chuck Lever @ 2020-01-31 17:09 UTC (permalink / raw)
  To: Olga Kornievskaia; +Cc: trond.myklebust, Anna Schumaker, Linux NFS Mailing List

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




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] NFSv4.0 encode nconnect-enabled client into clientid
  2020-01-31 17:09 ` Chuck Lever
@ 2020-01-31 17:46   ` Olga Kornievskaia
  2020-01-31 18:34     ` Chuck Lever
  0 siblings, 1 reply; 7+ messages in thread
From: Olga Kornievskaia @ 2020-01-31 17:46 UTC (permalink / raw)
  To: Chuck Lever; +Cc: Trond Myklebust, Anna Schumaker, Linux NFS Mailing List

Hi Chuck,

Thanks for the discussion!

On Fri, Jan 31, 2020 at 12:10 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> 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.

I was asked...

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

Indeed, to servers it should only be using it for equality no argument
there but I don't think they are prohibited from deriving info from it
but shouldn't complain if something changed.

My reasoning was that we are currently already putting some
descriptive stuff into the clientid string. We specify what version
this client is. So why not add something that speaks about its
nconnect ability?

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

The server is interested in identifying whether or not client is doing
nconnect or not in order to decide whether or not to apply extra
locking for a given client mount in order to provide best performance.
In 4.1 spec, we clearly define how to bind connections to
session/clientid so server can use that information but 4.0 is lacking
that and a server is left to just deal with existence of multiple
connection (trunking) at any give time.

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

I didn't think it would hurt there. But honestly, I just didn't know
which of the 3 functions that we have to create clientid were used for
what protocols so I added nconnect to all.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] NFSv4.0 encode nconnect-enabled client into clientid
  2020-01-31 17:46   ` Olga Kornievskaia
@ 2020-01-31 18:34     ` Chuck Lever
  2020-01-31 18:55       ` Olga Kornievskaia
  0 siblings, 1 reply; 7+ messages in thread
From: Chuck Lever @ 2020-01-31 18:34 UTC (permalink / raw)
  To: Olga Kornievskaia; +Cc: Trond Myklebust, Anna Schumaker, Linux NFS Mailing List



> On Jan 31, 2020, at 12:46 PM, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote:
> 
> Hi Chuck,
> 
> Thanks for the discussion!
> 
> On Fri, Jan 31, 2020 at 12:10 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>> 
>> 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.
> 
> I was asked...
> 
>> 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.
> 
> Indeed, to servers it should only be using it for equality no argument
> there but I don't think they are prohibited from deriving info from it
> but shouldn't complain if something changed.
> 
> My reasoning was that we are currently already putting some
> descriptive stuff into the clientid string. We specify what version
> this client is. So why not add something that speaks about its
> nconnect ability?

RFC 7350 Section 9.1.1 discusses what belongs in the client ID string.

- Does adding nconnect help distinguish this client from others?
I think that it adds no new value there.

- How do you guarantee that this client presents the same nconnect
setting after every reboot? If the nconnect setting varies for different
mount points, it's possible that the cl_nconnect value can be different
depending on the order the client performs the mounts.

In fact I don't see how the client is constrained to present the same
nconnect setting even during the same reboot, for similar reasons.

That will break open/lock state reclaim, iiuc. And it will be subtle,
silent, non-deterministic breakage.


>> 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?
> 
> The server is interested in identifying whether or not client is doing
> nconnect or not in order to decide whether or not to apply extra
> locking for a given client mount in order to provide best performance.
> In 4.1 spec, we clearly define how to bind connections to
> session/clientid so server can use that information but 4.0 is lacking
> that and a server is left to just deal with existence of multiple
> connection (trunking) at any give time.

You can't insist that clients use NFSv4.1 in those cases?

Seems like this is proposing that the Linux NFS client should be changed
to fix a server implementation issue for a protocol that already has been
fixed in newer versions.


>>> 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 ?
> 
> I didn't think it would hurt there. But honestly, I just didn't know
> which of the 3 functions that we have to create clientid were used for
> what protocols so I added nconnect to all.

non_uniform is for NFSv4.0 only. uniform can be used by any minor version.


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

--
Chuck Lever




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] NFSv4.0 encode nconnect-enabled client into clientid
  2020-01-31 18:34     ` Chuck Lever
@ 2020-01-31 18:55       ` Olga Kornievskaia
  2020-01-31 19:09         ` Chuck Lever
  0 siblings, 1 reply; 7+ messages in thread
From: Olga Kornievskaia @ 2020-01-31 18:55 UTC (permalink / raw)
  To: Chuck Lever; +Cc: Trond Myklebust, Anna Schumaker, Linux NFS Mailing List

On Fri, Jan 31, 2020 at 1:36 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
>
>
> > On Jan 31, 2020, at 12:46 PM, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote:
> >
> > Hi Chuck,
> >
> > Thanks for the discussion!
> >
> > On Fri, Jan 31, 2020 at 12:10 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>
> >> 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.
> >
> > I was asked...
> >
> >> 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.
> >
> > Indeed, to servers it should only be using it for equality no argument
> > there but I don't think they are prohibited from deriving info from it
> > but shouldn't complain if something changed.
> >
> > My reasoning was that we are currently already putting some
> > descriptive stuff into the clientid string. We specify what version
> > this client is. So why not add something that speaks about its
> > nconnect ability?
>
> RFC 7350 Section 9.1.1 discusses what belongs in the client ID string.
>
> - Does adding nconnect help distinguish this client from others?
> I think that it adds no new value there.

How does adding LinuxV4.0 helps?  I think "nconnect" servers the same
value. It distinguishes from another linux client that doesn't do
nconnect.

> - How do you guarantee that this client presents the same nconnect
> setting after every reboot? If the nconnect setting varies for different
> mount points, it's possible that the cl_nconnect value can be different
> depending on the order the client performs the mounts.

Different mount points share the same clientid. A submount will have
the same nconnect as the original mount. Given existing
implementation, we can't have different mounts with different nconnect
values.

What reboot are we talking about server or client?

On a client reboot, the same nconnect value is used (if say it's
mounted from /etc/fstab). If somebody, after a client reboot, manually
remounted and used a different nconnect value, so what, it's a
different mount, what problem are you thinking about?

If you are talking about a server reboot, then why would a client's
data structure change that stores the value that created the orginal
clientid string with the nconnect value.


> In fact I don't see how the client is constrained to present the same
> nconnect setting even during the same reboot, for similar reasons.
>
> That will break open/lock state reclaim, iiuc. And it will be subtle,
> silent, non-deterministic breakage.

I'm missing how this can be possible.
>
>
> >> 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?
> >
> > The server is interested in identifying whether or not client is doing
> > nconnect or not in order to decide whether or not to apply extra
> > locking for a given client mount in order to provide best performance.
> > In 4.1 spec, we clearly define how to bind connections to
> > session/clientid so server can use that information but 4.0 is lacking
> > that and a server is left to just deal with existence of multiple
> > connection (trunking) at any give time.
>
> You can't insist that clients use NFSv4.1 in those cases?
>
> Seems like this is proposing that the Linux NFS client should be changed
> to fix a server implementation issue for a protocol that already has been
> fixed in newer versions.
>
>
> >>> 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 ?
> >
> > I didn't think it would hurt there. But honestly, I just didn't know
> > which of the 3 functions that we have to create clientid were used for
> > what protocols so I added nconnect to all.
>
> non_uniform is for NFSv4.0 only. uniform can be used by any minor version.
>
>
> >>>      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
>
> --
> Chuck Lever
>
>
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] NFSv4.0 encode nconnect-enabled client into clientid
  2020-01-31 18:55       ` Olga Kornievskaia
@ 2020-01-31 19:09         ` Chuck Lever
  2020-01-31 19:46           ` Olga Kornievskaia
  0 siblings, 1 reply; 7+ messages in thread
From: Chuck Lever @ 2020-01-31 19:09 UTC (permalink / raw)
  To: Olga Kornievskaia; +Cc: Trond Myklebust, Anna Schumaker, Linux NFS Mailing List



> On Jan 31, 2020, at 1:55 PM, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote:
> 
> On Fri, Jan 31, 2020 at 1:36 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>> 
>> 
>> 
>>> On Jan 31, 2020, at 12:46 PM, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote:
>>> 
>>> Hi Chuck,
>>> 
>>> Thanks for the discussion!
>>> 
>>> On Fri, Jan 31, 2020 at 12:10 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>> 
>>>> 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.
>>> 
>>> I was asked...
>>> 
>>>> 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.
>>> 
>>> Indeed, to servers it should only be using it for equality no argument
>>> there but I don't think they are prohibited from deriving info from it
>>> but shouldn't complain if something changed.
>>> 
>>> My reasoning was that we are currently already putting some
>>> descriptive stuff into the clientid string. We specify what version
>>> this client is. So why not add something that speaks about its
>>> nconnect ability?
>> 
>> RFC 7350 Section 9.1.1 discusses what belongs in the client ID string.
>> 
>> - Does adding nconnect help distinguish this client from others?
>> I think that it adds no new value there.
> 
> How does adding LinuxV4.0 helps?  I think "nconnect" servers the same
> value. It distinguishes from another linux client that doesn't do
> nconnect.

"Linux NFSv4.x" is historical. But it does distinguish between
Linux and other operating systems.

If you add nconnect to the client ID string, then most Linux
NFSv4.x mounts will have "nconnect=x" where x is the default
setting. That's no better than just "Linux NFSv4.x".


>> - How do you guarantee that this client presents the same nconnect
>> setting after every reboot? If the nconnect setting varies for different
>> mount points, it's possible that the cl_nconnect value can be different
>> depending on the order the client performs the mounts.
> 
> Different mount points share the same clientid. A submount will have
> the same nconnect as the original mount. Given existing
> implementation, we can't have different mounts with different nconnect
> values.

The nconnect value can change over a client reboot. If the
/etc/fstab or /etc/nfsmount.conf settings are changed, the
cl_nconnect value will change.

It's not OK to add a hidden dependence on lock reclaim to
the nconnect mount option.


> What reboot are we talking about server or client?
> 
> On a client reboot, the same nconnect value is used (if say it's
> mounted from /etc/fstab). If somebody, after a client reboot, manually
> remounted and used a different nconnect value, so what, it's a
> different mount, what problem are you thinking about?
> 
> If you are talking about a server reboot, then why would a client's
> data structure change that stores the value that created the orginal
> clientid string with the nconnect value.
> 
> 
>> In fact I don't see how the client is constrained to present the same
>> nconnect setting even during the same reboot, for similar reasons.
>> 
>> That will break open/lock state reclaim, iiuc. And it will be subtle,
>> silent, non-deterministic breakage.
> 
> I'm missing how this can be possible.

If you have multiple NFSv4 mounts in /etc/fstab and they have different
nconnect settings, then the client's cl_nconnect value will depend on
which one gets mounted first. Isn't that how it works?

That would mean that the client presents a different client ID string
to the server depending on which of these mounts happens first after a
client reboot.

We have the same problem with the sec= setting, which is why the kernel
chooses a setting mostly independently of the sec= mount option.


>>>> 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?
>>> 
>>> The server is interested in identifying whether or not client is doing
>>> nconnect or not in order to decide whether or not to apply extra
>>> locking for a given client mount in order to provide best performance.
>>> In 4.1 spec, we clearly define how to bind connections to
>>> session/clientid so server can use that information but 4.0 is lacking
>>> that and a server is left to just deal with existence of multiple
>>> connection (trunking) at any give time.
>> 
>> You can't insist that clients use NFSv4.1 in those cases?

This seems like a pertinent question. What is wrong with suggesting
the use of NFSv4.1 in such cases?


>> Seems like this is proposing that the Linux NFS client should be changed
>> to fix a server implementation issue for a protocol that already has been
>> fixed in newer versions.
>> 
>> 
>>>>> 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 ?
>>> 
>>> I didn't think it would hurt there. But honestly, I just didn't know
>>> which of the 3 functions that we have to create clientid were used for
>>> what protocols so I added nconnect to all.
>> 
>> non_uniform is for NFSv4.0 only. uniform can be used by any minor version.
>> 
>> 
>>>>>     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
>> 
>> --
>> Chuck Lever

--
Chuck Lever




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] NFSv4.0 encode nconnect-enabled client into clientid
  2020-01-31 19:09         ` Chuck Lever
@ 2020-01-31 19:46           ` Olga Kornievskaia
  0 siblings, 0 replies; 7+ messages in thread
From: Olga Kornievskaia @ 2020-01-31 19:46 UTC (permalink / raw)
  To: Chuck Lever; +Cc: Trond Myklebust, Anna Schumaker, Linux NFS Mailing List

On Fri, Jan 31, 2020 at 2:09 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
>
>
> > On Jan 31, 2020, at 1:55 PM, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote:
> >
> > On Fri, Jan 31, 2020 at 1:36 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>
> >>
> >>
> >>> On Jan 31, 2020, at 12:46 PM, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote:
> >>>
> >>> Hi Chuck,
> >>>
> >>> Thanks for the discussion!
> >>>
> >>> On Fri, Jan 31, 2020 at 12:10 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>
> >>>> 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.
> >>>
> >>> I was asked...
> >>>
> >>>> 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.
> >>>
> >>> Indeed, to servers it should only be using it for equality no argument
> >>> there but I don't think they are prohibited from deriving info from it
> >>> but shouldn't complain if something changed.
> >>>
> >>> My reasoning was that we are currently already putting some
> >>> descriptive stuff into the clientid string. We specify what version
> >>> this client is. So why not add something that speaks about its
> >>> nconnect ability?
> >>
> >> RFC 7350 Section 9.1.1 discusses what belongs in the client ID string.
> >>
> >> - Does adding nconnect help distinguish this client from others?
> >> I think that it adds no new value there.
> >
> > How does adding LinuxV4.0 helps?  I think "nconnect" servers the same
> > value. It distinguishes from another linux client that doesn't do
> > nconnect.
>
> "Linux NFSv4.x" is historical. But it does distinguish between
> Linux and other operating systems.
>
> If you add nconnect to the client ID string, then most Linux
> NFSv4.x mounts will have "nconnect=x" where x is the default
> setting. That's no better than just "Linux NFSv4.x".
>
>
> >> - How do you guarantee that this client presents the same nconnect
> >> setting after every reboot? If the nconnect setting varies for different
> >> mount points, it's possible that the cl_nconnect value can be different
> >> depending on the order the client performs the mounts.
> >
> > Different mount points share the same clientid. A submount will have
> > the same nconnect as the original mount. Given existing
> > implementation, we can't have different mounts with different nconnect
> > values.
>
> The nconnect value can change over a client reboot. If the
> /etc/fstab or /etc/nfsmount.conf settings are changed, the
> cl_nconnect value will change.
>
> It's not OK to add a hidden dependence on lock reclaim to
> the nconnect mount option.
>
>
> > What reboot are we talking about server or client?
> >
> > On a client reboot, the same nconnect value is used (if say it's
> > mounted from /etc/fstab). If somebody, after a client reboot, manually
> > remounted and used a different nconnect value, so what, it's a
> > different mount, what problem are you thinking about?
> >
> > If you are talking about a server reboot, then why would a client's
> > data structure change that stores the value that created the orginal
> > clientid string with the nconnect value.
> >
> >
> >> In fact I don't see how the client is constrained to present the same
> >> nconnect setting even during the same reboot, for similar reasons.
> >>
> >> That will break open/lock state reclaim, iiuc. And it will be subtle,
> >> silent, non-deterministic breakage.
> >
> > I'm missing how this can be possible.
>
> If you have multiple NFSv4 mounts in /etc/fstab and they have different
> nconnect settings, then the client's cl_nconnect value will depend on
> which one gets mounted first. Isn't that how it works?

Ok, I see it now. if /vol1 was mount with nconnect=2 and /vol2 with
nconnect=3 then "first" mount wins.

I retract my patch then. Thank you for your help!

> That would mean that the client presents a different client ID string
> to the server depending on which of these mounts happens first after a
> client reboot.
>
> We have the same problem with the sec= setting, which is why the kernel
> chooses a setting mostly independently of the sec= mount option.
>
>
> >>>> 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?
> >>>
> >>> The server is interested in identifying whether or not client is doing
> >>> nconnect or not in order to decide whether or not to apply extra
> >>> locking for a given client mount in order to provide best performance.
> >>> In 4.1 spec, we clearly define how to bind connections to
> >>> session/clientid so server can use that information but 4.0 is lacking
> >>> that and a server is left to just deal with existence of multiple
> >>> connection (trunking) at any give time.
> >>
> >> You can't insist that clients use NFSv4.1 in those cases?
>
> This seems like a pertinent question. What is wrong with suggesting
> the use of NFSv4.1 in such cases?
>
>
> >> Seems like this is proposing that the Linux NFS client should be changed
> >> to fix a server implementation issue for a protocol that already has been
> >> fixed in newer versions.
> >>
> >>
> >>>>> 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 ?
> >>>
> >>> I didn't think it would hurt there. But honestly, I just didn't know
> >>> which of the 3 functions that we have to create clientid were used for
> >>> what protocols so I added nconnect to all.
> >>
> >> non_uniform is for NFSv4.0 only. uniform can be used by any minor version.
> >>
> >>
> >>>>>     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
> >>
> >> --
> >> Chuck Lever
>
> --
> Chuck Lever
>
>
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-31 16:57 [PATCH 1/1] NFSv4.0 encode nconnect-enabled client into clientid Olga Kornievskaia
2020-01-31 17:09 ` Chuck Lever
2020-01-31 17:46   ` Olga Kornievskaia
2020-01-31 18:34     ` Chuck Lever
2020-01-31 18:55       ` Olga Kornievskaia
2020-01-31 19:09         ` Chuck Lever
2020-01-31 19:46           ` Olga Kornievskaia

Linux-NFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nfs/0 linux-nfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nfs linux-nfs/ https://lore.kernel.org/linux-nfs \
		linux-nfs@vger.kernel.org
	public-inbox-index linux-nfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-nfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git