Re: [PATCH v3 2/2] ata: libata-scsi: avoid passthrough command starvation

[Damien Le Moal <dlemoal@kernel.org>]

On 12/31/25 20:17, Niklas Cassel wrote:
>> Implement such forward progress guarantee by limiting requeueing of
>> non-NCQ commands: when a non-NCQ command is received and NCQ commands
>> are in-flight, do not force a requeue of the non-NCQ command by
>> returning SCSI_MLQUEUE_XXX_BUSY in ata_scsi_translate() and instead
>> hold on to the qc using the new deferred_qc field of struct ata_port.
>>
>> This deferred qc will be issued using the work item deferred_qc_work
>> running the function ata_scsi_deferred_qc_work() once all in-flight
>> commands complete, which is checked with the port qc_defer() callback
>> indicating that no further delay is necessary. This check is done using
>> the helper function ata_scsi_schedule_deferred_qc() which is called from
>> ata_scsi_qc_complete(). This thus excludes this mechanism from all
>> internal non-NCQ commands issued by ATA EH.
>>
>> When a port deferred_qc is non NULL, that is, the port has a command
>> waiting for the device queue to drain, the issuing of all incoming
>> commands (both NCQ and non-NCQ) is deferred using the regular busy
>> mechanism. This simplifies the code and also avoids potential denial of
>> service problems if a user issues too many non-NCQ passthrough commands.
> 
> Instead of introducing a workqueue, did you try/consider to do it the same
> way that we handle CDL commands that completed without error:
> https://github.com/torvalds/linux/blob/v6.19-rc3/drivers/ata/libata-core.c#L4971-L4988
> 
> I.e., in qc_defer(), if we try to queue a non-NCQ command while there
> are NCQ commands active, set ATA_PFLAG_EH_PENDING so that we do not
> trigger fast drain (we wait for the active commands to finish),
> and then call ata_qc_schedule_eh() on the non-NCQ qc.

I thought about it but ruled it out so that we do not overload EH with handling
of non-NCQ commands. libata EH is already complicated enough :)

> Sure, for passthrough commands specifically, SCSI will not want to
> retry the command, because scsi_noretry_cmd() will return true:
> https://github.com/torvalds/linux/blob/v6.19-rc3/drivers/scsi/scsi_error.c#L2227
> 
> But this could easily be solved by e.g. modifying scsi_noretry_cmd()
> to add an additional "case DID_REQUEUE: return false;"
> 
> And also set set_host_byte(scmd, DID_REQUEUE); (probably based on a
> new ATA_QCFLAG_DEFER flag or simiar), after we have cleared DID_TIME_OUT
> using set_host_byte(scmd, DID_OK):
> https://github.com/torvalds/linux/blob/v6.19-rc3/drivers/ata/libata-eh.c#L640

You are kind of proving my point here: I feel that is all much more complicated
than using the libata internal deferred qc work. Also, involving the SCSI layer
about non-NCQ commands deferral is kind of a layering violation as the
non-queueable command concept does not exist in SCSI.

> Since this works for CDL commands, I don't see why we shouldn't be able to
> also send a non-NCQ command (if there are NCQ commands active), via SCSI
> EH, so that SCSI itself retries the command, rather than us holding/hiding
> a command from the SCSI queue, by using an additional workqueue in libata.

Sure, we could make that working, but as mentioned above, I feel that is a lot
more complicated.

> (If libata is just a SCSI LLDD, it somehow feels more logical to make use
> of the SCSI queue, rather than to add our own queueing in libata using a
> workqueue.)

libata must act as a SATL and the SAT specifications are clear about what a SATL
implementation can do for non-NCQ commands. Referring to SATA6r01:

6.2.5 Commands the SATL queues internally
If the translation of a SCSI command requires the SATL to send an ATA non-NCQ
command to the ATA device while one or more ATA commands are active, then the
SATL shall:
a) suspend processing of that SCSI command, and:
	1) maintain the SCSI command in a task set;
	2) resume processing of that SCSI command when the ATA device returns command
complete for all ATA commands the SATL has previously sent to the ATA device; and
	3) after that SCSI command completes, resume processing of other SCSI commands;
b) return TASK SET FULL status for that SCSI command; or
c) return BUSY status for that SCSI command.

What you are proposing does not nicely fit. This patch essentially implements
option (a). Option (c) is the current code, which causes the issue for
multi-queue cases (note that AHCI is not affected because it is single queue).
And option (c) would be mostly equivalent to (b) and cause the same issue as
that would involve a requeue too.


-- 
Damien Le Moal
Western Digital Research