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

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Tue, Oct 25, 2022 at 09:05:02AM +0200, Hannes Reinecke wrote:
> On 10/25/22 01: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.
> > 
> Now you got me confused.
> What exactly would be the semantics of a READ request with the FUA bit set?
> Ignore the cache and read from disk?

The opposite:
"If the device holds a modified copy of the requested data as a result of
having volatile cached writes, the modified data shall be written to the
non-volatile media before being retrieved from the non-volatile media as
part of this operation."


Kind regards,
Niklas