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

[Jeff Layton <jlayton@kernel.org>]

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>