[PATCH v2 0/5] nfsd/sunrpc: allow starting/stopping pooled NFS server via netlink

[PATCH v2 0/5] nfsd/sunrpc: allow starting/stopping pooled NFS server via netlink

[Jeff Layton]

This is a resend of the patchset I sent a little over a week ago, with
a couple of new patches that allow setting the pool-mode via netlink.

This patchset first attempts to detangle the pooled/non-pooled service
handling in the sunrpc layer, unifies the codepaths that start the
pooled vs. non-pooled nfsd, and then wires up the new netlink threads
interface to allow you to start a pooled server by specifying an
array of thread counts.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v2:
- add new pool-mode set/get netlink calls

---
Jeff Layton (5):
      sunrpc: fix up the special handling of sv_nrpools == 1
      nfsd: make nfsd_svc call nfsd_set_nrthreads
      nfsd: allow passing in array of thread counts via netlink
      sunrpc: refactor pool_mode setting code
      nfsd: new netlink ops to get/set server pool_mode

 Documentation/netlink/specs/nfsd.yaml |  27 +++++++++
 fs/nfsd/netlink.c                     |  17 ++++++
 fs/nfsd/netlink.h                     |   2 +
 fs/nfsd/nfsctl.c                      | 102 +++++++++++++++++++++++++++++-----
 fs/nfsd/nfsd.h                        |   3 +-
 fs/nfsd/nfssvc.c                      |  30 +++++-----
 include/linux/sunrpc/svc.h            |   3 +
 include/uapi/linux/nfsd_netlink.h     |  10 ++++
 net/sunrpc/svc.c                      | 102 +++++++++++++++++++++-------------
 9 files changed, 225 insertions(+), 71 deletions(-)
---
base-commit: fec4124bac55ad92c47585fe537e646fe108b8fa
change-id: 20240604-nfsd-next-b04c0d2d89a9

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH v2 1/5] sunrpc: fix up the special handling of sv_nrpools == 1

[Jeff Layton]

Only pooled services take a reference to the svc_pool_map. The sunrpc
code has always used the sv_nrpools value to detect whether the service
is pooled.

The problem there is that nfsd is a pooled service, but when it's
running in "global" pool_mode, it doesn't take a reference to the pool
map because it has a sv_nrpools value of 1. This means that we have
two separate codepaths for starting the server, depending on whether
it's pooled or not.

Fix this by adding a new flag to the svc_serv, that indicates whether
the serv is pooled. With this we can have the nfsd service
unconditionally take a reference, regardless of pool_mode.

Ideally we should prevent anyone from changing the pool mode while the
server is running, and nfsd does this if the server is in percpu or
pernode mode. Previously, you could change the pool_mode even if there
were nfsd's already running in "global" mode. This fixes that problem as
well.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 include/linux/sunrpc/svc.h |  1 +
 net/sunrpc/svc.c           | 26 +++++++-------------------
 2 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 23617da0e565..d0433e1642b3 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -85,6 +85,7 @@ struct svc_serv {
 	char *			sv_name;	/* service name */
 
 	unsigned int		sv_nrpools;	/* number of thread pools */
+	bool			sv_is_pooled;	/* is this a pooled service? */
 	struct svc_pool *	sv_pools;	/* array of thread pools */
 	int			(*sv_threadfn)(void *data);
 
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 2b4b1276d4e8..f80d94cbb8b1 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -250,10 +250,8 @@ svc_pool_map_get(void)
 	int npools = -1;
 
 	mutex_lock(&svc_pool_map_mutex);
-
 	if (m->count++) {
 		mutex_unlock(&svc_pool_map_mutex);
-		WARN_ON_ONCE(m->npools <= 1);
 		return m->npools;
 	}
 
@@ -275,32 +273,21 @@ svc_pool_map_get(void)
 		m->mode = SVC_POOL_GLOBAL;
 	}
 	m->npools = npools;
-
-	if (npools == 1)
-		/* service is unpooled, so doesn't hold a reference */
-		m->count--;
-
 	mutex_unlock(&svc_pool_map_mutex);
 	return npools;
 }
 
 /*
- * Drop a reference to the global map of cpus to pools, if
- * pools were in use, i.e. if npools > 1.
+ * Drop a reference to the global map of cpus to pools.
  * When the last reference is dropped, the map data is
- * freed; this allows the sysadmin to change the pool
- * mode using the pool_mode module option without
- * rebooting or re-loading sunrpc.ko.
+ * freed; this allows the sysadmin to change the pool.
  */
 static void
-svc_pool_map_put(int npools)
+svc_pool_map_put(void)
 {
 	struct svc_pool_map *m = &svc_pool_map;
 
-	if (npools <= 1)
-		return;
 	mutex_lock(&svc_pool_map_mutex);
-
 	if (!--m->count) {
 		kfree(m->to_pool);
 		m->to_pool = NULL;
@@ -308,7 +295,6 @@ svc_pool_map_put(int npools)
 		m->pool_to = NULL;
 		m->npools = 0;
 	}
-
 	mutex_unlock(&svc_pool_map_mutex);
 }
 
@@ -553,9 +539,10 @@ struct svc_serv *svc_create_pooled(struct svc_program *prog,
 	serv = __svc_create(prog, stats, bufsize, npools, threadfn);
 	if (!serv)
 		goto out_err;
+	serv->sv_is_pooled = true;
 	return serv;
 out_err:
-	svc_pool_map_put(npools);
+	svc_pool_map_put();
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(svc_create_pooled);
@@ -585,7 +572,8 @@ svc_destroy(struct svc_serv **servp)
 
 	cache_clean_deferred(serv);
 
-	svc_pool_map_put(serv->sv_nrpools);
+	if (serv->sv_is_pooled)
+		svc_pool_map_put();
 
 	for (i = 0; i < serv->sv_nrpools; i++) {
 		struct svc_pool *pool = &serv->sv_pools[i];

-- 
2.45.2

[PATCH v2 2/5] nfsd: make nfsd_svc call nfsd_set_nrthreads

[Jeff Layton]

Now that the refcounting is fixed, rework nfsd_svc to use the same
thread setup as the pool_threads interface. Since the new netlink
interface doesn't have the same restriction as pool_threads, move the
guard against shutting down all threads to write_pool_threads.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfsctl.c | 14 ++++++++++++--
 fs/nfsd/nfsd.h   |  3 ++-
 fs/nfsd/nfssvc.c | 18 ++----------------
 3 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 202140df8f82..121b866125d4 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -406,7 +406,7 @@ static ssize_t write_threads(struct file *file, char *buf, size_t size)
 			return -EINVAL;
 		trace_nfsd_ctl_threads(net, newthreads);
 		mutex_lock(&nfsd_mutex);
-		rv = nfsd_svc(newthreads, net, file->f_cred, NULL);
+		rv = nfsd_svc(1, &newthreads, net, file->f_cred, NULL);
 		mutex_unlock(&nfsd_mutex);
 		if (rv < 0)
 			return rv;
@@ -481,6 +481,16 @@ static ssize_t write_pool_threads(struct file *file, char *buf, size_t size)
 				goto out_free;
 			trace_nfsd_ctl_pool_threads(net, i, nthreads[i]);
 		}
+
+		/*
+		 * There must always be a thread in pool 0; the admin
+		 * can't shut down NFS completely using pool_threads.
+		 *
+		 * FIXME: do we really need this?
+		 */
+		if (nthreads[0] == 0)
+			nthreads[0] = 1;
+
 		rv = nfsd_set_nrthreads(i, nthreads, net);
 		if (rv)
 			goto out_free;
@@ -1722,7 +1732,7 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
 			scope = nla_data(attr);
 	}
 
-	ret = nfsd_svc(nthreads, net, get_current_cred(), scope);
+	ret = nfsd_svc(1, &nthreads, net, get_current_cred(), scope);
 
 out_unlock:
 	mutex_unlock(&nfsd_mutex);
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 8f4f239d9f8a..cec8697b1cd6 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -103,7 +103,8 @@ bool		nfssvc_encode_voidres(struct svc_rqst *rqstp,
 /*
  * Function prototypes.
  */
-int		nfsd_svc(int nrservs, struct net *net, const struct cred *cred, const char *scope);
+int		nfsd_svc(int n, int *nservers, struct net *net,
+			 const struct cred *cred, const char *scope);
 int		nfsd_dispatch(struct svc_rqst *rqstp);
 
 int		nfsd_nrthreads(struct net *);
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index cd9a6a1a9fc8..076f35dc17e4 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -744,13 +744,6 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
 		}
 	}
 
