Re: [PATCH v2 0/3] Improve libata support for FUA

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

On 10/25/22 08:26, Damien Le Moal wrote:
> On 10/25/22 07:09, Damien Le Moal wrote:
>> On 10/25/22 03:48, Maciej S. Szmigiero wrote:
>>> On 24.10.2022 09:26, Damien Le Moal wrote:
>>>> These patches cleanup and improve libata support for the FUA device
>>>> feature. Patch 3 enables FUA support by default for any drive that
>>>> reports supporting the feature.
>>>>
>>>> Changes from v1:
>>>>   - Removed Maciej's patch 2. Instead, blacklist drives which are known
>>>>     to have a buggy FUA support.
>>>>
>>>> Damien Le Moal (3):
>>>>    ata: libata: cleanup fua handling
>>>>    ata: libata: blacklist FUA support for known buggy drives
>>>>    ata: libata: Enable fua support by default
>>>>
>>>
>>> Thanks for the updated series.
>>>
>>> In general (besides the small commit message thing that Sergey had
>>> already mentioned) it looks good to me, so:
>>> Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>
>> Thanks. I need to do some more testing using some very old drives I found.
>> So far, no issues: detection works, some drives have FUA, other not. For
>> the ones that have FUA, I am running fstests (ext4 and xfs) to check for
>> weird behavior with REQ_FUA writes. Once I complete all tests I will queue
>> this.
> 
> Actually, I need to take this back. Checking again the code, I found an
> issue with this entire FUA support: for a drive that does not support NCQ,
> or one that has NCQ but has its queue depth set to one, then for a REQ_FUA
> write request, ATA_CMD_WRITE_MULTI_FUA_EXT or ATA_CMD_WRITE_FUA_EXT will
> be used. All good, BUT ! sd.c may also send read requests with the FUA bit
> set if the read request has REQ_FUA set. For read commands, the regular,
> non FUA commands ATA_CMD_READ_MULTI, ATA_CMD_READ_MULTI_EXT, ATA_CMD_READ
> or ATA_CMD_READ_EXT will be used since ATA does not define a FUA version
> of these. This means that the REQ_FUA flag will be ignored: this entire
> code is broken as it is assuming that the read command processing on the
> drive is consistent with executions of ATA_CMD_WRITE_MULTI_FUA_EXT or
> ATA_CMD_WRITE_FUA_EXT. I do not want to bet on that, especially with old
> drives.

Correction here: a REQ_FUA reads would end up being mapped to the "0"
command as returned by ata_rwcmd_protocol(), resulting in the user getting
back an EIO for any FUA read if libata FUA is enabled. With libata fua
disabled that was not happening. Digging further though, I do not see any
in-kernel code using REQ_FUA for reads. But it does not look like that is
forbidden either. Applications using SG_IO may use it anyway (e.g. to do
write verify type operations).

> 
> I would be tempted to restrict FUA support to drives that support NCQ,
> given that with NCQ, both READ FPDMA QUEUED and READ FPDMA WRITE have the
> FUA bit. But then, the problem is that if the user changes the queue depth
> of the drive to 1 through sysfs, ncq is turned off and we are back to
> using the EXT read & write commands, that is, only write has FUA.
> 
> So if we want a solid ata FUA support, we would need to always use NCQ
> regardless of the drive max queue depth setting...
> 
> Thoughts ?
> 
>>
>>>
>>> Thanks,
>>> Maciej
>>>
>>
> 

-- 
Damien Le Moal
Western Digital Research