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

On 7/14/26 03:07, Igor Pylypiv wrote:
> On Mon, Jul 13, 2026 at 01:12:51PM +0900, Damien Le Moal wrote:
>> If a command timeout occurs while we have a deferred non-NCQ command
>> waiting to be issued, the SCSI EH task is not immediately woken up as the
>> waiting deferred command is never issued nor completed, thus leaving this
>> command to always be counted as "busy" for the SCSI host. This results in
>> the test "shost->host_failed != scsi_host_busy(shost))" in the function
>> scsi_error_handler() to always be true, keeping the EH task sleeping.
>> Eventually, when the deferred command also times out, the SCSI EH task
>> is woken up and the timeout processing occurs.
>>
>> Avoid this unnecessary SCSI EH task wake-up additional time using the
>> eh_timed_out SCSI host template operation. The function
>> ata_scsi_eh_timed_out() is introduced to implement this operation. This
>> function calls the new helper ata_eh_schedule_deferred_qc_retry() to
>> schedule a retry through libata EH of all differed queued command, except
>> for a differed queued commands that timed out as that case is handled in
>> ata_scsi_cmd_error_handler().
>>
>> Since ata_scsi_eh_timed_out() does not directly handles the timeout itself
>> and eventual re-issuing of deferred commands, this function returns
>> SCSI_EH_NOT_HANDLED to have scsi_timeout() continue with the regular
>> timeout handling, using scsi_abort_command() and scsi_eh_scmd_add(), thus
>> preventing the wkae-up delay for SCSI EH task.
>>
>> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
>> Cc: [email protected]
>> Signed-off-by: Damien Le Moal <[email protected]>
>> ---
>>  drivers/ata/libata-eh.c   | 35 ++++++++++++++++++++++++++++++++++-
>>  drivers/ata/libata-scsi.c | 22 ++++++++++++++++++++++
>>  drivers/ata/libata.h      |  2 ++
>>  include/linux/libata.h    |  2 ++
>>  4 files changed, 60 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>> index 05df7ea6954a..8e9d57c6f039 100644
>> --- a/drivers/ata/libata-eh.c
>> +++ b/drivers/ata/libata-eh.c
>> @@ -546,6 +546,31 @@ static void ata_eh_unload(struct ata_port *ap)
>>  	spin_unlock_irqrestore(ap->lock, flags);
>>  }
>>  
>> +void ata_eh_schedule_deferred_qc_retry(struct ata_port *ap,
>> +				       struct scsi_cmnd *scmd)
>> +{
>> +	struct ata_queued_cmd *qc;
>> +	struct ata_link *link;
>> +	unsigned long flags;
>> +
>> +	/*
>> +	 * Trigger EH for retrying any deferred qc that is not the queued
>> +	 * command for scmd.
>> +	 */
>> +	spin_lock_irqsave(ap->lock, flags);
>> +	ata_for_each_link(link, ap, PMP_FIRST) {
>> +		qc = link->deferred_qc;
>> +		if (!qc || qc->scsicmd == scmd)
>> +			continue;
>> +
>> +		link->deferred_qc = NULL;
>> +		cancel_work(&link->deferred_qc_work);
>> +		qc->flags |= ATA_QCFLAG_RETRY;
> 
> 
> Gemini pointed out an issue:
> 
> ata_scsi_cmd_error_handler() would skip this command because
> ATA_QCFLAG_ACTIVE flag is not set and qc != qc->dev->link->deferred_qc
> because ata_eh_schedule_deferred_qc_retry() set link->deferred_qc to NULL.
> 
> https://github.com/torvalds/linux/blob/master/drivers/ata/libata-eh.c#L655-L657

And I think that is fine, since there was no errors with the deferred QCs. In
this case, it falls into the last else after the loop, which does:

	scmd->retries = scmd->allowed;
	scsi_eh_finish_cmd(scmd, &ap->eh_done_q);

and after that, ata_eh_finish() will retry the QCs because they are tagged with
ATA_QCFLAG_EH and ATA_QCFLAG_RETRY.

I tested with AHCI, and checked that.

If anything, the retries count should be checked because I think this path
messes it up.

-- 
Damien Le Moal
Western Digital Research