Re: [RFC PATCH 1/1] nfsd: rework how a listener is removed

[Chuck Lever <chuck.lever@oracle.com>]

On 8/28/25 1:29 PM, Jeff Layton wrote:
> 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.

That is roughly what Olga has done here. Now that I understand the
problem space a little better, maybe we can iterate on this.


>>>> 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@redhat.com>
>>>> ---
>>>>  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
>>>
>>>
>>
> 


-- 
Chuck Lever