Re: [PATCH V7 07/17] xfs: Introduce XFS_SB_FEAT_INCOMPAT_NREXT64 and associated per-fs feature bit

[Dave Chinner <david@fromorbit.com>]

On Tue, Mar 01, 2022 at 04:09:28PM +0530, Chandan Babu R wrote:
> XFS_SB_FEAT_INCOMPAT_NREXT64 incompat feature bit will be set on filesystems
> which support large per-inode extent counters. This commit defines the new
> incompat feature bit and the corresponding per-fs feature bit (along with
> inline functions to work on it).
> 
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_format.h | 1 +
>  fs/xfs/libxfs/xfs_sb.c     | 3 +++
>  fs/xfs/xfs_mount.h         | 2 ++
>  3 files changed, 6 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index e5654b578ec0..7972cbc22608 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -372,6 +372,7 @@ xfs_sb_has_ro_compat_feature(
>  #define XFS_SB_FEAT_INCOMPAT_META_UUID	(1 << 2)	/* metadata UUID */
>  #define XFS_SB_FEAT_INCOMPAT_BIGTIME	(1 << 3)	/* large timestamps */
>  #define XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR (1 << 4)	/* needs xfs_repair */
> +#define XFS_SB_FEAT_INCOMPAT_NREXT64	(1 << 5)	/* 64-bit data fork extent counter */
>  #define XFS_SB_FEAT_INCOMPAT_ALL \
>  		(XFS_SB_FEAT_INCOMPAT_FTYPE|	\
>  		 XFS_SB_FEAT_INCOMPAT_SPINODES|	\
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index f4e84aa1d50a..bd632389ae92 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -124,6 +124,9 @@ xfs_sb_version_to_features(
>  		features |= XFS_FEAT_BIGTIME;
>  	if (sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR)
>  		features |= XFS_FEAT_NEEDSREPAIR;
> +	if (sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_NREXT64)
> +		features |= XFS_FEAT_NREXT64;
> +
>  	return features;
>  }
>  
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 00720a02e761..10941481f7e6 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -276,6 +276,7 @@ typedef struct xfs_mount {
>  #define XFS_FEAT_INOBTCNT	(1ULL << 23)	/* inobt block counts */
>  #define XFS_FEAT_BIGTIME	(1ULL << 24)	/* large timestamps */
>  #define XFS_FEAT_NEEDSREPAIR	(1ULL << 25)	/* needs xfs_repair */
> +#define XFS_FEAT_NREXT64	(1ULL << 26)	/* 64-bit inode extent counters */
>  
>  /* Mount features */
>  #define XFS_FEAT_NOATTR2	(1ULL << 48)	/* disable attr2 creation */
> @@ -338,6 +339,7 @@ __XFS_HAS_FEAT(realtime, REALTIME)
>  __XFS_HAS_FEAT(inobtcounts, INOBTCNT)
>  __XFS_HAS_FEAT(bigtime, BIGTIME)
>  __XFS_HAS_FEAT(needsrepair, NEEDSREPAIR)
> +__XFS_HAS_FEAT(nrext64, NREXT64)

Not a big fan of "nrext64" naming.

I'd really like the feature macro to be human readable such as:

__XFS_HAS_FEAT(large_extent_counts, NREXT64)

So that it reads like this:

	if (xfs_has_large_extent_counts(mp)) {
		.....
	}

because then the code is much easier to read and is largely self
documenting. In this case, I don't really care about the flag names
(they can remain NREXT64) because they are only seen deep down in
the code.  But for (potentially complex) conditional logic, the
clarity of human readable names makes a big difference.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com