Re: [PATCH 20/25] qla2xxx: Remove redundant code

[Bart Van Assche <Bart.VanAssche@sandisk.com>]

On Sun, 2017-05-21 at 21:27 -0700, Nicholas A. Bellinger wrote:
> The three lists abort_cmd_for_tag() walks from __qlt_24xx_handle_abts()
> are used to track descriptors only before __qlt_do_work() is reached,
> and before a descriptor is submitted into tcm_qla2xxx code.
> 
> Or rather, the three lists in abort_cmd_for_tag() only contain
> qla_tgt_cmd or qla_tgt_sess_op descriptors that have not yet reached
> qla_tgt_func_tmpl->handle_cmd() code.
> 
> Both qlt_do_work() and qlt_create_sess_from_atio() drop their respective
> descriptors from ->cmd_list before dispatching into tcm_qla2xxx ->
> target-core, which means there is no way for a descriptor to be part of
> internal lists once __qlt_do_work() is called.
> 
> That said, the patch is correct and removes the redundant lookup.

I do not agree that this patch is makes ABTS handling fully robust. It seems
like you have not noticed that the following race can occur with or without
this patch applied: if abort_cmd_for_tag() and qlt_try_to_dequeue_unknown_atios()
are called concurrently, since the latter function calls list_del() after
qlt_send_term_exchange(), abort_cmd_for_tag() can return 1 and thereby trigger
a call to qlt_24xx_send_abts_resp() while qlt_try_to_dequeue_unknown_atios()
calls qlt_send_term_exchange() concurrently. I think an initiator could get
really confused if it receives two responses for the same exchange.

An existing issue that is not addressed by this patch is that if an ABTS is
received after qlt_do_work() has called list_del() and before __qlt_do_work()
triggers a call target_submit_cmd() that a command is not on any list and hence
that abort_cmd_for_tag() won't be able to find it anywhere.

Shouldn't these issues be addressed properly?

Bart.