On Thu, 2017-05-04 at 15:50 -0700, Bart Van Assche wrote:
> For several target drivers (e.g. ib_srpt and ib_isert) sleeping inside
> transport_generic_free_cmd() causes RDMA completion processing to stall.
> Hence only sleep inside this function if the (iSCSI) target driver
> requires this.
>
> This patch avoids that messages similar to the following appear in the
> kernel log:
>
> INFO: task kworker/u25:0:1013 blocked for more than 480 seconds.
> Tainted: G W 4.10.0-rc7-dbg+ #3
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> kworker/u25:0 D 0 1013 2 0x00000000
> Workqueue: ib-comp-wq ib_cq_poll_work [ib_core]
> Call Trace:
> __schedule+0x2da/0xb00
> schedule+0x38/0x90
> schedule_timeout+0x2fe/0x640
> wait_for_completion+0xfe/0x160
> transport_generic_free_cmd+0x2e/0x80 [target_core_mod]
> srpt_send_done+0x59/0x9f [ib_srpt]
> __ib_process_cq+0x4b/0xd0 [ib_core]
> ib_cq_poll_work+0x1b/0x60 [ib_core]
> process_one_work+0x208/0x6a0
> worker_thread+0x49/0x4a0
> kthread+0x107/0x140
> ret_from_fork+0x2e/0x40
>
> Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
> Cc: Hannes Reinecke <hare@xxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Andy Grover <agrover@xxxxxxxxxx>
> Cc: David Disseldorp <ddiss@xxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> ---
> drivers/target/target_core_transport.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 37f57357d4a0..df1ceb2dd110 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2553,7 +2553,8 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
> */
> if (aborted) {
> pr_debug("Detected CMD_T_ABORTED for ITT: %llu\n", cmd->tag);
> - wait_for_completion(&cmd->cmd_wait_comp);
> + if (wait_for_tasks)
> + wait_for_completion(&cmd->cmd_wait_comp);
> cmd->se_tfo->release_cmd(cmd);
> ret = 1;
> }
Grr, we have already been through this once before, remember..?
http://www.spinics.net/lists/target-devel/msg14598.html
To repeat, aborted = true is only set when transport_generic_free_cmd()
is called with wait_for_tasks = true, and neither srpt nor isert have
ever invoked transport_generic_free_cmd() with wait_for_tasks = true in
upstream code:
drivers/infiniband/ulp/isert/ib_isert.c: transport_generic_free_cmd(&cmd->se_cmd, 0);
drivers/infiniband/ulp/isert/ib_isert.c: transport_generic_free_cmd(&cmd->se_cmd, 0);
drivers/infiniband/ulp/isert/ib_isert.c: transport_generic_free_cmd(&cmd->se_cmd, 0);
drivers/infiniband/ulp/srpt/ib_srpt.c: transport_generic_free_cmd(&ioctx->cmd, 0);
drivers/infiniband/ulp/srpt/ib_srpt.c: transport_generic_free_cmd(&ioctx->cmd, 0);
drivers/infiniband/ulp/srpt/ib_srpt.c: transport_generic_free_cmd(&ioctx->cmd, 0);
And even if they did, this patch still would be a NOP and not make any
difference !
So it's clear what you did here, once upon a time you came up with a
bogus patch to address breakage for you own stuff in out-of-tree code,
and then copy-and-pasted the old backtrace from the out-of-tree bug and
posted it as an fix here, for a scenario that can't ever possibly occur
in upstream.
Really, please stop sending this type of garbage as it further erodes
what little trust I have left.