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

On 7/20/26 16:31, [email protected] wrote:
> 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 <[email protected]>
> 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:

Somehow, this patch seems to be working, but yet, in view of this comment, I do
not see how the host busy counter is decremented for the deferred QCs. Something
is missing. We somehow need to call scsi_complete() with a status that leads to
the command disposition to fall to the default scsi_eh_scmd_add() so that we do
not immediately retry the command, since that also does not work (previous
version issue).

Spending the weekend on this was a waste of time. Back to the drawing board on this.

-- 
Damien Le Moal
Western Digital Research