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

On 7/10/26 09:24, [email protected] wrote:
>> @@ -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?

Maybe. But that race exists since eons ago. And command timeouts are rare, so I
do not consider this to be "critical" at all.

>> +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.

No it will not because this is called with SCSI EH already waiting to run, so
newly incoming commands are not passed down by the scsi layer.

-- 
Damien Le Moal
Western Digital Research