Re: [PATCH 2/2] NFSv4: Fix callback server shutdown

[Kinglong Mee <kinglongmee@gmail.com>]

On 4/26/2017 23:55, Trond Myklebust wrote:
> We want to use kthread_stop() in order to ensure the threads are
> shut down before we tear down the nfs_callback_info in nfs_callback_down.
> 
> Reported-by: Kinglong Mee <kinglongmee@gmail.com>
> Fixes: bb6aeba736ba9 ("NFSv4.x: Switch to using svc_set_num_threads()...")
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> Cc: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/nfs/callback.c          | 24 ++++++++++++++++--------
>  include/linux/sunrpc/svc.h |  1 +
>  net/sunrpc/svc.c           | 38 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 55 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> index 773774531aff..8cabae9b140e 100644
> --- a/fs/nfs/callback.c
> +++ b/fs/nfs/callback.c
> @@ -76,7 +76,10 @@ nfs4_callback_svc(void *vrqstp)
>  
>  	set_freezable();
>  
> -	while (!kthread_should_stop()) {
> +	while (!kthread_freezable_should_stop(NULL)) {
> +
> +		if (signal_pending(current))
> +			flush_signals(current);
>  		/*
>  		 * Listen for a request on the socket
>  		 */
> @@ -85,6 +88,8 @@ nfs4_callback_svc(void *vrqstp)
>  			continue;
>  		svc_process(rqstp);
>  	}
> +	svc_exit_thread(rqstp);
> +	module_put_and_exit(0);
>  	return 0;
>  }
>  
> @@ -103,9 +108,10 @@ nfs41_callback_svc(void *vrqstp)
>  
>  	set_freezable();
>  
> -	while (!kthread_should_stop()) {
> -		if (try_to_freeze())
> -			continue;
> +	while (!kthread_freezable_should_stop(NULL)) {
> +
> +		if (signal_pending(current))
> +			flush_signals(current);
>  
>  		prepare_to_wait(&serv->sv_cb_waitq, &wq, TASK_INTERRUPTIBLE);
>  		spin_lock_bh(&serv->sv_cb_lock);
> @@ -121,11 +127,13 @@ nfs41_callback_svc(void *vrqstp)
>  				error);
>  		} else {
>  			spin_unlock_bh(&serv->sv_cb_lock);
> -			schedule();
> +			if (!kthread_should_stop())
> +				schedule();
>  			finish_wait(&serv->sv_cb_waitq, &wq);
>  		}
> -		flush_signals(current);
>  	}
> +	svc_exit_thread(rqstp);
> +	module_put_and_exit(0);
>  	return 0;
>  }
>  
> @@ -221,14 +229,14 @@ static int nfs_callback_up_net(int minorversion, struct svc_serv *serv,
>  static struct svc_serv_ops nfs40_cb_sv_ops = {
>  	.svo_function		= nfs4_callback_svc,
>  	.svo_enqueue_xprt	= svc_xprt_do_enqueue,
> -	.svo_setup		= svc_set_num_threads,
> +	.svo_setup		= svc_set_num_threads_sync,
>  	.svo_module		= THIS_MODULE,
>  };
>  #if defined(CONFIG_NFS_V4_1)
>  static struct svc_serv_ops nfs41_cb_sv_ops = {
>  	.svo_function		= nfs41_callback_svc,
>  	.svo_enqueue_xprt	= svc_xprt_do_enqueue,
> -	.svo_setup		= svc_set_num_threads,
> +	.svo_setup		= svc_set_num_threads_sync,
>  	.svo_module		= THIS_MODULE,
>  };
>  
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index e770abeed32d..11cef5a7bc87 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -474,6 +474,7 @@ void		   svc_pool_map_put(void);
>  struct svc_serv *  svc_create_pooled(struct svc_program *, unsigned int,
>  			struct svc_serv_ops *);
>  int		   svc_set_num_threads(struct svc_serv *, struct svc_pool *, int);
> +int		   svc_set_num_threads_sync(struct svc_serv *, struct svc_pool *, int);
>  int		   svc_pool_stats_open(struct svc_serv *serv, struct file *file);
>  void		   svc_destroy(struct svc_serv *);
>  void		   svc_shutdown_net(struct svc_serv *, struct net *);
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index 98dc33ae738b..bc0f5a0ecbdc 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -795,6 +795,44 @@ svc_set_num_threads(struct svc_serv *serv, struct svc_pool *pool, int nrservs)
>  }
>  EXPORT_SYMBOL_GPL(svc_set_num_threads);
>  
> +/* destroy old threads */
> +static int
> +svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs)
> +{
> +	struct task_struct *task;
> +	unsigned int state = serv->sv_nrthreads-1;
> +
> +	/* destroy old threads */
> +	do {
> +		task = choose_victim(serv, pool, &state);
> +		if (task == NULL)
> +			break;
> +		kthread_stop(task);
> +		nrservs++;
> +	} while (nrservs < 0);
> +	return 0;
> +}
> +
> +int
> +svc_set_num_threads_sync(struct svc_serv *serv, struct svc_pool *pool, int nrservs)
> +{
> +	if (pool == NULL) {
> +		/* The -1 assumes caller has done a svc_get() */
> +		nrservs -= (serv->sv_nrthreads-1);
> +	} else {
> +		spin_lock_bh(&pool->sp_lock);
> +		nrservs -= pool->sp_nrthreads;
> +		spin_unlock_bh(&pool->sp_lock);
> +	}
> +
> +	if (nrservs > 0)
> +		return svc_start_kthreads(serv, pool, nrservs);
> +	if (nrservs < 0)
> +		return svc_stop_kthreads(serv, pool, nrservs);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(svc_set_num_threads_sync);

There is only one use of svc_set_num_threads that by nfsd,
I'd like to change svc_set_num_threads and update nfsd than add a new function.

Ps,
"NFSv4.x/callback: Create the callback service through svc_create_pooled"
must be include before the fix. I will resend it later.

thanks,
Kinglong Mee