Re: [PATCH v2] sunrpc: fix lost wakeups when stopping service threads

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2026-11-02 at 23:53 +0100, Kal Cutter Conley wrote:
> Stopping NFS can leave rpc.nfsd 0 in an uninterruptible wait with
> this stack:
> 
>   svc_stop_kthreads [sunrpc]
>   svc_set_num_threads [sunrpc]
>   nfsd_svc [nfsd]
>   write_threads [nfsd]
>   nfsctl_transaction_write [nfsd]
> 
> SP_NEED_VICTIM and SP_VICTIM_REMAINS remain set while all remaining
> nfsd threads sleep in svc_recv() and none has RQ_VICTIM set.
> 
> The stop request is published with unordered set_bit() operations.
> svc_pool_wake_idle_thread() checks the idle list and task state before
> calling wake_up_process(). If it sees an empty list or TASK_RUNNING,
> it skips the wakeup and its memory barrier. The requester can therefore
> miss a sleeping worker while that worker misses the stop request.
> 
> Use smp_mb__after_atomic() after the flag updates in
> svc_stop_kthreads() and svc_wake_up() to order work publication before
> both lockless checks, even when no wakeup occurs. Pair with
> set_current_state() and llist_add() in svc_thread_wait_for_work(), so
> the worker observes the pending work or the requester observes the
> idle worker and wakes it.
> 
> Fixes: 60749cbe3d8a ("sunrpc: change sp_nrthreads from atomic_t to unsigned int.")
> Signed-off-by: Kal Cutter Conley <kalcutterc@nvidia.com>
> ---
> Changes in v2:
> - Move the barrier into svc_stop_kthreads() and svc_wake_up(), using
>   smp_mb__after_atomic(). This avoids adding a redundant barrier on the
>   transport enqueue path.
> 
> v1: https://lore.kernel.org/r/20261030130629.103716-1-kalcutterc@nvidia.com
> 
>  net/sunrpc/svc.c      | 7 +++++++
>  net/sunrpc/svc_xprt.c | 5 +++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index af8d87c1de83..76232774646c 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -29,6 +29,8 @@
>  #include <linux/sunrpc/clnt.h>
>  #include <linux/sunrpc/bc_xprt.h>
>  
> +#include <asm/barrier.h>
> +

As Neil said, this is probably not needed.

>  #include <trace/events/sunrpc.h>
>  
>  #include "fail.h"
> @@ -784,6 +786,11 @@ svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs)
>  	do {
>  		set_bit(SP_VICTIM_REMAINS, &pool->sp_flags);
>  		set_bit(SP_NEED_VICTIM, &pool->sp_flags);
> +		/*
> +		 * Order flag updates before the idle list and task state checks.
> +		 * Pairs with the barriers in svc_thread_wait_for_work().
> +		 */
> +		smp_mb__after_atomic();
>  		svc_pool_wake_idle_thread(pool);
>  		wait_on_bit(&pool->sp_flags, SP_VICTIM_REMAINS, TASK_IDLE);
>  		nrservs++;
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 076c624fe675..868e3da57b6c 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -632,6 +632,11 @@ void svc_wake_up(struct svc_serv *serv)
>  	struct svc_pool *pool = &serv->sv_pools[0];
>  
>  	set_bit(SP_TASK_PENDING, &pool->sp_flags);
> +	/*
> +	 * Order flag updates before the idle list and task state checks.
> +	 * Pairs with the barriers in svc_thread_wait_for_work().
> +	 */
> +	smp_mb__after_atomic();
>  	svc_pool_wake_idle_thread(pool);
>  }
>  EXPORT_SYMBOL_GPL(svc_wake_up);
> 
> base-commit: 2d3259f652bb0f1d137ccabecac6763da05064ec

Reviewed-by: Jeff Layton <jlayton@kernel.org>