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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] `ata_scsi_port_eh_timed_out` fails to set `ATA_PFLAG_EH_PENDING`, which allows requeued deferred commands to be immediately re-issued and the timed-out command itself to be erroneously executed, defeating the patch's optimization and causing unnecessary hardware resets.

Pre-existing issues:
- [Critical] A race condition between a deferred command's SCSI timeout and its requeue completion causes the command to be silently dropped and reported as successfully completed to the block layer, leading to silent data corruption.
--

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

ata: libata-scsi: terminate deferred commands on time out

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. Avoid this
unnecessary EH trigger additional wait time using the eh_timed_out SCSI
host template operation to force a requeue of deferred QCs.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c

[ ... ]

> @@ -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;
>  
> -		if (qc) {
> -			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);

[Severity: Critical]
This is a pre-existing issue, but won't the explicit invocation of this
requeue logic on every command timeout make a regression more likely?

When ata_scsi_requeue_deferred_qc() completes a deferred command using
ata_scsi_qc_done() with DID_REQUEUE, it unconditionally calls
ata_qc_free(), which clears the active flag.

If the block layer timer for this command expires concurrently,
scsi_timeout() can win the race against the scsi_done() completion. The
block layer marks the command as timed out and adds it to the SCSI EH queue.

When SCSI EH runs ata_scsi_cmd_error_handler(), it iterates over all QCs to
find the command. Since ata_qc_free() was already called and the deferred_qc
pointer was set to NULL, it won't match the deferred check.

Does this cause the loop to finish without finding the command and fall
through to the successful completion branch:

drivers/ata/libata-eh.c:ata_scsi_cmd_error_handler() {
    ...
    } else {
        /* Normal completion occurred after
         * SCSI timeout but before this point.
         * Successfully complete it.
         */
        set_host_byte(scmd, DID_OK);
        scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
    }
}

Could this result in the command being reported as successfully completed to
the block layer, resulting in silent data corruption?

[ ... ]

> +static enum scsi_timeout_action
> +ata_scsi_port_eh_timed_out(struct ata_port *ap, struct scsi_cmnd *scmd)
> +{
> +	unsigned long flags;
> +
> +	/*
> +	 * We had a timeout, either for an NCQ command or for one deferred
> +	 * queued command. If we have deferred QCs and we do not release them
> +	 * immediately, we will have shost->host_failed != scsi_host_busy()
> +	 * until the deferred QCs also timeout. This unnecessarilly increases
> +	 * the time it takes for scsi EH to start. Terminate all deferred QCs
> +	 * to avoid that.
> +	 */
> +	spin_lock_irqsave(ap->lock, flags);
> +	ata_scsi_requeue_deferred_qc(ap, scmd);
> +	spin_unlock_irqrestore(ap->lock, flags);

[Severity: High]
Does this function need to set ATA_PFLAG_EH_PENDING?

Because this flag is not set, ata_port_eh_scheduled() will evaluate to
false. This allows the block layer to immediately retry the requeued
commands, which __ata_scsi_queuecmd() will accept and re-defer, keeping
the host busy.

Furthermore, for the timed-out command itself (which is skipped by the
requeue logic), ata_scsi_deferred_qc_work() can run concurrently:

drivers/ata/libata-scsi.c:ata_scsi_deferred_qc_work() {
    ...
    qc = link->deferred_qc;
    if (qc && !ata_port_eh_scheduled(ap)) {
        WARN_ON_ONCE(ap->ops->qc_defer(qc));
        link->deferred_qc = NULL;
        ata_qc_issue(ap, qc);
    }
    ...
}

Since ata_port_eh_scheduled() evaluates to false, will this work item
erroneously issue the timed-out command to the hardware right as it enters
error handling?

When EH finally runs, it would find the timed-out command actively executing
in hardware instead of deferred, triggering a full port freeze and reset.
Can this defeat the patch's intended optimization of cleanly terminating the
timeout without a reset?

> +
> +	/*
> +	 * Let scsi_timeout() know that it must continue with handling the
> +	 * timeout as we in fact did not do much here.
> +	 */
> +	return SCSI_EH_NOT_HANDLED;
> +}

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