[PATCH v1 0/2] rpcbind unregistration clean-ups

[PATCH v1 0/2] rpcbind unregistration clean-ups

[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

[PATCH v1 1/2] NFS: Remove rpcbind cleanup for NFSv4.0 callback

[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

[PATCH v1 2/2] SUNRPC: Move the svc_rpcb_cleanup() call sites

[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

Re: [PATCH v1 2/2] SUNRPC: Move the svc_rpcb_cleanup() call sites

[Olga Kornievskaia]

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
>
>