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

On 7/21/26 16:00, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Missing call to ata_scsi_schedule_deferred_qc() in atapi_qc_complete() leads to starvation and timeouts of deferred commands.
> --
> 
> --- Patch [1]: [PATCH v7 1/2] ata: libata-scsi: terminate deferred commands on time out ---
> commit 3b26adffba8da435c70bdca03b617603f9f9ae76
> Author: Damien Le Moal <[email protected]>
> 
> ata: libata-scsi: terminate deferred commands on time out
> 
> This commit avoids unnecessary SCSI EH task wake-ups by scheduling
> a retry of all waiting deferred QCs using the new ata_scsi_eh_timed_out
> operation. It also updates ata_scsi_requeue_deferred_qc to terminate
> timed-out deferred commands directly and schedule libata EH to prevent
> the block layer from re-issuing them immediately.
> 
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 5868526301a2b..c2f2f711fd987 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
> 
> [ ... ]
> 
>> @@ -2936,7 +3014,15 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
>>  		if (qc->cdb[0] == ALLOW_MEDIUM_REMOVAL && qc->dev->sdev)
>>  			qc->dev->sdev->locked = 0;
>>  
>> -		ata_scsi_qc_done(qc, true, SAM_STAT_CHECK_CONDITION);
>> +		if (cmd->result)
>> +			ata_scsi_qc_done(qc, false, 0);
>> +		else
>> +			ata_scsi_qc_done(qc, true, SAM_STAT_CHECK_CONDITION);
>> +		return;
>> +	}
>> +
>> +	if (cmd->result) {
>> +		ata_scsi_qc_done(qc, false, 0);
>>  		return;
>>  	}
>>  
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but should atapi_qc_complete()
> be calling ata_scsi_schedule_deferred_qc(link) before returning?
> 
> Unlike ata_scsi_qc_complete() which kicks deferred commands,
> atapi_qc_complete() misses this call on all of its exit paths, including
> the ones modified here. If an ATAPI command is executing on a shared PATA
> link and a concurrent command is issued to the other device, it gets
> deferred by ata_std_qc_defer(). When the ATAPI command finishes, it
> appears the deferred command will be trapped in link->deferred_qc and
> never issued, eventually failing with DID_TIME_OUT and causing device hangs.
> 
> Could this function be missing a call to ata_scsi_schedule_deferred_qc()
> to prevent starvation?

Yes. Will send another patch for that.

-- 
Damien Le Moal
Western Digital Research