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

On Tue, Jul 21, 2026 at 03:40:26PM +0900, Damien Le Moal wrote:
> @@ -1685,26 +1685,75 @@ 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)
> +enum scsi_timeout_action ata_scsi_requeue_deferred_qc(struct ata_port *ap,
> +						struct scsi_cmnd *timedout_scmd)
>  {
> +	enum scsi_timeout_action action = SCSI_EH_NOT_HANDLED;
> +	struct ata_queued_cmd *qc;
>  	struct ata_link *link;
> +	u32 host_byte;
>  
>  	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.
> +	 * If we have deferred QCs when a reset, a timeout or an NCQ command
> +	 * fails, do not try to be smart about what to do with the deferred
> +	 * commands and simply terminate them and let the SCSI layer decide
> +	 * what to do.
>  	 */
>  	ata_for_each_link(link, ap, PMP_FIRST) {
> -		struct ata_queued_cmd *qc = link->deferred_qc;
> +		qc = link->deferred_qc;
> +		if (!qc)
> +			continue;
> +
> +		/*
> +		 * Clear the deferred QC so that the deferred work does not try
> +		 * to issue it.
> +		 */
> +		link->deferred_qc = NULL;
> +		cancel_work(&link->deferred_qc_work);
> +
> +		/*
> +		 * We are going to complete some scsi command, either with
> +		 * DID_TIME_OUT if the command timed out while waiting for being
> +		 * issued, or with DID_REQUEUE if another command timed out or
> +		 * we had a failed command. However, the block layer may re-issue
> +		 * immediately these commands, keeping the scsi host busy and
> +		 * thus preventing the SCSI EH task from running.
> +		 * So schedule EH on the port to prevent accepting new commands
> +		 * until everything is sorted out with the error or timeout that
> +		 * got us here in the first place. Note that we set EH pending
> +		 * on the port before calling ata_port_schedule_eh() so that we
> +		 * do not reenter this function from ata_eh_set_pending() with
> +		 * timedout_scmd being NULL and erroneously retry deferred QCs
> +		 * that have timed out on other links.
> +		 */
> +		if (!ata_port_eh_scheduled(ap)) {
> +			ap->pflags |= ATA_PFLAG_EH_PENDING;
> +			ata_port_schedule_eh(ap);
> +		}
>  
> -		if (qc) {
> -			link->deferred_qc = NULL;
> -			cancel_work(&link->deferred_qc_work);
> -			ata_scsi_qc_done(qc, true, DID_REQUEUE << 16);
> +		/*
> +		 * If we are being called from scsi_timeout(), then we have a
> +		 * non-NULL timedout_scmd. If the timed out command is for a
> +		 * deferred qc, terminate it with DID_TIME_OUT and tell
> +		 * scsi_timeout() that we are done. Otherwise, ask for a retry
> +		 * with DID_REQUEUE and tell scsi_timeout() that we have not
> +		 * handled the timeout so that the timed out command gets added
> +		 * to the EH work queue with scsi_eh_scmd_add(), for later
> +		 * handling with libata EH ata_scsi_cmd_error_handler().
> +		 */
> +		if (qc->scsicmd != timedout_scmd) {
> +			qc->scsicmd->allowed++;
> +			host_byte = DID_REQUEUE;
> +		} else {
> +			host_byte = DID_TIME_OUT;
> +			action = SCSI_EH_DONE;
>  		}
> +		ata_scsi_qc_done(qc, true, host_byte << 16);

Might be a quite subtle difference, but I think it is much clearer if you
would instead write this as:

		if (timedout_scmd && qc->scsicmd == timedout_scmd) {
			host_byte = DID_TIME_OUT;
			action = SCSI_EH_DONE;
		} else {
			host_byte = DID_REQUEUE;
		}
		ata_scsi_qc_done(qc, true, host_byte << 16);


Because that more clearly highlights the special case.
(And is more in line with my suggested comment, so it also makes it easier
to corroborate the comment with the actual code.)

The special case is if timedout_scmd is non-NULL AND is the deferred QC.

For all other cases, if timedout_scmd is non-NULL but is not the deferred
QC, or if the timedout_cmd is NULL, we want to requeue.


Kind regards,
Niklas

>  	}
> +
> +	return action;
>  }
>  
>  static void ata_scsi_schedule_deferred_qc(struct ata_link *link)