Re: [PATCH v4 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] Deferred commands retried via `ata_eh_retry_deferred_qc` will be erroneously processed as ghost timeouts during SCSI EH, leading to lost retries or erroneous `DRIVER_TIMEOUT` failures for unissued commands, and `host_failed` underflows.
--

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

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

This patch introduces ata_scsi_eh_timed_out() to immediately schedule a retry
of all waiting deferred QCs using the eh_timed_out SCSI host template
operation. It implements ata_eh_retry_deferred_qc() to schedule the retry
through libata EH with ata_qc_schedule_eh() and flags waiting QCs with
ATA_QCFLAG_RETRY.

Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")

> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 05df7ea6954a9..5c56e1dd580b6 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -546,6 +546,27 @@ static void ata_eh_unload(struct ata_port *ap)
>  	spin_unlock_irqrestore(ap->lock, flags);
>  }
>  
> +void ata_eh_retry_deferred_qc(struct ata_port *ap,
> +			      struct scsi_cmnd *timedout_scmd)
> +{
> +	struct ata_queued_cmd *qc;
> +	struct ata_link *link;
> +
> +	lockdep_assert_held(ap->lock);
> +
> +	/* Trigger EH for retrying any deferred qc that has not timed out. */
> +	ata_for_each_link(link, ap, PMP_FIRST) {
> +		qc = link->deferred_qc;
> +		if (!qc || qc->scsicmd == timedout_scmd)
> +			continue;
> +
> +		link->deferred_qc = NULL;
> +		cancel_work(&link->deferred_qc_work);
> +		qc->flags |= ATA_QCFLAG_RETRY;

[Severity: High]
Does clearing link->deferred_qc here break the deferred QC lookup in
ata_scsi_cmd_error_handler()?

When the SCSI EH thread runs ata_scsi_cmd_error_handler(), it attempts to
identify the deferred QC:

drivers/ata/libata-eh.c:ata_scsi_cmd_error_handler() {
    ...
    if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
        qc == qc->dev->link->deferred_qc)
        break;
    ...
}

Because ATA_QCFLAG_ACTIVE is not set on deferred commands and we have already
cleared link->deferred_qc to NULL above, the condition fails. The loop
finishes without breaking and falls through to the ghost timeout 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);
    }
}

Will this cause the retry budget for the deferred command to be artificially
exhausted, resulting in erroneous DRIVER_TIMEOUT failures for unissued
commands?

Additionally, when libata EH later processes this same qc in ata_eh_finish()
and calls ata_eh_qc_retry(), it will execute __ata_eh_qc_complete() and call
scsi_eh_finish_cmd() a second time for the same command. Could this double
completion cause host_failed to underflow and corrupt the SCSI EH state
machine?

> +		ata_qc_schedule_eh(qc);
> +	}
> +}
> +
[ ... ]

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