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

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

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

>  
> -		if (qc) {

Personally I would write this as:

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

to make the patch about 7 lines smaller.


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