-----Original Message-----
From: <linux-scsi-owner@xxxxxxxxxxxxxxx> on behalf of Bart Van Assche <Bart.VanAssche@xxxxxxxxxxx>
Date: Monday, May 22, 2017 at 11:23 AM
To: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
Cc: "linux-scsi@xxxxxxxxxxxxxxx" <linux-scsi@xxxxxxxxxxxxxxx>, "James.Bottomley@xxxxxxxxxxxxxxxxxxxxx" <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>, "Madhani, Himanshu" <Himanshu.Madhani@xxxxxxxxxx>, "martin.petersen@xxxxxxxxxx" <martin.petersen@xxxxxxxxxx>
Subject: Re: [PATCH 20/25] qla2xxx: Remove redundant code
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.
QT: “[PATCH 03/25] qla2xxx: Allow ABTS RX, RIDA on ATIOQ for ISP83XX/27XX” attempts to reduce the describe concurrency by moving the IOCB from the RSPQ thread to the ATIOQ thread. This helps preserve the ordering between SCSI cmd & ABTS.
I think an initiator could get
really confused if it receives two responses for the same exchange.
QT: I do see the window you’re describing in the qlt_try_to_dequeue_unknown_atios(). Will have to follow up with another patch. This patch is not meant for this window you’ve just identify.
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.
QT: Actually, we’re covered in this case. There 2 responds for 2 IOCB/transactions. 1) The abort_cmd_for_tag() finds the SCSI command on the qla_cmd_list and turns on the abort flag while cmd is in work queue & before __qlt_do_work is called. When __qlt_do_work is call and sees the aborted flag, the thread will terminate/cleanup the SCSI command and will not submit to TCM.
As for the ABTS, the abort_cmd_for_tag() returns 1 after finding the command will trigger an ABTS respond.
static void __qlt_do_work(struct qla_tgt_cmd *cmd)
{
if (cmd->aborted) {
ql_dbg(ql_dbg_tgt_mgt, vha, 0xf082,
"cmd with tag %u is aborted\n",
cmd->atio.u.isp24.exchange_addr);
goto out_term;
}
}
Shouldn't these issues be addressed properly?
QT: Will follow up with another patch for the new problem. thanks for the extra eyes.
Bart.
