Re: [PATCH 2/2] ata: allow enabling FUA support in Kconfig

[Maciej S. Szmigiero]

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?

Thanks,
Maciej