On Wed, 24 Nov 2021, Chuck Lever III wrote:
> > diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> > index b220e1b91726..135bd86ed3ad 100644
> > --- a/fs/lockd/svc.c
> > +++ b/fs/lockd/svc.c
> > @@ -430,14 +430,8 @@ static struct svc_serv *lockd_create_svc(void)
> > /*
> > * Check whether we're already up and running.
> > */
> > - if (nlmsvc_rqst) {
> > - /*
> > - * Note: increase service usage, because later in case of error
> > - * svc_destroy() will be called.
> > - */
> > - svc_get(nlmsvc_rqst->rq_server);
> > - return nlmsvc_rqst->rq_server;
> > - }
> > + if (nlmsvc_rqst)
> > + return svc_get(nlmsvc_rqst->rq_server);
>
> The svc_get-related changes seem like they could be split
> into a separate clean-up patch.
I guess so.
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index af8531c3854a..5eb564e58a9b 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -743,7 +743,7 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
> >
> > err = svc_addsock(nn->nfsd_serv, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred);
> > if (err < 0) {
> > - nfsd_destroy(net);
> > + nfsd_put(net);
>
> Seems like there should be a matching nfsd_get() somewhere.
> Perhaps it can just be an alias for svc_get()?
What purpose would that serve? I really don't like having simple aliases
- they seem to hide information.
In particular, I really don't like reading code, seeing some interface
that I haven't seen before, hunting it out to find out what it means,
and discovering that is just a wrapper around something that I already
know. Why should I have to learn 2 interfaces when 1 would suffice?
So I am not inclined to a nfsd_get().
> > -void nfsd_destroy(struct net *net)
> > +void nfsd_put(struct net *net)
> > {
> > struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > - int destroy = (nn->nfsd_serv->sv_nrthreads == 1);
> >
> > - if (destroy)
> > + nn->nfsd_serv->sv_nrthreads --;
>
> checkpatch.pl screamed about the whitespace between the variable
> and the unary operator here and in svc_put().
I've changed it to ".... -= 1;", which I generally prefer anyway.
But it'll probably become "if atomic_dec_and_test()" in a later patch.
> > +/**
> > + * svc_put - decrement reference count on a SUNRPC serv
> > + * @serv: the svc_serv to have count decremented
> > + *
> > + * When the reference count reaches zero, svc_destroy()
> > + * is called to clean up and free the serv.
> > + */
> > +static inline void svc_put(struct svc_serv *serv)
> > +{
> > + serv->sv_nrthreads --;
> > + if (serv->sv_nrthreads == 0)
>
> Nit: The usual idiom is "if (--serv->sv_nrthreads == 0)"
Is it? I thought that changing variables in if() conditions was
generally discouraged (though it is OK in while()).
So I'll leave it as it is (well... -= 1 ..) until it become atomic_t.
> > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> > index 4292278a9552..55a1bf0d129f 100644
> > --- a/net/sunrpc/svc.c
> > +++ b/net/sunrpc/svc.c
> > @@ -528,17 +528,7 @@ EXPORT_SYMBOL_GPL(svc_shutdown_net);
> > void
> > svc_destroy(struct svc_serv *serv)
> > {
> > - dprintk("svc: svc_destroy(%s, %d)\n",
> > - serv->sv_program->pg_name,
> > - serv->sv_nrthreads);
> > -
> > - if (serv->sv_nrthreads) {
> > - if (--(serv->sv_nrthreads) != 0) {
> > - svc_sock_update_bufs(serv);
> > - return;
> > - }
> > - } else
> > - printk("svc_destroy: no threads for serv=%p!\n", serv);
> > + dprintk("svc: svc_destroy(%s)\n", serv->sv_program->pg_name);
>
> Maybe the dprintk is unnecessary. I would prefer a trace
> point if there is real value in observing destruction of
> particular svc_serv objects.
>
> Likewise in subsequent patches.
>
> Also... since we're in the clean-up frame of mind, if you
> see a BUG() call site remaining in a hunk, ask yourself
> if we really need to kill the kernel at that point, or
> if a WARN would suffice.
cleanup of BUGs (and dprinkts) is, for me, a different frame of mind
that the cleanup I currently working on. I'd rather not get distracted.
>
> > del_timer_sync(&serv->sv_temptimer);
> >
> > @@ -892,9 +882,10 @@ svc_exit_thread(struct svc_rqst *rqstp)
> >
> > svc_rqst_free(rqstp);
> >
> > - /* Release the server */
> > - if (serv)
> > - svc_destroy(serv);
> > + if (!serv)
> > + return;
> > + svc_sock_update_bufs(serv);
>
> I don't object to moving the svc_sock_update_bufs() call
> site. But....
>
> Note for someday: I'm not sure of a better way of handling
> buffer size changes, but this seems like all kinds layering
> violation.
Despite the name, this function doesn't update any bufs. It just sets
some flags.
Maybe we should have a sequential "version" number in the svc_serv which
is updated when the number of threads changes. And each svc_sock holds
a copy of this. If it notices the svc_serv has changed version, it
reassesses its buffer space.
Thanks,
NeilBrown
