| Message ID | 20240613-nfsd-next-v2-2-20bf690d65fb@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | nfsd/sunrpc: allow starting/stopping pooled NFS server via netlink | expand |
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 >
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 > > >
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;
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(-)