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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] A race condition between block layer timeout work and the SCSI EH thread causes deferred QCs to fail permanently, resulting in spurious I/O errors.
--

--- Patch [1]: [PATCH v6 1/2] ata: libata-scsi: terminate deferred commands on time out ---
commit 65621fb40c8a19c964f38c70891c67cb0a3f9251
Author: Damien Le Moal <dlemoal@kernel.org>
Subject: ata: libata-scsi: terminate deferred commands on time out

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. Avoid this
unnecessary SCSI EH task wake-up additional time by scheduling a retry of
all waiting deferred QCs, using the eh_timed_out SCSI host template
operation.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 5868526301a2b..b02b4ca5c09e2 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c

[ ... ]

> -void ata_scsi_requeue_deferred_qc(struct ata_port *ap)
> +void ata_scsi_requeue_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);
>  
>  	/*
> -	 * 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.
> +	 * Trigger EH for any deferred qc, to either retry them or handle one
> +	 * that timed out.
>  	 */
>  	ata_for_each_link(link, ap, PMP_FIRST) {
> -		struct ata_queued_cmd *qc = link->deferred_qc;
> +		qc = link->deferred_qc;
> +		if (!qc)
> +			continue;
>  
> -		if (qc) {
> -			link->deferred_qc = NULL;
> -			cancel_work(&link->deferred_qc_work);
> +		link->deferred_qc = NULL;
> +		cancel_work(&link->deferred_qc_work);
> +
> +		if (!timedout_scmd) {
> +			/*
> +			 * We are retrying due to some error. Complete the
> +			 * request and ask for a requeue. In this case, since EH
> +			 * was scheduled already, the block layer attempting to
> +			 * re-issue the command immediately will not lead to the
> +			 * device being kept busy, thus allowing SCSI EH task to
> +			 * run.
> +			 */
>  			ata_scsi_qc_done(qc, true, DID_REQUEUE << 16);
> +			continue;
>  		}
> +
> +		/*
> +		 * We are being called from scsi_timeout(). scsi_eh_scmd_add()
> +		 * will add the command to eh_work_q and it will be handled by
> +		 * ata_scsi_cmd_error_handler(). So here, we only need to
> +		 * indicate if we want a retry if the command did not timeout.
> +		 */
> +		if (qc->scsicmd != timedout_scmd)
> +			qc->flags |= ATA_QCFLAG_RETRY;
> +
> +		/*
> +		 * Schedule EH, but set EH pending on the port 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.
> +		 */
> +		ap->pflags |= ATA_PFLAG_EH_PENDING;
> +		ata_qc_schedule_eh(qc);

[Severity: High]
Does this introduce a race condition between the block layer timeout work and
the SCSI EH thread that could cause deferred QCs to fail permanently with
spurious I/O errors?

When a command times out, ata_scsi_eh_timed_out() calls this and triggers
ata_qc_schedule_eh() on deferred QCs. This calls blk_abort_request(), which
schedules the block layer timeout_work asynchronously.

Meanwhile, the original timed-out command is added to eh_cmd_q and immediately
wakes up the SCSI EH thread. If the SCSI EH thread processes eh_cmd_q in
ata_scsi_cmd_error_handler() before the asynchronous timeout_work for the
deferred QCs runs, ata_scsi_cmd_error_handler() will miss the deferred QCs
since they aren't in eh_work_q yet:

drivers/ata/libata-eh.c:ata_scsi_cmd_error_handler() {
    ...
	list_for_each_entry_safe(scmd, tmp, eh_work_q, eh_entry) {
        /* deferred QCs skipped if timeout_work hasn't added them yet */
    ...
}

Later in the EH process, ata_eh_finish() will process these missed QCs
(because ATA_QCFLAG_EH is set), calling ata_eh_qc_retry() which frees the qc
without completing the block request.

When the delayed timeout_work finally adds the deferred QCs to SCSI EH, a
subsequent EH run will fail to find their freed qcs, fall back to the normal
completion path, set scmd->retries = scmd->allowed, and complete them with
DID_TIME_OUT. This prevents retries and fails the commands permanently.

>  	}
>  }

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