On Thu, 2021-06-10 at 10:31 -0400, Olga Kornievskaia wrote: > On Thu, Jun 10, 2021 at 10:13 AM Trond Myklebust > <trondmy@xxxxxxxxxxxxxxx> wrote: > > > > On Thu, 2021-06-10 at 13:56 +0000, Chuck Lever III wrote: > > > > > > > > > > On Jun 10, 2021, at 9:34 AM, Trond Myklebust < > > > > trondmy@xxxxxxxxxxxxxxx> 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@xxxxxxxxx> wrote: > > > > > > > > > > > > From: Olga Kornievskaia <kolga@xxxxxxxxxx> > > > > > > > > > > > > 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@xxxxxxxxxx> > > > > > > --- > > > > > > 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. > > I'm not suggesting to programmatically determine the number of NIC to > determine the value of max_connect. > > No, but that's what Chuck appeared to be suggesting in order to avoid the need for the mount option. To me, the main reason for the mount option is to allow the user to limit the number of new IP addresses being added so that if the DNS server is configured to hand out lots of different addresses for the same servername, the user can basically say 'no, I just want to use the one IP address that I'm already connected to' (i.e. max_connect=1). I can imagine that some clustered setups might need that ability in order to work efficiently. I'm fine with the idea of nconnect setting the number of connections per IP address, but that would need some plumbing in rpc_clnt_test_and_add_xprt() to allow us to add up to 'nconnect' copies of a given transport. Presumably rpc_xprt_switch_has_addr() would need to return a count of the number of copies of the transport that are already present so that we can decide whether or not we should add a new one. -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@xxxxxxxxxxxxxxx
