diff mbox series

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

Message ID 20210714041912.2625692-7-david@fromorbit.com (mailing list archive)
State Superseded
Headers show
Series xfs: rework feature flags | expand

Commit Message

Dave Chinner July 14, 2021, 4:19 a.m. UTC 
From: Dave Chinner <dchinner@redhat.com>

This provides separation of mount time feature flags from runtime
mount flags and mount option state. It also makes the feature
checks use the same interface as the superblock features. i.e. we
don't care if the feature is enabled by superblock flags or mount
options, we just care if it's enabled or not.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_mount.h | 50 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 3 deletions(-)

Comments

Christoph Hellwig July 14, 2021, 7:05 a.m. UTC | #1 
On Wed, Jul 14, 2021 at 02:19:02PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> This provides separation of mount time feature flags from runtime
> mount flags and mount option state. It also makes the feature
> checks use the same interface as the superblock features. i.e. we
> don't care if the feature is enabled by superblock flags or mount
> options, we just care if it's enabled or not.

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..
Dave Chinner July 14, 2021, 9:55 a.m. UTC | #2 
On Wed, Jul 14, 2021 at 08:05:40AM +0100, Christoph Hellwig wrote:
> On Wed, Jul 14, 2021 at 02:19:02PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > This provides separation of mount time feature flags from runtime
> > mount flags and mount option state. It also makes the feature
> > checks use the same interface as the superblock features. i.e. we
> > don't care if the feature is enabled by superblock flags or mount
> > options, we just care if it's enabled or not.
> 
> 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...

Cheers,

Dave.
Darrick J. Wong July 14, 2021, 11:02 p.m. UTC | #3 
On Wed, Jul 14, 2021 at 02:19:02PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> This provides separation of mount time feature flags from runtime
> mount flags and mount option state. It also makes the feature
> checks use the same interface as the superblock features. i.e. we
> don't care if the feature is enabled by superblock flags or mount
> options, we just care if it's enabled or not.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_mount.h | 50 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 8c0f928febac..b0e8c3825ce8 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -258,6 +258,25 @@ typedef struct xfs_mount {
>  #define XFS_FEAT_BIGTIME	(1ULL << 24)	/* large timestamps */
>  #define XFS_FEAT_NEEDSREPAIR	(1ULL << 25)	/* needs xfs_repair */
>  
> +/* Mount features */
> +#define XFS_FEAT_NOATTR2	(1ULL << 48)	/* disable attr2 creation */
> +#define XFS_FEAT_NOALIGN	(1ULL << 49)	/* ignore alignment */
> +#define XFS_FEAT_ALLOCSIZE	(1ULL << 50)	/* user specified allocation size */
> +#define XFS_FEAT_LARGE_IOSIZE	(1ULL << 51)	/* report large preferred
> +						 * I/O size in stat() */
> +#define XFS_FEAT_WSYNC		(1ULL << 52)	/* synchronous metadata ops */
> +#define XFS_FEAT_DIRSYNC	(1ULL << 53)	/* synchronous directory ops */
> +#define XFS_FEAT_DISCARD	(1ULL << 54)	/* discard unused blocks */
> +#define XFS_FEAT_GRPID		(1ULL << 55)	/* group-ID assigned from directory */
> +#define XFS_FEAT_SMALL_INUMS	(1ULL << 56)	/* user wants 32bit inodes */
> +#define XFS_FEAT_IKEEP		(1ULL << 57)	/* keep empty inode clusters*/
> +#define XFS_FEAT_SWALLOC	(1ULL << 58)	/* stripe width allocation */
> +#define XFS_FEAT_FILESTREAMS	(1ULL << 59)	/* use filestreams allocator */
> +#define XFS_FEAT_DAX_ALWAYS	(1ULL << 60)	/* DAX always enabled */
> +#define XFS_FEAT_DAX_NEVER	(1ULL << 61)	/* DAX never enabled */
> +#define XFS_FEAT_NORECOVERY	(1ULL << 62)	/* no recovery - dirty fs */
> +#define XFS_FEAT_NOUUID		(1ULL << 63)	/* ignore uuid during mount */
> +
>  #define __XFS_HAS_FEAT(name, NAME) \
>  static inline bool xfs_has_ ## name (struct xfs_mount *mp) \
>  { \
> @@ -273,6 +292,7 @@ static inline void xfs_add_ ## name (struct xfs_mount *mp) \
>  	xfs_sb_version_add ## name(&mp->m_sb); \
>  }
>  
> +/* Superblock features */
>  __XFS_ADD_FEAT(attr, ATTR)
>  __XFS_HAS_FEAT(nlink, NLINK)
>  __XFS_ADD_FEAT(quota, QUOTA)
> @@ -296,9 +316,33 @@ __XFS_HAS_FEAT(reflink, REFLINK)
>  __XFS_HAS_FEAT(sparseinodes, SPINODES)
>  __XFS_HAS_FEAT(metauuid, META_UUID)
>  __XFS_HAS_FEAT(realtime, REALTIME)
> -__XFS_HAS_FEAT(inobtcounts, REALTIME)
> -__XFS_HAS_FEAT(bigtime, REALTIME)
> -__XFS_HAS_FEAT(needsrepair, REALTIME)
> +__XFS_HAS_FEAT(inobtcounts, INOBTCNT)
> +__XFS_HAS_FEAT(bigtime, BIGTIME)
> +__XFS_HAS_FEAT(needsrepair, NEEDSREPAIR)

