Re: [PATCH v3 4/6] ata: libata: Fix FUA handling in ata_build_rw_tf()

Hannes Reinecke
2022-10-28T10:01:24Z

On 10/28/22 00:22, Damien Le Moal wrote:
> On 10/27/22 18:42, Hannes Reinecke wrote:
>> On 10/27/22 09:50, Damien Le Moal wrote:
>>> If a user issues a write command with the FUA bit set for a device with
>>> NCQ support disabled (that is, the device queue depth was set to 1), the
>>> LBA 48 command WRITE DMA FUA EXT must be used. However,
>>> ata_build_rw_tf() ignores this and first test if LBA 28 can be used.
>>> That is, for small FUA writes at low LBAs, ata_rwcmd_protocol() will
>>> cause the write to fail.
>>>
>>> Fix this by preventing the use of LBA 28 for any FUA write request.
>>> While at it, also early test if the request is a FUA read and fail these
>>> requests for the NCQ-disabled case instead of relying on
>>> ata_rwcmd_protocol() returning an error.
>>>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>> ---
>>> drivers/ata/libata-core.c | 17 +++++++++++++++--
>>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>> index 81b20ffb1554..fea06f41f371 100644
>>> --- a/drivers/ata/libata-core.c
>>> +++ b/drivers/ata/libata-core.c
>>> @@ -725,9 +725,21 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 block, u32 n_block,
>>> class == IOPRIO_CLASS_RT)
>>> tf->hob_nsect |= ATA_PRIO_HIGH << ATA_SHIFT_PRIO;
>>> } else if (dev->flags & ATA_DFLAG_LBA) {
>>> + bool lba28_ok;
>>> +
>>> + if (tf->flags & ATA_TFLAG_FUA) {
>>> + /* FUA reads are not defined */
>>> + if (!(tf->flags & ATA_TFLAG_WRITE))
>>> + return -EINVAL;
>>> + /* We need LBA48 / WRITE DMA FUA EXT for FUA writes */
>>> + lba28_ok = false;
>>> + } else {
>>> + lba28_ok = lba_28_ok(block, n_block);
>>> + }
>>> +
>>> tf->flags |= ATA_TFLAG_LBA;
>>>
>>> - if (lba_28_ok(block, n_block)) {
>>> + if (lba28_ok) {
>>> /* use LBA28 */
>>> tf->device |= (block >> 24) & 0xf;
>>> } else if (lba_48_ok(block, n_block)) {
>>
>> I am still skeptical about this change.
>> Having checked the code I don't think that we ever issue a
>> REQ_READ|REQ_FUA; but at the same time there doesn't seem to be a strict
>> rule. I wonder if we shouldn't move that check into the block layer, and
>> error out any attempts to issue such?
>
> That definitely would be good for ATA, but potentially restrictive for
> scsi ? Not sure about NVMe, I have not checked the specs. That said, the
> only reasonable reason to do an FUA read that I can think of would be a
> "scrub" like write-and-verify feature. And I do not think that any FS
> implement their scrub process using FUA.
>
But that was the point.
_If_ the block layer never issues a REQ_READ|REQ_FUA we could make this
a restriction of the block layer, and would be perfectly fine to error
that out in the libata stack, too.

So we wouldn't need to worry on how to map that command, seeing that
we'll never get it.

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer