Prevent non-NCQ command starvation

Factor out of ata_scsi_translate() the code handling queued command deferral using the port qc_defer callback into the new function ata_scsi_defer(), and simplify the goto used in ata_scsi_translate(). While at it, also add a lockdep annotation to check that the port lock is held when ata_scsi_translate() is called.

No functional changes.

Reviewed-by Hannes Reinecke <[email protected]>

Signed-off-by Damien Le Moal <[email protected]>

Factor out of ata_scsi_translate() the code handling queued command deferral using the port qc_defer callback into the new function ata_scsi_defer(), and simplify the goto used in ata_scsi_translate(). While at it, also add a lockdep annotation to check that the port lock is held when ata_scsi_translate() is called.

No functional changes.

Reviewed-by Hannes Reinecke <[email protected]>

Signed-off-by Damien Le Moal <[email protected]>

When a non-NCQ passthrough command is issued while NCQ commands are being executed, ata_scsi_defer() indicates to ata_scsi_translate() that ata_qc_issue() should not be called for the passthrough command, and instead returns SCSI_MLQUEUE_XXX_BUSY to defer the command execution. This command deferring is correct and as mandated by the ACS specifications since NCQ and non-NCQ commands cannot be mixed.

However, in the case of a host adapter using multiple submission queues, when the target device is under a constant load of NCQ read or write commands, there are no guarantees that requeueing the non-NCQ command will lead to it not being deferred again repeatedly, since other submission queues can constantly issue NCQ commands from different CPUs ahead of the non-NCQ command. This can lead to very long delays for the execution of non-NCQ passthrough commands, and even complete starvation in the worst case scenario.

Since the block layer and the SCSI layer do not distinguish between queuable (NCQ) and non queueable (non-NCQ) commands, libata-scsi SAT implementation must ensure forward progress for non-NCQ commands in the presence of NCQ command traffic. This is similar to what SAS HBAs with a hardware/firmware based implementation do.

Implement such forward progress guarantee by limiting requeueing of non-NCQ commands: when a non-NCQ command is received and NCQ commands are in-flight, do not force a requeue of the non-NCQ command by returning SCSI_MLQUEUE_XXX_BUSY in ata_scsi_translate() and instead hold on to the qc using the new deferred_qc field of struct ata_port.

This deferred qc will be issued using the work item deferred_qc_work running the function ata_scsi_deferred_qc_work() once all in-flight commands complete, which is checked with the port qc_defer() callback indicating that no further delay is necessary. This check is done using the helper function ata_scsi_schedule_deferred_qc() which is called from ata_scsi_qc_complete(). This thus excludes this mechanism from all internal non-NCQ commands issued by ATA EH.

When a port deferred_qc is non NULL, that is, the port has a command waiting for the device queue to drain, the issuing of all incoming commands (both NCQ and non-NCQ) is deferred using the regular busy mechanism. This simplifies the code and also avoids potential denial of service problems if a user issues too many non-NCQ passthrough commands.

Finally, whenever ata EH is scheduled, regardless of the reason, a deferred qc is always requeued so that it can be retried once EH completes. This is done by calling the function ata_scsi_requeue_deferred_qc() from ata_eh_set_pending(). This avoids the need for any special processing for the deferred qc in case of NCQ error, link or device reset, or device timeout.

Reported-by Xingui Yang <[email protected]>

Reported-by Igor Pylypiv <[email protected]>

Fixes 42f22fe36d51 ("scsi: pm8001: Expose hardware queues for pm80xx")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

---
 drivers/ata/libata-core.c |  1 +
 drivers/ata/libata-eh.c   |  6 +++
 drivers/ata/libata-scsi.c | 93 ++++++++++++++++++++++++++++++++++++++-
 drivers/ata/libata.h      |  2 +
 include/linux/libata.h    |  3 ++
 5 files changed, 104 insertions(+), 1 deletion(-)

drivers/ata/libata-core.c +1 -0

index 0b24bd169d61..121f35115d33 100644
@@ -5558,6 +5558,7 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
 	mutex_init(&ap->scsi_scan_mutex);
 	INIT_DELAYED_WORK(&ap->hotplug_task, ata_scsi_hotplug);
 	INIT_DELAYED_WORK(&ap->scsi_rescan_task, ata_scsi_dev_rescan);
+	INIT_WORK(&ap->deferred_qc_work, ata_scsi_deferred_qc_work);
 	INIT_LIST_HEAD(&ap->eh_done_q);
 	init_waitqueue_head(&ap->eh_wait_q);
 	init_completion(&ap->park_req_pending);

