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

[Damien Le Moal <dlemoal@kernel.org>]

On 7/16/26 20:15, Niklas Cassel wrote:
> 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?

I missed this... And yes, I think Sashiko is making a good 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?

Yes. That function only moves the command to the tail of the done queue though.
So calling it twice is completely harmless, hence why my testing did not reveal
anything wrong. But indeed that is dirty and useless.

> 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.)

I will see what the simplest fix is. I do not really want to add another flag to
special case the retry of deferred QCs. But it is already... So we need to do
something.

Oh well, I will send a v6 then.



-- 
Damien Le Moal
Western Digital Research