On Thu, 2025-08-28 at 11:21 -0400, Chuck Lever wrote:
> On 8/27/25 10:21 AM, Chuck Lever wrote:
> > On 8/26/25 6:00 PM, Olga Kornievskaia wrote:
> > > This patch tries to address the following failure:
> > > nfsdctl threads 0
> > > nfsdctl listener +rdma::20049
> > > nfsdctl listener +tcp::2049
> > > nfsdctl listener -tcp::2049
> > > nfsdctl: Error: Cannot assign requested address
> > >
> > > The reason for the failure is due to the fact that socket cleanup only
> > > happens in __svc_rdma_free() which is a deferred work triggers when an
> > > rdma transport is destroyed. To remove a listener nfsdctl is forced to
> > > first remove all transports via svc_xprt_destroy_all() and then re-add
> > > the ones that are left. Due to the fact that there isn't a way to
> > > delete a particular entry from server's lwq sp_xprts that stores
> > > transports. Going back to the deferred work done in __svc_rdma_free(),
> > > the work might not get to run before nfsd_nl_listener_set_doit() creates
> > > the new transports. As a result, it finds that something is still
> > > listening of the rdma port and rdma_bind_addr() fails.
> > >
> > > Instead of using svc_xprt_destroy_all() to manipulate the sp_xprt,
> > > instead introduce a function that just dequeues all transports. Then,
> > > we add non-removed transports back to the list.
> > >
> > > Still not allowing to remove a listener while the server is active.
> > >
> > > We need to make several passes over the list of existing/new list
> > > entries. On the first pass we determined if any of the entries need
> > > to be removed. If so, we then check if the server has no active
> > > threads. Then we dequeue all the transports and then go over the
> > > list and recreate both permsocks list and sp_xprts lists. Then,
> > > for the deleted transports, the transport is closed.
> >
> > > --- Comments:
> > > (1) There is still a restriction on removing an active listener as
> > > I dont know how to handle if the transport to be remove is currently
> > > serving a request (it won't be on the sp_xprt list I believe?).
> >
> > This is a good reason why just setting a bit in the xprt and waiting for
> > the close to complete is probably a better strategy than draining and
> > refilling the permsock list.
> >
> > The idea of setting XPT_CLOSE and enqueuing the transport ... you know,
> > like this:
> >
> > 151 /**
> >
> > 152 * svc_xprt_deferred_close - Close a transport
> >
> > 153 * @xprt: transport instance
> >
> > 154 *
> >
> > 155 * Used in contexts that need to defer the work of shutting down
> >
> > 156 * the transport to an nfsd thread.
> >
> > 157 */
> >
> > 158 void svc_xprt_deferred_close(struct svc_xprt *xprt)
> >
> > 159 {
> >
> > 160 trace_svc_xprt_close(xprt);
> >
> > 161 if (!test_and_set_bit(XPT_CLOSE, &xprt->xpt_flags))
> >
> > 162 svc_xprt_enqueue(xprt);
> >
> > 163 }
> >
> > 164 EXPORT_SYMBOL_GPL(svc_xprt_deferred_close);
> >
> > I expect that eventually the xprt will show up to svc_handle_xprt() and
> > get deleted there. But you might still need some serialization with
> > ->xpo_accept ?
>
> It occurred to me why the deferred close mechanism doesn't work: it
> relies on having an nfsd thread to pick up the deferred work.
>
> If listener removal requires all nfsd threads to be terminated, there
> is no thread to pick up the xprt and close it.
>
Interesting. I guess that the old nfsdfs file just didn't allow you to
get into this situation, since you couldn't remove a listener at all.
It really sounds like we just need a more selective version of
svc_clean_up_xprts(). Something that can dequeue everything, close the
ones that need to be closed (synchronously) and then requeues the rest.
>
> > > In general, I'm unsure if there are other things I'm not considering.
> > > (2) I'm questioning if in svc_xprt_dequeue_all() it is correct. I
> > > used svc_cleanup_up_xprts() as the example.
> > > > Fixes: d093c90892607 ("nfsd: fix management of listener transports")
> > > Signed-off-by: Olga Kornievskaia <okorniev@xxxxxxxxxx>
> > > ---
> > > fs/nfsd/nfsctl.c | 123 +++++++++++++++++++-------------
> > > include/linux/sunrpc/svc_xprt.h | 1 +
> > > include/linux/sunrpc/svcsock.h | 1 -
> > > net/sunrpc/svc_xprt.c | 12 ++++
> > > 4 files changed, 88 insertions(+), 49 deletions(-)
> > >
> > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > > index dd3267b4c203..38aaaef4734e 100644
> > > --- a/fs/nfsd/nfsctl.c
> > > +++ b/fs/nfsd/nfsctl.c
> > > @@ -1902,44 +1902,17 @@ int nfsd_nl_version_get_doit(struct sk_buff *skb, struct genl_info *info)
> > > return err;
> > > }
> > >
> > > -/**
> > > - * nfsd_nl_listener_set_doit - set the nfs running sockets
> > > - * @skb: reply buffer
> > > - * @info: netlink metadata and command arguments
> > > - *
> > > - * Return 0 on success or a negative errno.
> > > - */
> > > -int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
> > > +static void _nfsd_walk_listeners(struct genl_info *info, struct svc_serv *serv,
> > > + struct list_head *permsocks, int modify_xprt)
> >
> > So this function looks for the one listener we need to remove.
> >
> > Should removing a listener also close down all active temporary sockets
> > for the service, or should it kill only the ones that were established
> > via the listener being removed, or should it leave all active temporary
> > sockets in place?
> >
> > Perhaps this is why /all/ permanent and temporary sockets are currently
> > being removed. Once the target listener is gone, clients can't
> > re-establish new connections, and the service is effectively ready to
> > be shut down cleanly.
> >
> >
> > > {
> > > struct net *net = genl_info_net(info);
> > > - struct svc_xprt *xprt, *tmp;
> > > const struct nlattr *attr;
> > > - struct svc_serv *serv;
> > > - LIST_HEAD(permsocks);
> > > - struct nfsd_net *nn;
> > > - bool delete = false;
> > > - int err, rem;
> > > -
> > > - mutex_lock(&nfsd_mutex);
> > > -
> > > - err = nfsd_create_serv(net);
> > > - if (err) {
> > > - mutex_unlock(&nfsd_mutex);
> > > - return err;
> > > - }
> > > -
> > > - nn = net_generic(net, nfsd_net_id);
> > > - serv = nn->nfsd_serv;
> > > -
> > > - spin_lock_bh(&serv->sv_lock);
> > > + struct svc_xprt *xprt, *tmp;
> > > + int rem;
> > >
> > > - /* Move all of the old listener sockets to a temp list */
> > > - list_splice_init(&serv->sv_permsocks, &permsocks);
> > > + if (modify_xprt)
> > > + svc_xprt_dequeue_all(serv);
> > >
> > > - /*
> > > - * Walk the list of server_socks from userland and move any that match
> > > - * back to sv_permsocks
> > > - */
> > > nlmsg_for_each_attr(attr, info->nlhdr, GENL_HDRLEN, rem) {
> > > struct nlattr *tb[NFSD_A_SOCK_MAX + 1];
> > > const char *xcl_name;
> > > @@ -1962,7 +1935,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
> > > sa = nla_data(tb[NFSD_A_SOCK_ADDR]);
> > >
> > > /* Put back any matching sockets */
> > > - list_for_each_entry_safe(xprt, tmp, &permsocks, xpt_list) {
> > > + list_for_each_entry_safe(xprt, tmp, permsocks, xpt_list) {
> > > /* This shouldn't be possible */
> > > if (WARN_ON_ONCE(xprt->xpt_net != net)) {
> > > list_move(&xprt->xpt_list, &serv->sv_permsocks);
> > > @@ -1971,35 +1944,89 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
> > >
> > > /* If everything matches, put it back */
> > > if (!strcmp(xprt->xpt_class->xcl_name, xcl_name) &&
> > > - rpc_cmp_addr_port(sa, (struct sockaddr *)&xprt->xpt_local)) {
> > > + rpc_cmp_addr_port(sa,
> > > + (struct sockaddr *)&xprt->xpt_local)) {
> > > list_move(&xprt->xpt_list, &serv->sv_permsocks);
> > > + if (modify_xprt)
> > > + svc_xprt_enqueue(xprt);
> > > break;
> > > }
> > > }
> > > }
> > > +}
> > > +
> > > +/**
> > > + * nfsd_nl_listener_set_doit - set the nfs running sockets
> > > + * @skb: reply buffer
> > > + * @info: netlink metadata and command arguments
> > > + *
> > > + * Return 0 on success or a negative errno.
> > > + */
> > > +int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
> > > +{
> > > + struct net *net = genl_info_net(info);
> > > + struct svc_xprt *xprt;
> > > + const struct nlattr *attr;
> > > + struct svc_serv *serv;
> > > + LIST_HEAD(permsocks);
> > > + struct nfsd_net *nn;
> > > + bool delete = false;
> > > + int err, rem;
> > > +
> > > + mutex_lock(&nfsd_mutex);
> > > +
> > > + err = nfsd_create_serv(net);
> > > + if (err) {
> > > + mutex_unlock(&nfsd_mutex);
> > > + return err;
> > > + }
> > > +
> > > + nn = net_generic(net, nfsd_net_id);
> > > + serv = nn->nfsd_serv;
> > > +
> > > + spin_lock_bh(&serv->sv_lock);
> > > +
> > > + /* Move all of the old listener sockets to a temp list */
> > > + list_splice_init(&serv->sv_permsocks, &permsocks);
> > >
> > > /*
> > > - * If there are listener transports remaining on the permsocks list,
> > > - * it means we were asked to remove a listener.
> > > + * Walk the list of server_socks from userland and move any that match
> > > + * back to sv_permsocks. Determine if anything needs to be removed so
> > > + * don't manipulate sp_xprts list.
> > > */
> > > - if (!list_empty(&permsocks)) {
> > > - list_splice_init(&permsocks, &serv->sv_permsocks);
> > > - delete = true;
> > > - }
> > > - spin_unlock_bh(&serv->sv_lock);
> > > + _nfsd_walk_listeners(info, serv, &permsocks, false);
> > >
> > > - /* Do not remove listeners while there are active threads. */
> > > - if (serv->sv_nrthreads) {
> > > + /* For now, no removing old sockets while server is running */
> > > + if (serv->sv_nrthreads && !list_empty(&permsocks)) {
> > > + list_splice_init(&permsocks, &serv->sv_permsocks);
> > > + spin_unlock_bh(&serv->sv_lock);
> > > err = -EBUSY;
> > > goto out_unlock_mtx;
> > > }
> > >
> > > /*
> > > - * Since we can't delete an arbitrary llist entry, destroy the
> > > - * remaining listeners and recreate the list.
> > > + * If there are listener transports remaining on the permsocks list,
> > > + * it means we were asked to remove a listener. Walk the list again,
> > > + * but this time also manage the sp_xprts but first removing all of
> > > + * them and only adding back the ones not being deleted. Then close
> > > + * the ones left on the list.
> > > */
> > > - if (delete)
> > > - svc_xprt_destroy_all(serv, net, false);
> > > + if (!list_empty(&permsocks)) {
> > > + list_splice_init(&permsocks, &serv->sv_permsocks);
> > > + list_splice_init(&serv->sv_permsocks, &permsocks);
> > > + _nfsd_walk_listeners(info, serv, &permsocks, true);
> > > + while (!list_empty(&permsocks)) {
> > > + xprt = list_first_entry(&permsocks, struct svc_xprt, xpt_list);
> > > + clear_bit(XPT_BUSY, &xprt->xpt_flags);
> > > + set_bit(XPT_CLOSE, &xprt->xpt_flags);
> > > + spin_unlock_bh(&serv->sv_lock);
> > > + svc_xprt_close(xprt);
> > > + spin_lock_bh(&serv->sv_lock);
> > > + }
> > > + spin_unlock_bh(&serv->sv_lock);
> > > + goto out_unlock_mtx;
> > > + }
> > > + spin_unlock_bh(&serv->sv_lock);
> > >
> > > /* walk list of addrs again, open any that still don't exist */
> > > nlmsg_for_each_attr(attr, info->nlhdr, GENL_HDRLEN, rem) {
> > > diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
> > > index da2a2531e110..7038fd8ef20a 100644
> > > --- a/include/linux/sunrpc/svc_xprt.h
> > > +++ b/include/linux/sunrpc/svc_xprt.h
> > > @@ -186,6 +186,7 @@ int svc_xprt_names(struct svc_serv *serv, char *buf, const int buflen);
> > > void svc_add_new_perm_xprt(struct svc_serv *serv, struct svc_xprt *xprt);
> > > void svc_age_temp_xprts_now(struct svc_serv *, struct sockaddr *);
> > > void svc_xprt_deferred_close(struct svc_xprt *xprt);
> > > +void svc_xprt_dequeue_all(struct svc_serv *serv);
> > >
> > > static inline void svc_xprt_get(struct svc_xprt *xprt)
> > > {
> > > diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h
> > > index 963bbe251e52..4c1be01afdb7 100644
> > > --- a/include/linux/sunrpc/svcsock.h
> > > +++ b/include/linux/sunrpc/svcsock.h
> > > @@ -65,7 +65,6 @@ int svc_addsock(struct svc_serv *serv, struct net *net,
> > > const struct cred *cred);
> > > void svc_init_xprt_sock(void);
> > > void svc_cleanup_xprt_sock(void);
> > > -
> > > /*
> > > * svc_makesock socket characteristics
> > > */
> > > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> > > index 6973184ff667..2aa46b9468d4 100644
> > > --- a/net/sunrpc/svc_xprt.c
> > > +++ b/net/sunrpc/svc_xprt.c
> > > @@ -890,6 +890,18 @@ void svc_recv(struct svc_rqst *rqstp)
> > > }
> > > EXPORT_SYMBOL_GPL(svc_recv);
> > >
> > > +void svc_xprt_dequeue_all(struct svc_serv *serv)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < serv->sv_nrpools; i++) {
> > > + struct svc_pool *pool = &serv->sv_pools[i];
> > > +
> > > + lwq_dequeue_all(&pool->sp_xprts);
> > > + }
> > > +}
> > > +EXPORT_SYMBOL_GPL(svc_xprt_dequeue_all);
> > > +
> > > /**
> > > * svc_send - Return reply to client
> > > * @rqstp: RPC transaction context
> >
> >
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