drivers/ata/libata-eh.c +6 -0

index 2586e77ebf45..b90b17f680f8 100644
@@ -917,6 +917,12 @@ 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 (!fastdrain)
 		return;
 

drivers/ata/libata-scsi.c +52 -0

index 8e04fc173ea3..e39837846566 100644
@@ -1658,8 +1658,73 @@ static void ata_qc_done(struct ata_queued_cmd *qc)
 	done(cmd);
 }
 
+void ata_scsi_deferred_qc_work(struct work_struct *work)
+{
+	struct ata_port *ap =
+		container_of(work, struct ata_port, deferred_qc_work);
+	struct ata_queued_cmd *qc;
+	unsigned long flags;
+
+	spin_lock_irqsave(ap->lock, flags);
+
+	/*
+	 * If we still have a deferred qc and we are not in EH, issue it. In
+	 * such case, we should not need any more deferring the qc, so warn if
+	 * qc_defer() says otherwise.
+	 */
+	qc = ap->deferred_qc;
+	if (qc && !ata_port_eh_scheduled(ap)) {
+		WARN_ON_ONCE(ap->ops->qc_defer(qc));
+		ap->deferred_qc = NULL;
+		ata_qc_issue(qc);
+	}
+
+	spin_unlock_irqrestore(ap->lock, flags);
+}
+
+void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
+{
+	struct ata_queued_cmd *qc = ap->deferred_qc;
+	struct scsi_cmnd *scmd;
+
+	/*
+	 * 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 retry it by completing it with DID_SOFT_ERROR.
+	 */
+	if (!qc)
+		return;
+
+	scmd = qc->scsicmd;
+	ap->deferred_qc = NULL;
+	ata_qc_free(qc);
+	scmd->result = (DID_SOFT_ERROR << 16);
+	scsi_done(scmd);
+}
+
+static void ata_scsi_schedule_deferred_qc(struct ata_port *ap)
+{
+	struct ata_queued_cmd *qc = ap->deferred_qc;
+
+	/*
+	 * If we have a deferred qc, then qc_defer() is defined and we can use
+	 * this callback to determine if this qc is good to go, unless EH has
+	 * been scheduled.

drivers/ata/libata-scsi.c +30 -1

+}
+
 static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 {
+	struct ata_port *ap = qc->ap;
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	u8 *cdb = cmd->cmnd;
 	bool have_sense = qc->flags & ATA_QCFLAG_SENSE_VALID;

@@ -1689,12 +1754,24 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 	}
 
 	ata_qc_done(qc);
+
+	ata_scsi_schedule_deferred_qc(ap);
 }
 
 static int ata_scsi_defer(struct ata_port *ap, struct ata_queued_cmd *qc)
 {
 	int ret;
 
+	/*
+	 * If we already have a deferred qc, then rely on the SCSI layer to
+	 * requeue and defer all incoming commands until the deferred qc is
+	 * processed, once all on-going commands are completed.
+	 */
+	if (ap->deferred_qc) {
+		ata_qc_free(qc);
+		return SCSI_MLQUEUE_DEVICE_BUSY;
+	}
+
 	if (!ap->ops->qc_defer)
 		return 0;
 

@@ -1702,6 +1779,17 @@ static int ata_scsi_defer(struct ata_port *ap, struct ata_queued_cmd *qc)
 	if (!ret)
 		return 0;
 
+	/*
+	 * We must defer this qc: if this is not an NCQ command, keep this qc
+	 * as a deferred one and wait for all on-going NCQ commands to complete
+	 * before issuing it with the deferred qc work.
+	 */
+	if (!ata_is_ncq(qc->tf.protocol)) {
+		ap->deferred_qc = qc;
+		return SCSI_MLQUEUE_DEVICE_BUSY;
+	}
+
+	/* Use the SCSI layer to requeue and defer the command. */
 	ata_qc_free(qc);
 
 	switch (ret) {

@@ -1777,8 +1865,11 @@ static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
 		goto done;
 
 	rc = ata_scsi_defer(ap, qc);
-	if (rc)
+	if (rc) {
+		if (qc == ap->deferred_qc)
+			return 0;
 		return rc;
+	}
 
 	ata_qc_issue(qc);
 

drivers/ata/libata.h +2 -0

index 0e7ecac73680..60a675df61dc 100644
@@ -165,6 +165,8 @@ void ata_scsi_sdev_config(struct scsi_device *sdev);
 int ata_scsi_dev_config(struct scsi_device *sdev, struct queue_limits *lim,
 		struct ata_device *dev);
 int __ata_scsi_queuecmd(struct scsi_cmnd *scmd, struct ata_device *dev);
+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);

include/linux/libata.h +3 -0

index 39534fafa36a..c5b27d97dfaf 100644
@@ -903,6 +903,9 @@ struct ata_port {
 	u64			qc_active;
 	int			nr_active_links; /* #links with active qcs */
 
+	struct work_struct	deferred_qc_work;
+	struct ata_queued_cmd	*deferred_qc;
+
 	struct ata_link		link;		/* host default link */
 	struct ata_link		*slave_link;	/* see ata_slave_link_init() */
 

Signature · 1 line

2.52.0

Quoted 12 lines

drivers/ata/libata-scsi.c +12 -0

+static void ata_scsi_schedule_deferred_qc(struct ata_port *ap)
+{
+	struct ata_queued_cmd *qc = ap->deferred_qc;
+
+	/*
+	 * If we have a deferred qc, then qc_defer() is defined and we can use
+	 * this callback to determine if this qc is good to go, unless EH has
+	 * been scheduled.
+	 */
+	if (!qc)
+		return;
+

drivers/ata/libata-scsi.c static void ata_qc_done(struct ata_queued_cmd *qc) · line 1722

Quoted 6 lines

drivers/ata/libata-scsi.c +6 -0

+	if (ata_port_eh_scheduled(ap)) {
+		ata_scsi_requeue_deferred_qc(ap);
+		return;
+	}
+	if (!ap->ops->qc_defer(qc))
+		queue_work(system_highpri_wq, &ap->deferred_qc_work);

Do we need to call cancel_work_sync(&ap->deferred_qc_work) in ata_port_detach()?

When a non-NCQ passthrough command is issued while NCQ commands are being executed, ata_scsi_defer() indicates to ata_scsi_translate() that ata_qc_issue() should not be called for the passthrough command, and instead returns SCSI_MLQUEUE_XXX_BUSY to defer the command execution. This command deferring is correct and as mandated by the ACS specifications since NCQ and non-NCQ commands cannot be mixed.

However, in the case of a host adapter using multiple submission queues, when the target device is under a constant load of NCQ read or write commands, there are no guarantees that requeueing the non-NCQ command will lead to it not being deferred again repeatedly, since other submission queues can constantly issue NCQ commands from different CPUs ahead of the non-NCQ command. This can lead to very long delays for the execution of non-NCQ passthrough commands, and even complete starvation in the worst case scenario.

Since the block layer and the SCSI layer do not distinguish between queuable (NCQ) and non queueable (non-NCQ) commands, libata-scsi SAT implementation must ensure forward progress for non-NCQ commands in the presence of NCQ command traffic. This is similar to what SAS HBAs with a hardware/firmware based implementation do.

Implement such forward progress guarantee by limiting requeueing of non-NCQ commands: when a non-NCQ command is received and NCQ commands are in-flight, do not force a requeue of the non-NCQ command by returning SCSI_MLQUEUE_XXX_BUSY in ata_scsi_translate() and instead hold on to the qc using the new deferred_qc field of struct ata_port.

This deferred qc will be issued using the work item deferred_qc_work running the function ata_scsi_deferred_qc_work() once all in-flight commands complete, which is checked with the port qc_defer() callback indicating that no further delay is necessary. This check is done using the helper function ata_scsi_schedule_deferred_qc() which is called from ata_scsi_qc_complete(). This thus excludes this mechanism from all internal non-NCQ commands issued by ATA EH.

When a port deferred_qc is non NULL, that is, the port has a command waiting for the device queue to drain, the issuing of all incoming commands (both NCQ and non-NCQ) is deferred using the regular busy mechanism. This simplifies the code and also avoids potential denial of service problems if a user issues too many non-NCQ passthrough commands.

Finally, whenever ata EH is scheduled, regardless of the reason, a deferred qc is always requeued so that it can be retried once EH completes. This is done by calling the function ata_scsi_requeue_deferred_qc() from ata_eh_set_pending(). This avoids the need for any special processing for the deferred qc in case of NCQ error, link or device reset, or device timeout.

Reported-by Xingui Yang <[email protected]>

Reported-by Igor Pylypiv <[email protected]>

Fixes 42f22fe36d51 ("scsi: pm8001: Expose hardware queues for pm80xx")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

---
  drivers/ata/libata-core.c |  1 +
  drivers/ata/libata-eh.c   |  6 +++
  drivers/ata/libata-scsi.c | 93 ++++++++++++++++++++++++++++++++++++++-
  drivers/ata/libata.h      |  2 +
  include/linux/libata.h    |  3 ++
  5 files changed, 104 insertions(+), 1 deletion(-)

drivers/ata/libata-core.c +1 -0

index 0b24bd169d61..121f35115d33 100644
@@ -5558,6 +5558,7 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
  	mutex_init(&ap->scsi_scan_mutex);
  	INIT_DELAYED_WORK(&ap->hotplug_task, ata_scsi_hotplug);
  	INIT_DELAYED_WORK(&ap->scsi_rescan_task, ata_scsi_dev_rescan);
+	INIT_WORK(&ap->deferred_qc_work, ata_scsi_deferred_qc_work);
  	INIT_LIST_HEAD(&ap->eh_done_q);
  	init_waitqueue_head(&ap->eh_wait_q);
  	init_completion(&ap->park_req_pending);

drivers/ata/libata-eh.c +6 -0

index 2586e77ebf45..b90b17f680f8 100644
@@ -917,6 +917,12 @@ 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 (!fastdrain)
  		return;
 

drivers/ata/libata-scsi.c +92 -1

index 8e04fc173ea3..e39837846566 100644
@@ -1658,8 +1658,73 @@ static void ata_qc_done(struct ata_queued_cmd *qc)
  	done(cmd);
  }
 
+void ata_scsi_deferred_qc_work(struct work_struct *work)
+{
+	struct ata_port *ap =
+		container_of(work, struct ata_port, deferred_qc_work);
+	struct ata_queued_cmd *qc;
+	unsigned long flags;
+
+	spin_lock_irqsave(ap->lock, flags);
+
+	/*
+	 * If we still have a deferred qc and we are not in EH, issue it. In
+	 * such case, we should not need any more deferring the qc, so warn if
+	 * qc_defer() says otherwise.
+	 */
+	qc = ap->deferred_qc;
+	if (qc && !ata_port_eh_scheduled(ap)) {
+		WARN_ON_ONCE(ap->ops->qc_defer(qc));
+		ap->deferred_qc = NULL;
+		ata_qc_issue(qc);
+	}
+
+	spin_unlock_irqrestore(ap->lock, flags);
+}
+
+void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
+{
+	struct ata_queued_cmd *qc = ap->deferred_qc;
+	struct scsi_cmnd *scmd;
+
+	/*
+	 * 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 retry it by completing it with DID_SOFT_ERROR.
+	 */
+	if (!qc)
+		return;
+
+	scmd = qc->scsicmd;
+	ap->deferred_qc = NULL;
+	ata_qc_free(qc);
+	scmd->result = (DID_SOFT_ERROR << 16);
+	scsi_done(scmd);
+}
+
+static void ata_scsi_schedule_deferred_qc(struct ata_port *ap)
+{
+	struct ata_queued_cmd *qc = ap->deferred_qc;
+
+	/*
+	 * If we have a deferred qc, then qc_defer() is defined and we can use
+	 * this callback to determine if this qc is good to go, unless EH has
+	 * been scheduled.
+	 */
+	if (!qc)
+		return;
+
+	if (ata_port_eh_scheduled(ap)) {
+		ata_scsi_requeue_deferred_qc(ap);
+		return;
+	}
+	if (!ap->ops->qc_defer(qc))
+		queue_work(system_highpri_wq, &ap->deferred_qc_work);
+}
+
  static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
  {
+	struct ata_port *ap = qc->ap;
  	struct scsi_cmnd *cmd = qc->scsicmd;
  	u8 *cdb = cmd->cmnd;
  	bool have_sense = qc->flags & ATA_QCFLAG_SENSE_VALID;

@@ -1689,12 +1754,24 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
  	}
 
  	ata_qc_done(qc);
+
+	ata_scsi_schedule_deferred_qc(ap);
  }
 
  static int ata_scsi_defer(struct ata_port *ap, struct ata_queued_cmd *qc)
  {
  	int ret;
 
+	/*
+	 * If we already have a deferred qc, then rely on the SCSI layer to
+	 * requeue and defer all incoming commands until the deferred qc is
+	 * processed, once all on-going commands are completed.
+	 */
+	if (ap->deferred_qc) {
+		ata_qc_free(qc);
+		return SCSI_MLQUEUE_DEVICE_BUSY;
+	}
+
  	if (!ap->ops->qc_defer)
  		return 0;
 

@@ -1702,6 +1779,17 @@ static int ata_scsi_defer(struct ata_port *ap, struct ata_queued_cmd *qc)
  	if (!ret)
  		return 0;
 
+	/*
+	 * We must defer this qc: if this is not an NCQ command, keep this qc
+	 * as a deferred one and wait for all on-going NCQ commands to complete
+	 * before issuing it with the deferred qc work.
+	 */
+	if (!ata_is_ncq(qc->tf.protocol)) {
+		ap->deferred_qc = qc;
+		return SCSI_MLQUEUE_DEVICE_BUSY;
+	}
+
+	/* Use the SCSI layer to requeue and defer the command. */
  	ata_qc_free(qc);
 
  	switch (ret) {

@@ -1777,8 +1865,11 @@ static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
  		goto done;
 
  	rc = ata_scsi_defer(ap, qc);
-	if (rc)
+	if (rc) {
+		if (qc == ap->deferred_qc)
+			return 0;
  		return rc;
+	}
 
  	ata_qc_issue(qc);
 

drivers/ata/libata.h +2 -0

index 0e7ecac73680..60a675df61dc 100644
@@ -165,6 +165,8 @@ void ata_scsi_sdev_config(struct scsi_device *sdev);
  int ata_scsi_dev_config(struct scsi_device *sdev, struct queue_limits *lim,
  		struct ata_device *dev);
  int __ata_scsi_queuecmd(struct scsi_cmnd *scmd, struct ata_device *dev);
+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);

include/linux/libata.h +3 -0

index 39534fafa36a..c5b27d97dfaf 100644
@@ -903,6 +903,9 @@ struct ata_port {
  	u64			qc_active;
  	int			nr_active_links; /* #links with active qcs */
 
+	struct work_struct	deferred_qc_work;
+	struct ata_queued_cmd	*deferred_qc;
+
  	struct ata_link		link;		/* host default link */
  	struct ata_link		*slave_link;	/* see ata_slave_link_init() */
 

When a non-NCQ passthrough command is issued while NCQ commands are

being executed, ata_scsi_defer() indicates to ata_scsi_translate() that ata_qc_issue() should not be called for the passthrough command, and

instead returns SCSI_MLQUEUE_XXX_BUSY to defer the command execution. This command deferring is correct and as mandated by the ACS specifications since NCQ and non-NCQ commands cannot be mixed.

However, in the case of a host adapter using multiple submission queues, when the target device is under a constant load of NCQ read or write

commands, there are no guarantees that requeueing the non-NCQ command will lead to it not being deferred again repeatedly, since other submission queues can constantly issue NCQ commands from different CPUs ahead of the non-NCQ command. This can lead to very long delays for the execution of non-NCQ passthrough commands, and even complete starvation in the worst case scenario.

Since the block layer and the SCSI layer do not distinguish between queuable (NCQ) and non queueable (non-NCQ) commands, libata-scsi SAT

implementation must ensure forward progress for non-NCQ commands in the presence of NCQ command traffic. This is similar to what SAS HBAs with a hardware/firmware based implementation do.

Implement such forward progress guarantee by limiting requeueing of non-NCQ commands: when a non-NCQ command is received and NCQ commands are in-flight, do not force a requeue of the non-NCQ command by returning SCSI_MLQUEUE_XXX_BUSY in ata_scsi_translate() and instead hold on to the qc using the new deferred_qc field of struct ata_port.

This deferred qc will be issued using the work item deferred_qc_work running the function ata_scsi_deferred_qc_work() once all in-flight commands complete, which is checked with the port qc_defer() callback indicating that no further delay is necessary. This check is done using the helper function ata_scsi_schedule_deferred_qc() which is called from ata_scsi_qc_complete(). This thus excludes this mechanism from all internal non-NCQ commands issued by ATA EH.

When a port deferred_qc is non NULL, that is, the port has a command waiting for the device queue to drain, the issuing of all incoming commands (both NCQ and non-NCQ) is deferred using the regular busy mechanism. This simplifies the code and also avoids potential denial of service problems if a user issues too many non-NCQ passthrough commands.

Quoted 14 lines

Implement such forward progress guarantee by limiting requeueing of non-NCQ commands: when a non-NCQ command is received and NCQ commands are in-flight, do not force a requeue of the non-NCQ command by returning SCSI_MLQUEUE_XXX_BUSY in ata_scsi_translate() and instead hold on to the qc using the new deferred_qc field of struct ata_port.

This deferred qc will be issued using the work item deferred_qc_work running the function ata_scsi_deferred_qc_work() once all in-flight commands complete, which is checked with the port qc_defer() callback indicating that no further delay is necessary. This check is done using the helper function ata_scsi_schedule_deferred_qc() which is called from ata_scsi_qc_complete(). This thus excludes this mechanism from all internal non-NCQ commands issued by ATA EH.

Quoted 5 lines

When a port deferred_qc is non NULL, that is, the port has a command waiting for the device queue to drain, the issuing of all incoming commands (both NCQ and non-NCQ) is deferred using the regular busy mechanism. This simplifies the code and also avoids potential denial of service problems if a user issues too many non-NCQ passthrough commands.

Instead of introducing a workqueue, did you try/consider to do it the same way that we handle CDL commands that completed without error:
https://github.com/torvalds/linux/blob/v6.19-rc3/drivers/ata/libata-core.c#L4971-L4988

I.e., in qc_defer(), if we try to queue a non-NCQ command while there are NCQ commands active, set ATA_PFLAG_EH_PENDING so that we do not trigger fast drain (we wait for the active commands to finish), and then call ata_qc_schedule_eh() on the non-NCQ qc.

Sure, for passthrough commands specifically, SCSI will not want to retry the command, because scsi_noretry_cmd() will return true:
https://github.com/torvalds/linux/blob/v6.19-rc3/drivers/scsi/scsi_error.c#L2227

But this could easily be solved by e.g. modifying scsi_noretry_cmd() to add an additional "case DID_REQUEUE: return false;"

And also set set_host_byte(scmd, DID_REQUEUE); (probably based on a new ATA_QCFLAG_DEFER flag or simiar), after we have cleared DID_TIME_OUT using set_host_byte(scmd, DID_OK):
https://github.com/torvalds/linux/blob/v6.19-rc3/drivers/ata/libata-eh.c#L640

Since this works for CDL commands, I don't see why we shouldn't be able to also send a non-NCQ command (if there are NCQ commands active), via SCSI EH, so that SCSI itself retries the command, rather than us holding/hiding a command from the SCSI queue, by using an additional workqueue in libata.

(If libata is just a SCSI LLDD, it somehow feels more logical to make use of the SCSI queue, rather than to add our own queueing in libata using a workqueue.)

On 12/31/25 20:17, Niklas Cassel wrote:

You are kind of proving my point here: I feel that is all much more complicated than using the libata internal deferred qc work. Also, involving the SCSI layer about non-NCQ commands deferral is kind of a layering violation as the non-queueable command concept does not exist in SCSI.

libata must act as a SATL and the SAT specifications are clear about what a SATL implementation can do for non-NCQ commands. Referring to SATA6r01:

6.2.5 Commands the SATL queues internally
If the translation of a SCSI command requires the SATL to send an ATA non-NCQ command to the ATA device while one or more ATA commands are active, then the SATL shall:
a) suspend processing of that SCSI command, and:
1) maintain the SCSI command in a task set;
2) resume processing of that SCSI command when the ATA device returns command
complete for all ATA commands the SATL has previously sent to the ATA device; and
3) after that SCSI command completes, resume processing of other SCSI commands;
b) return TASK SET FULL status for that SCSI command; or
c) return BUSY status for that SCSI command.

