Re: [PATCH 06/16] xfs: consolidate mount option features in m_features

[Dave Chinner <david@fromorbit.com>]

On Thu, Jul 15, 2021 at 06:59:19AM +0100, Christoph Hellwig wrote:
> On Wed, Jul 14, 2021 at 07:55:07PM +1000, Dave Chinner wrote:
> > > What about using a separate field for these?  With this patch we've used
> > > up all 64-bits in the features field, which isn't exactly the definition
> > > of future proof..
> > 
> > I've used 16 mount option flags and 26 sb feature flags in this
> > patch set, so there's still 22 feature flags remaining before we
> > need to split them. This is all in-memory stuff so it's easy to
> > modify in future. Given that the flag sets are largely set in only
> > one place each and the check functions are all macro-ised, splitting
> > them when we do run out of bits is trivial.
> > 
> > I'm more interested in trying to keep the cache footprint of
> > frequently accessed read-only data down to a minimum right now,
> > which is why I aggregated them in the first place...
> 
> Oh, I missed the hole in the middle.  Still not sure if mixing up mount
> and on-disk flags entirely is something I'm fully comfortable with.  What
> do you think of at least marking the mount options in the name?

Then stuff like the XFS_FEAT_ATTR2 logic doesn't work - the
simplifications that this patchset introduces ends up relying on
both the mount option and the on-disk flag using the same feature
flag to enable creation of ATTR2 features. That becomes more complex
and error prone if we go and separate them back out again.

In reality, how often do you actually care whether a feature was set
by sb bit, mount option, etc, outside of the actual code that
manipulates the feature bit? For me, the answer is almost always
"never".

And that is the whole point of this patchset: the code doing the
feature checking does not care whether a feature is specified by
on-disk flag, mount option, sysctl or sysfs variable.

This change is intended to unify the feature checking under a single
consistent API that has minimal runtime overhead and is independent
of how the feature is managed. Encoded where the feature came from
into the name is a step sideways from the current code, not a step
forwards.

Indeed, what happens when I start adding feature flags for boolean
features that are currently specified by sysfs or proc variables?
e.g: mp->m_always_cow should be a feature flag, not a
boolean variable. This implementation is a third way we specify
mount features, and while these don't fit into either on-disk or mount
flags, they are feature flags that should be brought in under the
unified feature/opstate interfaces.

Hence I don't think encoding whether the feature came from into the
xfs_has_foo() interfaces or the flag names themselves really
provides any benefit. It's not necessary to use the interfaces, and
beyond the code that sets/clears the feature, nobody cares how it
was set/cleared...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com