From: Chuck Lever III <chuck.lever@oracle.com>
To: Neil Brown <neilb@suse.de>
Cc: Bruce Fields <bfields@fieldses.org>,
Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 01/19] SUNRPC/NFSD: clean up get/put functions.
Date: Tue, 23 Nov 2021 16:45:29 +0000 [thread overview]
Message-ID: <92C7D2FA-6C7B-4209-B1B4-3B81B2B276F4@oracle.com> (raw)
In-Reply-To: <163763097540.7284.6343105121634986097.stgit@noble.brown>
Hi Neil-
Some further custodial requests below...
> On Nov 22, 2021, at 8:29 PM, NeilBrown <neilb@suse.de> 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@suse.de>
> ---
> 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
next prev parent reply other threads:[~2021-11-23 16:45 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-23 1:29 [PATCH 00/19 v2] SUNRPC: clean up server thread management NeilBrown
2021-11-23 1:29 ` [PATCH 15/19] lockd: rename lockd_create_svc() to lockd_get() NeilBrown
2021-11-23 1:29 ` [PATCH 13/19] lockd: move svc_exit_thread() into the thread NeilBrown
2021-11-23 1:29 ` [PATCH 06/19] NFSD: narrow nfsd_mutex protection in nfsd thread NeilBrown
2021-11-23 1:29 ` [PATCH 05/19] SUNRPC: use sv_lock to protect updates to sv_nrthreads NeilBrown
2021-11-23 16:43 ` Chuck Lever III
2021-11-29 0:11 ` NeilBrown
2021-11-23 1:29 ` [PATCH 10/19] lockd: introduce nlmsvc_serv NeilBrown
2021-11-23 1:29 ` [PATCH 14/19] lockd: introduce lockd_put() NeilBrown
2021-11-23 1:29 ` [PATCH 02/19] NFSD: handle error better in write_ports_addfd() NeilBrown
2021-11-23 16:44 ` Chuck Lever III
2021-11-28 23:49 ` NeilBrown
2021-11-23 1:29 ` [PATCH 12/19] lockd: move lockd_start_svc() call into lockd_create_svc() NeilBrown
2021-11-23 1:29 ` [PATCH 16/19] SUNRPC: move the pool_map definitions (back) into svc.c NeilBrown
2021-11-23 1:29 ` [PATCH 07/19] NFSD: Make it possible to use svc_set_num_threads_sync NeilBrown
2021-11-23 1:29 ` [PATCH 04/19] nfsd: make nfsd_stats.th_cnt atomic_t NeilBrown
2021-11-23 1:29 ` [PATCH 01/19] SUNRPC/NFSD: clean up get/put functions NeilBrown
2021-11-23 16:45 ` Chuck Lever III [this message]
2021-11-28 23:36 ` NeilBrown
2021-11-23 1:29 ` [PATCH 18/19] lockd: use svc_set_num_threads() for thread start and stop NeilBrown
2021-11-23 1:29 ` [PATCH 09/19] NFSD: simplify locking for network notifier NeilBrown
2021-11-23 1:29 ` [PATCH 11/19] lockd: simplify management of network status notifiers NeilBrown
2021-11-23 1:29 ` [PATCH 08/19] SUNRPC: discard svo_setup and rename svc_set_num_threads_sync() NeilBrown
2021-11-23 1:29 ` [PATCH 19/19] NFS: switch the callback service back to non-pooled NeilBrown
2021-11-23 1:29 ` [PATCH 03/19] SUNRPC: stop using ->sv_nrthreads as a refcount NeilBrown
2021-11-23 1:29 ` [PATCH 17/19] SUNRPC: always treat sv_nrpools==1 as "not pooled" NeilBrown
2021-11-23 14:49 ` [PATCH 00/19 v2] SUNRPC: clean up server thread management J. Bruce Fields
2021-11-24 18:42 ` Chuck Lever III
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=92C7D2FA-6C7B-4209-B1B4-3B81B2B276F4@oracle.com \
--to=chuck.lever@oracle.com \
--cc=bfields@fieldses.org \
--cc=linux-nfs@vger.kernel.org \
--cc=neilb@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.