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

[PATCH v4 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 v3:
 - Reimplement ata_scsi_requeue_deferred_qc() in patch 1 as
   ata_eh_retry_deferred_qc() so that all requeue pathes use the same
   function.

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             | 38 ++++++++++++++++----
 drivers/ata/libata-scsi.c           | 56 +++++++++++++++++------------
 drivers/ata/libata.h                |  2 +-
 drivers/scsi/libsas/sas_scsi_host.c | 17 +++++++++
 include/linux/libata.h              |  3 ++
 include/scsi/libsas.h               |  2 ++
 6 files changed, 88 insertions(+), 30 deletions(-)

-- 
2.55.0

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

[Damien Le Moal]

If a command timeout occurs while we have deferred non-NCQ commands
waiting to be issued, the SCSI EH task is not immediately woken up as the
waiting deferred commands are never issued nor completed, thus leaving
these commands 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 SCSI EH task
sleeping. Eventually, when the deferred commands also time out, the SCSI
EH task is woken up and the timeout processing occurs.

Avoid this unnecessary SCSI EH task wake-up additional time by immediately
scheduling a retry of all waiting deferred QCs, using the eh_timed_out
SCSI host template operation. The function ata_scsi_eh_timed_out() is
introduced to implement this operation.

However, ata_scsi_eh_timed_out() cannot use the function
ata_scsi_requeue_deferred_qc() to force a retry of waiting deferred QCs,
because this function completes the waiting QCs using ata_scsi_qc_done(),
thus trigerring a requeue at the block layer, which itself may result in
an immediate reissuing of the QCs, thus keeping the device in a busy state
which prevents the SCSI EH task from waking up. To avoid this,
ata_scsi_requeue_deferred_qc() is reimplemented as
ata_eh_retry_deferred_qc() so that the retry of the waiting deferred QCs
is scheduled through libata EH with ata_qc_schedule_eh(), with the waiting
QCs flagged with ATA_QCFLAG_RETRY. Since ata_scsi_requeue_deferred_qc()
was already called only when EH was already scheduled or running, this new
scheduling of libata EH does not add overhead and guarantees that the
device is not kept in a busy state, allowing the SCSI EH task to run and
to execute libata EH. Once done, the deferred QCs retry is scheduled from
ata_eh_finish() calling ata_eh_qc_retry(). This new path requires a small
modification of __ata_eh_qc_complete() to avoid warnings due to the fact
that deferred QCs are not flagged as active (ATA_QCFLAG_ACTIVE is not set
as these QCs have not been issued yet).

Of note is that since ata_scsi_cmd_error_handler() already handles
directly deferred QCs that timed out, ata_eh_retry_deferred_qc() always
ignores a deferred QC that correspond to a timed out SCSI command.

Since ata_scsi_eh_timed_out() does not fully handle the timeout itself,
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   | 38 +++++++++++++++++++++-----
 drivers/ata/libata-scsi.c | 56 +++++++++++++++++++++++----------------
 drivers/ata/libata.h      |  2 +-
 include/linux/libata.h    |  2 ++
 4 files changed, 68 insertions(+), 30 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 29ec0f7fef4a..7943593d49c8 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;
+		ata_qc_schedule_eh(qc);
+	}
+}
+
 /**
  *	ata_scsi_error - SCSI layer error handler callback
  *	@host: SCSI host on which error occurred
@@ -947,11 +968,8 @@ static void ata_eh_set_pending(struct ata_port *ap, bool fastdrain)
 
 	ap->pflags |= ATA_PFLAG_EH_PENDING;
 
-	/*
-	 * If we have a deferred qc, requeue it so that it is retried once EH
-	 * completes.
-	 */
-	ata_scsi_requeue_deferred_qc(ap);
+	/* If we have deferred QCs, tell EH to retry them. */
+	ata_eh_retry_deferred_qc(ap, NULL);
 
 	if (!fastdrain)
 		return;
@@ -1214,9 +1232,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..1870669b8e05 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1685,28 +1685,6 @@ 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)
-{
-	struct ata_link *link;
-
-	lockdep_assert_held(ap->lock);
-
-	/*
-	 * 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.
-	 */
-	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);
-		}
-	}
-}
-
 static void ata_scsi_schedule_deferred_qc(struct ata_link *link)
 {
 	struct ata_queued_cmd *qc = link->deferred_qc;
@@ -1723,13 +1701,45 @@ 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_eh_retry_deferred_qc(ap, NULL);
 		return;
 	}
 	if (!ap->ops->qc_defer(qc))
 		queue_work(system_highpri_wq, &link->deferred_qc_work);
 }
 
+static void ata_scsi_retry_deferred_qc(struct ata_port *ap,
+				       struct scsi_cmnd *scmd)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(ap->lock, flags);
+	ata_eh_retry_deferred_qc(ap, scmd);
+	spin_unlock_irqrestore(ap->lock, flags);
+}
+
+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_scsi_retry_deferred_qc(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..efaccebe93f5 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -180,7 +180,6 @@ 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);
 
 /* libata-eh.c */
 extern unsigned int ata_internal_cmd_timeout(struct ata_device *dev, u8 cmd);
@@ -198,6 +197,7 @@ 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_retry_deferred_qc(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 30fdf673b887..51178acd68d7 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
@@ -1461,6 +1462,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 v4 1/2] ata: libata-scsi: terminate deferred commands on time out

[sashiko-bot]

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?

> +		ata_qc_schedule_eh(qc);
> +	}
> +}
> +
[ ... ]

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

[PATCH v4 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_scsi_retry_deferred_qc()
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-scsi.c           |  4 ++--
 drivers/scsi/libsas/sas_scsi_host.c | 17 +++++++++++++++++
 include/linux/libata.h              |  1 +
 include/scsi/libsas.h               |  2 ++
 4 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 1870669b8e05..878527cc2b0e 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1708,8 +1708,7 @@ static void ata_scsi_schedule_deferred_qc(struct ata_link *link)
 		queue_work(system_highpri_wq, &link->deferred_qc_work);
 }
 
-static void ata_scsi_retry_deferred_qc(struct ata_port *ap,
-				       struct scsi_cmnd *scmd)
+void ata_scsi_retry_deferred_qc(struct ata_port *ap, struct scsi_cmnd *scmd)
 {
 	unsigned long flags;
 
@@ -1717,6 +1716,7 @@ static void ata_scsi_retry_deferred_qc(struct ata_port *ap,
 	ata_eh_retry_deferred_qc(ap, scmd);
 	spin_unlock_irqrestore(ap->lock, flags);
 }
+EXPORT_SYMBOL_GPL(ata_scsi_retry_deferred_qc);
 
 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..ca497cfc1353 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_scsi_retry_deferred_qc(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 51178acd68d7..18f76c3db4a4 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);
+void ata_scsi_retry_deferred_qc(struct ata_port *ap, struct scsi_cmnd *scmd);
 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