This ought to go in the previous patch.

Otherwise looks fine to me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> +
> +/*
> + * Mount features
> + *
> + * These do not change dynamically - features that can come and go,
> + * such as 32 bit inodes and read-only state, are kept as flags rather than
> + * features.
> + */
> +__XFS_HAS_FEAT(noattr2, NOATTR2)
> +__XFS_HAS_FEAT(noalign, NOALIGN)
> +__XFS_HAS_FEAT(allocsize, ALLOCSIZE)
> +__XFS_HAS_FEAT(large_iosize, LARGE_IOSIZE)
> +__XFS_HAS_FEAT(wsync, WSYNC)
> +__XFS_HAS_FEAT(dirsync, DIRSYNC)
> +__XFS_HAS_FEAT(discard, DISCARD)
> +__XFS_HAS_FEAT(grpid, GRPID)
> +__XFS_HAS_FEAT(small_inums, SMALL_INUMS)
> +__XFS_HAS_FEAT(ikeep, IKEEP)
> +__XFS_HAS_FEAT(swalloc, SWALLOC)
> +__XFS_HAS_FEAT(filestreams, FILESTREAMS)
> +__XFS_HAS_FEAT(dax_always, DAX_ALWAYS)
> +__XFS_HAS_FEAT(dax_never, DAX_NEVER)
> +__XFS_HAS_FEAT(norecovery, NORECOVERY)
> +__XFS_HAS_FEAT(nouuid, NOUUID)
>  
>  /*
>   * Flags for m_flags.
> -- 
> 2.31.1
>
Christoph Hellwig July 15, 2021, 5:59 a.m. UTC | #4 
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?
Dave Chinner July 15, 2021, 11:43 p.m. UTC | #5 
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.
diff mbox series

Patch

diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 8c0f928febac..b0e8c3825ce8 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -258,6 +258,25 @@  typedef struct xfs_mount {
 #define XFS_FEAT_BIGTIME	(1ULL << 24)	/* large timestamps */
 #define XFS_FEAT_NEEDSREPAIR	(1ULL << 25)	/* needs xfs_repair */
 
