Re: [PATCH 03/19] target: Avoid that aborting a command sporadically hangs

["Nicholas A. Bellinger" <nab@linux-iscsi.org>]

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@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Andy Grover <agrover@redhat.com>
> Cc: David Disseldorp <ddiss@suse.de>
> Cc: <stable@vger.kernel.org>
> ---
>  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.