Re: [PATCH RFC v3 2/7] ata: libata-scsi: Add ata_internal_queuecommand()

Damien Le Moal
2022-10-28T08:07:38Z

On 10/28/22 17:01, John Garry wrote:
> On 27/10/2022 23:25, Damien Le Moal wrote:
>>> So we have this overall flow:
>>>
>>> ata_exec_internal_sg():
>>> -> alloc request
>>> -> blk_execute_rq_nowait()
>>> ... -> scsi_queue_rq()
>>> -> sht->reserved_queuecommd()
>>> -> ata_internal_queuecommand()
>>>
>>> And then we have ata_internal_queuecommand() -> ata_sas_queuecmd() ->
>>> ata_scsi_queue_internal() -> ata_qc_issue().
>>>
>>> Hope it makes sense.
>> OK. Got it.
>> However, ata_exec_internal_sg() being used only from EH context with the
>> queue quiesced, will blk_execute_rq_nowait() work ? Is there an exception
>> for internal reserved tags ?
>>
>
> Well, yeah. So if some error happens and EH kicks in, then full queue
> depth of requests may be allocated. I have seen this for NCQ error. So
> this is why I make in very first patch change allow us to allocate
> reserved request from sdev request queue even when budget is fully
> allocated.
>
> Please also note that for AHCI, I make reserved depth =1, while for SAS
> controllers it is greater. This means that in theory we could alloc > 1x
> reserved command for SATA disk, but I don't think it matters.

Yes, 1 is enough. However, is 1 reserved out of 32 total, meaning that the
user can only use 31 tags ? or is it 32+1 reserved ? which we can do since
when using the reserved request, we will not use a hw tag (all reserved
requests will be non-ncq).

The 32 + 1 scheme will work. But for CDL command completion handling, we
will need a NCQ command to do a read log, to avoid forcing a queue drain.
For that to reliably work, we'll need a 31+1+1 setup...

>
> Thanks,
> John

--
Damien Le Moal
Western Digital Research