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

[Niklas Cassel <cassel@kernel.org>]

On 15 July 2026 11:20:16 CEST, sashiko-bot@kernel.org wrote:
>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 <dlemoal@kernel.org>
>
>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

I'm on vacation mode, so not thinking clearly,
but does Sashiko have a point here?

You modify __ata_eh_qc_complete(), so it will NOT call __ata_qc_complete()
if ((qc->flags & ATA_QCFLAG_RETRY) && !(qc->flags & ATA_QCFLAG_ACTIVE))

But __ata_eh_qc_complete() will still call scsi_eh_finish_cmd(). So it there a chance that 
scsi_eh_finish_cmd() will be called twice?

Do we perhaps need a QC flag anyway, so that an "aborted" deferred QC does not call scsi_eh_finish_cmd() in ata_scsi_cmd_error_handler()? (Such that the 
scsi_eh_finish_cmd() in __ata_eh_qc_complete() is the only one that will be called.)


Kind regards,
Niklas