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