On 10/15/22 01:44, Maciej S. Szmigiero wrote: > On 8.10.2022 00:41, Damien Le Moal wrote: >> On 10/7/22 23:14, Maciej S. Szmigiero wrote: >>> On 7.10.2022 00:53, Damien Le Moal wrote: >>>> On 10/7/22 07:20, Damien Le Moal wrote: >>>>> On 10/6/22 22:06, Maciej S. Szmigiero wrote: >>>>>> On 6.10.2022 01:38, Damien Le Moal wrote: >>>>>>> On 9/27/22 04:51, Maciej S. Szmigiero wrote: >>>>>>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@xxxxxxxxxx> >>>>>>>> >>>>>>>> Currently, if one wants to make use of FUA support in libata it is >>>>>>>> necessary to provide an explicit kernel command line parameter in order to >>>>>>>> enable it (for drives that report such support). >>>>>>>> >>>>>>>> In terms of Git archaeology: FUA support was enabled by default in early >>>>>>>> libata versions but was disabled soon after. >>>>>>>> Since then there were a few attempts to enable this support by default: >>>>>>>> [1] (for NCQ drives only), [2] (for all drives). >>>>>>>> However, the second change had to be reverted after a report came of >>>>>>>> an incompatibility with the HDD in 2011 Mac Mini. >>>>>>>> >>>>>>>> Enabling FUA avoids having to emulate it by issuing an extra drive cache >>>>>>>> flush for every request that have this flag set. >>>>>>>> Since FUA support is required by the ATA/ATAPI spec for any drive that >>>>>>>> supports LBA48 and so these days should be pretty widespread let's provide >>>>>>>> an ability to enable it by default in Kconfig. >>>>>>> >>>>>>> This can be done by adding "libata.fua=1" to the CONFIG_CMDLINE option. So >>>>>>> I do not see the need to add yet another config option. >>>>>> >>>>>> A specific Kconfig option is more structured than a free-form >>>>>> CONFIG_CMDLINE (which is also technically a per-arch option, but seems >>>>>> to be widely supported across arches). >>>>>> >>>>>> That's why there is a lot (100+) of similar Kconfig default-changing >>>>>> options, a quick sample of these (in no particular order): >>>>>> SOUND_OSS_CORE_PRECLAIM, SND_INTEL_BYT_PREFER_SOF, LSM, >>>>>> SECURITY_SELINUX_CHECKREQPROT_VALUE, SECURITY_LOADPIN_ENFORCE, >>>>>> SECURITY_APPARMOR_DEBUG_MESSAGES, IP_VS_TAB_BITS, IP_SET_MAX, >>>>>> MAC80211_HAS_RC, SLUB_DEBUG_ON, KFENCE_SAMPLE_INTERVAL, PRINTK_TIME, >>>>>> DEBUG_OBJECTS_ENABLE_DEFAULT, RCU_NOCB_CPU_DEFAULT_ALL, ... >>>>>> >>>>>> libata currently has only one similar option: SATA_MOBILE_LPM_POLICY, >>>>>> so it's not like a person performing kernel configuration is >>>>>> overloaded with questions here. >>>>>> >>>>>> But at the same time, I respect your decision as a maintainer of >>>>>> this code. >>>>> >>>>> I am not dead set on pushing back on this, but as usual, whenever we can >>>>> avoid adding config options, we should. Given that libata has had fua >>>>> disabled forever, I am not convinced yet that there is a strong need for >>>>> that new option. But if distros prefer the config option approach, we can >>>>> make that happen. >>>>> >>>>> If anything, I would be tempted to switch fua support to on by default >>>>> after some time if we do not get many reports of broken drives. You did >>>>> mention that old mac minis drives did not like it, but these are not even >>>>> blacklisted in libata-scsi. They should. Only one model of maxtor drives >>>>> is. We should add an ATA_HORKAGE_NO_FUA flag and start a proper blacklist >>>>> of drives not liking fua. Without that in place, switching to on by >>>>> default as your config option allows could break many (old) systems. >>>> >>>> To be extra clear, I think that this fua module parameter is silly. If a >>>> drive says it supports fua, we should use it and not have a global >>>> parameter to disable it for all drives. So no config option needed for it. >>>> >>>> That is also why I am not keen on taking that config option. It is not >>>> really improving anything at all and would prefer nuking the fua module >>>> argument and have a proper blacklisting of buggy drives. >>>> >>>> But such a change is painful as we'll be able to update the blacklist with >>>> users getting corrupted FSes on buggy drives. The time may have come to do >>>> this change though as the number of buggy drives out there is hopefully >>>> small enough now. >>> >>> So your proposal is basically to switch the existing fua option default >>> to "on" and deal with the fallout (hopefully minimal) by blacklisting >>> misbehaving drives as they get reported, right? >> >> Yes. The risk though is that if the fallout are not minimal and we get too >> many bug reports, we'll likely have to revert. So this needs to be >> attempted early at the beginning of a cycle to get plenty of testing. >> >>> In this case, my vote would be to still keep the "libata.fua" parameter >>> available (at least for the time being) so people have some way of >>> working broken drives around without having to recompile their kernel >>> (maybe also print a kernel log message if libata.fua=0 is provided asking >>> people to report these drive models to linux-ide@). >> >> If we add a proper "nofua" horkage flag to blacklist buggy drives, we need >> to move the fua parameter to be an argument of the force parameter so that >> disabling fua can be done per drive (port) or for all drives, similarly to >> other tweak (noncq, nodmalog, etc) > > So would you like an updated patch set or do you prefer to do the changes > yourself? Almost done already :) Need a lot of testing though. Will post the patches when done. Note that for now, I kept the fua module parameter, for compatibility with existing setups. But I added the parameter force=[no]fua which allows doing the same global enable/disable but also per drive enable/disable. And known bad drives can be marked with the horkage flag. > > Thanks, > Maciej > -- Damien Le Moal Western Digital Research
