On Mon, Nov 21, 2022 at 01:25:55PM -0600, Mike Christie wrote:
>
> On 11/21/22 7:35 AM, Dmitry Bogdanov wrote:
> >>
> > I will vote to your old patch. qla2xxx was fixed long time ago.
>
> What is the qla fix? I think we still leak. In commit
>
> commit 605e74025f953b995a3a241ead43bde71c1c99b5
> Author: Mike Christie <michael.christie@xxxxxxxxxx>
> Date: Sun Nov 1 12:59:31 2020 -0600
>
> scsi: qla2xxx: Move sess cmd list/lock to driver
>
> when I changed the locking I had added the check:
>
> static void tcm_qla2xxx_aborted_task(struct se_cmd *se_cmd)
> {
> struct qla_tgt_cmd *cmd;
> unsigned long flags;
>
> if (se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
> return;
>
Yes, I was thinking about that commit.
> because tmrs are not on the sess_cmd_list that's accessed the
> next line down. We don't crash as a result, but I think we need
> to add code to send the cleanup command to the FW. Bodo and I
> were working on that part, but someone with more qla experience
> needed to work on it so it could be properly tested. We didn't
> hear back from the qla engineers so progress had stalled.
>
Yes, you are right, FW expects some response on every ABORT IOCB
to clear its resources.
I can prepare the patch for qla2xxx.
But still, I do not see a sense of new Bodo's solution.
Calling target_put_sess_cmd due to SCF_ACK_REF forbids any async long
term work in fabric drivers in aborted_task callback. It is
intentionally. qla2xxx does not require to wait for the completion of
all requests to FW. All terminate exchange requests are such.
SAM-5 states that an aborted(due to LUN_RESET) TMR should not be
responded to initiator:
7.11 Task management function lifetime
The task management function shall exist until:
a) the task manager sends a service response for the task management function;
b) an I_T nexus loss (see 6.3.4);
* c) a logical unit reset (see 6.3.3);
d) a hard reset (see 6.3.2);
e) power loss expected (see 6.3.5); or
f) a power on condition (see 6.3.1)
BR,
Dmitry
