On Fri, Jan 31, 2020 at 2:09 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote: > > > > > On Jan 31, 2020, at 1:55 PM, Olga Kornievskaia <olga.kornievskaia@xxxxxxxxx> wrote: > > > > On Fri, Jan 31, 2020 at 1:36 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote: > >> > >> > >> > >>> On Jan 31, 2020, at 12:46 PM, Olga Kornievskaia <olga.kornievskaia@xxxxxxxxx> wrote: > >>> > >>> Hi Chuck, > >>> > >>> Thanks for the discussion! > >>> > >>> On Fri, Jan 31, 2020 at 12:10 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote: > >>>> > >>>> Hi Olga- > >>>> > >>>>> On Jan 31, 2020, at 11:57 AM, Olga Kornievskaia <olga.kornievskaia@xxxxxxxxx> wrote: > >>>>> > >>>>> From: Olga Kornievskaia <kolga@xxxxxxxxxx> > >>>>> > >>>>> 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@xxxxxxxxxx> > >>>>> --- > >>>>> 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 > > >
