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

[PATCH v3 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 v2:
 - Modified patch 1 to avoid the problem reported by Sashiko that requeued
   deferred QCs may be re-ssued immediately by the block layer, thus
   potentially keeping the device busy. The modification now relies on
   libata-EH to perform the requeue instead of immediately doing it from
   the eh_timed_out operation.
 - Modified patch 2 to use the new helper function defined in patch 1.

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             | 36 ++++++++++++++++++++++++++++-
 drivers/ata/libata-scsi.c           | 22 ++++++++++++++++++
 drivers/scsi/libsas/sas_scsi_host.c | 17 ++++++++++++++
 include/linux/libata.h              |  4 ++++
 include/scsi/libsas.h               |  2 ++
 5 files changed, 80 insertions(+), 1 deletion(-)

-- 
2.55.0

[PATCH v3 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 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: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 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;
+		ata_qc_schedule_eh(qc);
+	}
+	spin_unlock_irqrestore(ap->lock, flags);
+}
+
 /**
  *	ata_scsi_error - SCSI layer error handler callback
  *	@host: SCSI host on which error occurred
@@ -1214,9 +1239,17 @@ static void __ata_eh_qc_complete(struct ata_queued_cmd *qc)
 	struct scsi_cmnd *scmd = qc->scsicmd;
 	unsigned long flags;
 
+
+	/*
+	 * If we are retrying a deferred QC after a timeout, it is not active
+	 * and all we need to do is to complete it directly.
+	 */
 	spin_lock_irqsave(ap->lock, flags);
 	qc->scsidone = ata_eh_scsidone;
