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

On Thu, Jul 09, 2026 at 06:14:08PM +0900, Damien Le Moal wrote:
> On 7/9/26 18:00, [email protected] wrote:
> > [Severity: High]
> > Does calling ata_scsi_qc_done() here prematurely complete the timed-out
> > deferred command and free the libata qc?
> 
> Yes it does. But unless it is the deferred QC that suffered the timeout, the
> scsi command will be requeued and retried.

I think Sashiko has a good point.


After this patch:

We will have some deferred QC handling in ata_scsi_eh_timed_out(scmd):

-Sets link->deferred_qc = NULL;

-Cancels the workqueue.

ata_scsi_eh_timed_out() will do the following:
If scmd (the QC that timed out) was the deferred QC:
-Sets DID_REQUEUE on the deferred QC.
Else:
-Sets DID_TIMEOUT on the deferred QC.

-Calls ata_scsi_qc_done(), which frees the deferred QC and calls scsi_done()

-scsi_done() will call scsi_done_internal(cmd, false);
 scsi_done_internal(cmd, false) will set SCMD_STATE_COMPLETE,
 but will defer the actual completion (scsi_complete() to softirq context.
-ata_scsi_eh_timed_out() will return SCSI_EH_NOT_HANDLED to scsi_timeout(),
 which will break; and will then evaluate if SCMD_STATE_COMPLETE is set,
 if it is, it will do nothing.
 (If it is not set it will add the scmd to the list of failed scmds.)



In case scmd == the deferred QC, since scsi_done_internal() will set
SCMD_STATE_COMPLETE, before deferring the completion to softirq context,
the code in scsi_timeout() will not add the scmd to the error list using
scsi_eh_scmd_add(), instead it will return BLK_EH_DONE;


Thus, Sashikos comment can not happen in realtity, because if the deferred
QC timed out, it will never be added to the list of scmds which
ata_scsi_cmd_error_handler() will loop over.


I think it would be cleaner if we:

1) Modify ata_scsi_eh_timed_out():
if scmd == the deferred QC, set DID_TIMEOUT, but return SCSI_EH_DONE.
This way it is more obvious that no further EH will be done. (Instead of
relying on SCMD_STATE_COMPLETE already have been set, even though the
completion was deferred to softirq context.)

If scmd != the deferred QC, continue to set DID_REQUEUE and return
SCSI_EH_NOT_HANDLED.




2) If it was a timeout of the deferred QC, then we know that
ata_scsi_eh_timed_out() has been called, before ata_scsi_cmd_error_handler()
is called.

On NCQ error, while having a deferred non-NCQ QC,
ata_scsi_cmd_error_handler() will be called without ata_scsi_eh_timed_out()
having been called first.

Thus, we should be able to remove the code which specifically handles
the case where the deferred QC timed out in ata_scsi_eh_timed_out().

If the deferred QC timed out, ata_scsi_eh_timed_out() must have been called
with the timed out deferred QC as argument. And ata_scsi_eh_timed_out() will
set link->deferred_qc = NULL; so the code which handles the deferred QC timing
out, should now be dead code that can never be reached.

(On a hard NCQ error, the deferred QC will be requeued using:
ata_do_link_abort() -> ata_eh_set_pending() -> ata_scsi_requeue_deferred_qc(),
which sets link->deferred_qc = NULL; cancels the workqueue and calls
ata_scsi_qc_done(qc, true, DID_REQUEUE << 16))


Kind regards,
Niklas