* [PATCH v1 0/2] rpcbind unregistration clean-ups @ 2025-08-20 14:27 Chuck Lever 2025-08-20 14:27 ` [PATCH v1 1/2] NFS: Remove rpcbind cleanup for NFSv4.0 callback Chuck Lever 2025-08-20 14:27 ` [PATCH v1 2/2] SUNRPC: Move the svc_rpcb_cleanup() call sites Chuck Lever 0 siblings, 2 replies; 4+ messages in thread From: Chuck Lever @ 2025-08-20 14:27 UTC (permalink / raw) To: linux-nfs; +Cc: Chuck Lever From: Chuck Lever <chuck.lever@oracle.com> Following on Olga's fix, here are two possible clean-ups I found during code review. Chuck Lever (2): NFS: Remove rpcbind cleanup for NFSv4.0 callback SUNRPC: Move the svc_rpcb_cleanup() call sites fs/lockd/svc.c | 6 ++---- fs/nfs/callback.c | 10 ++++------ fs/nfsd/nfsctl.c | 2 +- fs/nfsd/nfssvc.c | 7 ++----- include/linux/sunrpc/svc_xprt.h | 3 ++- net/sunrpc/svc.c | 1 - net/sunrpc/svc_xprt.c | 7 ++++++- 7 files changed, 17 insertions(+), 19 deletions(-) -- 2.50.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v1 1/2] NFS: Remove rpcbind cleanup for NFSv4.0 callback 2025-08-20 14:27 [PATCH v1 0/2] rpcbind unregistration clean-ups Chuck Lever @ 2025-08-20 14:27 ` Chuck Lever 2025-08-20 14:27 ` [PATCH v1 2/2] SUNRPC: Move the svc_rpcb_cleanup() call sites Chuck Lever 1 sibling, 0 replies; 4+ messages in thread From: Chuck Lever @ 2025-08-20 14:27 UTC (permalink / raw) To: linux-nfs; +Cc: Chuck Lever From: Chuck Lever <chuck.lever@oracle.com> The NFS client's NFSv4.0 callback listeners are created with SVC_SOCK_ANONYMOUS, therefore svc_setup_socket() does not register them with the client's rpcbind service. And, note that nfs_callback_down_net() does not call svc_rpcb_cleanup() at all when shutting down the callback server. Even if svc_setup_socket() were to attempt to register or unregister these sockets, the callback service has vs_hidden set, which shunts the rpcbind upcalls. The svc_rpcb_cleanup() error flow was introduced by commit c946556b8749 ("NFS: move per-net callback thread initialization to nfs_callback_up_net()"). It doesn't appear in the code that was relocated by that commit. Therefore, there is no need to call svc_rpcb_cleanup() when listener creation fails during callback server start-up. Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- fs/nfs/callback.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c index 86bdc7d23fb9..511f80878809 100644 --- a/fs/nfs/callback.c +++ b/fs/nfs/callback.c @@ -153,7 +153,7 @@ static int nfs_callback_up_net(int minorversion, struct svc_serv *serv, ret = svc_bind(serv, net); if (ret < 0) { printk(KERN_WARNING "NFS: bind callback service failed\n"); - goto err_bind; + goto err; } ret = 0; @@ -166,13 +166,11 @@ static int nfs_callback_up_net(int minorversion, struct svc_serv *serv, if (ret < 0) { printk(KERN_ERR "NFS: callback service start failed\n"); - goto err_socks; + goto err; } return 0; -err_socks: - svc_rpcb_cleanup(serv, net); -err_bind: +err: nn->cb_users[minorversion]--; dprintk("NFS: Couldn't create callback socket: err = %d; " "net = %x\n", ret, net->ns.inum); -- 2.50.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v1 2/2] SUNRPC: Move the svc_rpcb_cleanup() call sites 2025-08-20 14:27 [PATCH v1 0/2] rpcbind unregistration clean-ups Chuck Lever 2025-08-20 14:27 ` [PATCH v1 1/2] NFS: Remove rpcbind cleanup for NFSv4.0 callback Chuck Lever @ 2025-08-20 14:27 ` Chuck Lever 2025-08-20 15:53 ` Olga Kornievskaia 1 sibling, 1 reply; 4+ messages in thread From: Chuck Lever @ 2025-08-20 14:27 UTC (permalink / raw) To: linux-nfs; +Cc: Chuck Lever From: Chuck Lever <chuck.lever@oracle.com> Clean up: because svc_rpcb_cleanup() and svc_xprt_destroy_all() are always invoked in pairs, we can deduplicate code by moving the svc_rpcb_cleanup() call sites into svc_xprt_destroy_all(). Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- fs/lockd/svc.c | 6 ++---- fs/nfs/callback.c | 2 +- fs/nfsd/nfsctl.c | 2 +- fs/nfsd/nfssvc.c | 7 ++----- include/linux/sunrpc/svc_xprt.h | 3 ++- net/sunrpc/svc.c | 1 - net/sunrpc/svc_xprt.c | 7 ++++++- 7 files changed, 14 insertions(+), 14 deletions(-) diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c index e80262a51884..d68afa196535 100644 --- a/fs/lockd/svc.c +++ b/fs/lockd/svc.c @@ -216,8 +216,7 @@ static int make_socks(struct svc_serv *serv, struct net *net, if (warned++ == 0) printk(KERN_WARNING "lockd_up: makesock failed, error=%d\n", err); - svc_xprt_destroy_all(serv, net); - svc_rpcb_cleanup(serv, net); + svc_xprt_destroy_all(serv, net, true); return err; } @@ -255,8 +254,7 @@ static void lockd_down_net(struct svc_serv *serv, struct net *net) nlm_shutdown_hosts_net(net); cancel_delayed_work_sync(&ln->grace_period_end); locks_end_grace(&ln->lockd_manager); - svc_xprt_destroy_all(serv, net); - svc_rpcb_cleanup(serv, net); + svc_xprt_destroy_all(serv, net, true); } } else { pr_err("%s: no users! net=%x\n", diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c index 511f80878809..c8b837006bb2 100644 --- a/fs/nfs/callback.c +++ b/fs/nfs/callback.c @@ -136,7 +136,7 @@ static void nfs_callback_down_net(u32 minorversion, struct svc_serv *serv, struc return; dprintk("NFS: destroy per-net callback data; net=%x\n", net->ns.inum); - svc_xprt_destroy_all(serv, net); + svc_xprt_destroy_all(serv, net, false); } static int nfs_callback_up_net(int minorversion, struct svc_serv *serv, diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c index bc6b776fc657..63d52edcad72 100644 --- a/fs/nfsd/nfsctl.c +++ b/fs/nfsd/nfsctl.c @@ -1993,7 +1993,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) * remaining listeners and recreate the list. */ if (delete) - svc_xprt_destroy_all(serv, net); + svc_xprt_destroy_all(serv, net, false); /* walk list of addrs again, open any that still don't exist */ nlmsg_for_each_attr_type(attr, NFSD_A_SERVER_SOCK_ADDR, info->nlhdr, diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c index 82b0111ac469..7057ddd7a0a8 100644 --- a/fs/nfsd/nfssvc.c +++ b/fs/nfsd/nfssvc.c @@ -535,16 +535,13 @@ void nfsd_destroy_serv(struct net *net) #endif } - svc_xprt_destroy_all(serv, net); - /* * write_ports can create the server without actually starting - * any threads--if we get shut down before any threads are + * any threads. If we get shut down before any threads are * started, then nfsd_destroy_serv will be run before any of this * other initialization has been done except the rpcb information. */ - svc_rpcb_cleanup(serv, net); - + svc_xprt_destroy_all(serv, net, true); nfsd_shutdown_net(net); svc_destroy(&serv); } diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h index 369a89aea186..fde60d4e2cd5 100644 --- a/include/linux/sunrpc/svc_xprt.h +++ b/include/linux/sunrpc/svc_xprt.h @@ -165,7 +165,8 @@ int svc_xprt_create(struct svc_serv *serv, const char *xprt_name, struct net *net, const int family, const unsigned short port, int flags, const struct cred *cred); -void svc_xprt_destroy_all(struct svc_serv *serv, struct net *net); +void svc_xprt_destroy_all(struct svc_serv *serv, struct net *net, + bool unregister); void svc_xprt_received(struct svc_xprt *xprt); void svc_xprt_enqueue(struct svc_xprt *xprt); void svc_xprt_put(struct svc_xprt *xprt); diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c index b1fab3a69544..9c7245d811eb 100644 --- a/net/sunrpc/svc.c +++ b/net/sunrpc/svc.c @@ -436,7 +436,6 @@ void svc_rpcb_cleanup(struct svc_serv *serv, struct net *net) svc_unregister(serv, net); rpcb_put_local(net); } -EXPORT_SYMBOL_GPL(svc_rpcb_cleanup); static int svc_uses_rpcbind(struct svc_serv *serv) { diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c index 8b1837228799..049ab53088e9 100644 --- a/net/sunrpc/svc_xprt.c +++ b/net/sunrpc/svc_xprt.c @@ -1102,6 +1102,7 @@ static void svc_clean_up_xprts(struct svc_serv *serv, struct net *net) * svc_xprt_destroy_all - Destroy transports associated with @serv * @serv: RPC service to be shut down * @net: target network namespace + * @unregister: true if it is OK to unregister the destroyed xprts * * Server threads may still be running (especially in the case where the * service is still running in other network namespaces). @@ -1114,7 +1115,8 @@ static void svc_clean_up_xprts(struct svc_serv *serv, struct net *net) * threads, we may need to wait a little while and then check again to * see if they're done. */ -void svc_xprt_destroy_all(struct svc_serv *serv, struct net *net) +void svc_xprt_destroy_all(struct svc_serv *serv, struct net *net, + bool unregister) { int delay = 0; @@ -1124,6 +1126,9 @@ void svc_xprt_destroy_all(struct svc_serv *serv, struct net *net) svc_clean_up_xprts(serv, net); msleep(delay++); } + + if (unregister) + svc_rpcb_cleanup(serv, net); } EXPORT_SYMBOL_GPL(svc_xprt_destroy_all); -- 2.50.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1 2/2] SUNRPC: Move the svc_rpcb_cleanup() call sites 2025-08-20 14:27 ` [PATCH v1 2/2] SUNRPC: Move the svc_rpcb_cleanup() call sites Chuck Lever @ 2025-08-20 15:53 ` Olga Kornievskaia 0 siblings, 0 replies; 4+ messages in thread From: Olga Kornievskaia @ 2025-08-20 15:53 UTC (permalink / raw) To: Chuck Lever; +Cc: linux-nfs, Chuck Lever On Wed, Aug 20, 2025 at 10:32 AM Chuck Lever <cel@kernel.org> wrote: > > From: Chuck Lever <chuck.lever@oracle.com> > > Clean up: because svc_rpcb_cleanup() and svc_xprt_destroy_all() > are always invoked in pairs, we can deduplicate code by moving > the svc_rpcb_cleanup() call sites into svc_xprt_destroy_all(). Tested-by: Olga Kornievskaia <okorniev@redhat.com> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > fs/lockd/svc.c | 6 ++---- > fs/nfs/callback.c | 2 +- > fs/nfsd/nfsctl.c | 2 +- > fs/nfsd/nfssvc.c | 7 ++----- > include/linux/sunrpc/svc_xprt.h | 3 ++- > net/sunrpc/svc.c | 1 - > net/sunrpc/svc_xprt.c | 7 ++++++- > 7 files changed, 14 insertions(+), 14 deletions(-) > > diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c > index e80262a51884..d68afa196535 100644 > --- a/fs/lockd/svc.c > +++ b/fs/lockd/svc.c > @@ -216,8 +216,7 @@ static int make_socks(struct svc_serv *serv, struct net *net, > if (warned++ == 0) > printk(KERN_WARNING > "lockd_up: makesock failed, error=%d\n", err); > - svc_xprt_destroy_all(serv, net); > - svc_rpcb_cleanup(serv, net); > + svc_xprt_destroy_all(serv, net, true); > return err; > } > > @@ -255,8 +254,7 @@ static void lockd_down_net(struct svc_serv *serv, struct net *net) > nlm_shutdown_hosts_net(net); > cancel_delayed_work_sync(&ln->grace_period_end); > locks_end_grace(&ln->lockd_manager); > - svc_xprt_destroy_all(serv, net); > - svc_rpcb_cleanup(serv, net); > + svc_xprt_destroy_all(serv, net, true); > } > } else { > pr_err("%s: no users! net=%x\n", > diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c > index 511f80878809..c8b837006bb2 100644 > --- a/fs/nfs/callback.c > +++ b/fs/nfs/callback.c > @@ -136,7 +136,7 @@ static void nfs_callback_down_net(u32 minorversion, struct svc_serv *serv, struc > return; > > dprintk("NFS: destroy per-net callback data; net=%x\n", net->ns.inum); > - svc_xprt_destroy_all(serv, net); > + svc_xprt_destroy_all(serv, net, false); > } > > static int nfs_callback_up_net(int minorversion, struct svc_serv *serv, > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > index bc6b776fc657..63d52edcad72 100644 > --- a/fs/nfsd/nfsctl.c > +++ b/fs/nfsd/nfsctl.c > @@ -1993,7 +1993,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) > * remaining listeners and recreate the list. > */ > if (delete) > - svc_xprt_destroy_all(serv, net); > + svc_xprt_destroy_all(serv, net, false); > > /* walk list of addrs again, open any that still don't exist */ > nlmsg_for_each_attr_type(attr, NFSD_A_SERVER_SOCK_ADDR, info->nlhdr, > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c > index 82b0111ac469..7057ddd7a0a8 100644 > --- a/fs/nfsd/nfssvc.c > +++ b/fs/nfsd/nfssvc.c > @@ -535,16 +535,13 @@ void nfsd_destroy_serv(struct net *net) > #endif > } > > - svc_xprt_destroy_all(serv, net); > - > /* > * write_ports can create the server without actually starting > - * any threads--if we get shut down before any threads are > + * any threads. If we get shut down before any threads are > * started, then nfsd_destroy_serv will be run before any of this > * other initialization has been done except the rpcb information. > */ > - svc_rpcb_cleanup(serv, net); > - > + svc_xprt_destroy_all(serv, net, true); > nfsd_shutdown_net(net); > svc_destroy(&serv); > } > diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h > index 369a89aea186..fde60d4e2cd5 100644 > --- a/include/linux/sunrpc/svc_xprt.h > +++ b/include/linux/sunrpc/svc_xprt.h > @@ -165,7 +165,8 @@ int svc_xprt_create(struct svc_serv *serv, const char *xprt_name, > struct net *net, const int family, > const unsigned short port, int flags, > const struct cred *cred); > -void svc_xprt_destroy_all(struct svc_serv *serv, struct net *net); > +void svc_xprt_destroy_all(struct svc_serv *serv, struct net *net, > + bool unregister); > void svc_xprt_received(struct svc_xprt *xprt); > void svc_xprt_enqueue(struct svc_xprt *xprt); > void svc_xprt_put(struct svc_xprt *xprt); > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > index b1fab3a69544..9c7245d811eb 100644 > --- a/net/sunrpc/svc.c > +++ b/net/sunrpc/svc.c > @@ -436,7 +436,6 @@ void svc_rpcb_cleanup(struct svc_serv *serv, struct net *net) > svc_unregister(serv, net); > rpcb_put_local(net); > } > -EXPORT_SYMBOL_GPL(svc_rpcb_cleanup); > > static int svc_uses_rpcbind(struct svc_serv *serv) > { > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c > index 8b1837228799..049ab53088e9 100644 > --- a/net/sunrpc/svc_xprt.c > +++ b/net/sunrpc/svc_xprt.c > @@ -1102,6 +1102,7 @@ static void svc_clean_up_xprts(struct svc_serv *serv, struct net *net) > * svc_xprt_destroy_all - Destroy transports associated with @serv > * @serv: RPC service to be shut down > * @net: target network namespace > + * @unregister: true if it is OK to unregister the destroyed xprts > * > * Server threads may still be running (especially in the case where the > * service is still running in other network namespaces). > @@ -1114,7 +1115,8 @@ static void svc_clean_up_xprts(struct svc_serv *serv, struct net *net) > * threads, we may need to wait a little while and then check again to > * see if they're done. > */ > -void svc_xprt_destroy_all(struct svc_serv *serv, struct net *net) > +void svc_xprt_destroy_all(struct svc_serv *serv, struct net *net, > + bool unregister) > { > int delay = 0; > > @@ -1124,6 +1126,9 @@ void svc_xprt_destroy_all(struct svc_serv *serv, struct net *net) > svc_clean_up_xprts(serv, net); > msleep(delay++); > } > + > + if (unregister) > + svc_rpcb_cleanup(serv, net); > } > EXPORT_SYMBOL_GPL(svc_xprt_destroy_all); > > -- > 2.50.0 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-08-20 15:54 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-08-20 14:27 [PATCH v1 0/2] rpcbind unregistration clean-ups Chuck Lever 2025-08-20 14:27 ` [PATCH v1 1/2] NFS: Remove rpcbind cleanup for NFSv4.0 callback Chuck Lever 2025-08-20 14:27 ` [PATCH v1 2/2] SUNRPC: Move the svc_rpcb_cleanup() call sites Chuck Lever 2025-08-20 15:53 ` Olga Kornievskaia
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox