[PATCH v2 0/2] fixup handling of timeouts with deferred QCs

[PATCH v2 0/2] fixup handling of timeouts with deferred QCs

[Damien Le Moal]

This patch series fixes libata and libsas to correctly handles deferred
queued commands in case of a timeout error, to avoid excessive delays in
waking up the scsi EH task.

Igor,

My apologies for the churn, but please retest !
Also, I added your Signed-off-by on patch 2 since half of it is yours.

Martin,

Once reviewed, I or you can take both patches ?

Changes from v1:
 - Modified patch 1 to ignore timed out deferred QCs in
   ata_scsi_requeue_deferred_qc() to let ata_scsi_cmd_error_handler()
   correctly handle this case.

Damien Le Moal (2):
  ata: libata-scsi: terminate deferred commands on time out
  scsi: libsas: terminate deferred commands on time out

 drivers/ata/libata-eh.c             |  2 +-
 drivers/ata/libata-scsi.c           | 52 ++++++++++++++++++++++++-----
 drivers/ata/libata.h                |  3 +-
 drivers/scsi/libsas/sas_scsi_host.c | 17 ++++++++++
 include/linux/libata.h              |  4 +++
 include/scsi/libsas.h               |  2 ++
 6 files changed, 70 insertions(+), 10 deletions(-)

-- 
2.55.0

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

[Damien Le Moal]

If a command timeout occurs while we have a deferred non-NCQ command
waiting to be issued, the SCSI EH task is never 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
scsi_error_handler() function test "shost->host_failed !=
scsi_host_busy(shost))" to always be true, keeping the EH task sleeping.
Eventuially, when the deferred command also times out, the SCSI EH task
is woken up and the timeout processing occurs.

Avoid this unnecessary EH trigger additional wait time using the
eh_timed_out SCSI host template operation. The function
ata_scsi_eh_timed_out() is introduced to implement this operation using
the helper function ata_scsi_port_eh_timed_out(). This function calls
ata_scsi_requeue_deferred_qc() to force a requeue of deferred QCs, except
for QCs that actually timed out. In this case,
ata_scsi_cmd_error_handler() processing of timed out deferred QCs still
applies.  Since the timeout itself and eventual re-issuing of deferred
commands is not handled by this helper, SCSI_EH_NOT_HANDLED is returned
to have scsi_timeout() continue with the regular timeout handling.

Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-eh.c   |  2 +-
 drivers/ata/libata-scsi.c | 51 +++++++++++++++++++++++++++++++++------
 drivers/ata/libata.h      |  3 ++-
 include/linux/libata.h    |  2 ++
 4 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 05df7ea6954a..57d3d2d11dd8 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -951,7 +951,7 @@ static void ata_eh_set_pending(struct ata_port *ap, bool fastdrain)
 	 * If we have a deferred qc, requeue it so that it is retried once EH
 	 * completes.
 	 */
-	ata_scsi_requeue_deferred_qc(ap);
+	ata_scsi_requeue_deferred_qc(ap, NULL);
 
 	if (!fastdrain)
 		return;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 5868526301a2..b6e25aab01d5 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)
+void ata_scsi_requeue_deferred_qc(struct ata_port *ap,
+				  struct scsi_cmnd *timed_out_scmd)
 {
 	struct ata_link *link;
 
@@ -1694,16 +1695,19 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
 	/*
 	 * If we have a deferred qc when a reset occurs or NCQ commands fail,
 	 * do not try to be smart about what to do with this deferred command
-	 * and simply requeue it by completing it with DID_REQUEUE.
+	 * and simply requeue it by completing it with DID_REQUEUE. The
+	 * exception here is if the deferred qc timed out, in which case, we
+	 * leave it as is as ata_scsi_cmd_error_handler() will take care of it.
 	 */
 	ata_for_each_link(link, ap, PMP_FIRST) {
 		struct ata_queued_cmd *qc = link->deferred_qc;
 
-		if (qc) {
-			link->deferred_qc = NULL;
-			cancel_work(&link->deferred_qc_work);
-			ata_scsi_qc_done(qc, true, DID_REQUEUE << 16);
-		}
+		if (!qc || qc->scsicmd == timed_out_scmd)
+			continue;
+
+		link->deferred_qc = NULL;
+		cancel_work(&link->deferred_qc_work);
+		ata_scsi_qc_done(qc, true, DID_REQUEUE << 16);
 	}
 }
 