+/* Mount features */
+#define XFS_FEAT_NOATTR2	(1ULL << 48)	/* disable attr2 creation */
+#define XFS_FEAT_NOALIGN	(1ULL << 49)	/* ignore alignment */
+#define XFS_FEAT_ALLOCSIZE	(1ULL << 50)	/* user specified allocation size */
+#define XFS_FEAT_LARGE_IOSIZE	(1ULL << 51)	/* report large preferred
+						 * I/O size in stat() */
+#define XFS_FEAT_WSYNC		(1ULL << 52)	/* synchronous metadata ops */
+#define XFS_FEAT_DIRSYNC	(1ULL << 53)	/* synchronous directory ops */
+#define XFS_FEAT_DISCARD	(1ULL << 54)	/* discard unused blocks */
+#define XFS_FEAT_GRPID		(1ULL << 55)	/* group-ID assigned from directory */
+#define XFS_FEAT_SMALL_INUMS	(1ULL << 56)	/* user wants 32bit inodes */
+#define XFS_FEAT_IKEEP		(1ULL << 57)	/* keep empty inode clusters*/
+#define XFS_FEAT_SWALLOC	(1ULL << 58)	/* stripe width allocation */
+#define XFS_FEAT_FILESTREAMS	(1ULL << 59)	/* use filestreams allocator */
+#define XFS_FEAT_DAX_ALWAYS	(1ULL << 60)	/* DAX always enabled */
+#define XFS_FEAT_DAX_NEVER	(1ULL << 61)	/* DAX never enabled */
+#define XFS_FEAT_NORECOVERY	(1ULL << 62)	/* no recovery - dirty fs */
+#define XFS_FEAT_NOUUID		(1ULL << 63)	/* ignore uuid during mount */
+
 #define __XFS_HAS_FEAT(name, NAME) \
 static inline bool xfs_has_ ## name (struct xfs_mount *mp) \
 { \
@@ -273,6 +292,7 @@  static inline void xfs_add_ ## name (struct xfs_mount *mp) \
 	xfs_sb_version_add ## name(&mp->m_sb); \
 }
 
+/* Superblock features */
 __XFS_ADD_FEAT(attr, ATTR)
 __XFS_HAS_FEAT(nlink, NLINK)
 __XFS_ADD_FEAT(quota, QUOTA)
@@ -296,9 +316,33 @@  __XFS_HAS_FEAT(reflink, REFLINK)
 __XFS_HAS_FEAT(sparseinodes, SPINODES)
 __XFS_HAS_FEAT(metauuid, META_UUID)
 __XFS_HAS_FEAT(realtime, REALTIME)
-__XFS_HAS_FEAT(inobtcounts, REALTIME)
-__XFS_HAS_FEAT(bigtime, REALTIME)
-__XFS_HAS_FEAT(needsrepair, REALTIME)
+__XFS_HAS_FEAT(inobtcounts, INOBTCNT)
+__XFS_HAS_FEAT(bigtime, BIGTIME)
+__XFS_HAS_FEAT(needsrepair, NEEDSREPAIR)
+
+/*
+ * Mount features
+ *
+ * These do not change dynamically - features that can come and go,
+ * such as 32 bit inodes and read-only state, are kept as flags rather than
+ * features.
+ */
+__XFS_HAS_FEAT(noattr2, NOATTR2)
+__XFS_HAS_FEAT(noalign, NOALIGN)
+__XFS_HAS_FEAT(allocsize, ALLOCSIZE)
+__XFS_HAS_FEAT(large_iosize, LARGE_IOSIZE)
+__XFS_HAS_FEAT(wsync, WSYNC)
+__XFS_HAS_FEAT(dirsync, DIRSYNC)
+__XFS_HAS_FEAT(discard, DISCARD)
+__XFS_HAS_FEAT(grpid, GRPID)
+__XFS_HAS_FEAT(small_inums, SMALL_INUMS)
+__XFS_HAS_FEAT(ikeep, IKEEP)
+__XFS_HAS_FEAT(swalloc, SWALLOC)
+__XFS_HAS_FEAT(filestreams, FILESTREAMS)
+__XFS_HAS_FEAT(dax_always, DAX_ALWAYS)
+__XFS_HAS_FEAT(dax_never, DAX_NEVER)
+__XFS_HAS_FEAT(norecovery, NORECOVERY)
+__XFS_HAS_FEAT(nouuid, NOUUID)
 
 /*
  * Flags for m_flags.