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@xxxxxxxxxx> > > > > > > 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
