[RFC,1/4] SUNRPC: Add flag to kill new tasks
diff mbox

Message ID 20171108145942.5127-2-JPEWhacker@gmail.com
State New
Headers show

Commit Message

Joshua Watt Nov. 8, 2017, 2:59 p.m. UTC
The new flag to rpc_killall_tasks() causes new tasks that are killed
after to call to be killed immediately instead of being executed. This
will all 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>
---
 fs/nfs/super.c               |  4 ++--
 include/linux/sunrpc/clnt.h  |  1 +
 include/linux/sunrpc/sched.h |  2 +-
 net/sunrpc/clnt.c            | 13 ++++++++++---
 net/sunrpc/sched.c           |  7 +++++++
 5 files changed, 21 insertions(+), 6 deletions(-)

Comments

NeilBrown Nov. 10, 2017, 1:39 a.m. UTC | #1
On Wed, Nov 08 2017, Joshua Watt wrote:

> The new flag to rpc_killall_tasks() causes new tasks that are killed
> after to call to be killed immediately instead of being executed. This
> will all clients (particularly NFS) to prevents these task from delaying
> shutdown of the RPC session longer than necessary.

I have trouble remembering to proof-read my commit messages too. :-)
(I count 3 typos).

>
> Signed-off-by: Joshua Watt <JPEWhacker@gmail.com>
> ---
>  fs/nfs/super.c               |  4 ++--
>  include/linux/sunrpc/clnt.h  |  1 +
>  include/linux/sunrpc/sched.h |  2 +-
>  net/sunrpc/clnt.c            | 13 ++++++++++---
>  net/sunrpc/sched.c           |  7 +++++++
>  5 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 216f67d628b3..66fda2dcadd0 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -903,10 +903,10 @@ void nfs_umount_begin(struct super_block *sb)
>  	/* -EIO all pending I/O */
>  	rpc = server->client_acl;
>  	if (!IS_ERR(rpc))
> -		rpc_killall_tasks(rpc);
> +		rpc_killall_tasks(rpc, 0);
>  	rpc = server->client;
>  	if (!IS_ERR(rpc))
> -		rpc_killall_tasks(rpc);
> +		rpc_killall_tasks(rpc, 0);
>  }
>  EXPORT_SYMBOL_GPL(nfs_umount_begin);
>  
> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> index 71c237e8240e..94f4e464de1b 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 */
> +	bool			cl_kill_new_tasks;	/* Kill all new tasks */
>  
>  	struct rpc_rtt *	cl_rtt;		/* RTO estimator data */
>  	const struct rpc_timeout *cl_timeout;	/* Timeout strategy */
> diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
> index d96e74e114c0..9b8bebaee0f4 100644
> --- a/include/linux/sunrpc/sched.h
> +++ b/include/linux/sunrpc/sched.h
> @@ -218,7 +218,7 @@ void		rpc_put_task_async(struct rpc_task *);
>  void		rpc_exit_task(struct rpc_task *);
>  void		rpc_exit(struct rpc_task *, int);
>  void		rpc_release_calldata(const struct rpc_call_ops *, void *);
> -void		rpc_killall_tasks(struct rpc_clnt *);
> +void		rpc_killall_tasks(struct rpc_clnt *clnt, int kill_new_tasks);
>  void		rpc_execute(struct rpc_task *);
>  void		rpc_init_priority_wait_queue(struct rpc_wait_queue *, const char *);
>  void		rpc_init_wait_queue(struct rpc_wait_queue *, const char *);
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index df4ecb042ebe..70005252b32f 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -814,14 +814,21 @@ EXPORT_SYMBOL_GPL(rpc_clnt_iterate_for_each_xprt);
>   * Kill all tasks for the given client.
>   * XXX: kill their descendants as well?
>   */
> -void rpc_killall_tasks(struct rpc_clnt *clnt)
> +void rpc_killall_tasks(struct rpc_clnt *clnt, int kill_new_tasks)
>  {
>  	struct rpc_task	*rovr;
>  
> +	dprintk("RPC:       killing all tasks for client %p\n", clnt);
> +
> +	if (kill_new_tasks) {
> +		WRITE_ONCE(clnt->cl_kill_new_tasks, true);
> +
> +		/* Ensure the kill flag is set before checking the task list */
> +		smp_mb();
> +	}

I don't find this smp_mb() usage convincing.  It might be "right", but
to me it looks weird.
Partly, this is because there is a perfectly good spinlock that would
make the code obviously correct.

I would remove the short-circuit (which returns if list_empty()) and
always take the spinlock.  Then
   if (kill_new_tasks)
   	clnt->cl_kill_new_tasks = true;


In __rpc_execute() I would just have
  if (task->tk_client->cl_kill_new_tasks)
  	rpc_exit(task, -EIO);

As the task was added to the client list with the cl_lock spinlock
helds, there are no visibility issues to require a memory barrier.

Otherwise, the patch seems to make sense.

Thanks,
NeilBrown


>  
>  	if (list_empty(&clnt->cl_tasks))
>  		return;
> -	dprintk("RPC:       killing all tasks for client %p\n", clnt);
>  	/*
>  	 * Spin lock all_tasks to prevent changes...
>  	 */
> @@ -854,7 +861,7 @@ void rpc_shutdown_client(struct rpc_clnt *clnt)
>  			rcu_dereference(clnt->cl_xprt)->servername);
>  
>  	while (!list_empty(&clnt->cl_tasks)) {
> -		rpc_killall_tasks(clnt);
> +		rpc_killall_tasks(clnt, 0);
>  		wait_event_timeout(destroy_wait,
>  			list_empty(&clnt->cl_tasks), 1*HZ);
>  	}
> diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
> index 0cc83839c13c..c9bf1ab9e941 100644
> --- a/net/sunrpc/sched.c
> +++ b/net/sunrpc/sched.c
> @@ -748,6 +748,13 @@ static void __rpc_execute(struct rpc_task *task)
>  	dprintk("RPC: %5u __rpc_execute flags=0x%x\n",
>  			task->tk_pid, task->tk_flags);
>  
> +	/* Ensure the task is in the client task list before checking the kill
> +	 * flag
> +	 */
> +	smp_mb();
> +	if (READ_ONCE(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

Patch
diff mbox

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 216f67d628b3..66fda2dcadd0 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -903,10 +903,10 @@  void nfs_umount_begin(struct super_block *sb)
 	/* -EIO all pending I/O */
 	rpc = server->client_acl;
 	if (!IS_ERR(rpc))
-		rpc_killall_tasks(rpc);
+		rpc_killall_tasks(rpc, 0);
 	rpc = server->client;
 	if (!IS_ERR(rpc))
-		rpc_killall_tasks(rpc);
+		rpc_killall_tasks(rpc, 0);
 }
 EXPORT_SYMBOL_GPL(nfs_umount_begin);
 
diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index 71c237e8240e..94f4e464de1b 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 */
+	bool			cl_kill_new_tasks;	/* Kill all new tasks */
 
 	struct rpc_rtt *	cl_rtt;		/* RTO estimator data */
 	const struct rpc_timeout *cl_timeout;	/* Timeout strategy */
diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index d96e74e114c0..9b8bebaee0f4 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -218,7 +218,7 @@  void		rpc_put_task_async(struct rpc_task *);
 void		rpc_exit_task(struct rpc_task *);
 void		rpc_exit(struct rpc_task *, int);
 void		rpc_release_calldata(const struct rpc_call_ops *, void *);
-void		rpc_killall_tasks(struct rpc_clnt *);
+void		rpc_killall_tasks(struct rpc_clnt *clnt, int kill_new_tasks);
 void		rpc_execute(struct rpc_task *);
 void		rpc_init_priority_wait_queue(struct rpc_wait_queue *, const char *);
 void		rpc_init_wait_queue(struct rpc_wait_queue *, const char *);
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index df4ecb042ebe..70005252b32f 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -814,14 +814,21 @@  EXPORT_SYMBOL_GPL(rpc_clnt_iterate_for_each_xprt);
  * Kill all tasks for the given client.
  * XXX: kill their descendants as well?
  */
-void rpc_killall_tasks(struct rpc_clnt *clnt)
+void rpc_killall_tasks(struct rpc_clnt *clnt, int kill_new_tasks)
 {
 	struct rpc_task	*rovr;
 
+	dprintk("RPC:       killing all tasks for client %p\n", clnt);
+
+	if (kill_new_tasks) {
+		WRITE_ONCE(clnt->cl_kill_new_tasks, true);
+
+		/* Ensure the kill flag is set before checking the task list */
+		smp_mb();
+	}
 
 	if (list_empty(&clnt->cl_tasks))
 		return;
-	dprintk("RPC:       killing all tasks for client %p\n", clnt);
 	/*
 	 * Spin lock all_tasks to prevent changes...
 	 */
@@ -854,7 +861,7 @@  void rpc_shutdown_client(struct rpc_clnt *clnt)
 			rcu_dereference(clnt->cl_xprt)->servername);
 
 	while (!list_empty(&clnt->cl_tasks)) {
-		rpc_killall_tasks(clnt);
+		rpc_killall_tasks(clnt, 0);
 		wait_event_timeout(destroy_wait,
 			list_empty(&clnt->cl_tasks), 1*HZ);
 	}
diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index 0cc83839c13c..c9bf1ab9e941 100644
--- a/net/sunrpc/sched.c
+++ b/net/sunrpc/sched.c
@@ -748,6 +748,13 @@  static void __rpc_execute(struct rpc_task *task)
 	dprintk("RPC: %5u __rpc_execute flags=0x%x\n",
 			task->tk_pid, task->tk_flags);
 
+	/* Ensure the task is in the client task list before checking the kill
+	 * flag
+	 */
+	smp_mb();
+	if (READ_ONCE(task->tk_client->cl_kill_new_tasks))
+		rpc_exit(task, -EIO);
+
 	WARN_ON_ONCE(RPC_IS_QUEUED(task));
 	if (RPC_IS_QUEUED(task))
 		return;