> On 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. The "Oh, hell no!" was an overreaction. But thank you for providing the additional explanation, that helped me understand your position. I agree that the number of local NICs is frequently unrelated to the topology of the whole network. > 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. Fair enough. -- Chuck Lever
