Re: [PATCH v1 14/17] xfs: Add the parent pointer support to the superblock version 5.

[Dave Chinner <david@fromorbit.com>]

On Sat, Jun 11, 2022 at 02:41:57AM -0700, Allison Henderson wrote:
> [dchinner: forward ported and cleaned up]
> [achender: rebased and added parent pointer attribute to
>            compatible attributes mask]
> 
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_format.h | 14 +++++++++-----
>  fs/xfs/libxfs/xfs_fs.h     |  1 +
>  fs/xfs/libxfs/xfs_sb.c     |  2 ++
>  fs/xfs/xfs_super.c         |  4 ++++
>  4 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index 96976497306c..e85d6b643622 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -83,6 +83,7 @@ struct xfs_ifork;
>  #define	XFS_SB_VERSION2_OKBITS		\
>  	(XFS_SB_VERSION2_LAZYSBCOUNTBIT	| \
>  	 XFS_SB_VERSION2_ATTR2BIT	| \
> +	 XFS_SB_VERSION2_PARENTBIT	| \
>  	 XFS_SB_VERSION2_PROJID32BIT	| \
>  	 XFS_SB_VERSION2_FTYPE)

No need for a v4 filesystem format feature bit - this is v4 only.

>  
> @@ -353,11 +354,13 @@ xfs_sb_has_compat_feature(
>  #define XFS_SB_FEAT_RO_COMPAT_RMAPBT   (1 << 1)		/* reverse map btree */
>  #define XFS_SB_FEAT_RO_COMPAT_REFLINK  (1 << 2)		/* reflinked files */
>  #define XFS_SB_FEAT_RO_COMPAT_INOBTCNT (1 << 3)		/* inobt block counts */
> +#define XFS_SB_FEAT_RO_COMPAT_PARENT	(1 << 4)		/* parent inode ptr */
>  #define XFS_SB_FEAT_RO_COMPAT_ALL \
> -		(XFS_SB_FEAT_RO_COMPAT_FINOBT | \
> -		 XFS_SB_FEAT_RO_COMPAT_RMAPBT | \
> -		 XFS_SB_FEAT_RO_COMPAT_REFLINK| \
> -		 XFS_SB_FEAT_RO_COMPAT_INOBTCNT)
> +		(XFS_SB_FEAT_RO_COMPAT_FINOBT  | \
> +		 XFS_SB_FEAT_RO_COMPAT_RMAPBT  | \
> +		 XFS_SB_FEAT_RO_COMPAT_REFLINK | \
> +		 XFS_SB_FEAT_RO_COMPAT_INOBTCNT| \
> +		 XFS_SB_FEAT_RO_COMPAT_PARENT)

I'm not sure this is a RO Compat feature - we added an attribute
namespace flag on disk, and the older kernels do not know about
that (i.e. we changed XFS_ATTR_NSP_ONDISK_MASK). This may result in
parent pointer attrs being exposed as user attrs rather than being
hidden, or maybe parent pointer attrs being seen as corrupt because
they have a flag that isn't defined set, etc.

Hence I'm not sure that this classification is correct.

>  #define XFS_SB_FEAT_RO_COMPAT_UNKNOWN	~XFS_SB_FEAT_RO_COMPAT_ALL
>  static inline bool
>  xfs_sb_has_ro_compat_feature(
> @@ -392,7 +395,8 @@ xfs_sb_has_incompat_feature(
>  
>  static inline bool xfs_sb_version_hasparent(struct xfs_sb *sbp)
>  {
> -	return false; /* We'll enable this at the end of the set */
> +	return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 &&
> +		(sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_PARENT));
>  }

This should go away and the feature bit in the mount get set by
xfs_sb_version_to_features().

>  #define XFS_SB_FEAT_INCOMPAT_LOG_XATTRS   (1 << 0)	/* Delayed Attributes */
> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> index 1cfd5bc6520a..b0b4d7a3aa15 100644
> --- a/fs/xfs/libxfs/xfs_fs.h
> +++ b/fs/xfs/libxfs/xfs_fs.h
> @@ -237,6 +237,7 @@ typedef struct xfs_fsop_resblks {
>  #define XFS_FSOP_GEOM_FLAGS_BIGTIME	(1 << 21) /* 64-bit nsec timestamps */
>  #define XFS_FSOP_GEOM_FLAGS_INOBTCNT	(1 << 22) /* inobt btree counter */
>  #define XFS_FSOP_GEOM_FLAGS_NREXT64	(1 << 23) /* large extent counters */
> +#define XFS_FSOP_GEOM_FLAGS_PARENT	(1 << 24) /* parent pointers 	    */
>  
>  /*
>   * Minimum and maximum sizes need for growth checks.
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index a20cade590e9..d90b05456dba 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -1187,6 +1187,8 @@ xfs_fs_geometry(
>  		geo->flags |= XFS_FSOP_GEOM_FLAGS_BIGTIME;
>  	if (xfs_has_inobtcounts(mp))
>  		geo->flags |= XFS_FSOP_GEOM_FLAGS_INOBTCNT;
> +	if(xfs_sb_version_hasparent(sbp))

	if (xfs_has_parent_pointers(mp))

> +		geo->flags |= XFS_FSOP_GEOM_FLAGS_PARENT;
>  	if (xfs_has_sector(mp)) {
>  		geo->flags |= XFS_FSOP_GEOM_FLAGS_SECTOR;
>  		geo->logsectsize = sbp->sb_logsectsize;
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index a6e7b4176faf..cbb492fea4a5 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1655,6 +1655,10 @@ xfs_fs_fill_super(
>  		xfs_warn(mp,
>  	"EXPERIMENTAL Large extent counts feature in use. Use at your own risk!");
>  
> +	if (xfs_sb_version_hasparent(&mp->m_sb))

	if (xfs_has_parent_pointers(mp))

> +		xfs_alert(mp,
> +	"EXPERIMENTAL parent pointer feature enabled. Use at your own risk!");
> +
>  	error = xfs_mountfs(mp);
>  	if (error)
>  		goto out_filestream_unmount;
> -- 
> 2.25.1
> 
> 

-- 
Dave Chinner
david@fromorbit.com