I've started a series of patches to clean out and remove struct
svc_serv_ops, as suggested by Neil Brown. First two patches remove
svo_enqueue_xprt and svo_shutdown, but I'm not quite sure how to
replace svo_module, so I've stopped there for the moment.
Neil, do you have a suggestion or patch that removes svo_module?
Once that is done we can finish with a patch that hoists
.svo_function into struct svc_serv.
---
Chuck Lever (2):
SUNRPC: Remove the .svo_enqueue_xprt method
SUNRPC: Remove svo_shutdown method
fs/lockd/svc.c | 6 ++----
fs/nfs/callback.c | 2 --
fs/nfsd/nfssvc.c | 3 +--
include/linux/sunrpc/svc.h | 6 ------
include/linux/sunrpc/svc_xprt.h | 1 -
net/sunrpc/svc.c | 3 ---
net/sunrpc/svc_xprt.c | 10 +++++-----
7 files changed, 8 insertions(+), 23 deletions(-)
--
Chuck Lever
Clean up. Neil observed that "any code that calls svc_shutdown_net()
knows what the shutdown function should be, and so can call it
directly."
Signed-off-by: Chuck Lever <[email protected]>
---
fs/lockd/svc.c | 5 ++---
fs/nfsd/nfssvc.c | 2 +-
include/linux/sunrpc/svc.h | 3 ---
net/sunrpc/svc.c | 3 ---
4 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 3a05af873625..f5b688a844aa 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -249,6 +249,7 @@ static int make_socks(struct svc_serv *serv, struct net *net,
printk(KERN_WARNING
"lockd_up: makesock failed, error=%d\n", err);
svc_shutdown_net(serv, net);
+ svc_rpcb_cleanup(serv, net);
return err;
}
@@ -287,8 +288,7 @@ static void lockd_down_net(struct svc_serv *serv, struct net *net)
cancel_delayed_work_sync(&ln->grace_period_end);
locks_end_grace(&ln->lockd_manager);
svc_shutdown_net(serv, net);
- dprintk("%s: per-net data destroyed; net=%x\n",
- __func__, net->ns.inum);
+ svc_rpcb_cleanup(serv, net);
}
} else {
pr_err("%s: no users! net=%x\n",
@@ -351,7 +351,6 @@ static struct notifier_block lockd_inet6addr_notifier = {
#endif
static const struct svc_serv_ops lockd_sv_ops = {
- .svo_shutdown = svc_rpcb_cleanup,
.svo_function = lockd,
.svo_module = THIS_MODULE,
};
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index aeeac6de1f0a..0c6b216e439e 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -613,7 +613,6 @@ static int nfsd_get_default_max_blksize(void)
}
static const struct svc_serv_ops nfsd_thread_sv_ops = {
- .svo_shutdown = nfsd_last_thread,
.svo_function = nfsd,
.svo_module = THIS_MODULE,
};
@@ -724,6 +723,7 @@ void nfsd_put(struct net *net)
if (kref_put(&nn->nfsd_serv->sv_refcnt, nfsd_noop)) {
svc_shutdown_net(nn->nfsd_serv, net);
+ nfsd_last_thread(nn->nfsd_serv, net);
svc_destroy(&nn->nfsd_serv->sv_refcnt);
spin_lock(&nfsd_notifier_lock);
nn->nfsd_serv = NULL;
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 6ef9c1cafd0b..63794d772eb3 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -55,9 +55,6 @@ struct svc_pool {
struct svc_serv;
struct svc_serv_ops {
- /* Callback to use when last thread exits. */
- void (*svo_shutdown)(struct svc_serv *, struct net *);
-
/* function for service threads to run */
int (*svo_function)(void *);
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 2aabec2b4bec..74a75a22da9a 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -539,9 +539,6 @@ EXPORT_SYMBOL_GPL(svc_create_pooled);
void svc_shutdown_net(struct svc_serv *serv, struct net *net)
{
svc_close_net(serv, net);
-
- if (serv->sv_ops->svo_shutdown)
- serv->sv_ops->svo_shutdown(serv, net);
}
EXPORT_SYMBOL_GPL(svc_shutdown_net);
We have never been able to track down and address the underlying
cause of the performance issues with workqueue-based service
support. svo_enqueue_xprt is called multiple times per RPC, so
it adds instruction path length, but always ends up at the same
function: svc_xprt_do_enqueue(). We do not anticipate needing
this flexibility for dynamic nfsd thread management support.
As a micro-optimization, remove .svo_enqueue_xprt because
Spectre/Meltdown makes virtual function calls more costly.
This change essentially reverts commit b9e13cdfac70 ("nfsd/sunrpc:
turn enqueueing a svc_xprt into a svc_serv operation").
Signed-off-by: Chuck Lever <[email protected]>
---
fs/lockd/svc.c | 1 -
fs/nfs/callback.c | 2 --
fs/nfsd/nfssvc.c | 1 -
include/linux/sunrpc/svc.h | 3 ---
include/linux/sunrpc/svc_xprt.h | 1 -
net/sunrpc/svc_xprt.c | 10 +++++-----
6 files changed, 5 insertions(+), 13 deletions(-)
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 0475c5a5d061..3a05af873625 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -353,7 +353,6 @@ static struct notifier_block lockd_inet6addr_notifier = {
static const struct svc_serv_ops lockd_sv_ops = {
.svo_shutdown = svc_rpcb_cleanup,
.svo_function = lockd,
- .svo_enqueue_xprt = svc_xprt_do_enqueue,
.svo_module = THIS_MODULE,
};
diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 054cc1255fac..7a810f885063 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -234,13 +234,11 @@ static int nfs_callback_up_net(int minorversion, struct svc_serv *serv,
static const struct svc_serv_ops nfs40_cb_sv_ops = {
.svo_function = nfs4_callback_svc,
- .svo_enqueue_xprt = svc_xprt_do_enqueue,
.svo_module = THIS_MODULE,
};
#if defined(CONFIG_NFS_V4_1)
static const struct svc_serv_ops nfs41_cb_sv_ops = {
.svo_function = nfs41_callback_svc,
- .svo_enqueue_xprt = svc_xprt_do_enqueue,
.svo_module = THIS_MODULE,
};
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index b8c682b62d29..aeeac6de1f0a 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -615,7 +615,6 @@ static int nfsd_get_default_max_blksize(void)
static const struct svc_serv_ops nfsd_thread_sv_ops = {
.svo_shutdown = nfsd_last_thread,
.svo_function = nfsd,
- .svo_enqueue_xprt = svc_xprt_do_enqueue,
.svo_module = THIS_MODULE,
};
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index f35c22b3355f..6ef9c1cafd0b 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -61,9 +61,6 @@ struct svc_serv_ops {
/* function for service threads to run */
int (*svo_function)(void *);
- /* queue up a transport for servicing */
- void (*svo_enqueue_xprt)(struct svc_xprt *);
-
/* optional module to count when adding threads.
* Thread function must call module_put_and_kthread_exit() to exit.
*/
diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index 571f605bc91e..a3ba027fb4ba 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -131,7 +131,6 @@ int svc_create_xprt(struct svc_serv *, const char *, struct net *,
const int, const unsigned short, int,
const struct cred *);
void svc_xprt_received(struct svc_xprt *xprt);
-void svc_xprt_do_enqueue(struct svc_xprt *xprt);
void svc_xprt_enqueue(struct svc_xprt *xprt);
void svc_xprt_put(struct svc_xprt *xprt);
void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt);
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index b21ad7994147..9fce4f7774bb 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -32,6 +32,7 @@ static int svc_deferred_recv(struct svc_rqst *rqstp);
static struct cache_deferred_req *svc_defer(struct cache_req *req);
static void svc_age_temp_xprts(struct timer_list *t);
static void svc_delete_xprt(struct svc_xprt *xprt);
+static void svc_xprt_do_enqueue(struct svc_xprt *xprt);
/* apparently the "standard" is that clients close
* idle connections after 5 minutes, servers after
@@ -266,12 +267,12 @@ void svc_xprt_received(struct svc_xprt *xprt)
}
/* As soon as we clear busy, the xprt could be closed and
- * 'put', so we need a reference to call svc_enqueue_xprt with:
+ * 'put', so we need a reference to call svc_xprt_do_enqueue with:
*/
svc_xprt_get(xprt);
smp_mb__before_atomic();
clear_bit(XPT_BUSY, &xprt->xpt_flags);
- xprt->xpt_server->sv_ops->svo_enqueue_xprt(xprt);
+ svc_xprt_do_enqueue(xprt);
svc_xprt_put(xprt);
}
EXPORT_SYMBOL_GPL(svc_xprt_received);
@@ -423,7 +424,7 @@ static bool svc_xprt_ready(struct svc_xprt *xprt)
return false;
}
-void svc_xprt_do_enqueue(struct svc_xprt *xprt)
+static void svc_xprt_do_enqueue(struct svc_xprt *xprt)
{
struct svc_pool *pool;
struct svc_rqst *rqstp = NULL;
@@ -467,7 +468,6 @@ void svc_xprt_do_enqueue(struct svc_xprt *xprt)
put_cpu();
trace_svc_xprt_enqueue(xprt, rqstp);
}
-EXPORT_SYMBOL_GPL(svc_xprt_do_enqueue);
/*
* Queue up a transport with data pending. If there are idle nfsd
@@ -478,7 +478,7 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
{
if (test_bit(XPT_BUSY, &xprt->xpt_flags))
return;
- xprt->xpt_server->sv_ops->svo_enqueue_xprt(xprt);
+ svc_xprt_do_enqueue(xprt);
}
EXPORT_SYMBOL_GPL(svc_xprt_enqueue);
On Wed, 26 Jan 2022, Chuck Lever wrote:
> We have never been able to track down and address the underlying
> cause of the performance issues with workqueue-based service
> support. svo_enqueue_xprt is called multiple times per RPC, so
> it adds instruction path length, but always ends up at the same
> function: svc_xprt_do_enqueue(). We do not anticipate needing
> this flexibility for dynamic nfsd thread management support.
>
> As a micro-optimization, remove .svo_enqueue_xprt because
> Spectre/Meltdown makes virtual function calls more costly.
>
> This change essentially reverts commit b9e13cdfac70 ("nfsd/sunrpc:
> turn enqueueing a svc_xprt into a svc_serv operation").
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> fs/lockd/svc.c | 1 -
> fs/nfs/callback.c | 2 --
> fs/nfsd/nfssvc.c | 1 -
> include/linux/sunrpc/svc.h | 3 ---
> include/linux/sunrpc/svc_xprt.h | 1 -
> net/sunrpc/svc_xprt.c | 10 +++++-----
> 6 files changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index 0475c5a5d061..3a05af873625 100644
> --- a/fs/lockd/svc.c
> +++ b/fs/lockd/svc.c
> @@ -353,7 +353,6 @@ static struct notifier_block lockd_inet6addr_notifier = {
> static const struct svc_serv_ops lockd_sv_ops = {
> .svo_shutdown = svc_rpcb_cleanup,
> .svo_function = lockd,
> - .svo_enqueue_xprt = svc_xprt_do_enqueue,
> .svo_module = THIS_MODULE,
> };
>
> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> index 054cc1255fac..7a810f885063 100644
> --- a/fs/nfs/callback.c
> +++ b/fs/nfs/callback.c
> @@ -234,13 +234,11 @@ static int nfs_callback_up_net(int minorversion, struct svc_serv *serv,
>
> static const struct svc_serv_ops nfs40_cb_sv_ops = {
> .svo_function = nfs4_callback_svc,
> - .svo_enqueue_xprt = svc_xprt_do_enqueue,
> .svo_module = THIS_MODULE,
> };
> #if defined(CONFIG_NFS_V4_1)
> static const struct svc_serv_ops nfs41_cb_sv_ops = {
> .svo_function = nfs41_callback_svc,
> - .svo_enqueue_xprt = svc_xprt_do_enqueue,
> .svo_module = THIS_MODULE,
> };
>
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index b8c682b62d29..aeeac6de1f0a 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -615,7 +615,6 @@ static int nfsd_get_default_max_blksize(void)
> static const struct svc_serv_ops nfsd_thread_sv_ops = {
> .svo_shutdown = nfsd_last_thread,
> .svo_function = nfsd,
> - .svo_enqueue_xprt = svc_xprt_do_enqueue,
> .svo_module = THIS_MODULE,
> };
>
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index f35c22b3355f..6ef9c1cafd0b 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -61,9 +61,6 @@ struct svc_serv_ops {
> /* function for service threads to run */
> int (*svo_function)(void *);
>
> - /* queue up a transport for servicing */
> - void (*svo_enqueue_xprt)(struct svc_xprt *);
> -
> /* optional module to count when adding threads.
> * Thread function must call module_put_and_kthread_exit() to exit.
> */
> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
> index 571f605bc91e..a3ba027fb4ba 100644
> --- a/include/linux/sunrpc/svc_xprt.h
> +++ b/include/linux/sunrpc/svc_xprt.h
> @@ -131,7 +131,6 @@ int svc_create_xprt(struct svc_serv *, const char *, struct net *,
> const int, const unsigned short, int,
> const struct cred *);
> void svc_xprt_received(struct svc_xprt *xprt);
> -void svc_xprt_do_enqueue(struct svc_xprt *xprt);
> void svc_xprt_enqueue(struct svc_xprt *xprt);
> void svc_xprt_put(struct svc_xprt *xprt);
> void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt);
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index b21ad7994147..9fce4f7774bb 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -32,6 +32,7 @@ static int svc_deferred_recv(struct svc_rqst *rqstp);
> static struct cache_deferred_req *svc_defer(struct cache_req *req);
> static void svc_age_temp_xprts(struct timer_list *t);
> static void svc_delete_xprt(struct svc_xprt *xprt);
> +static void svc_xprt_do_enqueue(struct svc_xprt *xprt);
>
> /* apparently the "standard" is that clients close
> * idle connections after 5 minutes, servers after
> @@ -266,12 +267,12 @@ void svc_xprt_received(struct svc_xprt *xprt)
> }
>
> /* As soon as we clear busy, the xprt could be closed and
> - * 'put', so we need a reference to call svc_enqueue_xprt with:
> + * 'put', so we need a reference to call svc_xprt_do_enqueue with:
> */
> svc_xprt_get(xprt);
> smp_mb__before_atomic();
> clear_bit(XPT_BUSY, &xprt->xpt_flags);
> - xprt->xpt_server->sv_ops->svo_enqueue_xprt(xprt);
> + svc_xprt_do_enqueue(xprt);
> svc_xprt_put(xprt);
> }
> EXPORT_SYMBOL_GPL(svc_xprt_received);
> @@ -423,7 +424,7 @@ static bool svc_xprt_ready(struct svc_xprt *xprt)
> return false;
> }
>
> -void svc_xprt_do_enqueue(struct svc_xprt *xprt)
> +static void svc_xprt_do_enqueue(struct svc_xprt *xprt)
> {
> struct svc_pool *pool;
> struct svc_rqst *rqstp = NULL;
> @@ -467,7 +468,6 @@ void svc_xprt_do_enqueue(struct svc_xprt *xprt)
> put_cpu();
> trace_svc_xprt_enqueue(xprt, rqstp);
> }
> -EXPORT_SYMBOL_GPL(svc_xprt_do_enqueue);
>
> /*
> * Queue up a transport with data pending. If there are idle nfsd
> @@ -478,7 +478,7 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
> {
> if (test_bit(XPT_BUSY, &xprt->xpt_flags))
> return;
> - xprt->xpt_server->sv_ops->svo_enqueue_xprt(xprt);
> + svc_xprt_do_enqueue(xprt);
> }
> EXPORT_SYMBOL_GPL(svc_xprt_enqueue);
>
I obviously like this, but I think we can do better - merge
svc_xprt_enqueue and svc_xprt_do_enqueue, and drop the extra test on
XPT_BUSY.
They were separated in Commit 0971374e2818 ("SUNRPC: Reduce contention
in svc_xprt_enqueue()") so that the XPT_BUSY check happened before
taking any spinlocks.
We have since moved or removed the spinlocks so the extra test is fairly
pointless.
Possibly we could add
if (xpt_flags & BIT(XPR_BUSY))
return false;
at the start of svc_xprt_ready(). It would make sense there and be
close to free.
Thanks,
NeilBrown
On Wed, 26 Jan 2022, Chuck Lever wrote:
> Clean up. Neil observed that "any code that calls svc_shutdown_net()
> knows what the shutdown function should be, and so can call it
> directly."
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> fs/lockd/svc.c | 5 ++---
> fs/nfsd/nfssvc.c | 2 +-
> include/linux/sunrpc/svc.h | 3 ---
> net/sunrpc/svc.c | 3 ---
> 4 files changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index 3a05af873625..f5b688a844aa 100644
> --- a/fs/lockd/svc.c
> +++ b/fs/lockd/svc.c
> @@ -249,6 +249,7 @@ static int make_socks(struct svc_serv *serv, struct net *net,
> printk(KERN_WARNING
> "lockd_up: makesock failed, error=%d\n", err);
> svc_shutdown_net(serv, net);
> + svc_rpcb_cleanup(serv, net);
> return err;
> }
>
> @@ -287,8 +288,7 @@ static void lockd_down_net(struct svc_serv *serv, struct net *net)
> cancel_delayed_work_sync(&ln->grace_period_end);
> locks_end_grace(&ln->lockd_manager);
> svc_shutdown_net(serv, net);
> - dprintk("%s: per-net data destroyed; net=%x\n",
> - __func__, net->ns.inum);
> + svc_rpcb_cleanup(serv, net);
> }
> } else {
> pr_err("%s: no users! net=%x\n",
> @@ -351,7 +351,6 @@ static struct notifier_block lockd_inet6addr_notifier = {
> #endif
>
> static const struct svc_serv_ops lockd_sv_ops = {
> - .svo_shutdown = svc_rpcb_cleanup,
> .svo_function = lockd,
> .svo_module = THIS_MODULE,
> };
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index aeeac6de1f0a..0c6b216e439e 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -613,7 +613,6 @@ static int nfsd_get_default_max_blksize(void)
> }
>
> static const struct svc_serv_ops nfsd_thread_sv_ops = {
> - .svo_shutdown = nfsd_last_thread,
> .svo_function = nfsd,
> .svo_module = THIS_MODULE,
> };
> @@ -724,6 +723,7 @@ void nfsd_put(struct net *net)
>
> if (kref_put(&nn->nfsd_serv->sv_refcnt, nfsd_noop)) {
> svc_shutdown_net(nn->nfsd_serv, net);
> + nfsd_last_thread(nn->nfsd_serv, net);
> svc_destroy(&nn->nfsd_serv->sv_refcnt);
> spin_lock(&nfsd_notifier_lock);
> nn->nfsd_serv = NULL;
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 6ef9c1cafd0b..63794d772eb3 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -55,9 +55,6 @@ struct svc_pool {
> struct svc_serv;
>
> struct svc_serv_ops {
> - /* Callback to use when last thread exits. */
> - void (*svo_shutdown)(struct svc_serv *, struct net *);
> -
> /* function for service threads to run */
> int (*svo_function)(void *);
>
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index 2aabec2b4bec..74a75a22da9a 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -539,9 +539,6 @@ EXPORT_SYMBOL_GPL(svc_create_pooled);
> void svc_shutdown_net(struct svc_serv *serv, struct net *net)
> {
> svc_close_net(serv, net);
> -
> - if (serv->sv_ops->svo_shutdown)
> - serv->sv_ops->svo_shutdown(serv, net);
> }
> EXPORT_SYMBOL_GPL(svc_shutdown_net);
Could we rename svc_close_net() to svc_shutdown_net() and drop this
function?
Either way:
Reviewed-by: NeilBrown <[email protected]>
Thanks,
NeilBrown
> On Jan 25, 2022, at 4:53 PM, NeilBrown <[email protected]> wrote:
>
> On Wed, 26 Jan 2022, Chuck Lever wrote:
>> Clean up. Neil observed that "any code that calls svc_shutdown_net()
>> knows what the shutdown function should be, and so can call it
>> directly."
>>
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>> fs/lockd/svc.c | 5 ++---
>> fs/nfsd/nfssvc.c | 2 +-
>> include/linux/sunrpc/svc.h | 3 ---
>> net/sunrpc/svc.c | 3 ---
>> 4 files changed, 3 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
>> index 3a05af873625..f5b688a844aa 100644
>> --- a/fs/lockd/svc.c
>> +++ b/fs/lockd/svc.c
>> @@ -249,6 +249,7 @@ static int make_socks(struct svc_serv *serv, struct net *net,
>> printk(KERN_WARNING
>> "lockd_up: makesock failed, error=%d\n", err);
>> svc_shutdown_net(serv, net);
>> + svc_rpcb_cleanup(serv, net);
>> return err;
>> }
>>
>> @@ -287,8 +288,7 @@ static void lockd_down_net(struct svc_serv *serv, struct net *net)
>> cancel_delayed_work_sync(&ln->grace_period_end);
>> locks_end_grace(&ln->lockd_manager);
>> svc_shutdown_net(serv, net);
>> - dprintk("%s: per-net data destroyed; net=%x\n",
>> - __func__, net->ns.inum);
>> + svc_rpcb_cleanup(serv, net);
>> }
>> } else {
>> pr_err("%s: no users! net=%x\n",
>> @@ -351,7 +351,6 @@ static struct notifier_block lockd_inet6addr_notifier = {
>> #endif
>>
>> static const struct svc_serv_ops lockd_sv_ops = {
>> - .svo_shutdown = svc_rpcb_cleanup,
>> .svo_function = lockd,
>> .svo_module = THIS_MODULE,
>> };
>> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
>> index aeeac6de1f0a..0c6b216e439e 100644
>> --- a/fs/nfsd/nfssvc.c
>> +++ b/fs/nfsd/nfssvc.c
>> @@ -613,7 +613,6 @@ static int nfsd_get_default_max_blksize(void)
>> }
>>
>> static const struct svc_serv_ops nfsd_thread_sv_ops = {
>> - .svo_shutdown = nfsd_last_thread,
>> .svo_function = nfsd,
>> .svo_module = THIS_MODULE,
>> };
>> @@ -724,6 +723,7 @@ void nfsd_put(struct net *net)
>>
>> if (kref_put(&nn->nfsd_serv->sv_refcnt, nfsd_noop)) {
>> svc_shutdown_net(nn->nfsd_serv, net);
>> + nfsd_last_thread(nn->nfsd_serv, net);
>> svc_destroy(&nn->nfsd_serv->sv_refcnt);
>> spin_lock(&nfsd_notifier_lock);
>> nn->nfsd_serv = NULL;
>> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>> index 6ef9c1cafd0b..63794d772eb3 100644
>> --- a/include/linux/sunrpc/svc.h
>> +++ b/include/linux/sunrpc/svc.h
>> @@ -55,9 +55,6 @@ struct svc_pool {
>> struct svc_serv;
>>
>> struct svc_serv_ops {
>> - /* Callback to use when last thread exits. */
>> - void (*svo_shutdown)(struct svc_serv *, struct net *);
>> -
>> /* function for service threads to run */
>> int (*svo_function)(void *);
>>
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index 2aabec2b4bec..74a75a22da9a 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -539,9 +539,6 @@ EXPORT_SYMBOL_GPL(svc_create_pooled);
>> void svc_shutdown_net(struct svc_serv *serv, struct net *net)
>> {
>> svc_close_net(serv, net);
>> -
>> - if (serv->sv_ops->svo_shutdown)
>> - serv->sv_ops->svo_shutdown(serv, net);
>> }
>> EXPORT_SYMBOL_GPL(svc_shutdown_net);
>
> Could we rename svc_close_net() to svc_shutdown_net() and drop this
> function?
I considered that, but svc_close_net() seems to be transport-related,
whereas svc_shutdown_net() seems to be generic to the NFS server, so
I left them separate. A better rationale might push me into merging
them. :-)
> Either way:
> Reviewed-by: NeilBrown <[email protected]>
>
> Thanks,
> NeilBrown
--
Chuck Lever
On Wed, 26 Jan 2022, Chuck Lever III wrote:
>
> > On Jan 25, 2022, at 4:53 PM, NeilBrown <[email protected]> wrote:
> >
> >
> > Could we rename svc_close_net() to svc_shutdown_net() and drop this
> > function?
>
> I considered that, but svc_close_net() seems to be transport-related,
> whereas svc_shutdown_net() seems to be generic to the NFS server, so
> I left them separate. A better rationale might push me into merging
> them. :-)
>
svc_close_net() is effectively the inverse of svc_create_xprt(), though
the later can be called several times, and the former cleans up them
all.
So maybe rename svc_close_net() to svc_close_xprts() (plural), and call
it from the places which currently call svc_close_net(). Those places
(nfsd, lockd, nfs/callback) already call svc_create_xprt(). Having them
explicitly call svc_close_xprts() to balance that would arguably make
the code clearer.
Thanks,
NeilBrown