diff mbox series

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

Message ID 20220611094200.129502-15-allison.henderson@oracle.com (mailing list archive)
State Superseded
Headers show
Series Return of the Parent Pointers | expand

Commit Message

Allison Henderson June 11, 2022, 9:41 a.m. UTC
[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(-)

Comments

Dave Chinner June 16, 2022, 6:03 a.m. UTC | #1
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
> 
>
Allison Henderson June 18, 2022, 12:32 a.m. UTC | #2
On Thu, 2022-06-16 at 16:03 +1000, Dave Chinner wrote:
> 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.
Ok, I ended up having to add this in the rebase or we get an "SB
validate failed".  I think it has to go over in
xfs_sb_validate_v5_features next to the manual crc bit check.  Will
move

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

Gosh, I'm sure there was a reason we did this, but what ever it was
goes all the way back in the first re-appearance of the set back in
2018 and I just cant remember the discussion at the time.  It may have
just been done to get mkfs working and we just never got to reviewing
it.

Should we drop it and just use XFS_SB_VERSION2_PARENTBIT?

> 
> >  #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().
> 
Alrighty

> >  #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))
> 
Will update

> > +		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))
> 
Will update

Thanks for the reviews!
Allison

> > +		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 June 20, 2022, 12:21 a.m. UTC | #3
On Fri, Jun 17, 2022 at 05:32:36PM -0700, Alli wrote:
> On Thu, 2022-06-16 at 16:03 +1000, Dave Chinner wrote:
> > 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.
> Ok, I ended up having to add this in the rebase or we get an "SB
> validate failed".  I think it has to go over in
> xfs_sb_validate_v5_features next to the manual crc bit check.  Will
> move

Ah, I meant that parent pointers are a v5 only feature, and so we
don't need a "v4 only" feature bit for it. As it is, we can't use
that specific bit because SGI shipped a version of parent pointers
on v4 filesystems on IRIX under that feature bit that was broken and
subsequently recalled and killed. Essentially, that means
XFS_SB_VERSION2_PARENTBIT is blacklisted and cannot ever be used by
upstream kernels.

> > > @@ -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.
> 
> Gosh, I'm sure there was a reason we did this, but what ever it was
> goes all the way back in the first re-appearance of the set back in
> 2018 and I just cant remember the discussion at the time.  It may have
> just been done to get mkfs working and we just never got to reviewing
> it.
> 
> Should we drop it and just use XFS_SB_VERSION2_PARENTBIT?

No, it needs to be a v5 feature bit - create a v5 parent pointer
filesystem, create some files on it, and then go an mount it on a
kernel that doesn't have PP support. If you can see the parent
pointer attributes from userspace as "user.<binary garbage>"
attributes, then we need to use an INCOMPAT feature bit rather than
a RO_COMPAT bit.

Cheers,

Dave.
Darrick J. Wong June 29, 2022, 6:16 p.m. UTC | #4
On Mon, Jun 20, 2022 at 10:21:26AM +1000, Dave Chinner wrote:
> On Fri, Jun 17, 2022 at 05:32:36PM -0700, Alli wrote:
> > On Thu, 2022-06-16 at 16:03 +1000, Dave Chinner wrote:
> > > 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.
> > Ok, I ended up having to add this in the rebase or we get an "SB
> > validate failed".  I think it has to go over in
> > xfs_sb_validate_v5_features next to the manual crc bit check.  Will
> > move
> 
> Ah, I meant that parent pointers are a v5 only feature, and so we
> don't need a "v4 only" feature bit for it. As it is, we can't use
> that specific bit because SGI shipped a version of parent pointers
> on v4 filesystems on IRIX under that feature bit that was broken and
> subsequently recalled and killed. Essentially, that means
> XFS_SB_VERSION2_PARENTBIT is blacklisted and cannot ever be used by
> upstream kernels.
> 
> > > > @@ -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.
> > 
> > Gosh, I'm sure there was a reason we did this, but what ever it was
> > goes all the way back in the first re-appearance of the set back in
> > 2018 and I just cant remember the discussion at the time.  It may have
> > just been done to get mkfs working and we just never got to reviewing
> > it.
> > 
> > Should we drop it and just use XFS_SB_VERSION2_PARENTBIT?
> 
> No, it needs to be a v5 feature bit - create a v5 parent pointer
> filesystem, create some files on it, and then go an mount it on a
> kernel that doesn't have PP support. If you can see the parent
> pointer attributes from userspace as "user.<binary garbage>"
> attributes, then we need to use an INCOMPAT feature bit rather than
> a RO_COMPAT bit.

Agreed, this needs to be a v5 feature bit.

If the current kernel ignores xattr leaf entries with namespaces it
doesn't know about, *then* this could be an rocompat feature.
Otherwise, it has to be incompat.

As it is, I think the attr list functions will return *every* xattr
regardless of namespace, so I think it's not safe to let old kernels
mount pptr filesystems even in readonly mode.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
diff mbox series

Patch

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)
 
@@ -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)
 #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));
 }
 
 #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))
+		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))
+		xfs_alert(mp,
+	"EXPERIMENTAL parent pointer feature enabled. Use at your own risk!");
+
 	error = xfs_mountfs(mp);
 	if (error)
 		goto out_filestream_unmount;