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

[Trond Myklebust <trondmy@hammerspace.com>]

On Thu, 2021-06-10 at 13:56 +0000, Chuck Lever III wrote:
> 
> 
> > 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.

It's not an overreaction. It's insane to think that counting NICs gives
you any notion whatsoever about the network topology and connectivity
between the client and server. It doesn't even tell you how many of
those NICs might potentially be available to your application.

We're not doing any automation based on that kind of layering
violation.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com