Hi Neil-
Some further custodial requests below...
> On Nov 22, 2021, at 8:29 PM, NeilBrown <neilb@xxxxxxx> wrote:
>
> svc_destroy() is poorly named - it doesn't necessarily destroy the svc,
> it might just reduce the ref count.
> nfsd_destroy() is poorly named for the same reason.
>
> This patch:
> - removes the refcount functionality from svc_destroy(), moving it to
> a new svc_put(). Almost all previous callers of svc_destroy() now
> call svc_put().
> - renames nfsd_destroy() to nfsd_put() and improves the code, using
> the new svc_destroy() rather than svc_put()
> - also changes svc_get() to return the serv, which simplifies
> some code a little.
>
> The only non-trivial part of this is that svc_destroy() would call
> svc_sock_update() on a non-final decrement. It can no longer do that,
> and svc_put() isn't really a good place of it. This call is now made
> from svc_exit_thread() which seems like a good place. This makes the
> call *before* sv_nrthreads is decremented rather than after. This
> is not particularly important as the call just sets a flag which
> causes sv_nrthreads set be checked later. A subsequent patch will
> improve the ordering.
>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> fs/lockd/svc.c | 12 +++---------
> fs/nfs/callback.c | 20 ++++----------------
> fs/nfsd/nfsctl.c | 4 ++--
> fs/nfsd/nfsd.h | 2 +-
> fs/nfsd/nfssvc.c | 30 ++++++++++++++++--------------
> include/linux/sunrpc/svc.h | 29 +++++++++++++++++++++++++----
> net/sunrpc/svc.c | 19 +++++--------------
> 7 files changed, 56 insertions(+), 60 deletions(-)
>
> 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.
> /*
> * Sanity check: if there's no pid,
> @@ -497,7 +491,7 @@ int lockd_up(struct net *net, const struct cred *cred)
> * so we exit through here on both success and failure.
> */
> err_put:
> - svc_destroy(serv);
> + svc_put(serv);
> err_create:
> mutex_unlock(&nlmsvc_mutex);
> return error;
> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> index 86d856de1389..edbc7579b4aa 100644
> --- a/fs/nfs/callback.c
> +++ b/fs/nfs/callback.c
> @@ -266,14 +266,8 @@ static struct svc_serv *nfs_callback_create_svc(int minorversion)
> /*
> * Check whether we're already up and running.
> */
> - if (cb_info->serv) {
> - /*
> - * Note: increase service usage, because later in case of error
> - * svc_destroy() will be called.
> - */
> - svc_get(cb_info->serv);
> - return cb_info->serv;
> - }
> + if (cb_info->serv)
> + return svc_get(cb_info->serv);
>
> switch (minorversion) {
> case 0:
> @@ -335,16 +329,10 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
> goto err_start;
>
> cb_info->users++;
> - /*
> - * svc_create creates the svc_serv with sv_nrthreads == 1, and then
> - * svc_prepare_thread increments that. So we need to call svc_destroy
> - * on both success and failure so that the refcount is 1 when the
> - * thread exits.
> - */
> err_net:
> if (!cb_info->users)
> cb_info->serv = NULL;
> - svc_destroy(serv);
> + svc_put(serv);
> err_create:
> mutex_unlock(&nfs_callback_mutex);
> return ret;
> @@ -370,7 +358,7 @@ void nfs_callback_down(int minorversion, struct net *net)
> if (cb_info->users == 0) {
> svc_get(serv);
> serv->sv_ops->svo_setup(serv, NULL, 0);
> - svc_destroy(serv);
> + svc_put(serv);
> dprintk("nfs_callback_down: service destroyed\n");
> cb_info->serv = NULL;
> }
> 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()?
> return err;
> }
>
> @@ -796,7 +796,7 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
> if (!list_empty(&nn->nfsd_serv->sv_permsocks))
> nn->nfsd_serv->sv_nrthreads--;
> else
> - nfsd_destroy(net);
> + nfsd_put(net);
> return err;
> }
>
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index 498e5a489826..3e5008b475ff 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -97,7 +97,7 @@ int nfsd_pool_stats_open(struct inode *, struct file *);
> int nfsd_pool_stats_release(struct inode *, struct file *);
> void nfsd_shutdown_threads(struct net *net);
>
> -void nfsd_destroy(struct net *net);
> +void nfsd_put(struct net *net);
>
> bool i_am_nfsd(void);
>
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 80431921e5d7..2ab0e650a0e2 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -623,7 +623,7 @@ void nfsd_shutdown_threads(struct net *net)
> svc_get(serv);
> /* Kill outstanding nfsd threads */
> serv->sv_ops->svo_setup(serv, NULL, 0);
> - nfsd_destroy(net);
> + nfsd_put(net);
> mutex_unlock(&nfsd_mutex);
> /* Wait for shutdown of nfsd_serv to complete */
> wait_for_completion(&nn->nfsd_shutdown_complete);
> @@ -656,7 +656,10 @@ int nfsd_create_serv(struct net *net)
> nn->nfsd_serv->sv_maxconn = nn->max_connections;
> error = svc_bind(nn->nfsd_serv, net);
> if (error < 0) {
> - svc_destroy(nn->nfsd_serv);
> + /* NOT nfsd_put() as notifiers (see below) haven't
> + * been set up yet.
> + */
> + svc_put(nn->nfsd_serv);
> nfsd_complete_shutdown(net);
> return error;
> }
> @@ -697,16 +700,16 @@ int nfsd_get_nrthreads(int n, int *nthreads, struct net *net)
> return 0;
> }
>
> -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().
> + if (nn->nfsd_serv->sv_nrthreads == 0) {
> svc_shutdown_net(nn->nfsd_serv, net);
> - svc_destroy(nn->nfsd_serv);
> - if (destroy)
> + svc_destroy(nn->nfsd_serv);
> nfsd_complete_shutdown(net);
> + }
> }
>
> int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
> @@ -758,7 +761,7 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
> if (err)
> break;
> }
> - nfsd_destroy(net);
> + nfsd_put(net);
> return err;
> }
>
> @@ -795,7 +798,7 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred)
>
> error = nfsd_startup_net(net, cred);
> if (error)
> - goto out_destroy;
> + goto out_put;
> error = nn->nfsd_serv->sv_ops->svo_setup(nn->nfsd_serv,
> NULL, nrservs);
> if (error)
> @@ -808,8 +811,8 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred)
> out_shutdown:
> if (error < 0 && !nfsd_up_before)
> nfsd_shutdown_net(net);
> -out_destroy:
> - nfsd_destroy(net); /* Release server */
> +out_put:
> + nfsd_put(net);
> out:
> mutex_unlock(&nfsd_mutex);
> return error;
> @@ -982,7 +985,7 @@ nfsd(void *vrqstp)
> /* Release the thread */
> svc_exit_thread(rqstp);
>
> - nfsd_destroy(net);
> + nfsd_put(net);
>
> /* Release module */
> mutex_unlock(&nfsd_mutex);
> @@ -1109,8 +1112,7 @@ int nfsd_pool_stats_release(struct inode *inode, struct file *file)
> struct net *net = inode->i_sb->s_fs_info;
>
> mutex_lock(&nfsd_mutex);
> - /* this function really, really should have been called svc_put() */
> - nfsd_destroy(net);
> + nfsd_put(net);
> mutex_unlock(&nfsd_mutex);
> return ret;
> }
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 0ae28ae6caf2..d87c3392a1e9 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -114,15 +114,37 @@ struct svc_serv {
> #endif /* CONFIG_SUNRPC_BACKCHANNEL */
> };
>
> -/*
> - * We use sv_nrthreads as a reference count. svc_destroy() drops
> +/**
> + * svc_get() - increment reference count on a SUNRPC serv
> + * @serv: the svc_serv to have count incremented
> + *
> + * Returns: the svc_serv that was passed in.
> + *
> + * We use sv_nrthreads as a reference count. svc_put() drops
> * this refcount, so we need to bump it up around operations that
> * change the number of threads. Horrible, but there it is.
> * Should be called with the "service mutex" held.
> */
> -static inline void svc_get(struct svc_serv *serv)
> +static inline struct svc_serv *svc_get(struct svc_serv *serv)
> {
> serv->sv_nrthreads++;
> + return serv;
> +}
> +
> +void svc_destroy(struct svc_serv *serv);
> +
> +/**
> + * 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)"
> + svc_destroy(serv);
> }
>
> /*
> @@ -514,7 +536,6 @@ struct svc_serv * svc_create_pooled(struct svc_program *, unsigned int,
> int svc_set_num_threads(struct svc_serv *, struct svc_pool *, int);
> int svc_set_num_threads_sync(struct svc_serv *, struct svc_pool *, int);
> int svc_pool_stats_open(struct svc_serv *serv, struct file *file);
> -void svc_destroy(struct svc_serv *);
> void svc_shutdown_net(struct svc_serv *, struct net *);
> int svc_process(struct svc_rqst *);
> int bc_svc_process(struct svc_serv *, struct rpc_rqst *,
> 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.
> 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.
> + svc_destroy(serv);
> }
> EXPORT_SYMBOL_GPL(svc_exit_thread);
>
>
>
--
Chuck Lever