@@ -1723,13 +1727,44 @@ static void ata_scsi_schedule_deferred_qc(struct ata_link *link)
 		return;
 
 	if (ata_port_eh_scheduled(ap)) {
-		ata_scsi_requeue_deferred_qc(ap);
+		ata_scsi_requeue_deferred_qc(ap, NULL);
 		return;
 	}
 	if (!ap->ops->qc_defer(qc))
 		queue_work(system_highpri_wq, &link->deferred_qc_work);
 }
 
+static enum scsi_timeout_action
+ata_scsi_port_eh_timed_out(struct ata_port *ap, struct scsi_cmnd *scmd)
+{
+	unsigned long flags;
+
+	/*
+	 * We had a timeout, either for an NCQ command or for one deferred
+	 * queued command. If we have deferred QCs and we do not release them
+	 * immediately, we will have shost->host_failed != scsi_host_busy()
+	 * until the deferred QCs also timeout. This unnecessarilly increases
+	 * the time it takes for scsi EH to start. Terminate all deferred QCs
+	 * to avoid that.
+	 */
+	spin_lock_irqsave(ap->lock, flags);
+	ata_scsi_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;
+}
+
+enum scsi_timeout_action ata_scsi_eh_timed_out(struct scsi_cmnd *scmd)
+{
+	return ata_scsi_port_eh_timed_out(ata_shost_to_port(scmd->device->host),
+					  scmd);
+}
+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/drivers/ata/libata.h b/drivers/ata/libata.h
index 700627596ce1..14912e140686 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -180,7 +180,8 @@ enum scsi_qc_status __ata_scsi_queuecmd(struct scsi_cmnd *scmd,
 					struct ata_port *ap)
 	__must_hold(ap->lock);
 void ata_scsi_deferred_qc_work(struct work_struct *work);
-void ata_scsi_requeue_deferred_qc(struct ata_port *ap);
+void ata_scsi_requeue_deferred_qc(struct ata_port *ap,
+				  struct scsi_cmnd *timed_out_scmd);
 
 /* libata-eh.c */
 extern unsigned int ata_internal_cmd_timeout(struct ata_device *dev, u8 cmd);
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,		\
-- 
2.55.0

[PATCH v2 2/2] scsi: libsas: terminate deferred commands on time out

[Damien Le Moal]

If a command timeout occurs while we have a deferred non-NCQ command
waiting to be issued, the scsi EH task is never 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
scsi_error_handler() function test "shost->host_failed !=
scsi_host_busy(shost))" to always be true, keeping the SCSI EH task
sleeping. Eventually, when the deferred command also times out, the EH
task is woken up and the timeout processing starts.

Avoid this unnecessary additional EH trigger wait time with the same
method as implemented in libata-scsi, using the eh_timed_out SCSI host
template operation. The function sas_eh_timed_out() implements this
operation and executes the helper function ata_scsi_port_eh_timed_out()
if the device is a sata one.

Co-developed-by: Igor Pylypiv <ipylypiv@google.com>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-scsi.c           |  5 +++--
 drivers/scsi/libsas/sas_scsi_host.c | 17 +++++++++++++++++
 include/linux/libata.h              |  2 ++
 include/scsi/libsas.h               |  2 ++
 4 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index b6e25aab01d5..78f19537119a 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1734,8 +1734,8 @@ static void ata_scsi_schedule_deferred_qc(struct ata_link *link)
 		queue_work(system_highpri_wq, &link->deferred_qc_work);
 }
 
-static enum scsi_timeout_action
-ata_scsi_port_eh_timed_out(struct ata_port *ap, struct scsi_cmnd *scmd)
+enum scsi_timeout_action ata_scsi_port_eh_timed_out(struct ata_port *ap,
+						    struct scsi_cmnd *scmd)
 {
 	unsigned long flags;
 
@@ -1757,6 +1757,7 @@ ata_scsi_port_eh_timed_out(struct ata_port *ap, struct scsi_cmnd *scmd)
 	 */
 	return SCSI_EH_NOT_HANDLED;
 }
+EXPORT_SYMBOL_GPL(ata_scsi_port_eh_timed_out);
 
 enum scsi_timeout_action ata_scsi_eh_timed_out(struct scsi_cmnd *scmd)
 {
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index c83282733ec4..578fa8fd70f0 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -502,6 +502,23 @@ int sas_eh_target_reset_handler(struct scsi_cmnd *cmd)
 }
 EXPORT_SYMBOL_GPL(sas_eh_target_reset_handler);
 
+/*
+ * Handle deferred QCs in case of a timeout.
+ * See ata_scsi_port_eh_timed_out() for details.
+ */
+enum scsi_timeout_action sas_eh_timed_out(struct scsi_cmnd *cmd)
+{
+	struct domain_device *dev = NULL;
+
+	if (cmd)
+		dev = cmd_to_domain_dev(cmd);
+	if (!dev || !dev_is_sata(dev))
+		return SCSI_EH_NOT_HANDLED;
+
+	return ata_scsi_port_eh_timed_out(dev->sata_dev.ap, cmd);
+}
+EXPORT_SYMBOL_GPL(sas_eh_timed_out);
+
 /* Try to reset a device */
 static int try_to_reset_cmd_device(struct scsi_cmnd *cmd)
 {
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 327da43d7496..2958a2fa3da0 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1153,6 +1153,8 @@ 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_port_eh_timed_out(struct ata_port *ap,
+						    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);
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 163f23c92b41..c7017ae76c61 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -705,6 +705,7 @@ void sas_task_abort(struct sas_task *);
 int sas_eh_abort_handler(struct scsi_cmnd *cmd);
 int sas_eh_device_reset_handler(struct scsi_cmnd *cmd);
 int sas_eh_target_reset_handler(struct scsi_cmnd *cmd);
+enum scsi_timeout_action sas_eh_timed_out(struct scsi_cmnd *cmd);
 
 extern void sas_target_destroy(struct scsi_target *);
 extern int sas_sdev_init(struct scsi_device *);
@@ -743,6 +744,7 @@ void sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event,
 	.this_id			= -1,				\
 	.eh_device_reset_handler	= sas_eh_device_reset_handler,	\
 	.eh_target_reset_handler	= sas_eh_target_reset_handler,	\
+	.eh_timed_out			= sas_eh_timed_out,		\
 	.target_destroy			= sas_target_destroy,		\
 	.ioctl				= sas_ioctl,			\
 
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] `ata_scsi_port_eh_timed_out` fails to set `ATA_PFLAG_EH_PENDING`, which allows requeued deferred commands to be immediately re-issued and the timed-out command itself to be erroneously executed, defeating the patch's optimization and causing unnecessary hardware resets.

Pre-existing issues:
- [Critical] A race condition between a deferred command's SCSI timeout and its requeue completion causes the command to be silently dropped and reported as successfully completed to the block layer, leading to silent data corruption.
--

--- Patch [1]: [PATCH v2 1/2] ata: libata-scsi: terminate deferred commands on time out ---
commit a21f27abd9e61a32aa55fa1005071af9c225247b
Author: Damien Le Moal <dlemoal@kernel.org>

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

If a command timeout occurs while we have a deferred non-NCQ command
waiting to be issued, the SCSI EH task is never woken up. Avoid this
unnecessary EH trigger additional wait time using the eh_timed_out SCSI
host template operation to force a requeue of deferred QCs.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c

[ ... ]