-	__ata_qc_complete(qc);
+	if ((qc->flags & ATA_QCFLAG_RETRY) && !(qc->flags & ATA_QCFLAG_ACTIVE))
+		qc->complete_fn(qc);
+	else
+		__ata_qc_complete(qc);
 	WARN_ON(ata_tag_valid(qc->tag));
 	spin_unlock_irqrestore(ap->lock, flags);
 
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 5868526301a2..89ef2eef72a0 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1730,6 +1730,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);
+
+	/*
+	 * ata_scsi_cmd_error_handler() takes care of timed-out deferred queued
+	 * commands. However, if we had any other command time out and we have
+	 * deferred queued commands, we must let scsi_timeout() handle them
+	 * with scsi_eh_scmd_add() so that we do not unnecessarilly delay
+	 * starting the SCSI EH task. So schedule all deferred queued commands
+	 * for retry through EH.
+	 */
+	ata_eh_schedule_deferred_qc_retry(ap, scmd);
+
+	/*
+	 * 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/drivers/ata/libata.h b/drivers/ata/libata.h
index 700627596ce1..e1fdecd93ccf 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -198,6 +198,8 @@ extern void ata_eh_about_to_do(struct ata_link *link, struct ata_device *dev,
 			       unsigned int action);
 extern void ata_eh_done(struct ata_link *link, struct ata_device *dev,
 			unsigned int action);
+void ata_eh_schedule_deferred_qc_retry(struct ata_port *ap,
+				       struct scsi_cmnd *scmd);
 extern void ata_eh_autopsy(struct ata_port *ap);
 const char *ata_get_cmd_name(u8 command);
 extern void ata_eh_report(struct ata_port *ap);
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 v3 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 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 additional SCSI 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 function ata_eh_schedule_deferred_qc_retry()
for SATA devices.

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-eh.c             |  1 +
 drivers/ata/libata.h                |  2 --
 drivers/scsi/libsas/sas_scsi_host.c | 17 +++++++++++++++++
 include/linux/libata.h              |  2 ++
 include/scsi/libsas.h               |  2 ++
 5 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 8e9d57c6f039..ff6d9f94ebed 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -570,6 +570,7 @@ void ata_eh_schedule_deferred_qc_retry(struct ata_port *ap,
 	}
 	spin_unlock_irqrestore(ap->lock, flags);
 }
+EXPORT_SYMBOL_GPL(ata_eh_schedule_deferred_qc_retry);
 
 /**
  *	ata_scsi_error - SCSI layer error handler callback
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index e1fdecd93ccf..700627596ce1 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -198,8 +198,6 @@ extern void ata_eh_about_to_do(struct ata_link *link, struct ata_device *dev,
 			       unsigned int action);
 extern void ata_eh_done(struct ata_link *link, struct ata_device *dev,
 			unsigned int action);
-void ata_eh_schedule_deferred_qc_retry(struct ata_port *ap,
-				       struct scsi_cmnd *scmd);
 extern void ata_eh_autopsy(struct ata_port *ap);
 const char *ata_get_cmd_name(u8 command);
 extern void ata_eh_report(struct ata_port *ap);
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index c83282733ec4..6b62522bd0b2 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 command timeout.
+ * See ata_scsi_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))
+		ata_eh_schedule_deferred_qc_retry(dev->sata_dev.ap, cmd);
+
+	return SCSI_EH_NOT_HANDLED;
+}
+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..2ea7bfbdd867 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1261,6 +1261,8 @@ extern int sata_link_hardreset(struct ata_link *link,
 extern int sata_link_resume(struct ata_link *link, const unsigned int *params,
 			    unsigned long deadline);
 extern void ata_eh_analyze_ncq_error(struct ata_link *link);
+void ata_eh_schedule_deferred_qc_retry(struct ata_port *ap,
+				       struct scsi_cmnd *scmd);
 #else
 static inline const unsigned int *
 sata_ehc_deb_timing(struct ata_eh_context *ehc)
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 v3 2/2] scsi: libsas: terminate deferred commands on time out

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Missing function declaration for `ata_eh_schedule_deferred_qc_retry()` when `CONFIG_SATA_HOST` is disabled, leading to a build failure.
--

--- Patch [2]: [PATCH v3 2/2] scsi: libsas: terminate deferred commands on time out ---
commit 3af8343f458da773cfa43a85c2e86949acf28cc1
Author: Damien Le Moal <dlemoal@kernel.org>

scsi: libsas: 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 not immediately woken up. This patch avoids
this unnecessary wait by using the eh_timed_out SCSI host template operation.

> diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
> index c83282733ec4c..6b62522bd0b2e 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 command timeout.
> + * See ata_scsi_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))
> +		ata_eh_schedule_deferred_qc_retry(dev->sata_dev.ap, cmd);
> +
> +	return SCSI_EH_NOT_HANDLED;
> +}
> +EXPORT_SYMBOL_GPL(sas_eh_timed_out);
> +
[ ... ]
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 327da43d74967..2ea7bfbdd867b 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1261,6 +1261,8 @@ extern int sata_link_hardreset(struct ata_link *link,
>  extern int sata_link_resume(struct ata_link *link, const unsigned int *params,
>  			    unsigned long deadline);
>  extern void ata_eh_analyze_ncq_error(struct ata_link *link);
> +void ata_eh_schedule_deferred_qc_retry(struct ata_port *ap,
> +				       struct scsi_cmnd *scmd);
>  #else

[Severity: Low]
Will this result in a build failure when CONFIG_SATA_HOST is disabled?

The declaration for ata_eh_schedule_deferred_qc_retry() is placed inside
this conditional block. When CONFIG_SATA_HOST is disabled, this declaration
is omitted, but the function is still called unconditionally by
sas_eh_timed_out() in sas_scsi_host.c (as well as ata_scsi_eh_timed_out()
in libata-scsi.c). 

Since modern compilers treat implicit function declarations as errors, could
this break the build under these configurations?

>  static inline const unsigned int *
>  sata_ehc_deb_timing(struct ata_eh_context *ehc)

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

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

[Niklas Cassel]

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

s/differed/deferred/

> for a differed queued commands that timed out as that case is handled in

s/differed/deferred/

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

s/wkae-up/wake-up/

> 
> 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   | 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;
> +		ata_qc_schedule_eh(qc);
> +	}
> +	spin_unlock_irqrestore(ap->lock, flags);

I was expecting you to modify ata_scsi_requeue_deferred_qc() to also
requeue commands using this method, so we only have one of the functions.

Would it work if you modify ata_scsi_requeue_deferred_qc() to essentially
call this function instead?

(They seem to have different locking requirements, so possibly create a
__ata_scsi_requeue_deferred_qc() that has a lockdep_assert_held(ap->lock)
and let ata_eh_schedule_deferred_qc_retry() take the lock and call
__ata_scsi_requeue_deferred_qc(). Perhaps we can come up with better names,
but you get the point.)


Because, as far as I understood the Sashiko comment:

"""
[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.
"""

I don't see how this patch will address the pre-existing issue Sashiko
warned about.

I think that we need to modify ata_scsi_requeue_deferred_qc() to
also requeue commands via EH.

Possibly it works because ata_scsi_requeue_deferred_qc() is currently
always called by EH. But I think it would be simpler if we just have
one function that always requeues/retries via EH, regardless if we
already are in EH context or not.


> +}
> +
>  /**
>   *	ata_scsi_error - SCSI layer error handler callback
>   *	@host: SCSI host on which error occurred
> @@ -1214,9 +1239,17 @@ static void __ata_eh_qc_complete(struct ata_queued_cmd *qc)
>  	struct scsi_cmnd *scmd = qc->scsicmd;
>  	unsigned long flags;
>  
> +
> +	/*
> +	 * If we are retrying a deferred QC after a timeout, it is not active
> +	 * and all we need to do is to complete it directly.
> +	 */
>  	spin_lock_irqsave(ap->lock, flags);
>  	qc->scsidone = ata_eh_scsidone;
> -	__ata_qc_complete(qc);
> +	if ((qc->flags & ATA_QCFLAG_RETRY) && !(qc->flags & ATA_QCFLAG_ACTIVE))
> +		qc->complete_fn(qc);
> +	else
> +		__ata_qc_complete(qc);