What you are proposing does not nicely fit. This patch essentially implements option (a). Option (c) is the current code, which causes the issue for multi-queue cases (note that AHCI is not affected because it is single queue). And option (c) would be mostly equivalent to (b) and cause the same issue as that would involve a requeue too.

Quoted 6 lines · Damien Le Moal

On 12/31/25 20:17, Niklas Cassel wrote:

You are kind of proving my point here: I feel that is all much more complicated than using the libata internal deferred qc work. Also, involving the SCSI layer about non-NCQ commands deferral is kind of a layering violation as the non-queueable command concept does not exist in SCSI.

Personally, I think my suggestion is much less complicated, as we do not need to think about when to clear ap->deferred_qc or when to stop the workqueue, during the special cases like when a reset occurs, or when a NCQ command fails. I'm a little bit worried that there are some other special cases that we might be overlooking.

Reusing the method we already use for CDL commands would completely sidestep these issues, as we are putting the command in the SCSI EH workqueue, which will requeue the command for us.

I do not see it as a layering violation, as we simply wait for the queue to drain, then let SCSI EH retry/insert the failed/deferred non-NCQ command in the normal SCSI queue.

The non-NCQ should be the only command that is failed, but even if there was a NCQ command that failed as well, we will retry scmd->allowed times anyway, so the non-NCQ command should eventually be the only command that is retried.

Quoted 15 lines

libata must act as a SATL and the SAT specifications are clear about what a SATL implementation can do for non-NCQ commands. Referring to SATA6r01:

6.2.5 Commands the SATL queues internally
If the translation of a SCSI command requires the SATL to send an ATA non-NCQ command to the ATA device while one or more ATA commands are active, then the SATL shall:
a) suspend processing of that SCSI command, and:
1) maintain the SCSI command in a task set;
2) resume processing of that SCSI command when the ATA device returns command
complete for all ATA commands the SATL has previously sent to the ATA device; and
3) after that SCSI command completes, resume processing of other SCSI commands;
b) return TASK SET FULL status for that SCSI command; or
c) return BUSY status for that SCSI command.

Quoted 5 lines

What you are proposing does not nicely fit. This patch essentially implements option (a). Option (c) is the current code, which causes the issue for multi-queue cases (note that AHCI is not affected because it is single queue). And option (c) would be mostly equivalent to (b) and cause the same issue as that would involve a requeue too.

I think my suggestion would be a)
since by triggering EH, we wait for all commands to complete, then resume processing of the non-NCQ command.

That said, I know that you had plans to change the processing of CDL commands to use a workqueue instead of EH, so in the grand scheme of things, if we will migrate to use a workqueue instead of EH for CDL, perhaps it makes sense if we also use a workqueue for a deferred non-NCQ command.

