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