I thought that we had to call scsi_eh_finish_cmd() for a command that was
on the SCSI list of failed commands.

See e.g. how ata_scsi_cmd_error_handler() currently calls scsi_eh_finish_cmd().

What am I missing?

Does the code still work if you simply unconditionally call
__ata_qc_complete() here?

(Even if it is slightly less efficient, I would prefer a single/unified path.
This is only if we have a NCQ error or timeout, so I don't think it is super
critical to prioritize efficiency over maintainability here.)


Kind regards,
Niklas

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

[Igor Pylypiv]

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: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  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


> +		ata_qc_schedule_eh(qc);
> +	}
> +	spin_unlock_irqrestore(ap->lock, flags);
> +}
> +
>  /**
>   *	ata_scsi_error - SCSI layer error handler callback
>   *	@host: SCSI host on which error occurred
> @@ -1214,9 +1239,17 @@ static void __ata_eh_qc_complete(struct ata_queued_cmd *qc)
>  	struct scsi_cmnd *scmd = qc->scsicmd;
>  	unsigned long flags;
>  
> +
> +	/*
> +	 * If we are retrying a deferred QC after a timeout, it is not active
> +	 * and all we need to do is to complete it directly.
> +	 */
>  	spin_lock_irqsave(ap->lock, flags);
>  	qc->scsidone = ata_eh_scsidone;
> -	__ata_qc_complete(qc);
> +	if ((qc->flags & ATA_QCFLAG_RETRY) && !(qc->flags & ATA_QCFLAG_ACTIVE))
> +		qc->complete_fn(qc);
> +	else
> +		__ata_qc_complete(qc);
>  	WARN_ON(ata_tag_valid(qc->tag));
>  	spin_unlock_irqrestore(ap->lock, flags);
>  
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 5868526301a2..89ef2eef72a0 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1730,6 +1730,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);
> +
> +	/*
> +	 * ata_scsi_cmd_error_handler() takes care of timed-out deferred queued
> +	 * commands. However, if we had any other command time out and we have
> +	 * deferred queued commands, we must let scsi_timeout() handle them
> +	 * with scsi_eh_scmd_add() so that we do not unnecessarilly delay
> +	 * starting the SCSI EH task. So schedule all deferred queued commands
> +	 * for retry through EH.
> +	 */
> +	ata_eh_schedule_deferred_qc_retry(ap, scmd);
> +
> +	/*
> +	 * 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/drivers/ata/libata.h b/drivers/ata/libata.h
> index 700627596ce1..e1fdecd93ccf 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -198,6 +198,8 @@ extern void ata_eh_about_to_do(struct ata_link *link, struct ata_device *dev,
>  			       unsigned int action);
>  extern void ata_eh_done(struct ata_link *link, struct ata_device *dev,
>  			unsigned int action);
> +void ata_eh_schedule_deferred_qc_retry(struct ata_port *ap,
> +				       struct scsi_cmnd *scmd);
>  extern void ata_eh_autopsy(struct ata_port *ap);
>  const char *ata_get_cmd_name(u8 command);
>  extern void ata_eh_report(struct ata_port *ap);
> 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
> 

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

[Damien Le Moal]

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: stable@vger.kernel.org
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>  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

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

[Damien Le Moal]

On 7/13/26 19:51, Niklas Cassel wrote:
>> +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;
>> +		ata_qc_schedule_eh(qc);
>> +	}
>> +	spin_unlock_irqrestore(ap->lock, flags);
> 
> I was expecting you to modify ata_scsi_requeue_deferred_qc() to also
> requeue commands using this method, so we only have one of the functions.
> 
> Would it work if you modify ata_scsi_requeue_deferred_qc() to essentially
> call this function instead?

Yes, good point. I will do more tests but looks OK so far to do the retry
through EH. Initially, I did not want to do that to not trigger EH too much,
but as you pointed out, ata_scsi_requeue_deferred_qc() is called only if EH is
already scheduled or already running. So there is no point in not using it.

> """
> [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.
> """

When I read this comment from Sashiko, it seems to me that it refers to the
race between command normal completion and timeout timer triggering. Both may
happen at nearly the same time, and one may "win" depending on which is
processed first. Though if it is the timeout, we still endup doing a normal
completion if there was one. The code comment:

int ata_scsi_cmd_error_handler()
{
	...

	/* 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);

Which incidentally is the code path we take when retrying deferred QCs.

> I don't see how this patch will address the pre-existing issue Sashiko
> warned about.

It does not.

> I think that we need to modify ata_scsi_requeue_deferred_qc() to
> also requeue commands via EH.

Done. Testing now.

> Possibly it works because ata_scsi_requeue_deferred_qc() is currently
> always called by EH. But I think it would be simpler if we just have
> one function that always requeues/retries via EH, regardless if we
> already are in EH context or not.

Yes. Agree.

> 
> 
>> +}
>> +
>>  /**
>>   *	ata_scsi_error - SCSI layer error handler callback
>>   *	@host: SCSI host on which error occurred
>> @@ -1214,9 +1239,17 @@ static void __ata_eh_qc_complete(struct ata_queued_cmd *qc)
>>  	struct scsi_cmnd *scmd = qc->scsicmd;
>>  	unsigned long flags;
>>  
>> +
>> +	/*
>> +	 * If we are retrying a deferred QC after a timeout, it is not active
>> +	 * and all we need to do is to complete it directly.
>> +	 */
>>  	spin_lock_irqsave(ap->lock, flags);
>>  	qc->scsidone = ata_eh_scsidone;
>> -	__ata_qc_complete(qc);
>> +	if ((qc->flags & ATA_QCFLAG_RETRY) && !(qc->flags & ATA_QCFLAG_ACTIVE))
>> +		qc->complete_fn(qc);
>> +	else
>> +		__ata_qc_complete(qc);
> 
> I thought that we had to call scsi_eh_finish_cmd() for a command that was
> on the SCSI list of failed commands.

