Re: SCSI EH wakeup deadlock from deferred_qc

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

On 7/9/26 11:24, Igor Pylypiv wrote:
> On Thu, Jul 09, 2026 at 10:01:11AM +0900, Damien Le Moal wrote:
>> On 7/9/26 08:44, Damien Le Moal wrote:
>>> On 7/9/26 04:36, Igor Pylypiv wrote:
>>>> Hi Damien,
>>>>
>>>> I've stumbled upon an issue where SCSI EH didn't run upon command timeout
>>>> but ran ~10 or so seconds later. It seems like a recent regression from
>>>> the introduction of deffered_qc.
>>>>
>>>> When an active command times out while a non-NCQ command is waiting in
>>>> deferred_qc, SCSI EH fails to wake up. Recovery stalls until the deferred
>>>> command's own timer expires.
>>>>
>>>> When an NCQ command times out, scsi_timeout() calls scsi_eh_scmd_add(),
>>>> incrementing shost->host_failed (1). However, when scsi_eh_wakeup() checks
>>>> whether to wake the EH thread, scsi_host_busy(shost) counts 2 active
>>>> commands (1 timed-out + 1 in deferred_qc).
>>>>
>>>> Because busy (2) != host_failed (1), scsi_eh_wakeup() refuses to wake
>>>> the EH thread, deadlocking error recovery until the deferred command
>>>> times out on its own.
>>>
>>> Igor,
>>>
>>> Can you try the attached diff ?
>>> I will test on my end, but I need to hack something to trigger a timeout :)
>>
>> Hacking libahci to trigger a timeout for a particular LBA read, I tested the
>> attached v2 and I see the deferred QC running right after the read command
>> timeout. So this looks like a good fix to me. Can you test please ?
> 
> Thank you for coming up with a fix so quickly, Damien!
> The fix works for the issue I'm seeing!
> 
> Note: I tested on a 6.18 based kernel with your changes ported to libsas
> to fix the issue for the pm80xx driver. Attached libsas diff for reference.

Thanks for the diff. I will add it to the patch, and if that's OK with you, also
add a co-developed-by tag ?

> 
> Thank you!
> Igor
> 
>>
>> -- 
>> Damien Le Moal
>> Western Digital Research
> 
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 5868526301a2..5221d7004c0e 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -1685,7 +1685,8 @@ void ata_scsi_deferred_qc_work(struct work_struct *work)
>>  	spin_unlock_irqrestore(ap->lock, flags);
>>  }
>>  
>> -void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
>> +static void ata_scsi_do_requeue_deferred_qc(struct ata_port *ap,
>> +					    struct scsi_cmnd *timed_out_scmd)
>>  {
>>  	struct ata_link *link;
>>  
>> @@ -1698,15 +1699,27 @@ 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);
>>  	}
>>  }
>>  
>> +void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
>> +{
>> +	ata_scsi_do_requeue_deferred_qc(ap, NULL);
>> +}
>> +
>>  static void ata_scsi_schedule_deferred_qc(struct ata_link *link)
>>  {
>>  	struct ata_queued_cmd *qc = link->deferred_qc;
>> @@ -1730,6 +1743,28 @@ static void ata_scsi_schedule_deferred_qc(struct ata_link *link)
>>  		queue_work(system_highpri_wq, &link->deferred_qc_work);
>>  }
>>  
>> +enum scsi_timeout_action ata_scsi_eh_timed_out(struct scsi_cmnd *scmd)
>> +{
>> +	struct ata_port *ap = ata_shost_to_port(scmd->device->host);
>> +	unsigned long flags;
>> +
>> +	/*
>> +	 * We had a timeout, either for an NCQ command or for one deferred
>> +	 * queued command. Either way, we must release all deferred queued
>> +	 * command so that scsi EH can trigger.
>> +	 */
>> +	spin_lock_irqsave(ap->lock, flags);
>> +	ata_scsi_do_requeue_deferred_qc(ap, scmd);
>> +	spin_unlock_irqrestore(ap->lock, flags);
>> +
>> +	/*
>> +	 * Let scsi_timeout() know that it must continue with handling the
>> +	 * timeout as we in fact did not do much here.
>> +	 */
>> +	return SCSI_EH_NOT_HANDLED;
>> +}
>> +EXPORT_SYMBOL_GPL(ata_scsi_eh_timed_out);
>> +
>>  static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>>  {
>>  	struct ata_link *link = qc->dev->link;
>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index 96e626d6a7ca..327da43d7496 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -1153,6 +1153,7 @@ extern int ata_scsi_ioctl(struct scsi_device *dev, unsigned int cmd,
>>  #endif
>>  extern enum scsi_qc_status ata_scsi_queuecmd(struct Scsi_Host *h,
>>  					     struct scsi_cmnd *cmd);
>> +enum scsi_timeout_action ata_scsi_eh_timed_out(struct scsi_cmnd *cmd);
>>  #if IS_REACHABLE(CONFIG_ATA)
>>  bool ata_scsi_dma_need_drain(struct request *rq);
>>  #else
>> @@ -1464,6 +1465,7 @@ extern const struct attribute_group *ata_common_sdev_groups[];
>>  	.ioctl			= ata_scsi_ioctl,		\
>>  	ATA_SCSI_COMPAT_IOCTL					\
>>  	.queuecommand		= ata_scsi_queuecmd,		\
>> +	.eh_timed_out		= ata_scsi_eh_timed_out,	\
>>  	.dma_need_drain		= ata_scsi_dma_need_drain,	\
>>  	.this_id		= ATA_SHT_THIS_ID,		\
>>  	.emulated		= ATA_SHT_EMULATED,		\
> 


-- 
Damien Le Moal
Western Digital Research