Re: [PATCH 2/2] sunrpc: remove all connection limit configuration

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2024-12-09 at 12:27 -0500, Chuck Lever wrote:
> On 12/8/24 7:41 PM, NeilBrown wrote:
> > Now that the connection limit only apply to unconfirmed connections,
> > there is no need to configure it.  So remove all the configuration and
> > fix the number of unconfirmed connections as always 64 - which is
> > now given a name: XPT_MAX_TMP_CONN
> > 
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >   fs/lockd/svc.c                  |  8 -------
> >   fs/nfsd/netns.h                 |  6 -----
> >   fs/nfsd/nfsctl.c                | 42 ---------------------------------
> >   fs/nfsd/nfssvc.c                |  5 ----
> >   include/linux/sunrpc/svc.h      |  4 ----
> >   include/linux/sunrpc/svc_xprt.h |  6 +++++
> >   net/sunrpc/svc_xprt.c           |  8 +------
> >   7 files changed, 7 insertions(+), 72 deletions(-)
> > 
> > diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> > index 4ec22c2f2ea3..7ded57ec3a60 100644
> > --- a/fs/lockd/svc.c
> > +++ b/fs/lockd/svc.c
> > @@ -70,9 +70,6 @@ static unsigned long		nlm_grace_period;
> >   unsigned long			nlm_timeout = LOCKD_DFLT_TIMEO;
> >   static int			nlm_udpport, nlm_tcpport;
> >   
> > -/* RLIM_NOFILE defaults to 1024. That seems like a reasonable default here. */
> > -static unsigned int		nlm_max_connections = 1024;
> > -
> >   /*
> >    * Constants needed for the sysctl interface.
> >    */
> > @@ -136,9 +133,6 @@ lockd(void *vrqstp)
> >   	 * NFS mount or NFS daemon has gone away.
> >   	 */
> >   	while (!svc_thread_should_stop(rqstp)) {
> > -		/* update sv_maxconn if it has changed */
> > -		rqstp->rq_server->sv_maxconn = nlm_max_connections;
> > -
> >   		nlmsvc_retry_blocked(rqstp);
> >   		svc_recv(rqstp);
> >   	}
> > @@ -340,7 +334,6 @@ static int lockd_get(void)
> >   		return -ENOMEM;
> >   	}
> >   
> > -	serv->sv_maxconn = nlm_max_connections;
> >   	error = svc_set_num_threads(serv, NULL, 1);
> >   	if (error < 0) {
> >   		svc_destroy(&serv);
> > @@ -542,7 +535,6 @@ module_param_call(nlm_udpport, param_set_port, param_get_int,
> >   module_param_call(nlm_tcpport, param_set_port, param_get_int,
> >   		  &nlm_tcpport, 0644);
> >   module_param(nsm_use_hostnames, bool, 0644);
> > -module_param(nlm_max_connections, uint, 0644);
> 
> We've discussed deprecation and removal of items from /proc/fs/nfsd
> before, but removing a module parameter seems like it needs to be
> handled with the usual deprecation schedule?
> 

Yeah, that could break someone on an upgrade. What we should probably
do is keep the knob around, but just make it not do anything now, and
throw a pr_warn message or something if someone tries to set it.
Eventually in a year or two, we should be able to remove it.


> >   static int lockd_init_net(struct net *net)
> >   {
> > diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> > index a05a45bb1978..4a07b8d0837b 100644
> > --- a/fs/nfsd/netns.h
> > +++ b/fs/nfsd/netns.h
> > @@ -128,12 +128,6 @@ struct nfsd_net {
> >   	seqlock_t writeverf_lock;
> >   	unsigned char writeverf[8];
> >   
> > -	/*
> > -	 * Max number of non-validated connections this nfsd container
> > -	 * will allow.  Defaults to '0' gets mapped to 64.
> > -	 */
> > -	unsigned int max_connections;
> > -
> >   	u32 clientid_base;
> >   	u32 clientid_counter;
> >   	u32 clverifier_counter;
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 3adbc05ebaac..95ea4393305b 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -48,7 +48,6 @@ enum {
> >   	NFSD_Versions,
> >   	NFSD_Ports,
> >   	NFSD_MaxBlkSize,
> > -	NFSD_MaxConnections,
> >   	NFSD_Filecache,
> >   	NFSD_Leasetime,
> >   	NFSD_Gracetime,
> > @@ -68,7 +67,6 @@ static ssize_t write_pool_threads(struct file *file, char *buf, size_t size);
> >   static ssize_t write_versions(struct file *file, char *buf, size_t size);
> >   static ssize_t write_ports(struct file *file, char *buf, size_t size);
> >   static ssize_t write_maxblksize(struct file *file, char *buf, size_t size);
> > -static ssize_t write_maxconn(struct file *file, char *buf, size_t size);
> >   #ifdef CONFIG_NFSD_V4
> >   static ssize_t write_leasetime(struct file *file, char *buf, size_t size);
> >   static ssize_t write_gracetime(struct file *file, char *buf, size_t size);
> > @@ -87,7 +85,6 @@ static ssize_t (*const write_op[])(struct file *, char *, size_t) = {
> >   	[NFSD_Versions] = write_versions,
> >   	[NFSD_Ports] = write_ports,
> >   	[NFSD_MaxBlkSize] = write_maxblksize,
> > -	[NFSD_MaxConnections] = write_maxconn,
> >   #ifdef CONFIG_NFSD_V4
> >   	[NFSD_Leasetime] = write_leasetime,
> >   	[NFSD_Gracetime] = write_gracetime,
> > @@ -902,44 +899,6 @@ static ssize_t write_maxblksize(struct file *file, char *buf, size_t size)
> >   							nfsd_max_blksize);
> >   }
> >   
> > -/*
> > - * write_maxconn - Set or report the current max number of connections
> > - *
> > - * Input:
> > - *			buf:		ignored
> > - *			size:		zero
> > - * OR
> > - *
> > - * Input:
> > - *			buf:		C string containing an unsigned
> > - *					integer value representing the new
> > - *					number of max connections
> > - *			size:		non-zero length of C string in @buf
> > - * Output:
> > - *	On success:	passed-in buffer filled with '\n'-terminated C string
> > - *			containing numeric value of max_connections setting
> > - *			for this net namespace;
> > - *			return code is the size in bytes of the string
> > - *	On error:	return code is zero or a negative errno value
> > - */
> > -static ssize_t write_maxconn(struct file *file, char *buf, size_t size)
> > -{
> > -	char *mesg = buf;
> > -	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
> > -	unsigned int maxconn = nn->max_connections;
> > -
> > -	if (size > 0) {
> > -		int rv = get_uint(&mesg, &maxconn);
> > -
> > -		if (rv)
> > -			return rv;
> > -		trace_nfsd_ctl_maxconn(netns(file), maxconn);
> > -		nn->max_connections = maxconn;
> > -	}
> > -
> > -	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%u\n", maxconn);
> > -}
> > -
> >   #ifdef CONFIG_NFSD_V4
> >   static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,
> >   				  time64_t *time, struct nfsd_net *nn)
> > @@ -1372,7 +1331,6 @@ static int nfsd_fill_super(struct super_block *sb, struct fs_context *fc)
> >   		[NFSD_Versions] = {"versions", &transaction_ops, S_IWUSR|S_IRUSR},
> >   		[NFSD_Ports] = {"portlist", &transaction_ops, S_IWUSR|S_IRUGO},
> >   		[NFSD_MaxBlkSize] = {"max_block_size", &transaction_ops, S_IWUSR|S_IRUGO},
> > -		[NFSD_MaxConnections] = {"max_connections", &transaction_ops, S_IWUSR|S_IRUGO},
> >   		[NFSD_Filecache] = {"filecache", &nfsd_file_cache_stats_fops, S_IRUGO},
> >   #ifdef CONFIG_NFSD_V4
> >   		[NFSD_Leasetime] = {"nfsv4leasetime", &transaction_ops, S_IWUSR|S_IRUSR},
> > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > index 49e2f32102ab..b77097de5936 100644
> > --- a/fs/nfsd/nfssvc.c
> > +++ b/fs/nfsd/nfssvc.c
> > @@ -668,7 +668,6 @@ int nfsd_create_serv(struct net *net)
> >   	if (serv == NULL)
> >   		return -ENOMEM;
> >   
> > -	serv->sv_maxconn = nn->max_connections;
> >   	error = svc_bind(serv, net);
> >   	if (error < 0) {
> >   		svc_destroy(&serv);
> > @@ -954,11 +953,7 @@ nfsd(void *vrqstp)
> >   	 * The main request loop
> >   	 */
> >   	while (!svc_thread_should_stop(rqstp)) {
> > -		/* Update sv_maxconn if it has changed */
> > -		rqstp->rq_server->sv_maxconn = nn->max_connections;
> > -
> >   		svc_recv(rqstp);
> > -
> >   		nfsd_file_net_dispose(nn);
> >   	}
> >   
> > diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> > index 617ebfff2f30..9d288a673705 100644
> > --- a/include/linux/sunrpc/svc.h
> > +++ b/include/linux/sunrpc/svc.h
> > @@ -72,10 +72,6 @@ struct svc_serv {
> >   	spinlock_t		sv_lock;
> >   	unsigned int		sv_nprogs;	/* Number of sv_programs */
> >   	unsigned int		sv_nrthreads;	/* # of server threads */
> > -	unsigned int		sv_maxconn;	/* max connections allowed or
> > -						 * '0' causing max to be based
> > -						 * on number of threads. */
> > -
> >   	unsigned int		sv_max_payload;	/* datagram payload size */
> >   	unsigned int		sv_max_mesg;	/* max_payload + 1 page for overheads */
> >   	unsigned int		sv_xdrsize;	/* XDR buffer size */
> > diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
> > index 35929a7727c7..114051ad985a 100644
> > --- a/include/linux/sunrpc/svc_xprt.h
> > +++ b/include/linux/sunrpc/svc_xprt.h
> > @@ -105,6 +105,12 @@ enum {
> >   				 */
> >   };
> >   
> > +/*
> > + * Maximum number of "tmp" connections - those without XPT_PEER_VALID -
> > + * permitted on any service.
> > + */
> > +#define XPT_MAX_TMP_CONN	64
> > +
> >   static inline void svc_xprt_set_valid(struct svc_xprt *xpt)
> >   {
> >   	if (test_bit(XPT_TEMP, &xpt->xpt_flags) &&
> > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> > index ff5b8bb8a88f..070bdeb50496 100644
> > --- a/net/sunrpc/svc_xprt.c
> > +++ b/net/sunrpc/svc_xprt.c
> > @@ -619,16 +619,10 @@ int svc_port_is_privileged(struct sockaddr *sin)
> >    * The only somewhat efficient mechanism would be if drop old
> >    * connections from the same IP first. But right now we don't even
> >    * record the client IP in svc_sock.
> > - *
> > - * single-threaded services that expect a lot of clients will probably
> > - * need to set sv_maxconn to override the default value which is based
> > - * on the number of threads
> >    */
> >   static void svc_check_conn_limits(struct svc_serv *serv)
> >   {
> > -	unsigned int limit = serv->sv_maxconn ? serv->sv_maxconn : 64;
> > -
> > -	if (serv->sv_tmpcnt > limit) {
> > +	if (serv->sv_tmpcnt > XPT_MAX_TMP_CONN) {
> >   		struct svc_xprt *xprt = NULL, *xprti;
> >   		spin_lock_bh(&serv->sv_lock);
> >   		if (!list_empty(&serv->sv_tempsocks)) {
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>