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

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

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.

> Otherwise we would error out an otherwise fine I/O (which we _could_ 
> have handled via PREFLUSH etc semantics), which I don't think is a good 
> idea.

Well no. Given that there is no FUA read command for the non-ncq case,
doing the same as for a write FUA in reverse order (synchronize cache
first, then read) would still not necessarily force the drive to access
the media because synchronize cache is *not* and "invalidate cache"
operation. So we cannot use the block layer either as we potentially would
end up lying about the media access part of "FUA". With that in mind,
failing the FUA read is a much safer option I think.

What we could do given that we now have FUA restricted to NCQ is this:

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 77a7be74e65e..61e449877d8d 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -693,7 +693,7 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64
block, u32 n_block,
        tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
        tf->flags |= tf_flags;

-       if (ata_ncq_enabled(dev)) {
+       if (ata_ncq_enabled(dev) || tf->flags & ATA_TFLAG_FUA) {
                /* yay, NCQ */
                if (!lba_48_ok(block, n_block))
                        return -ERANGE;

That is, ignore if NCQ is off and always use NCQ read/write for FUA.
I am not a huge fan of this as that would lead to mixing NCQ and non-NCQ
commands when the drive QD is set to 1. Not exactly nice, which is why I
did not initially modify the patch to do that.
However, with this change, we would be fully on par with scsi and can do
both read and write FUA with the same semantic, as expected from the user
if we declare that we support FUA.

This hunk:

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) {

Would then not really be needed.

Thoughts ?

> 
> Cheers,
> 
> Hannes

-- 
Damien Le Moal
Western Digital Research