Will we be able to use the same workqueue, or will we soon have two different workqueues?

drivers/ata/libata-scsi.c +3 -0

+	}
+
+	/* Use the SCSI layer to requeue and defer the command. */
 	ata_qc_free(qc);
 
 	switch (ret) {

drivers/ata/libata-scsi.c +1 -0

 		return rc;
+	}
 
 	ata_qc_issue(qc);
 

Quoted 4 lines · Damien Le Moal

drivers/ata/libata-scsi.c +0 -0

@@ -1702,6 +1779,17 @@ static int ata_scsi_defer(struct ata_port *ap, struct ata_queued_cmd *qc)
 	if (!ret)
 		return 0;
 

drivers/ata/libata-scsi.c static int ata_scsi_defer(struct ata_port *ap, struct ata_queued_cmd *qc) · line 1789

Quoted 8 lines

drivers/ata/libata-scsi.c +8 -0

+	/*
+	 * We must defer this qc: if this is not an NCQ command, keep this qc
+	 * as a deferred one and wait for all on-going NCQ commands to complete
+	 * before issuing it with the deferred qc work.
+	 */
+	if (!ata_is_ncq(qc->tf.protocol)) {
+		ap->deferred_qc = qc;
+		return SCSI_MLQUEUE_DEVICE_BUSY;

Here you save the qc, and you return SCSI_MLQUEUE_DEVICE_BUSY; SCSI_MLQUEUE_DEVICE_BUSY will cause the block layer to reinsert the request in the queue, so you will get the same request sent to ata_scsi_translate() again, even though you have save it. A little ugly IMO.

It seems like you are relying on the ata_scsi_qc_new() call will give you the same *qc as the one you have stored...

drivers/ata/libata-scsi.c static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd, · line 1870

Quoted 5 lines

drivers/ata/libata-scsi.c +3 -1

 	rc = ata_scsi_defer(ap, qc);
-	if (rc)
+	if (rc) {
+		if (qc == ap->deferred_qc)
+			return 0;

...and then you have this code here, to do nothing in that case.

But, are we guaranteed that we will always get the same qc?

Have you tried this both with and without mq-deadline? And on both ahci and libsas HBAs?

Quoted 5 lines · Niklas Cassel

Quoted 4 lines · Damien Le Moal

drivers/ata/libata-scsi.c +0 -0

@@ -1702,6 +1779,17 @@ static int ata_scsi_defer(struct ata_port *ap, struct ata_queued_cmd *qc)
 	if (!ret)
 		return 0;
 

Quoted 13 lines

drivers/ata/libata-scsi.c static int ata_scsi_defer(struct ata_port *ap, struct ata_queued_cmd *qc) · line 1789

Quoted 8 lines

drivers/ata/libata-scsi.c +8 -0

+	/*
+	 * We must defer this qc: if this is not an NCQ command, keep this qc
+	 * as a deferred one and wait for all on-going NCQ commands to complete
+	 * before issuing it with the deferred qc work.
+	 */
+	if (!ata_is_ncq(qc->tf.protocol)) {
+		ap->deferred_qc = qc;
+		return SCSI_MLQUEUE_DEVICE_BUSY;

Here you save the qc, and you return SCSI_MLQUEUE_DEVICE_BUSY; SCSI_MLQUEUE_DEVICE_BUSY will cause the block layer to reinsert the request in the queue, so you will get the same request sent to ata_scsi_translate() again, even though you have save it. A little ugly IMO.

No it will not cause a requeue in this case because this change:

drivers/ata/libata-scsi.c +3 -1

 	rc = ata_scsi_defer(ap, qc);
-	if (rc)
+	if (rc) {
+		if (qc == ap->deferred_qc)
+			return 0;

ensures that we return "0", thus telling the scsi and block layer that the command/request was accepted, but we do *not* call ata_qc_issue() for that qc. It is deferred and will be issued by the deferred qc work once the queue drains.

Quoted 28 lines · Damien Le Moal

Quoted 5 lines · Niklas Cassel

Quoted 4 lines · Damien Le Moal

drivers/ata/libata-scsi.c +0 -0

@@ -1702,6 +1779,17 @@ static int ata_scsi_defer(struct ata_port *ap, struct ata_queued_cmd *qc)
 	if (!ret)
 		return 0;
 

Quoted 13 lines

drivers/ata/libata-scsi.c static int ata_scsi_defer(struct ata_port *ap, struct ata_queued_cmd *qc) · line 1789

Quoted 8 lines

drivers/ata/libata-scsi.c +8 -0

+	/*
+	 * We must defer this qc: if this is not an NCQ command, keep this qc
+	 * as a deferred one and wait for all on-going NCQ commands to complete
+	 * before issuing it with the deferred qc work.
+	 */
+	if (!ata_is_ncq(qc->tf.protocol)) {
+		ap->deferred_qc = qc;
+		return SCSI_MLQUEUE_DEVICE_BUSY;

Here you save the qc, and you return SCSI_MLQUEUE_DEVICE_BUSY; SCSI_MLQUEUE_DEVICE_BUSY will cause the block layer to reinsert the request in the queue, so you will get the same request sent to ata_scsi_translate() again, even though you have save it. A little ugly IMO.

No it will not cause a requeue in this case because this change:

drivers/ata/libata-scsi.c +3 -1

 	rc = ata_scsi_defer(ap, qc);
-	if (rc)
+	if (rc) {
+		if (qc == ap->deferred_qc)
+			return 0;

Quoted 3 lines

ensures that we return "0", thus telling the scsi and block layer that the command/request was accepted, but we do *not* call ata_qc_issue() for that qc. It is deferred and will be issued by the deferred qc work once the queue drains.

I see how I got confused.

In the case where we store the deferred QC, you overload the return value. But the original return value did not make sense (SCSI_MLQUEUE_DEVICE_BUSY), as it means ask upper layer to requeue. I suggest that you let ata_scsi_defer() return some other value in this case.

ATA_DEFER_ACTION_CMD_STORED or something.

Then at the call site:

	rc = ata_scsi_defer(ap, qc);
	if (rc) {
		if (rc == ATA_DEFER_ACTION_CMD_STORED)
			return 0;
		return rc;
	}

This small patch series addresses potential command starvation issues with non-NCQ passthrough commands issued to a device accessed through

Quoted 2 lines · Damien Le Moal

This small patch series addresses potential command starvation issues with non-NCQ passthrough commands issued to a device accessed through

Here and everywhere else:
s/non-NCQ passthrough commands/non-NCQ commands/

The problem is really not related to passthrough commands, but rather the mix between NCQ and non-NCQ commands.

E.g. you can perform a ioctl with the REQ_OP_DISCARD command, for most ATA devices this is a SCSI WRITE SAME 16 CMD, which gets translated to either a NCQ or non-NCQ command depending on if the device supports NCQ encapsulated Data Set Management (DSM) commands, and specifically if the device has the QUEUED DATA SET MANAGEMENT SUPPORTS TRIM bit set or not, which is not a given.

TL;DR: the starvation problem is not directly tied to non-NCQ passthrough commands, but any command, even commands that goes via the block layer, that happens to be translated to a non-NCQ command.