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@xxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: PGP signature
