Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 10 Jun 2021 10:22:30 +0800
From:   Wang Yugui <wangyugui@e16-tech.com>
To:     Olga Kornievskaia <olga.kornievskaia@gmail.com>,
        trond.myklebust@hammerspace.com, anna.schumaker@netapp.com,
        linux-nfs@vger.kernel.org
Subject: Re: [PATCH v2 2/3] NFSv4 introduce max_connect mount options
In-Reply-To: <20210610094912.4D54.409509F4@e16-tech.com>
References: <20210609215319.5518-3-olga.kornievskaia@gmail.com> <20210610094912.4D54.409509F4@e16-tech.com>
Message-Id: <20210610102229.4D5B.409509F4@e16-tech.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

> We could extend 'nconnect' option rather than a new option 'max_connect'?
> 
> max of nconnect is 16 when trunk support of single ip (kernel 5.3-5.?).
> max of nconnect is 64 when trunk support of multiple ip (kernel 5.?-).

I'm sorry that is a bad idea.

we still need 'nconnect' for single ip.

'nconnect' have a default value of '1', but 'max_connect'  may have a
default value of '256' or '128', even hard-coded in nfs server.

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2021/06/10


> Best Regards
> Wang Yugui (wangyugui@e16-tech.com)
> 2021/06/10
> 
> > 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;
> >  	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
> >  
> >  enum nfs_param {
> >  	Opt_ac,
> > @@ -60,6 +61,7 @@ enum nfs_param {
> >  	Opt_mountvers,
> >  	Opt_namelen,
> >  	Opt_nconnect,
> > +	Opt_max_connect,
> >  	Opt_port,
> >  	Opt_posix,
> >  	Opt_proto,
> > @@ -158,6 +160,7 @@ static const struct fs_parameter_spec nfs_fs_parameters[] = {
> >  	fsparam_u32   ("mountvers",	Opt_mountvers),
> >  	fsparam_u32   ("namlen",	Opt_namelen),
> >  	fsparam_u32   ("nconnect",	Opt_nconnect),
> > +	fsparam_u32   ("max_connect",	Opt_max_connect),
> >  	fsparam_string("nfsvers",	Opt_vers),
> >  	fsparam_u32   ("port",		Opt_port),
> >  	fsparam_flag_no("posix",	Opt_posix),
> > @@ -770,6 +773,11 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
> >  			goto out_of_bounds;
> >  		ctx->nfs_server.nconnect = result.uint_32;
> >  		break;
> > +	case Opt_max_connect:
> > +		if (result.uint_32 < 1 || result.uint_32 > NFS_MAX_TRANSPORTS)
> > +			goto out_of_bounds;
> > +		ctx->nfs_server.max_connect = result.uint_32;
> > +		break;
> >  	case Opt_lookupcache:
> >  		switch (result.uint_32) {
> >  		case Opt_lookupcache_all:
> > diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> > index a36af04188c2..66fc936834f2 100644
> > --- a/fs/nfs/internal.h
> > +++ b/fs/nfs/internal.h
> > @@ -67,6 +67,7 @@ struct nfs_client_initdata {
> >  	int proto;
> >  	u32 minorversion;
> >  	unsigned int nconnect;
> > +	unsigned int max_connect;
> >  	struct net *net;
> >  	const struct rpc_timeout *timeparms;
> >  	const struct cred *cred;
> > @@ -121,6 +122,7 @@ struct nfs_fs_context {
> >  		int			port;
> >  		unsigned short		protocol;
> >  		unsigned short		nconnect;
> > +		unsigned short		max_connect;
> >  		unsigned short		export_path_len;
> >  	} nfs_server;
> >  
> > diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
> > index 42719384e25f..640c8235d817 100644
> > --- a/fs/nfs/nfs4client.c
> > +++ b/fs/nfs/nfs4client.c
> > @@ -863,6 +863,7 @@ static int nfs4_set_client(struct nfs_server *server,
> >  		const char *ip_addr,
> >  		int proto, const struct rpc_timeout *timeparms,
> >  		u32 minorversion, unsigned int nconnect,
> > +		unsigned int max_connect,
> >  		struct net *net)
> >  {
> >  	struct nfs_client_initdata cl_init = {
> > @@ -881,6 +882,8 @@ static int nfs4_set_client(struct nfs_server *server,
> >  
> >  	if (minorversion == 0)
> >  		__set_bit(NFS_CS_REUSEPORT, &cl_init.init_flags);
> > +	else
> > +		cl_init.max_connect = max_connect;
> >  	if (proto == XPRT_TRANSPORT_TCP)
> >  		cl_init.nconnect = nconnect;
> >  
> > @@ -950,8 +953,10 @@ struct nfs_client *nfs4_set_ds_client(struct nfs_server *mds_srv,
> >  		return ERR_PTR(-EINVAL);
> >  	cl_init.hostname = buf;
> >  
> > -	if (mds_clp->cl_nconnect > 1 && ds_proto == XPRT_TRANSPORT_TCP)
> > +	if (mds_clp->cl_nconnect > 1 && ds_proto == XPRT_TRANSPORT_TCP) {
> >  		cl_init.nconnect = mds_clp->cl_nconnect;
> > +		cl_init.max_connect = mds_clp->cl_max_connect;
> > +	}
> >  
> >  	if (mds_srv->flags & NFS_MOUNT_NORESVPORT)
> >  		__set_bit(NFS_CS_NORESVPORT, &cl_init.init_flags);
> > @@ -1120,6 +1125,7 @@ static int nfs4_init_server(struct nfs_server *server, struct fs_context *fc)
> >  				&timeparms,
> >  				ctx->minorversion,
> >  				ctx->nfs_server.nconnect,
> > +				ctx->nfs_server.max_connect,
> >  				fc->net_ns);
> >  	if (error < 0)
> >  		return error;
> > @@ -1209,6 +1215,7 @@ struct nfs_server *nfs4_create_referral_server(struct fs_context *fc)
> >  				parent_server->client->cl_timeout,
> >  				parent_client->cl_mvops->minor_version,
> >  				parent_client->cl_nconnect,
> > +				parent_client->cl_max_connect,
> >  				parent_client->cl_net);
> >  	if (!error)
> >  		goto init_server;
> > @@ -1224,6 +1231,7 @@ struct nfs_server *nfs4_create_referral_server(struct fs_context *fc)
> >  				parent_server->client->cl_timeout,
> >  				parent_client->cl_mvops->minor_version,
> >  				parent_client->cl_nconnect,
> > +				parent_client->cl_max_connect,
> >  				parent_client->cl_net);
> >  	if (error < 0)
> >  		goto error;
> > @@ -1321,7 +1329,7 @@ int nfs4_update_server(struct nfs_server *server, const char *hostname,
> >  	error = nfs4_set_client(server, hostname, sap, salen, buf,
> >  				clp->cl_proto, clnt->cl_timeout,
> >  				clp->cl_minorversion,
> > -				clp->cl_nconnect, net);
> > +				clp->cl_nconnect, clp->cl_max_connect, net);
> >  	clear_bit(NFS_MIG_TSM_POSSIBLE, &server->mig_status);
> >  	if (error != 0) {
> >  		nfs_server_insert_lists(server);
> > diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> > index fe58525cfed4..e65c83494c05 100644
> > --- a/fs/nfs/super.c
> > +++ b/fs/nfs/super.c
> > @@ -480,6 +480,8 @@ static void nfs_show_mount_options(struct seq_file *m, struct nfs_server *nfss,
> >  	if (clp->cl_nconnect > 0)
> >  		seq_printf(m, ",nconnect=%u", clp->cl_nconnect);
> >  	if (version == 4) {
> > +		if (clp->cl_max_connect > 1)
> > +			seq_printf(m, ",max_connect=%u", clp->cl_max_connect);
> >  		if (nfss->port != NFS_PORT)
> >  			seq_printf(m, ",port=%u", nfss->port);
> >  	} else
> > diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> > index d71a0e90faeb..2a9acbfe00f0 100644
> > --- a/include/linux/nfs_fs_sb.h
> > +++ b/include/linux/nfs_fs_sb.h
> > @@ -62,6 +62,7 @@ struct nfs_client {
> >  
> >  	u32			cl_minorversion;/* NFSv4 minorversion */
> >  	unsigned int		cl_nconnect;	/* Number of connections */
> > +	unsigned int		cl_max_connect; /* max number of xprts allowed */
> >  	const char *		cl_principal;  /* used for machine cred */
> >  
> >  #if IS_ENABLED(CONFIG_NFS_V4)
> > -- 
> > 2.27.0
>