[PATCH 0/2] nfsd: don't use sv_nrthreads in connection limiting

[PATCH 0/2] nfsd: don't use sv_nrthreads in connection limiting

[NeilBrown]

This is a resend of a couple of patches from a series that didn't get
accepted in its entirety, but I think these patches were not objected
to.  Hopefully they can land so that when I get back to the series it
will be smaller.

Thanks,
NeilBrown

[PATCH 1/2] nfsd: don't use sv_nrthreads in connection limiting calculations.

[NeilBrown]

The heuristic for limiting the number of incoming connections to nfsd
currently uses sv_nrthreads - allowing more connections if more threads
were configured.

A future patch will allow number of threads to grow dynamically so that
there will be no need to configure sv_nrthreads.  So we need a different
solution for limiting connections.

It isn't clear what problem is solved by limiting connections (as
mentioned in a code comment) but the most likely problem is a connection
storm - many connections that are not doing productive work.  These will
be closed after about 6 minutes already but it might help to slow down a
storm.

This patch adds a per-connection flag XPT_PEER_VALID which indicates
that the peer has presented a filehandle for which it has some sort of
access.  i.e the peer is known to be trusted in some way.  We now only
count connections which have NOT been determined to be valid.  There
should be relative few of these at any given time.

If the number of non-validated peer exceed a limit - currently 64 - we
close the oldest non-validated peer to avoid having too many of these
useless connections.

Note that this patch significantly changes the meaning of the various
configuration parameters for "max connections".  The next patch will
remove all of these.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfs/callback.c               |  4 ----
 fs/nfs/callback_xdr.c           |  1 +
 fs/nfsd/netns.h                 |  4 ++--
 fs/nfsd/nfsfh.c                 |  2 ++
 include/linux/sunrpc/svc.h      |  2 +-
 include/linux/sunrpc/svc_xprt.h | 15 +++++++++++++++
 net/sunrpc/svc_xprt.c           | 33 +++++++++++++++++----------------
 7 files changed, 38 insertions(+), 23 deletions(-)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 6cf92498a5ac..86bdc7d23fb9 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -211,10 +211,6 @@ static struct svc_serv *nfs_callback_create_svc(int minorversion)
 		return ERR_PTR(-ENOMEM);
 	}
 	cb_info->serv = serv;
-	/* As there is only one thread we need to over-ride the
-	 * default maximum of 80 connections
-	 */
-	serv->sv_maxconn = 1024;
 	dprintk("nfs_callback_create_svc: service created\n");
 	return serv;
 }
diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
index fdeb0b34a3d3..4254ba3ee7c5 100644
--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -984,6 +984,7 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp)
 			nfs_put_client(cps.clp);
 			goto out_invalidcred;
 		}
+		svc_xprt_set_valid(rqstp->rq_xprt);
 	}
 
 	cps.minorversion = hdr_arg.minorversion;
diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index 26f7b34d1a03..a05a45bb1978 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -129,8 +129,8 @@ struct nfsd_net {
 	unsigned char writeverf[8];
 
 	/*
-	 * Max number of connections this nfsd container will allow. Defaults
-	 * to '0' which is means that it bases this on the number of threads.
+	 * Max number of non-validated connections this nfsd container
+	 * will allow.  Defaults to '0' gets mapped to 64.
 	 */
 	unsigned int max_connections;
 
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 6a831cb242df..bf59f83c6224 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -382,6 +382,8 @@ __fh_verify(struct svc_rqst *rqstp,
 	if (error)
 		goto out;
 
+	svc_xprt_set_valid(rqstp->rq_xprt);
+
 	/* Finally, check access permissions. */
 	error = nfsd_permission(cred, exp, dentry, access);
 out:
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index e68fecf6eab5..617ebfff2f30 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -81,7 +81,7 @@ struct svc_serv {
 	unsigned int		sv_xdrsize;	/* XDR buffer size */
 	struct list_head	sv_permsocks;	/* all permanent sockets */
 	struct list_head	sv_tempsocks;	/* all temporary sockets */
-	int			sv_tmpcnt;	/* count of temporary sockets */
+	int			sv_tmpcnt;	/* count of temporary "valid" sockets */
 	struct timer_list	sv_temptimer;	/* timer for aging temporary sockets */
 
 	char *			sv_name;	/* service name */
diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index 0981e35a9fed..35929a7727c7 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -99,8 +99,23 @@ enum {
 	XPT_HANDSHAKE,		/* xprt requests a handshake */
 	XPT_TLS_SESSION,	/* transport-layer security established */
 	XPT_PEER_AUTH,		/* peer has been authenticated */
+	XPT_PEER_VALID,		/* peer has presented a filehandle that
+				 * it has access to.  It is NOT counted
+				 * in ->sv_tmpcnt.
+				 */
 };
 
+static inline void svc_xprt_set_valid(struct svc_xprt *xpt)
+{
+	if (test_bit(XPT_TEMP, &xpt->xpt_flags) &&
+	    !test_and_set_bit(XPT_PEER_VALID, &xpt->xpt_flags)) {
+		struct svc_serv *serv = xpt->xpt_server;
+		spin_lock(&serv->sv_lock);
+		serv->sv_tmpcnt -= 1;
+		spin_unlock(&serv->sv_lock);
+	}
+}
+
 static inline void unregister_xpt_user(struct svc_xprt *xpt, struct svc_xpt_user *u)
 {
 	spin_lock(&xpt->xpt_lock);
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 43c57124de52..ff5b8bb8a88f 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -606,7 +606,8 @@ int svc_port_is_privileged(struct sockaddr *sin)
 }
 
 /*
- * Make sure that we don't have too many active connections. If we have,
+ * Make sure that we don't have too many connections that have not yet
+ * demonstrated that they have access the the NFS server. If we have,
  * something must be dropped. It's not clear what will happen if we allow
  * "too many" connections, but when dealing with network-facing software,
  * we have to code defensively. Here we do that by imposing hard limits.
@@ -625,27 +626,26 @@ int svc_port_is_privileged(struct sockaddr *sin)
  */
 static void svc_check_conn_limits(struct svc_serv *serv)
 {
-	unsigned int limit = serv->sv_maxconn ? serv->sv_maxconn :
-				(serv->sv_nrthreads+3) * 20;
+	unsigned int limit = serv->sv_maxconn ? serv->sv_maxconn : 64;
 
 	if (serv->sv_tmpcnt > limit) {
-		struct svc_xprt *xprt = NULL;
+		struct svc_xprt *xprt = NULL, *xprti;
 		spin_lock_bh(&serv->sv_lock);
 		if (!list_empty(&serv->sv_tempsocks)) {
-			/* Try to help the admin */
-			net_notice_ratelimited("%s: too many open connections, consider increasing the %s\n",
-					       serv->sv_name, serv->sv_maxconn ?
-					       "max number of connections" :
-					       "number of threads");
 			/*
 			 * Always select the oldest connection. It's not fair,
-			 * but so is life
+			 * but nor is life.
 			 */
-			xprt = list_entry(serv->sv_tempsocks.prev,
-					  struct svc_xprt,
-					  xpt_list);
-			set_bit(XPT_CLOSE, &xprt->xpt_flags);
-			svc_xprt_get(xprt);
+			list_for_each_entry_reverse(xprti, &serv->sv_tempsocks,
+						    xpt_list)
+			{
+				if (!test_bit(XPT_PEER_VALID, &xprti->xpt_flags)) {
+					xprt = xprti;
+					set_bit(XPT_CLOSE, &xprt->xpt_flags);
+					svc_xprt_get(xprt);
+					break;
+				}
+			}
 		}
 		spin_unlock_bh(&serv->sv_lock);
 
@@ -1039,7 +1039,8 @@ static void svc_delete_xprt(struct svc_xprt *xprt)
 
 	spin_lock_bh(&serv->sv_lock);
 	list_del_init(&xprt->xpt_list);
-	if (test_bit(XPT_TEMP, &xprt->xpt_flags))
+	if (test_bit(XPT_TEMP, &xprt->xpt_flags) &&
+	    !test_bit(XPT_PEER_VALID, &xprt->xpt_flags))
 		serv->sv_tmpcnt--;
 	spin_unlock_bh(&serv->sv_lock);
 
-- 
2.47.0

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

[NeilBrown]

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);
 
 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)) {
-- 
2.47.0

Re: [PATCH 0/2] nfsd: don't use sv_nrthreads in connection limiting

[Jeff Layton]

On Mon, 2024-12-09 at 11:41 +1100, NeilBrown wrote:
> This is a resend of a couple of patches from a series that didn't get
> accepted in its entirety, but I think these patches were not objected
> to.  Hopefully they can land so that when I get back to the series it
> will be smaller.
> 
> Thanks,
> NeilBrown
> 
> 

The new heuristic makes a lot more sense than the hard cap that we have
now:

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/2] nfsd: don't use sv_nrthreads in connection limiting calculations.

[Chuck Lever]

On 12/8/24 7:41 PM, NeilBrown wrote:
> The heuristic for limiting the number of incoming connections to nfsd
> currently uses sv_nrthreads - allowing more connections if more threads
> were configured.
> 
> A future patch will allow number of threads to grow dynamically so that
> there will be no need to configure sv_nrthreads.  So we need a different
> solution for limiting connections.
> 
> It isn't clear what problem is solved by limiting connections (as
> mentioned in a code comment) but the most likely problem is a connection
> storm - many connections that are not doing productive work.  These will
> be closed after about 6 minutes already but it might help to slow down a
> storm.
> 
> This patch adds a per-connection flag XPT_PEER_VALID which indicates
> that the peer has presented a filehandle for which it has some sort of
> access.  i.e the peer is known to be trusted in some way.  We now only
> count connections which have NOT been determined to be valid.  There
> should be relative few of these at any given time.
> 
> If the number of non-validated peer exceed a limit - currently 64 - we
> close the oldest non-validated peer to avoid having too many of these
> useless connections.
> 
> Note that this patch significantly changes the meaning of the various
> configuration parameters for "max connections".  The next patch will
> remove all of these.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>   fs/nfs/callback.c               |  4 ----
>   fs/nfs/callback_xdr.c           |  1 +

I've pulled this one into nfsd-testing, but it would be great if the
fs/nfs/ hunks could be Acked-by a client maintainer.


>   fs/nfsd/netns.h                 |  4 ++--
>   fs/nfsd/nfsfh.c                 |  2 ++
>   include/linux/sunrpc/svc.h      |  2 +-
>   include/linux/sunrpc/svc_xprt.h | 15 +++++++++++++++
>   net/sunrpc/svc_xprt.c           | 33 +++++++++++++++++----------------
>   7 files changed, 38 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> index 6cf92498a5ac..86bdc7d23fb9 100644
> --- a/fs/nfs/callback.c
> +++ b/fs/nfs/callback.c
> @@ -211,10 +211,6 @@ static struct svc_serv *nfs_callback_create_svc(int minorversion)
>   		return ERR_PTR(-ENOMEM);
>   	}
>   	cb_info->serv = serv;
> -	/* As there is only one thread we need to over-ride the
> -	 * default maximum of 80 connections
> -	 */
> -	serv->sv_maxconn = 1024;
>   	dprintk("nfs_callback_create_svc: service created\n");
>   	return serv;
>   }
> diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
> index fdeb0b34a3d3..4254ba3ee7c5 100644
> --- a/fs/nfs/callback_xdr.c
> +++ b/fs/nfs/callback_xdr.c
> @@ -984,6 +984,7 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp)
>   			nfs_put_client(cps.clp);
>   			goto out_invalidcred;
>   		}
> +		svc_xprt_set_valid(rqstp->rq_xprt);
>   	}
>   
>   	cps.minorversion = hdr_arg.minorversion;
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index 26f7b34d1a03..a05a45bb1978 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -129,8 +129,8 @@ struct nfsd_net {
>   	unsigned char writeverf[8];
>   
>   	/*
> -	 * Max number of connections this nfsd container will allow. Defaults
> -	 * to '0' which is means that it bases this on the number of threads.
> +	 * Max number of non-validated connections this nfsd container
> +	 * will allow.  Defaults to '0' gets mapped to 64.
>   	 */
>   	unsigned int max_connections;
>   
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 6a831cb242df..bf59f83c6224 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -382,6 +382,8 @@ __fh_verify(struct svc_rqst *rqstp,
>   	if (error)
>   		goto out;
>   
> +	svc_xprt_set_valid(rqstp->rq_xprt);
> +
>   	/* Finally, check access permissions. */
>   	error = nfsd_permission(cred, exp, dentry, access);
>   out:
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index e68fecf6eab5..617ebfff2f30 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -81,7 +81,7 @@ struct svc_serv {
>   	unsigned int		sv_xdrsize;	/* XDR buffer size */
>   	struct list_head	sv_permsocks;	/* all permanent sockets */
>   	struct list_head	sv_tempsocks;	/* all temporary sockets */
> -	int			sv_tmpcnt;	/* count of temporary sockets */
> +	int			sv_tmpcnt;	/* count of temporary "valid" sockets */
>   	struct timer_list	sv_temptimer;	/* timer for aging temporary sockets */
>   
>   	char *			sv_name;	/* service name */
> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
> index 0981e35a9fed..35929a7727c7 100644
> --- a/include/linux/sunrpc/svc_xprt.h
> +++ b/include/linux/sunrpc/svc_xprt.h
> @@ -99,8 +99,23 @@ enum {
>   	XPT_HANDSHAKE,		/* xprt requests a handshake */
>   	XPT_TLS_SESSION,	/* transport-layer security established */
>   	XPT_PEER_AUTH,		/* peer has been authenticated */
> +	XPT_PEER_VALID,		/* peer has presented a filehandle that
> +				 * it has access to.  It is NOT counted
> +				 * in ->sv_tmpcnt.
> +				 */
>   };
>   
> +static inline void svc_xprt_set_valid(struct svc_xprt *xpt)
> +{
> +	if (test_bit(XPT_TEMP, &xpt->xpt_flags) &&
> +	    !test_and_set_bit(XPT_PEER_VALID, &xpt->xpt_flags)) {
> +		struct svc_serv *serv = xpt->xpt_server;
> +		spin_lock(&serv->sv_lock);
> +		serv->sv_tmpcnt -= 1;
> +		spin_unlock(&serv->sv_lock);
> +	}
> +}
> +
>   static inline void unregister_xpt_user(struct svc_xprt *xpt, struct svc_xpt_user *u)
>   {
>   	spin_lock(&xpt->xpt_lock);
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 43c57124de52..ff5b8bb8a88f 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -606,7 +606,8 @@ int svc_port_is_privileged(struct sockaddr *sin)
>   }
>   
>   /*
> - * Make sure that we don't have too many active connections. If we have,
> + * Make sure that we don't have too many connections that have not yet
> + * demonstrated that they have access the the NFS server. If we have,
>    * something must be dropped. It's not clear what will happen if we allow
>    * "too many" connections, but when dealing with network-facing software,
>    * we have to code defensively. Here we do that by imposing hard limits.
> @@ -625,27 +626,26 @@ int svc_port_is_privileged(struct sockaddr *sin)
>    */
>   static void svc_check_conn_limits(struct svc_serv *serv)
>   {
> -	unsigned int limit = serv->sv_maxconn ? serv->sv_maxconn :
> -				(serv->sv_nrthreads+3) * 20;
> +	unsigned int limit = serv->sv_maxconn ? serv->sv_maxconn : 64;
>   
>   	if (serv->sv_tmpcnt > limit) {
> -		struct svc_xprt *xprt = NULL;
> +		struct svc_xprt *xprt = NULL, *xprti;
>   		spin_lock_bh(&serv->sv_lock);
>   		if (!list_empty(&serv->sv_tempsocks)) {
> -			/* Try to help the admin */
> -			net_notice_ratelimited("%s: too many open connections, consider increasing the %s\n",
> -					       serv->sv_name, serv->sv_maxconn ?
> -					       "max number of connections" :
> -					       "number of threads");
>   			/*
>   			 * Always select the oldest connection. It's not fair,
> -			 * but so is life
> +			 * but nor is life.
>   			 */
> -			xprt = list_entry(serv->sv_tempsocks.prev,
> -					  struct svc_xprt,
> -					  xpt_list);
> -			set_bit(XPT_CLOSE, &xprt->xpt_flags);
> -			svc_xprt_get(xprt);
> +			list_for_each_entry_reverse(xprti, &serv->sv_tempsocks,
> +						    xpt_list)
> +			{
> +				if (!test_bit(XPT_PEER_VALID, &xprti->xpt_flags)) {
> +					xprt = xprti;
> +					set_bit(XPT_CLOSE, &xprt->xpt_flags);
> +					svc_xprt_get(xprt);
> +					break;
> +				}
> +			}
>   		}
>   		spin_unlock_bh(&serv->sv_lock);
>   
> @@ -1039,7 +1039,8 @@ static void svc_delete_xprt(struct svc_xprt *xprt)
>   
>   	spin_lock_bh(&serv->sv_lock);
>   	list_del_init(&xprt->xpt_list);
> -	if (test_bit(XPT_TEMP, &xprt->xpt_flags))
> +	if (test_bit(XPT_TEMP, &xprt->xpt_flags) &&
> +	    !test_bit(XPT_PEER_VALID, &xprt->xpt_flags))
>   		serv->sv_tmpcnt--;
>   	spin_unlock_bh(&serv->sv_lock);
>   


-- 
Chuck Lever

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

[Chuck Lever]

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?


>   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)) {


-- 
Chuck Lever

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

[Jeff Layton]

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>

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

[NeilBrown]

On Tue, 10 Dec 2024, Jeff Layton wrote:
> On Mon, 2024-12-09 at 12:27 -0500, Chuck Lever wrote:
> > On 12/8/24 7:41 PM, NeilBrown wrote:

> > >   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.
> 

What might break?

modprobe or insmod won't have a problem.  The kernel will emit 
   nfsd: unknow parameter '...' ignored

but that is not much more than a deprecation warning.

 echo 42 > /sys/modules/nfsd/parameters/nlm_max_connections

will produce "permission denied" but is unlikely to abort a shell script
(unless "-e" is used - which seems unlikely).

I don't think there is a risk here.

Thanks,
NeilBrown