Prevent non-NCQ command starvation

drivers/ata/libata-scsi.c +4 -2

+}
+
  /**
   *	ata_scsi_translate - Translate then issue SCSI command to ATA device
   *	@dev: ATA device to which the command is addressed

@@ -1714,8 +1738,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
   *	spin_lock_irqsave(host lock)
   *
   *	RETURNS:
- *	0 on success, SCSI_ML_QUEUE_DEVICE_BUSY if the command
- *	needs to be deferred.
+ *	0 on success, SCSI_ML_QUEUE_DEVICE_BUSY or SCSI_MLQUEUE_HOST_BUSY if the
+ *	command needs to be deferred.
   */
  static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
  			      ata_xlat_func_t xlat_func)

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

+	 * completes.
+	 */
+	ata_scsi_requeue_deferred_qc(ap);
+
  	if (!fastdrain)
  		return;
 

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

index 42d103542525..1f8c103a7808 100644
@@ -1658,8 +1658,75 @@ 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));

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

+	 * 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) {
+		struct scsi_cmnd *scmd = qc->scsicmd;

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

+	struct ata_queued_cmd *qc = ap->deferred_qc;
+
+	lockdep_assert_held(ap->lock);
+
+	/*
+	 * If we have a differed 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) {
+		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 +1756,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)

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

+		return SCSI_MLQUEUE_DEVICE_BUSY;
+	}
+
  	if (!ap->ops->qc_defer)
  		return 0;
 

@@ -1702,6 +1781,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 defer and requeue the command. */
  	ata_qc_free(qc);
 
  	switch (ret) {

@@ -1777,8 +1867,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() */
 

Quoted 8 lines

  	init_waitqueue_head(&ap->eh_wait_q);
  	init_completion(&ap->park_req_pending);

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

index 2586e77ebf45..b90b17f680f8 100644
@@ -917,6 +917,12 @@ static void ata_eh_set_pending(struct ata_port *ap, bool fastdrain)
 

drivers/ata/libata-eh.c static void ata_eh_set_pending(struct ata_port *ap, bool fastdrain) · line 921

Quoted 4 lines

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

  	ap->pflags |= ATA_PFLAG_EH_PENDING;
 
+	/*
+	 * If we have a deferred qc, requeue it so that it is retried once EH

big nitpick: qc or QC? It seems quite inconsistent.

drivers/ata/libata-eh.c static void ata_eh_set_pending(struct ata_port *ap, bool fastdrain) · line 924

Quoted 3 lines

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

+void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
+{
+	struct ata_queued_cmd *qc = ap->deferred_qc;

I think that you can just return early for !qc, no need to always be doing the lockdep_assert_held check. However, lockdep is slow anyway and I don't think that lockdep_assert_held does anything when lockdep is disabled (so returning early does not nothing, apart from reduce indentation).

Quoted 4 lines

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

  static int ata_scsi_defer(struct ata_port *ap, struct ata_queued_cmd *qc)
  {
  	int ret;

drivers/ata/libata-eh.c static void ata_eh_set_pending(struct ata_port *ap, bool fastdrain)

Quoted 7 lines

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

+	/*
+	 * If we already have a deferred QC, then rely on the SCSI layer to
+	 * defer and requeue all incoming commands until the deferred QC is
+	 * processed, once all on-going commands are completed.
+	 */
+	if (ap->deferred_qc) {
+		ata_qc_free(qc);

Would adding WARN_ON_ONCE(ap->deferred_qc == qc) be totally unnecessary?