Re: [RFC v4 1/9] SUNRPC: Add flag to kill new tasks

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 2848 bytes --]

On Fri, Nov 17 2017, Joshua Watt wrote:

> The flag causes any new tasks that are queued to exit immediately with
> -EIO instead of executing. This will allow clients (particularly NFS) to
> prevents these task from delaying shutdown of the RPC session longer
> than necessary.
>
> Signed-off-by: Joshua Watt <JPEWhacker@gmail.com>
> ---
>  include/linux/sunrpc/clnt.h | 1 +
>  net/sunrpc/clnt.c           | 5 ++---
>  net/sunrpc/sched.c          | 3 +++
>  3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> index 71c237e8240e..d36dc529fdf0 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -54,6 +54,7 @@ struct rpc_clnt {
>  				cl_noretranstimeo: 1,/* No retransmit timeouts */
>  				cl_autobind : 1,/* use getport() */
>  				cl_chatty   : 1;/* be verbose */
> +	atomic_t		cl_kill_new_tasks;	/* Kill all new tasks */

It looks a little weird that this is an atomic_t.
Looking further ahead, it seems that it is a counter of threads that
have request new tasks be killed.
It might help to make that clear, at least in a comment.

>  
>  	struct rpc_rtt *	cl_rtt;		/* RTO estimator data */
>  	const struct rpc_timeout *cl_timeout;	/* Timeout strategy */
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index df4ecb042ebe..d5eedabf0b4d 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -626,6 +626,8 @@ static struct rpc_clnt *__rpc_clone_client(struct rpc_create_args *args,
>  	new->cl_noretranstimeo = clnt->cl_noretranstimeo;
>  	new->cl_discrtry = clnt->cl_discrtry;
>  	new->cl_chatty = clnt->cl_chatty;
> +	atomic_set(&new->cl_kill_new_tasks,
> +		   atomic_read(&clnt->cl_kill_new_tasks));

However ... as it is a counter, it isn't clear that copying the counter
makes sense, and the new counter will never get decremented.
Maybe __rpc_clone_client() should fail if ->cl_kill_new_tasks > 0 ??

Thanks,
NeilBrown


>  	return new;
>  
>  out_err:
> @@ -818,9 +820,6 @@ void rpc_killall_tasks(struct rpc_clnt *clnt)
>  {
>  	struct rpc_task	*rovr;
>  
> -
> -	if (list_empty(&clnt->cl_tasks))
> -		return;
>  	dprintk("RPC:       killing all tasks for client %p\n", clnt);
>  	/*
>  	 * Spin lock all_tasks to prevent changes...
> diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
> index 0cc83839c13c..e70034e19ccd 100644
> --- a/net/sunrpc/sched.c
> +++ b/net/sunrpc/sched.c
> @@ -748,6 +748,9 @@ static void __rpc_execute(struct rpc_task *task)
>  	dprintk("RPC: %5u __rpc_execute flags=0x%x\n",
>  			task->tk_pid, task->tk_flags);
>  
> +	if (atomic_read(&task->tk_client->cl_kill_new_tasks))
> +		rpc_exit(task, -EIO);
> +
>  	WARN_ON_ONCE(RPC_IS_QUEUED(task));
>  	if (RPC_IS_QUEUED(task))
>  		return;
> -- 
> 2.13.6

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]