Re: [PATCH v2 2/3] NFSv4 introduce max_connect mount options

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

> On Jun 10, 2021, at 9:34 AM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> On Thu, 2021-06-10 at 13:30 +0000, Chuck Lever III wrote:
>> 
>> 
>>> On Jun 9, 2021, at 5:53 PM, Olga Kornievskaia < 
>>> olga.kornievskaia@gmail.com> wrote:
>>> 
>>> From: Olga Kornievskaia <kolga@netapp.com>
>>> 
>>> This option will control up to how many xprts can the client
>>> establish to the server. This patch parses the value and sets
>>> up structures that keep track of max_connect.
>>> 
>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>>> ---
>>> fs/nfs/client.c           |  1 +
>>> fs/nfs/fs_context.c       |  8 ++++++++
>>> fs/nfs/internal.h         |  2 ++
>>> fs/nfs/nfs4client.c       | 12 ++++++++++--
>>> fs/nfs/super.c            |  2 ++
>>> include/linux/nfs_fs_sb.h |  1 +
>>> 6 files changed, 24 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
>>> index 330f65727c45..486dec59972b 100644
>>> --- a/fs/nfs/client.c
>>> +++ b/fs/nfs/client.c
>>> @@ -179,6 +179,7 @@ struct nfs_client *nfs_alloc_client(const
>>> struct nfs_client_initdata *cl_init)
>>> 
>>>         clp->cl_proto = cl_init->proto;
>>>         clp->cl_nconnect = cl_init->nconnect;
>>> +       clp->cl_max_connect = cl_init->max_connect ? cl_init-
>>>> max_connect : 1;
>> 
>> So, 1 is the default setting, meaning the "add another transport"
>> facility is disabled by default. Would it be less surprising for
>> an admin to allow some extra connections by default?
>> 
>> 
>>>         clp->cl_net = get_net(cl_init->net);
>>> 
>>>         clp->cl_principal = "*";
>>> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
>>> index d95c9a39bc70..cfbff7098f8e 100644
>>> --- a/fs/nfs/fs_context.c
>>> +++ b/fs/nfs/fs_context.c
>>> @@ -29,6 +29,7 @@
>>> #endif
>>> 
>>> #define NFS_MAX_CONNECTIONS 16
>>> +#define NFS_MAX_TRANSPORTS 128
>> 
>> This maximum seems excessive... again, there are diminishing
>> returns to adding more connections to the same server. what's
>> wrong with re-using NFS_MAX_CONNECTIONS for the maximum?
>> 
>> As always, I'm a little queasy about adding yet another mount
>> option. Are there real use cases where a whole-client setting
>> (like a sysfs attribute) would be inadequate? Is there a way
>> the client could figure out a reasonable maximum without a
>> human intervention, say, by counting the number of NICs on
>> the system?
> 
> Oh, hell no! We're not tying anything to the number of NICs...

That's a bit of an over-reaction. :-) A little more explanation
would be welcome. I mean, don't you expect someone to ask "How
do I pick a good value?" and someone might reasonably answer
"Well, start with the number of NICs on your client times 3" or
something like that.

IMO we're about to add another admin setting without understanding
how it will be used, how to select a good maximum value, or even
whether this maximum needs to be adjustable. In a previous e-mail
Olga has already demonstrated that it will be difficult to explain
how to use this setting with nconnect=.

Thus I would favor a (moderate) soldered-in maximum to start with,
and then as real world use cases arise, consider adding a tuning
mechanism based on actual requirements.


--
Chuck Lever