> @@ -1694,16 +1695,19 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
>  	/*
>  	 * If we have a deferred qc when a reset occurs or NCQ commands fail,
>  	 * do not try to be smart about what to do with this deferred command
> -	 * and simply requeue it by completing it with DID_REQUEUE.
> +	 * and simply requeue it by completing it with DID_REQUEUE. The
> +	 * exception here is if the deferred qc timed out, in which case, we
> +	 * leave it as is as ata_scsi_cmd_error_handler() will take care of it.
>  	 */
>  	ata_for_each_link(link, ap, PMP_FIRST) {
>  		struct ata_queued_cmd *qc = link->deferred_qc;
>  
> -		if (qc) {
> -			link->deferred_qc = NULL;
> -			cancel_work(&link->deferred_qc_work);
> -			ata_scsi_qc_done(qc, true, DID_REQUEUE << 16);
> -		}
> +		if (!qc || qc->scsicmd == timed_out_scmd)
> +			continue;
> +
> +		link->deferred_qc = NULL;
> +		cancel_work(&link->deferred_qc_work);
> +		ata_scsi_qc_done(qc, true, DID_REQUEUE << 16);

[Severity: Critical]
This is a pre-existing issue, but won't the explicit invocation of this
requeue logic on every command timeout make a regression more likely?

When ata_scsi_requeue_deferred_qc() completes a deferred command using
ata_scsi_qc_done() with DID_REQUEUE, it unconditionally calls
ata_qc_free(), which clears the active flag.

If the block layer timer for this command expires concurrently,
scsi_timeout() can win the race against the scsi_done() completion. The
block layer marks the command as timed out and adds it to the SCSI EH queue.

When SCSI EH runs ata_scsi_cmd_error_handler(), it iterates over all QCs to
find the command. Since ata_qc_free() was already called and the deferred_qc
pointer was set to NULL, it won't match the deferred check.

Does this cause the loop to finish without finding the command and fall
through to the successful completion 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.
         */
        set_host_byte(scmd, DID_OK);
        scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
    }
}

Could this result in the command being reported as successfully completed to
the block layer, resulting in silent data corruption?

[ ... ]