-	/*
-	 * There must always be a thread in pool 0; the admin
-	 * can't shut down NFS completely using pool_threads.
-	 */
-	if (nthreads[0] == 0)
-		nthreads[0] = 1;
-
 	/* apply the new numbers */
 	for (i = 0; i < n; i++) {
 		err = svc_set_num_threads(nn->nfsd_serv,
@@ -768,7 +761,7 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
  * this is the first time nrservs is nonzero.
  */
 int
-nfsd_svc(int nrservs, struct net *net, const struct cred *cred, const char *scope)
+nfsd_svc(int n, int *nthreads, struct net *net, const struct cred *cred, const char *scope)
 {
 	int	error;
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
@@ -778,13 +771,6 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred, const char *scop
 
 	dprintk("nfsd: creating service\n");
 
-	nrservs = max(nrservs, 0);
-	nrservs = min(nrservs, NFSD_MAXSERVS);
-	error = 0;
-
-	if (nrservs == 0 && nn->nfsd_serv == NULL)
-		goto out;
-
 	strscpy(nn->nfsd_name, scope ? scope : utsname()->nodename,
 		sizeof(nn->nfsd_name));
 
@@ -796,7 +782,7 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred, const char *scop
 	error = nfsd_startup_net(net, cred);
 	if (error)
 		goto out_put;
-	error = svc_set_num_threads(serv, NULL, nrservs);
+	error = nfsd_set_nrthreads(n, nthreads, net);
 	if (error)
 		goto out_put;
 	error = serv->sv_nrthreads;

-- 
2.45.2

Re: [PATCH v2 2/5] nfsd: make nfsd_svc call nfsd_set_nrthreads

[Chuck Lever]

On Thu, Jun 13, 2024 at 08:16:39AM -0400, Jeff Layton wrote:
> Now that the refcounting is fixed, rework nfsd_svc to use the same
> thread setup as the pool_threads interface. Since the new netlink
> interface doesn't have the same restriction as pool_threads, move the
> guard against shutting down all threads to write_pool_threads.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/nfsctl.c | 14 ++++++++++++--
>  fs/nfsd/nfsd.h   |  3 ++-
>  fs/nfsd/nfssvc.c | 18 ++----------------
>  3 files changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 202140df8f82..121b866125d4 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -406,7 +406,7 @@ static ssize_t write_threads(struct file *file, char *buf, size_t size)
>  			return -EINVAL;
>  		trace_nfsd_ctl_threads(net, newthreads);
>  		mutex_lock(&nfsd_mutex);
> -		rv = nfsd_svc(newthreads, net, file->f_cred, NULL);
> +		rv = nfsd_svc(1, &newthreads, net, file->f_cred, NULL);
>  		mutex_unlock(&nfsd_mutex);
>  		if (rv < 0)
>  			return rv;
> @@ -481,6 +481,16 @@ static ssize_t write_pool_threads(struct file *file, char *buf, size_t size)
>  				goto out_free;
>  			trace_nfsd_ctl_pool_threads(net, i, nthreads[i]);
>  		}
> +
> +		/*
> +		 * There must always be a thread in pool 0; the admin
> +		 * can't shut down NFS completely using pool_threads.
> +		 *
> +		 * FIXME: do we really need this?

Hi, how do you plan to decide this question?


> +		 */
> +		if (nthreads[0] == 0)
> +			nthreads[0] = 1;
> +
>  		rv = nfsd_set_nrthreads(i, nthreads, net);
>  		if (rv)
>  			goto out_free;
> @@ -1722,7 +1732,7 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
>  			scope = nla_data(attr);
>  	}
>  
> -	ret = nfsd_svc(nthreads, net, get_current_cred(), scope);
> +	ret = nfsd_svc(1, &nthreads, net, get_current_cred(), scope);
>  
>  out_unlock:
>  	mutex_unlock(&nfsd_mutex);
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index 8f4f239d9f8a..cec8697b1cd6 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -103,7 +103,8 @@ bool		nfssvc_encode_voidres(struct svc_rqst *rqstp,
>  /*
>   * Function prototypes.
>   */
> -int		nfsd_svc(int nrservs, struct net *net, const struct cred *cred, const char *scope);
> +int		nfsd_svc(int n, int *nservers, struct net *net,
> +			 const struct cred *cred, const char *scope);
>  int		nfsd_dispatch(struct svc_rqst *rqstp);
>  
>  int		nfsd_nrthreads(struct net *);
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index cd9a6a1a9fc8..076f35dc17e4 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -744,13 +744,6 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)

Since you are slightly changing the API contract for this publicly
visible function, now would be a good time to add a kdoc comment.


>  		}
>  	}
>  
> -	/*
> -	 * There must always be a thread in pool 0; the admin
> -	 * can't shut down NFS completely using pool_threads.
> -	 */
> -	if (nthreads[0] == 0)
> -		nthreads[0] = 1;
> -
>  	/* apply the new numbers */
>  	for (i = 0; i < n; i++) {
>  		err = svc_set_num_threads(nn->nfsd_serv,
> @@ -768,7 +761,7 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
>   * this is the first time nrservs is nonzero.
>   */
>  int
> -nfsd_svc(int nrservs, struct net *net, const struct cred *cred, const char *scope)
> +nfsd_svc(int n, int *nthreads, struct net *net, const struct cred *cred, const char *scope)

Ditto: the patch changes the synopsis of nfsd_svc(), so I'd like a
kdoc comment to go with it.

And, this particular change is the reason for this patch, so the
description should state that (especially since subsequent
patch descriptions refer to "now that nfsd_svc takes an array
of threads..." : I had to come back to this patch and blink twice
to see why it said that).

A kdoc comment from sunrpc_get_pool_mode() should also be added
in 4/5.


>  {
>  	int	error;
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> @@ -778,13 +771,6 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred, const char *scop
>  
>  	dprintk("nfsd: creating service\n");
>  
> -	nrservs = max(nrservs, 0);
> -	nrservs = min(nrservs, NFSD_MAXSERVS);
> -	error = 0;
> -
> -	if (nrservs == 0 && nn->nfsd_serv == NULL)
> -		goto out;
> -
>  	strscpy(nn->nfsd_name, scope ? scope : utsname()->nodename,
>  		sizeof(nn->nfsd_name));
>  
> @@ -796,7 +782,7 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred, const char *scop
>  	error = nfsd_startup_net(net, cred);
>  	if (error)
>  		goto out_put;
> -	error = svc_set_num_threads(serv, NULL, nrservs);
> +	error = nfsd_set_nrthreads(n, nthreads, net);
>  	if (error)
>  		goto out_put;
>  	error = serv->sv_nrthreads;
> 
> -- 
> 2.45.2
> 

-- 
Chuck Lever

Re: [PATCH v2 2/5] nfsd: make nfsd_svc call nfsd_set_nrthreads

[Jeff Layton]

On Thu, 2024-06-13 at 11:57 -0400, Chuck Lever wrote:
> On Thu, Jun 13, 2024 at 08:16:39AM -0400, Jeff Layton wrote:
> > Now that the refcounting is fixed, rework nfsd_svc to use the same
> > thread setup as the pool_threads interface. Since the new netlink
> > interface doesn't have the same restriction as pool_threads, move
> > the
> > guard against shutting down all threads to write_pool_threads.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/nfsctl.c | 14 ++++++++++++--
> >  fs/nfsd/nfsd.h   |  3 ++-
> >  fs/nfsd/nfssvc.c | 18 ++----------------
> >  3 files changed, 16 insertions(+), 19 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 202140df8f82..121b866125d4 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -406,7 +406,7 @@ static ssize_t write_threads(struct file *file,
> > char *buf, size_t size)
> >  			return -EINVAL;
> >  		trace_nfsd_ctl_threads(net, newthreads);
> >  		mutex_lock(&nfsd_mutex);
> > -		rv = nfsd_svc(newthreads, net, file->f_cred,
> > NULL);
> > +		rv = nfsd_svc(1, &newthreads, net, file->f_cred,
> > NULL);
> >  		mutex_unlock(&nfsd_mutex);
> >  		if (rv < 0)
> >  			return rv;
> > @@ -481,6 +481,16 @@ static ssize_t write_pool_threads(struct file
> > *file, char *buf, size_t size)
> >  				goto out_free;
> >  			trace_nfsd_ctl_pool_threads(net, i,
> > nthreads[i]);
> >  		}
> > +
> > +		/*
> > +		 * There must always be a thread in pool 0; the
> > admin
> > +		 * can't shut down NFS completely using
> > pool_threads.
> > +		 *
> > +		 * FIXME: do we really need this?
> 
> Hi, how do you plan to decide this question?
> 

Probably by ignoring it and letting the restriction (eventually) die
with the old pool_threads interface. I'm amenable to dropping this
restriction altogether though.

> 
> > +		 */
> > +		if (nthreads[0] == 0)
> > +			nthreads[0] = 1;
> > +
> >  		rv = nfsd_set_nrthreads(i, nthreads, net);
> >  		if (rv)
> >  			goto out_free;
> > @@ -1722,7 +1732,7 @@ int nfsd_nl_threads_set_doit(struct sk_buff
> > *skb, struct genl_info *info)
> >  			scope = nla_data(attr);
> >  	}
> >  
> > -	ret = nfsd_svc(nthreads, net, get_current_cred(), scope);
> > +	ret = nfsd_svc(1, &nthreads, net, get_current_cred(),
> > scope);
> >  
> >  out_unlock:
> >  	mutex_unlock(&nfsd_mutex);
> > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> > index 8f4f239d9f8a..cec8697b1cd6 100644
> > --- a/fs/nfsd/nfsd.h
> > +++ b/fs/nfsd/nfsd.h
> > @@ -103,7 +103,8 @@
> > bool		nfssvc_encode_voidres(struct svc_rqst *rqstp,
> >  /*
> >   * Function prototypes.
> >   */
> > -int		nfsd_svc(int nrservs, struct net *net, const
> > struct cred *cred, const char *scope);
> > +int		nfsd_svc(int n, int *nservers, struct net *net,
> > +			 const struct cred *cred, const char
> > *scope);
> >  int		nfsd_dispatch(struct svc_rqst *rqstp);
> >  
> >  int		nfsd_nrthreads(struct net *);
> > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > index cd9a6a1a9fc8..076f35dc17e4 100644
> > --- a/fs/nfsd/nfssvc.c
> > +++ b/fs/nfsd/nfssvc.c
> > @@ -744,13 +744,6 @@ int nfsd_set_nrthreads(int n, int *nthreads,
> > struct net *net)
> 
> Since you are slightly changing the API contract for this publicly
> visible function, now would be a good time to add a kdoc comment.
> 

Ok.

> 
> >  		}
> >  	}
> >  
> > -	/*
> > -	 * There must always be a thread in pool 0; the admin
> > -	 * can't shut down NFS completely using pool_threads.
> > -	 */
> > -	if (nthreads[0] == 0)
> > -		nthreads[0] = 1;
> > -
> >  	/* apply the new numbers */
> >  	for (i = 0; i < n; i++) {
> >  		err = svc_set_num_threads(nn->nfsd_serv,
> > @@ -768,7 +761,7 @@ int nfsd_set_nrthreads(int n, int *nthreads,
> > struct net *net)
> >   * this is the first time nrservs is nonzero.
> >   */
> >  int
> > -nfsd_svc(int nrservs, struct net *net, const struct cred *cred,
> > const char *scope)
> > +nfsd_svc(int n, int *nthreads, struct net *net, const struct cred
> > *cred, const char *scope)
> 
> Ditto: the patch changes the synopsis of nfsd_svc(), so I'd like a
> kdoc comment to go with it.
> 
> And, this particular change is the reason for this patch, so the
> description should state that (especially since subsequent
> patch descriptions refer to "now that nfsd_svc takes an array
> of threads..." : I had to come back to this patch and blink twice
> to see why it said that).
> 
> A kdoc comment from sunrpc_get_pool_mode() should also be added
> in 4/5.
> 

I'll do that and resend soon.

> 
> >  {
> >  	int	error;
> >  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > @@ -778,13 +771,6 @@ nfsd_svc(int nrservs, struct net *net, const
> > struct cred *cred, const char *scop
> >  
> >  	dprintk("nfsd: creating service\n");
> >  
> > -	nrservs = max(nrservs, 0);
> > -	nrservs = min(nrservs, NFSD_MAXSERVS);
> > -	error = 0;
> > -
> > -	if (nrservs == 0 && nn->nfsd_serv == NULL)
> > -		goto out;
> > -
> >  	strscpy(nn->nfsd_name, scope ? scope : utsname()-
> > >nodename,
> >  		sizeof(nn->nfsd_name));
> >  
> > @@ -796,7 +782,7 @@ nfsd_svc(int nrservs, struct net *net, const
> > struct cred *cred, const char *scop
> >  	error = nfsd_startup_net(net, cred);
> >  	if (error)
> >  		goto out_put;
> > -	error = svc_set_num_threads(serv, NULL, nrservs);
> > +	error = nfsd_set_nrthreads(n, nthreads, net);
> >  	if (error)
> >  		goto out_put;
> >  	error = serv->sv_nrthreads;
> > 
> > -- 
> > 2.45.2
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>

[PATCH v2 3/5] nfsd: allow passing in array of thread counts via netlink

[Jeff Layton]

Now that nfsd_svc can handle an array of thread counts, fix up the
netlink threads interface to construct one from the netlink call
and pass it through so we can start a pooled server the same way we
would start a normal one.

Note that any unspecified values in the array are considered zeroes,
so it's possible to shut down a pooled server by passing in a short
array that has only zeros, or even an empty array.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfsctl.c | 32 ++++++++++++++++++++------------
 fs/nfsd/nfssvc.c | 12 +++++++++++-
 2 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 121b866125d4..d67057d5b858 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1673,7 +1673,7 @@ int nfsd_nl_rpc_status_get_done(struct netlink_callback *cb)
  */
 int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	int nthreads = 0, count = 0, nrpools, ret = -EOPNOTSUPP, rem;
+	int *nthreads, count = 0, nrpools, i, ret = -EOPNOTSUPP, rem;
 	struct net *net = genl_info_net(info);
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 	const struct nlattr *attr;
@@ -1690,15 +1690,22 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
 
 	mutex_lock(&nfsd_mutex);
 
-	nrpools = nfsd_nrpools(net);
-	if (nrpools && count > nrpools)
-		count = nrpools;
-
-	/* XXX: make this handle non-global pool-modes */
-	if (count > 1)
+	nrpools = max(count, nfsd_nrpools(net));
+	nthreads = kcalloc(nrpools, sizeof(int), GFP_KERNEL);
+	if (!nthreads) {
+		ret = -ENOMEM;
 		goto out_unlock;
+	}
+
+	i = 0;
+	nlmsg_for_each_attr(attr, info->nlhdr, GENL_HDRLEN, rem) {
+		if (nla_type(attr) == NFSD_A_SERVER_THREADS) {
+			nthreads[i++] = nla_get_u32(attr);
+			if (i >= nrpools)
+				break;
+		}
+	}
 
-	nthreads = nla_get_u32(info->attrs[NFSD_A_SERVER_THREADS]);
 	if (info->attrs[NFSD_A_SERVER_GRACETIME] ||
 	    info->attrs[NFSD_A_SERVER_LEASETIME] ||
 	    info->attrs[NFSD_A_SERVER_SCOPE]) {
@@ -1732,12 +1739,13 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
 			scope = nla_data(attr);
 	}
 
-	ret = nfsd_svc(1, &nthreads, net, get_current_cred(), scope);
-
+	ret = nfsd_svc(nrpools, nthreads, net, get_current_cred(), scope);
+	if (ret > 0)
+		ret = 0;
 out_unlock:
 	mutex_unlock(&nfsd_mutex);
-
-	return ret == nthreads ? 0 : ret;
+	kfree(nthreads);
+	return ret;
 }
 
 /**
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 076f35dc17e4..6754cbc27c71 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -750,8 +750,18 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
 					  &nn->nfsd_serv->sv_pools[i],
 					  nthreads[i]);
 		if (err)
-			break;
+			goto out;
 	}
+
+	/* Anything undefined in array is considered to be 0 */
+	for (i = n; i < nn->nfsd_serv->sv_nrpools; ++i) {
+		err = svc_set_num_threads(nn->nfsd_serv,
+					  &nn->nfsd_serv->sv_pools[i],
+					  0);
+		if (err)
+			goto out;
+	}
+out:
 	return err;
 }
 

-- 
2.45.2

[PATCH v2 4/5] sunrpc: refactor pool_mode setting code

[Jeff Layton]

Allow the pool_mode setting code to be called from internal callers
so we can call it from a new netlink op.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 include/linux/sunrpc/svc.h |  2 ++
 net/sunrpc/svc.c           | 76 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 57 insertions(+), 21 deletions(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index d0433e1642b3..a7d0406b9ef5 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -399,6 +399,8 @@ struct svc_procedure {
 /*
  * Function prototypes.
  */
+int sunrpc_set_pool_mode(const char *val);
+int sunrpc_get_pool_mode(char *val, size_t size);
 int svc_rpcb_setup(struct svc_serv *serv, struct net *net);
 void svc_rpcb_cleanup(struct svc_serv *serv, struct net *net);
 int svc_bind(struct svc_serv *serv, struct net *net);
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index f80d94cbb8b1..6806868b3129 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -72,57 +72,91 @@ static struct svc_pool_map svc_pool_map = {
 static DEFINE_MUTEX(svc_pool_map_mutex);/* protects svc_pool_map.count only */
 
 static int
-param_set_pool_mode(const char *val, const struct kernel_param *kp)
+__param_set_pool_mode(const char *val, struct svc_pool_map *m)
 {
-	int *ip = (int *)kp->arg;
-	struct svc_pool_map *m = &svc_pool_map;
-	int err;
+	int err, mode;
 
 	mutex_lock(&svc_pool_map_mutex);
 
-	err = -EBUSY;
-	if (m->count)
-		goto out;
-
 	err = 0;
 	if (!strncmp(val, "auto", 4))
-		*ip = SVC_POOL_AUTO;
+		mode = SVC_POOL_AUTO;
 	else if (!strncmp(val, "global", 6))
-		*ip = SVC_POOL_GLOBAL;
+		mode = SVC_POOL_GLOBAL;
 	else if (!strncmp(val, "percpu", 6))
-		*ip = SVC_POOL_PERCPU;
+		mode = SVC_POOL_PERCPU;
 	else if (!strncmp(val, "pernode", 7))
-		*ip = SVC_POOL_PERNODE;
+		mode = SVC_POOL_PERNODE;
 	else
 		err = -EINVAL;
 
+	if (err)
+		goto out;
+
+	if (m->count == 0)
+		m->mode = mode;
+	else if (mode != m->mode)
+		err = -EBUSY;
 out:
 	mutex_unlock(&svc_pool_map_mutex);
 	return err;
 }
 
 static int
-param_get_pool_mode(char *buf, const struct kernel_param *kp)
+param_set_pool_mode(const char *val, const struct kernel_param *kp)
 {
-	int *ip = (int *)kp->arg;
+	struct svc_pool_map *m = kp->arg;
 
-	switch (*ip)
+	return __param_set_pool_mode(val, m);
+}
+
+int sunrpc_set_pool_mode(const char *val)
+{
+	return __param_set_pool_mode(val, &svc_pool_map);
+}
+EXPORT_SYMBOL(sunrpc_set_pool_mode);
+
+int
+sunrpc_get_pool_mode(char *buf, size_t size)
+{
+	struct svc_pool_map *m = &svc_pool_map;
+
+	switch (m->mode)
 	{
 	case SVC_POOL_AUTO:
-		return sysfs_emit(buf, "auto\n");
+		return snprintf(buf, size, "auto");
 	case SVC_POOL_GLOBAL:
-		return sysfs_emit(buf, "global\n");
+		return snprintf(buf, size, "global");
 	case SVC_POOL_PERCPU:
-		return sysfs_emit(buf, "percpu\n");
+		return snprintf(buf, size, "percpu");
 	case SVC_POOL_PERNODE:
-		return sysfs_emit(buf, "pernode\n");
+		return snprintf(buf, size, "pernode");
 	default:
-		return sysfs_emit(buf, "%d\n", *ip);
+		return snprintf(buf, size, "%d", m->mode);
 	}
 }
+EXPORT_SYMBOL(sunrpc_get_pool_mode);
+
+static int
+param_get_pool_mode(char *buf, const struct kernel_param *kp)
+{
+	char str[16];
+	int len;
+
+	len = sunrpc_get_pool_mode(str, ARRAY_SIZE(str));
+
+	/* Ensure we have room for newline and NUL */
+	len = min_t(int, len, ARRAY_SIZE(str) - 2);
+
+	/* tack on the newline */
+	str[len] = '\n';
+	str[len + 1] = '\0';
+
+	return sysfs_emit(buf, str);
+}
 
 module_param_call(pool_mode, param_set_pool_mode, param_get_pool_mode,
-		 &svc_pool_map.mode, 0644);
+		  &svc_pool_map, 0644);
 
 /*
  * Detect best pool mapping mode heuristically,

-- 
2.45.2

[PATCH v2 5/5] nfsd: new netlink ops to get/set server pool_mode

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 Documentation/netlink/specs/nfsd.yaml | 27 ++++++++++++++++
 fs/nfsd/netlink.c                     | 17 ++++++++++
 fs/nfsd/netlink.h                     |  2 ++
 fs/nfsd/nfsctl.c                      | 58 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/nfsd_netlink.h     | 10 ++++++
 5 files changed, 114 insertions(+)

diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
index d21234097167..5a98e5a06c68 100644
--- a/Documentation/netlink/specs/nfsd.yaml
+++ b/Documentation/netlink/specs/nfsd.yaml
@@ -115,6 +115,15 @@ attribute-sets:
         type: nest
         nested-attributes: sock
         multi-attr: true
+  -
+    name: pool-mode
+    attributes:
+      -
+        name: mode
+        type: string
+      -
+        name: npools
+        type: u32
 
 operations:
   list:
@@ -197,3 +206,21 @@ operations:
         reply:
           attributes:
             - addr
+    -
+      name: pool-mode-set
+      doc: set the current server pool-mode
+      attribute-set: pool-mode
+      flags: [ admin-perm ]
+      do:
+        request:
+          attributes:
+            - mode
+    -
+      name: pool-mode-get
+      doc: get info about server pool-mode
+      attribute-set: pool-mode
+      do:
+        reply:
+          attributes:
+            - mode
+            - npools
diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
index 62d2586d9902..137701153c9e 100644
--- a/fs/nfsd/netlink.c
+++ b/fs/nfsd/netlink.c
@@ -40,6 +40,11 @@ static const struct nla_policy nfsd_listener_set_nl_policy[NFSD_A_SERVER_SOCK_AD
 	[NFSD_A_SERVER_SOCK_ADDR] = NLA_POLICY_NESTED(nfsd_sock_nl_policy),
 };
 
+/* NFSD_CMD_POOL_MODE_SET - do */
+static const struct nla_policy nfsd_pool_mode_set_nl_policy[NFSD_A_POOL_MODE_MODE + 1] = {
+	[NFSD_A_POOL_MODE_MODE] = { .type = NLA_NUL_STRING, },
+};
+
 /* Ops table for nfsd */
 static const struct genl_split_ops nfsd_nl_ops[] = {
 	{
@@ -85,6 +90,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
 		.doit	= nfsd_nl_listener_get_doit,
 		.flags	= GENL_CMD_CAP_DO,
 	},
+	{
+		.cmd		= NFSD_CMD_POOL_MODE_SET,
+		.doit		= nfsd_nl_pool_mode_set_doit,
+		.policy		= nfsd_pool_mode_set_nl_policy,
+		.maxattr	= NFSD_A_POOL_MODE_MODE,
+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
+	},
+	{
+		.cmd	= NFSD_CMD_POOL_MODE_GET,
+		.doit	= nfsd_nl_pool_mode_get_doit,
+		.flags	= GENL_CMD_CAP_DO,
+	},
 };
 
 struct genl_family nfsd_nl_family __ro_after_init = {
diff --git a/fs/nfsd/netlink.h b/fs/nfsd/netlink.h
index e3724637d64d..9459547de04e 100644
--- a/fs/nfsd/netlink.h
+++ b/fs/nfsd/netlink.h
@@ -26,6 +26,8 @@ int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
 int nfsd_nl_version_get_doit(struct sk_buff *skb, struct genl_info *info);
 int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info);
 int nfsd_nl_listener_get_doit(struct sk_buff *skb, struct genl_info *info);
+int nfsd_nl_pool_mode_set_doit(struct sk_buff *skb, struct genl_info *info);
+int nfsd_nl_pool_mode_get_doit(struct sk_buff *skb, struct genl_info *info);
 
 extern struct genl_family nfsd_nl_family;
 
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index d67057d5b858..d019d4b06f2a 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -2184,6 +2184,64 @@ int nfsd_nl_listener_get_doit(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
+/**
+ * nfsd_nl_pool_mode_set_doit - set the number of running threads
+ * @skb: reply buffer
+ * @info: netlink metadata and command arguments
+ *
+ * Return 0 on success or a negative errno.
+ */
+int nfsd_nl_pool_mode_set_doit(struct sk_buff *skb, struct genl_info *info)
+{
+	const struct nlattr *attr;
+
+	if (GENL_REQ_ATTR_CHECK(info, NFSD_A_POOL_MODE_MODE))
+		return -EINVAL;
+
+	attr = info->attrs[NFSD_A_POOL_MODE_MODE];
+	return sunrpc_set_pool_mode(nla_data(attr));
+}
+
+/**
+ * nfsd_nl_pool_mode_get_doit - get info about pool_mode
+ * @skb: reply buffer
+ * @info: netlink metadata and command arguments
+ *
+ * Return 0 on success or a negative errno.
+ */
+int nfsd_nl_pool_mode_get_doit(struct sk_buff *skb, struct genl_info *info)
+{
+	struct net *net = genl_info_net(info);
+	char buf[16];
+	void *hdr;
+	int err;
+
+	skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	err = -EMSGSIZE;
+	hdr = genlmsg_iput(skb, info);
+	if (!hdr)
+		goto err_free_msg;
+
+	err = -ERANGE;
+	if (sunrpc_get_pool_mode(buf, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf))
+		goto err_free_msg;
+
+	err = nla_put_string(skb, NFSD_A_POOL_MODE_MODE, buf) ||
+	      nla_put_u32(skb, NFSD_A_POOL_MODE_NPOOLS, nfsd_nrpools(net));
+	if (err)
+		goto err_free_msg;
+
+	genlmsg_end(skb, hdr);
+	return genlmsg_reply(skb, info);
+
+err_free_msg:
+	nlmsg_free(skb);
+	return err;
+}
+
 /**
  * nfsd_net_init - Prepare the nfsd_net portion of a new net namespace
  * @net: a freshly-created network namespace
diff --git a/include/uapi/linux/nfsd_netlink.h b/include/uapi/linux/nfsd_netlink.h
index 24c86dbc7ed5..887cbd12b695 100644
--- a/include/uapi/linux/nfsd_netlink.h
+++ b/include/uapi/linux/nfsd_netlink.h
@@ -70,6 +70,14 @@ enum {
 	NFSD_A_SERVER_SOCK_MAX = (__NFSD_A_SERVER_SOCK_MAX - 1)
 };
 
+enum {
+	NFSD_A_POOL_MODE_MODE = 1,
+	NFSD_A_POOL_MODE_NPOOLS,
+
+	__NFSD_A_POOL_MODE_MAX,
+	NFSD_A_POOL_MODE_MAX = (__NFSD_A_POOL_MODE_MAX - 1)
+};
+
 enum {
 	NFSD_CMD_RPC_STATUS_GET = 1,
 	NFSD_CMD_THREADS_SET,
@@ -78,6 +86,8 @@ enum {
 	NFSD_CMD_VERSION_GET,
 	NFSD_CMD_LISTENER_SET,
 	NFSD_CMD_LISTENER_GET,
+	NFSD_CMD_POOL_MODE_SET,
+	NFSD_CMD_POOL_MODE_GET,
 
 	__NFSD_CMD_MAX,
 	NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)

-- 
2.45.2

Re: [PATCH v2 0/5] nfsd/sunrpc: allow starting/stopping pooled NFS server via netlink

[Chuck Lever]

On Thu, Jun 13, 2024 at 08:16:37AM -0400, Jeff Layton wrote:
> This is a resend of the patchset I sent a little over a week ago, with
> a couple of new patches that allow setting the pool-mode via netlink.
> 
> This patchset first attempts to detangle the pooled/non-pooled service
> handling in the sunrpc layer, unifies the codepaths that start the
> pooled vs. non-pooled nfsd, and then wires up the new netlink threads
> interface to allow you to start a pooled server by specifying an
> array of thread counts.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> Changes in v2:
> - add new pool-mode set/get netlink calls

Applying this series to nfsd-next for broader exposure. Review
comments and test results are still welcome.

There are a couple of nits to be addressed in 2/5 and 4/5, and
we already discussed adding a disclaimer about changes to the
behavior of the /proc interface. So, can you fix these up and send
me a refresh?


> ---
> Jeff Layton (5):
>       sunrpc: fix up the special handling of sv_nrpools == 1
>       nfsd: make nfsd_svc call nfsd_set_nrthreads
>       nfsd: allow passing in array of thread counts via netlink
>       sunrpc: refactor pool_mode setting code
>       nfsd: new netlink ops to get/set server pool_mode
> 
>  Documentation/netlink/specs/nfsd.yaml |  27 +++++++++
>  fs/nfsd/netlink.c                     |  17 ++++++
>  fs/nfsd/netlink.h                     |   2 +
>  fs/nfsd/nfsctl.c                      | 102 +++++++++++++++++++++++++++++-----
>  fs/nfsd/nfsd.h                        |   3 +-
>  fs/nfsd/nfssvc.c                      |  30 +++++-----
>  include/linux/sunrpc/svc.h            |   3 +
>  include/uapi/linux/nfsd_netlink.h     |  10 ++++
>  net/sunrpc/svc.c                      | 102 +++++++++++++++++++++-------------
>  9 files changed, 225 insertions(+), 71 deletions(-)
> ---
> base-commit: fec4124bac55ad92c47585fe537e646fe108b8fa
> change-id: 20240604-nfsd-next-b04c0d2d89a9
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@kernel.org>
> 

-- 
Chuck Lever