Re: [PATCH v1 1/2] ata: libata-scsi: terminate deferred commands on time out

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The eh_timed_out handler prematurely completes the timed-out deferred command, freeing the libata `qc` and causing the SCSI error handler to permanently fail the command instead of retrying it.
--

--- Patch [1]: [PATCH v1 1/2] ata: libata-scsi: terminate deferred commands on time out ---
commit 5d7ac082b6ce256e2174fbf7d209e8acc3d6b3a9
Author: Damien Le Moal <[email protected]>

ata: libata-scsi: terminate deferred commands on time out

If a command timeout occurs while we have a deferred non-NCQ command
waiting to be issued, the scsi EH task is never woken up. Avoid this
unnecessary EH trigger wait time by using the eh_timed_out scsi host
template operation to terminate deferred QCs.

Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
Cc: [email protected]

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 5868526301a2b..8328778ec0464 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c

[ ... ]

> @@ -1698,12 +1699,19 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
>  	 */
>  	ata_for_each_link(link, ap, PMP_FIRST) {
>  		struct ata_queued_cmd *qc = link->deferred_qc;
> +		u32 host_byte;
>  
> -		if (qc) {
> -			link->deferred_qc = NULL;
> -			cancel_work(&link->deferred_qc_work);
> -			ata_scsi_qc_done(qc, true, DID_REQUEUE << 16);
> -		}
> +		if (!qc)
> +			continue;
> +
> +		link->deferred_qc = NULL;
> +		cancel_work(&link->deferred_qc_work);
> +
> +		if (qc->scsicmd == timed_out_scmd)
> +			host_byte = DID_TIME_OUT;
> +		else
> +			host_byte = DID_REQUEUE;
> +		ata_scsi_qc_done(qc, true, host_byte << 16);

[Severity: High]
Does calling ata_scsi_qc_done() here prematurely complete the timed-out
deferred command and free the libata qc?

When a deferred command times out, ata_scsi_eh_timed_out() calls this function
which sets the host byte to DID_TIME_OUT and calls ata_scsi_qc_done().
ata_scsi_qc_done() then frees the qc and invokes the qc->scsidone callback.

Later, when the SCSI completion softirq routes the DID_TIME_OUT command to the
EH queue and the EH thread runs ata_scsi_cmd_error_handler():

drivers/ata/libata-eh.c:ata_scsi_cmd_error_handler() {
    ...
    set_host_byte(scmd, DID_OK);
    ata_qc_for_each_raw(ap, qc, i) {
        if (qc->scsicmd != scmd)
            continue;
        if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
            qc == qc->dev->link->deferred_qc)
            break;
    }
    ...
}

Because the qc was already freed, the ATA_QCFLAG_ACTIVE flag is cleared
and deferred_qc is NULL. The loop will fail to find the active command.

It seems this causes the handler to fall into the 'raced completion' branch:

drivers/ata/libata-eh.c:ata_scsi_cmd_error_handler() {
    ...
    } else {
        /* Normal completion occurred after
         * SCSI timeout but before this point.
         * Successfully complete it.
         */
        scmd->retries = scmd->allowed;
        scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
    }
    ...
}

Could this result in the midlayer completely dropping the timed-out deferred
command (completed with DID_OK and no retries) instead of properly retrying it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1