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

On 7/10/26 02:56, Niklas Cassel wrote:
> 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.)

Yes, you are right. For a timedout command, we always get +1 to the scsi host
failed counter, so we do not need to do anything special to decrement the busy
counter for a deferred QC when it times out.

As for the change, since ata_scsi_cmd_error_handler() already correctly handles
timed-out deferred QCs, when we have scmd == the deferred QC, we should just
not do anything at all in ata_scsi_requeue_deferred_qc().
ata_scsi_cmd_error_handler() will take care of it.

Sending a v2 with that change.

-- 
Damien Le Moal
Western Digital Research