The new use of the eh_timed_out operation puts them in shost->eh_cmd_q with
scsi_eh_scmd_add() because we do not have a abort handler for ATA. commands in
shost->eh_cmd_q go into the local eh_work_q of ata_scsi_cmd_error_handler() and
for all deferred QCs, we endup calling scsi_eh_finish_cmd(), which finally move
the commands into the done_q that ata_eh_finish() uses.
So this is all good, because we always have scsi_eh_scmd_add() to "unbusy" the
host so that we do not block SCSI EH task wakeup.

> See e.g. how ata_scsi_cmd_error_handler() currently calls scsi_eh_finish_cmd().
> 
> What am I missing?

Nothing.

> Does the code still work if you simply unconditionally call
> __ata_qc_complete() here?

You get a WARN_ON() because the QC does *not* have ATA_QCFLAG_ACTIVE flag set
because for deferred QCs, we have not yet issued them. Hence the little change
above to avoid the warn splat. We could avoid it with a ATA_QCFLAG_DEFERRED flag...

> (Even if it is slightly less efficient, I would prefer a single/unified path.
> This is only if we have a NCQ error or timeout, so I don't think it is super
> critical to prioritize efficiency over maintainability here.)

This is not about efficiency. It is to be consistent with the ATA_QCFLAG_ACTIVE
flag being set or not.

-- 
Damien Le Moal
Western Digital Research