Re: [PATCH 02/14] nfsd: don't allow nfsd threads to be signalled.

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2023-07-18 at 16:38 +1000, NeilBrown wrote:
> The original implementation of nfsd used signals to stop threads during
> shutdown.
> In Linux 2.3.46pre5 nfsd gained the ability to shutdown threads
> internally it if was asked to run "0" threads.  After this user-space
> transitioned to using "rpc.nfsd 0" to stop nfsd and sending signals to
> threads was no longer an important part of the API.
> 
> In Commit 3ebdbe5203a8 ("SUNRPC: discard svo_setup and rename
> svc_set_num_threads_sync()") (v5.17-rc1~75^2~41) we finally removed the
> use of signals for stopping threads, using kthread_stop() instead.
> 
> This patch makes the "obvious" next step and removes the ability to
> signal nfsd threads - or any svc threads.  nfsd stops allowing signals
> and we don't check for their delivery any more.
> 
> This will allow for some simplification in later patches.
> 
> A change worth noting is in nfsd4_ssc_setup_dul().  There was previously
> a signal_pending() check which would only succeed when the thread was
> being shut down.  It should really have tested kthread_should_stop() as
> well.  Now it just does the later, not the former.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfs/callback.c     |    9 +--------
>  fs/nfsd/nfs4proc.c    |    4 ++--
>  fs/nfsd/nfssvc.c      |   12 ------------
>  net/sunrpc/svc_xprt.c |   16 ++++++----------
>  4 files changed, 9 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> index 456af7d230cf..46a0a2d6962e 100644
> --- a/fs/nfs/callback.c
> +++ b/fs/nfs/callback.c
> @@ -80,9 +80,6 @@ nfs4_callback_svc(void *vrqstp)
>  	set_freezable();
>  
>  	while (!kthread_freezable_should_stop(NULL)) {
> -
> -		if (signal_pending(current))
> -			flush_signals(current);
>  		/*
>  		 * Listen for a request on the socket
>  		 */
> @@ -112,11 +109,7 @@ nfs41_callback_svc(void *vrqstp)
>  	set_freezable();
>  
>  	while (!kthread_freezable_should_stop(NULL)) {
> -
> -		if (signal_pending(current))
> -			flush_signals(current);
> -
> -		prepare_to_wait(&serv->sv_cb_waitq, &wq, TASK_INTERRUPTIBLE);
> +		prepare_to_wait(&serv->sv_cb_waitq, &wq, TASK_IDLE);
>  		spin_lock_bh(&serv->sv_cb_lock);
>  		if (!list_empty(&serv->sv_cb_list)) {
>  			req = list_first_entry(&serv->sv_cb_list,
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index d8e7a533f9d2..157488290676 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1325,11 +1325,11 @@ static __be32 nfsd4_ssc_setup_dul(struct nfsd_net *nn, char *ipaddr,
>  		if (ni->nsui_busy) {
>  			/*  wait - and try again */
>  			prepare_to_wait(&nn->nfsd_ssc_waitq, &wait,
> -				TASK_INTERRUPTIBLE);
> +				TASK_IDLE);
>  			spin_unlock(&nn->nfsd_ssc_lock);
>  
>  			/* allow 20secs for mount/unmount for now - revisit */
> -			if (signal_pending(current) ||
> +			if (kthread_should_stop() ||
>  					(schedule_timeout(20*HZ) == 0)) {
>  				finish_wait(&nn->nfsd_ssc_waitq, &wait);
>  				kfree(work);
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 97830e28c140..439fca195925 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -965,15 +965,6 @@ nfsd(void *vrqstp)
>  
>  	current->fs->umask = 0;
>  
> -	/*
> -	 * thread is spawned with all signals set to SIG_IGN, re-enable
> -	 * the ones that will bring down the thread
> -	 */
> -	allow_signal(SIGKILL);
> -	allow_signal(SIGHUP);
> -	allow_signal(SIGINT);
> -	allow_signal(SIGQUIT);
> -
>  	atomic_inc(&nfsdstats.th_cnt);
>  
>  	set_freezable();
> @@ -998,9 +989,6 @@ nfsd(void *vrqstp)
>  		validate_process_creds();
>  	}
>  
> -	/* Clear signals before calling svc_exit_thread() */
> -	flush_signals(current);
> -
>  	atomic_dec(&nfsdstats.th_cnt);
>  
>  out:
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 71b19d0ed642..93395606a0ba 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -686,8 +686,8 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
>  			/* Made progress, don't sleep yet */
>  			continue;
>  
> -		set_current_state(TASK_INTERRUPTIBLE);
> -		if (signalled() || kthread_should_stop()) {
> +		set_current_state(TASK_IDLE);
> +		if (kthread_should_stop()) {
>  			set_current_state(TASK_RUNNING);
>  			return -EINTR;
>  		}
> @@ -725,7 +725,7 @@ rqst_should_sleep(struct svc_rqst *rqstp)
>  		return false;
>  
>  	/* are we shutting down? */
> -	if (signalled() || kthread_should_stop())
> +	if (kthread_should_stop())
>  		return false;
>  
>  	/* are we freezing? */
> @@ -749,11 +749,7 @@ static struct svc_xprt *svc_get_next_xprt(struct svc_rqst *rqstp, long timeout)
>  		goto out_found;
>  	}
>  
> -	/*
> -	 * We have to be able to interrupt this wait
> -	 * to bring down the daemons ...
> -	 */
> -	set_current_state(TASK_INTERRUPTIBLE);
> +	set_current_state(TASK_IDLE);
>  	smp_mb__before_atomic();
>  	clear_bit(SP_CONGESTED, &pool->sp_flags);
>  	clear_bit(RQ_BUSY, &rqstp->rq_flags);
> @@ -776,7 +772,7 @@ static struct svc_xprt *svc_get_next_xprt(struct svc_rqst *rqstp, long timeout)
>  
>  	if (!time_left)
>  		percpu_counter_inc(&pool->sp_threads_timedout);
> -	if (signalled() || kthread_should_stop())
> +	if (kthread_should_stop())
>  		return ERR_PTR(-EINTR);
>  	percpu_counter_inc(&pool->sp_threads_no_work);
>  	return ERR_PTR(-EAGAIN);
> @@ -873,7 +869,7 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
>  	try_to_freeze();
>  	cond_resched();
>  	err = -EINTR;
> -	if (signalled() || kthread_should_stop())
> +	if (kthread_should_stop())
>  		goto out;
>  
>  	xprt = svc_get_next_xprt(rqstp, timeout);
> 
> 

LGTM

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