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. 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