> +static enum scsi_timeout_action
> +ata_scsi_port_eh_timed_out(struct ata_port *ap, struct scsi_cmnd *scmd)
> +{
> +	unsigned long flags;
> +
> +	/*
> +	 * We had a timeout, either for an NCQ command or for one deferred
> +	 * queued command. If we have deferred QCs and we do not release them
> +	 * immediately, we will have shost->host_failed != scsi_host_busy()
> +	 * until the deferred QCs also timeout. This unnecessarilly increases
> +	 * the time it takes for scsi EH to start. Terminate all deferred QCs
> +	 * to avoid that.
> +	 */
> +	spin_lock_irqsave(ap->lock, flags);
> +	ata_scsi_requeue_deferred_qc(ap, scmd);
> +	spin_unlock_irqrestore(ap->lock, flags);

[Severity: High]
Does this function need to set ATA_PFLAG_EH_PENDING?

Because this flag is not set, ata_port_eh_scheduled() will evaluate to
false. This allows the block layer to immediately retry the requeued
commands, which __ata_scsi_queuecmd() will accept and re-defer, keeping
the host busy.

Furthermore, for the timed-out command itself (which is skipped by the
requeue logic), ata_scsi_deferred_qc_work() can run concurrently:

drivers/ata/libata-scsi.c:ata_scsi_deferred_qc_work() {
    ...
    qc = link->deferred_qc;
    if (qc && !ata_port_eh_scheduled(ap)) {
        WARN_ON_ONCE(ap->ops->qc_defer(qc));
        link->deferred_qc = NULL;
        ata_qc_issue(ap, qc);
    }
    ...
}

Since ata_port_eh_scheduled() evaluates to false, will this work item
erroneously issue the timed-out command to the hardware right as it enters
error handling?

When EH finally runs, it would find the timed-out command actively executing
in hardware instead of deferred, triggering a full port freeze and reset.
Can this defeat the patch's intended optimization of cleanly terminating the
timeout without a reset?

> +
> +	/*
> +	 * 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;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710000646.1202200-1-dlemoal@kernel.org?part=1

Re: [PATCH v2 2/2] scsi: libsas: terminate deferred commands on time out

[Jason Yan]

在 2026/7/10 8:06, Damien Le Moal 写道:
> If a command timeout occurs while we have a deferred non-NCQ command
> waiting to be issued, the scsi EH task is never 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
> scsi_error_handler() function test "shost->host_failed !=
> scsi_host_busy(shost))" to always be true, keeping the SCSI EH task
> sleeping. Eventually, when the deferred command also times out, the EH
> task is woken up and the timeout processing starts.
> 
> Avoid this unnecessary additional EH trigger wait time with the same
> method as implemented in libata-scsi, using the eh_timed_out SCSI host
> template operation. The function sas_eh_timed_out() implements this
> operation and executes the helper function ata_scsi_port_eh_timed_out()
> if the device is a sata one.
> 
> Co-developed-by: Igor Pylypiv<ipylypiv@google.com>
> Signed-off-by: Igor Pylypiv<ipylypiv@google.com>
> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> Cc:stable@vger.kernel.org
> Signed-off-by: Damien Le Moal<dlemoal@kernel.org>
> ---
>   drivers/ata/libata-scsi.c           |  5 +++--
>   drivers/scsi/libsas/sas_scsi_host.c | 17 +++++++++++++++++
>   include/linux/libata.h              |  2 ++
>   include/scsi/libsas.h               |  2 ++
>   4 files changed, 24 insertions(+), 2 deletions(-)

Looks good to me,

Reviewed-by: Jason Yan <yanaijie@huawei.com>

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

[Damien Le Moal]

On 7/10/26 09:24, sashiko-bot@kernel.org wrote:
>> @@ -1694,16 +1695,19 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
>>  	/*
>>  	 * If we have a deferred qc when a reset occurs or NCQ commands fail,
>>  	 * do not try to be smart about what to do with this deferred command
>> -	 * and simply requeue it by completing it with DID_REQUEUE.
>> +	 * and simply requeue it by completing it with DID_REQUEUE. The
>> +	 * exception here is if the deferred qc timed out, in which case, we
>> +	 * leave it as is as ata_scsi_cmd_error_handler() will take care of it.
>>  	 */
>>  	ata_for_each_link(link, ap, PMP_FIRST) {
>>  		struct ata_queued_cmd *qc = link->deferred_qc;
>>  
>> -		if (qc) {
>> -			link->deferred_qc = NULL;
>> -			cancel_work(&link->deferred_qc_work);
>> -			ata_scsi_qc_done(qc, true, DID_REQUEUE << 16);
>> -		}
>> +		if (!qc || qc->scsicmd == timed_out_scmd)
>> +			continue;
>> +
>> +		link->deferred_qc = NULL;
>> +		cancel_work(&link->deferred_qc_work);
>> +		ata_scsi_qc_done(qc, true, DID_REQUEUE << 16);
> 
> [Severity: Critical]
> This is a pre-existing issue, but won't the explicit invocation of this
> requeue logic on every command timeout make a regression more likely?

Maybe. But that race exists since eons ago. And command timeouts are rare, so I
do not consider this to be "critical" at all.

>> +static enum scsi_timeout_action
>> +ata_scsi_port_eh_timed_out(struct ata_port *ap, struct scsi_cmnd *scmd)
>> +{
>> +	unsigned long flags;
>> +
>> +	/*
>> +	 * We had a timeout, either for an NCQ command or for one deferred
>> +	 * queued command. If we have deferred QCs and we do not release them
>> +	 * immediately, we will have shost->host_failed != scsi_host_busy()
>> +	 * until the deferred QCs also timeout. This unnecessarilly increases
>> +	 * the time it takes for scsi EH to start. Terminate all deferred QCs
>> +	 * to avoid that.
>> +	 */
>> +	spin_lock_irqsave(ap->lock, flags);
>> +	ata_scsi_requeue_deferred_qc(ap, scmd);
>> +	spin_unlock_irqrestore(ap->lock, flags);
> 
> [Severity: High]
> Does this function need to set ATA_PFLAG_EH_PENDING?
> 
> Because this flag is not set, ata_port_eh_scheduled() will evaluate to
> false. This allows the block layer to immediately retry the requeued
> commands, which __ata_scsi_queuecmd() will accept and re-defer, keeping
> the host busy.

No it will not because this is called with SCSI EH already waiting to run, so
newly incoming commands are not passed down by the scsi layer.

-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

On Fri, Jul 10, 2026 at 09:06:45AM +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 never 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
> scsi_error_handler() function test "shost->host_failed !=
> scsi_host_busy(shost))" to always be true, keeping the EH task sleeping.
> Eventuially, when the deferred command also times out, the SCSI EH task
> is woken up and the timeout processing occurs.
> 
> Avoid this unnecessary EH trigger additional wait time using the
> eh_timed_out SCSI host template operation. The function
> ata_scsi_eh_timed_out() is introduced to implement this operation using
> the helper function ata_scsi_port_eh_timed_out(). This function calls
> ata_scsi_requeue_deferred_qc() to force a requeue of deferred QCs, except
> for QCs that actually timed out. In this case,
> ata_scsi_cmd_error_handler() processing of timed out deferred QCs still
> applies.  Since the timeout itself and eventual re-issuing of deferred
> commands is not handled by this helper, SCSI_EH_NOT_HANDLED is returned
> to have scsi_timeout() continue with the regular timeout handling.
> 
> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-eh.c   |  2 +-
>  drivers/ata/libata-scsi.c | 51 +++++++++++++++++++++++++++++++++------
>  drivers/ata/libata.h      |  3 ++-
>  include/linux/libata.h    |  2 ++
>  4 files changed, 48 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 05df7ea6954a..57d3d2d11dd8 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -951,7 +951,7 @@ static void ata_eh_set_pending(struct ata_port *ap, bool fastdrain)
>  	 * If we have a deferred qc, requeue it so that it is retried once EH
>  	 * completes.
>  	 */
> -	ata_scsi_requeue_deferred_qc(ap);
> +	ata_scsi_requeue_deferred_qc(ap, NULL);
>  
>  	if (!fastdrain)
>  		return;
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 5868526301a2..b6e25aab01d5 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)
> +void ata_scsi_requeue_deferred_qc(struct ata_port *ap,
> +				  struct scsi_cmnd *timed_out_scmd)
>  {
>  	struct ata_link *link;
>  
> @@ -1694,16 +1695,19 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
>  	/*
>  	 * If we have a deferred qc when a reset occurs or NCQ commands fail,
>  	 * do not try to be smart about what to do with this deferred command
> -	 * and simply requeue it by completing it with DID_REQUEUE.
> +	 * and simply requeue it by completing it with DID_REQUEUE. The
> +	 * exception here is if the deferred qc timed out, in which case, we
> +	 * leave it as is as ata_scsi_cmd_error_handler() will take care of it.
>  	 */
>  	ata_for_each_link(link, ap, PMP_FIRST) {
>  		struct ata_queued_cmd *qc = link->deferred_qc;

I like this patch much more than V1, as it does not duplicate the code which
specifically handles the case where the deferred QC timed out.

>  
> -		if (qc) {

Personally I would write this as:

if (qc && qc->scsicmd != timed_out_scmd)

to make the patch about 7 lines smaller.


> -			link->deferred_qc = NULL;
> -			cancel_work(&link->deferred_qc_work);
> -			ata_scsi_qc_done(qc, true, DID_REQUEUE << 16);
> -		}
> +		if (!qc || qc->scsicmd == timed_out_scmd)
> +			continue;
> +
> +		link->deferred_qc = NULL;
> +		cancel_work(&link->deferred_qc_work);
> +		ata_scsi_qc_done(qc, true, DID_REQUEUE << 16);
>  	}
>  }
>  

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

[Niklas Cassel]

On Fri, Jul 10, 2026 at 04:25:38PM +0900, Damien Le Moal wrote:
> > [Severity: High]
> > Does this function need to set ATA_PFLAG_EH_PENDING?
> > 
> > Because this flag is not set, ata_port_eh_scheduled() will evaluate to
> > false. This allows the block layer to immediately retry the requeued
> > commands, which __ata_scsi_queuecmd() will accept and re-defer, keeping
> > the host busy.
> 
> No it will not because this is called with SCSI EH already waiting to run, so
> newly incoming commands are not passed down by the scsi layer.

Could you please point to specific functions + lines in the code?

AFAICT, I don't see how SCSI EH is waiting to run when
scsi_eh_scmd_add() has not yet been called.

Note that all the places that currently call ata_scsi_requeue_deferred_qc()
are called when ATA_PFLAG_EH_PENDING | ATA_PFLAG_EH_IN_PROGRESS
(ata_port_eh_scheduled() evaluates to true).

We could add an WARN_ON(!ata_port_eh_scheduled) in ata_scsi_requeue_deferred_qc()
and we would never see the warning.

With the new function ata_scsi_port_eh_timed_out(), AFAICT, it can call
ata_scsi_requeue_deferred_qc() before ata_port_eh_scheduled() evaluates to true.

See e.g. the commit message for:
e20e81a24a4d ("ata: libata-core: do not issue non-internal commands once EH is pending")
of why I think __ata_scsi_queuecmd() will accept new commands.


Kind regards,
Niklas

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

[Damien Le Moal]

On 7/10/26 17:06, Niklas Cassel wrote:
> On Fri, Jul 10, 2026 at 04:25:38PM +0900, Damien Le Moal wrote:
>>> [Severity: High]
>>> Does this function need to set ATA_PFLAG_EH_PENDING?
>>>
>>> Because this flag is not set, ata_port_eh_scheduled() will evaluate to
>>> false. This allows the block layer to immediately retry the requeued
>>> commands, which __ata_scsi_queuecmd() will accept and re-defer, keeping
>>> the host busy.
>>
>> No it will not because this is called with SCSI EH already waiting to run, so
>> newly incoming commands are not passed down by the scsi layer.
> 
> Could you please point to specific functions + lines in the code?
> 
> AFAICT, I don't see how SCSI EH is waiting to run when
> scsi_eh_scmd_add() has not yet been called.

The wait is at the beginning of the EH task:

int scsi_error_handler(void *data)
{
...
              if ((shost->host_failed == 0 && shost->host_eh_scheduled == 0) ||
                  shost->host_failed != scsi_host_busy(shost)) {
                        SCSI_LOG_ERROR_RECOVERY(1,
                                shost_printk(KERN_INFO, shost,
                                             "scsi_eh_%d: sleeping\n",
                                             shost->host_no));
                        schedule();
                        continue;
                }

and... looking at the code from there, indeed, scsi_timeout() calls eventually
scsi_eh_scmd_add() which increases host_failed. So looks like Sashiko has a
point here, and we could see the requeued deferred QCs immediately re-issued,
which would keep scsi_host_busy() incremented.

Grrr... Need to dig further.
> 
> Note that all the places that currently call ata_scsi_requeue_deferred_qc()
> are called when ATA_PFLAG_EH_PENDING | ATA_PFLAG_EH_IN_PROGRESS
> (ata_port_eh_scheduled() evaluates to true).
> 
> We could add an WARN_ON(!ata_port_eh_scheduled) in ata_scsi_requeue_deferred_qc()
> and we would never see the warning.

You will see the WARN for a timeout because in that case, we enter EH from an
expired block layer timeout calling scsi_timeout(), not from an error IRQ
signaled by the ATA controller. So we do not have ATA_PFLAG_EH_PENDING set.

> With the new function ata_scsi_port_eh_timed_out(), AFAICT, it can call
> ata_scsi_requeue_deferred_qc() before ata_port_eh_scheduled() evaluates to true.

Yes.

> See e.g. the commit message for:
> e20e81a24a4d ("ata: libata-core: do not issue non-internal commands once EH is pending")
> of why I think __ata_scsi_queuecmd() will accept new commands.

Yes, it seems that nothing blocks new commands until scsi_eh_scmd_add() as that
is the function setting the host state to recovery.

So back to the drawing board. A simple requeue will not cut it.

-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

On Fri, Jul 10, 2026 at 12:24:30AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] `ata_scsi_port_eh_timed_out` fails to set `ATA_PFLAG_EH_PENDING`, which allows requeued deferred commands to be immediately re-issued and the timed-out command itself to be erroneously executed, defeating the patch's optimization and causing unnecessary hardware resets.
> 
> Pre-existing issues:
> - [Critical] A race condition between a deferred command's SCSI timeout and its requeue completion causes the command to be silently dropped and reported as successfully completed to the block layer, leading to silent data corruption.
> --
> 
> --- Patch [1]: [PATCH v2 1/2] ata: libata-scsi: terminate deferred commands on time out ---
> commit a21f27abd9e61a32aa55fa1005071af9c225247b
> Author: Damien Le Moal <dlemoal@kernel.org>
> 
> ata: libata-scsi: terminate deferred commands on time out
> 
> If a command timeout occurs while we have a deferred non-NCQ command
> waiting to be issued, the SCSI EH task is never woken up. Avoid this
> unnecessary EH trigger additional wait time using the eh_timed_out SCSI
> host template operation to force a requeue of deferred QCs.
> 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> 
> [ ... ]
> 
> > @@ -1694,16 +1695,19 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
> >  	/*
> >  	 * If we have a deferred qc when a reset occurs or NCQ commands fail,
> >  	 * do not try to be smart about what to do with this deferred command
> > -	 * and simply requeue it by completing it with DID_REQUEUE.
> > +	 * and simply requeue it by completing it with DID_REQUEUE. The
> > +	 * exception here is if the deferred qc timed out, in which case, we
> > +	 * leave it as is as ata_scsi_cmd_error_handler() will take care of it.
> >  	 */
> >  	ata_for_each_link(link, ap, PMP_FIRST) {
> >  		struct ata_queued_cmd *qc = link->deferred_qc;
> >  
> > -		if (qc) {
> > -			link->deferred_qc = NULL;
> > -			cancel_work(&link->deferred_qc_work);
> > -			ata_scsi_qc_done(qc, true, DID_REQUEUE << 16);
> > -		}
> > +		if (!qc || qc->scsicmd == timed_out_scmd)
> > +			continue;
> > +
> > +		link->deferred_qc = NULL;
> > +		cancel_work(&link->deferred_qc_work);
> > +		ata_scsi_qc_done(qc, true, DID_REQUEUE << 16);
> 
> [Severity: Critical]
> This is a pre-existing issue, but won't the explicit invocation of this
> requeue logic on every command timeout make a regression more likely?
> 
> When ata_scsi_requeue_deferred_qc() completes a deferred command using
> ata_scsi_qc_done() with DID_REQUEUE, it unconditionally calls
> ata_qc_free(), which clears the active flag.
> 
> If the block layer timer for this command expires concurrently,
> scsi_timeout() can win the race against the scsi_done() completion. The
> block layer marks the command as timed out and adds it to the SCSI EH queue.
> 
> When SCSI EH runs ata_scsi_cmd_error_handler(), it iterates over all QCs to
> find the command. Since ata_qc_free() was already called and the deferred_qc
> pointer was set to NULL, it won't match the deferred check.
> 
> Does this cause the loop to finish without finding the command and fall
> through to the successful completion 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.
>          */
>         set_host_byte(scmd, DID_OK);
>         scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
>     }
> }
> 
> Could this result in the command being reported as successfully completed to
> the block layer, resulting in silent data corruption?

This specific race between ata_scsi_requeue_deferred_qc() and scsi_timeout()
has not existed for that long, because ata_scsi_requeue_deferred_qc() itself
is not that old.

Perhaps ata_scsi_requeue_deferred_qc(), instead of calling
ata_scsi_qc_done(qc, ...) directly to finish the command, should instead set
ATA_QCFLAG_RETRY (or similar) and call ata_qc_schedule_eh(qc, ...), such that
scsi_eh_scmd_add() will be called, and then EH will requeue the command instead
the completion path.

This way, there should be no race between ata_scsi_requeue_deferred_qc() ->
scsi_done() -> scsi_complete() -> scsi_queue_insert() and scsi_timeout(), as
it will be EH itself that requeues the command.


Note that while link->deferred_qc is set, we can only get a block layer timeout.
If the command was actually issued by the workqueue, link->deferred_qc will be
NULL, so the code in ata_scsi_cmd_error_handler() to handle the race between
normal completion and scsi_timeout() is not really applicable for link->deferred_qc.
(If link->deferred_qc is set, this qc cannot have raced against completion, only
against timeout.)


Kind regards,
Niklas

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

[Niklas Cassel]

On Fri, Jul 10, 2026 at 05:29:56PM +0900, Damien Le Moal wrote:
> On 7/10/26 17:06, Niklas Cassel wrote:
> 
> Yes, it seems that nothing blocks new commands until scsi_eh_scmd_add() as that
> is the function setting the host state to recovery.
> 
> So back to the drawing board. A simple requeue will not cut it.
> 

My thinking is that if we set ATA_QCFLAG_RETRY and call
ata_qc_schedule_eh(qc, ...).

We should end up in:
https://github.com/torvalds/linux/blob/v7.2-rc2/drivers/ata/libata-eh.c#L4075-L4082

And this function will only run from EH itself, so obviously new
command will be blocked at this time.

And, if EH itself requeues the command, there should not be any
race between scsi_timeout() and scsi_complete() (since scsi_timeout()
will only add the command to the list of failed commands if SCMD_STATE_COMPLETE
is not set).


Kind regards,
Niklas