On Wed, Oct 16, 2024 at 10:03:24AM +1100, Dave Chinner wrote:
> On Tue, Oct 15, 2024 at 11:25:41AM -0700, Darrick J. Wong wrote:
> > > > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> > > > index be7d4b26aaea3f..4b36dc2c9bf48b 100644
> > > > --- a/fs/xfs/xfs_inode.h
> > > > +++ b/fs/xfs/xfs_inode.h
> > > > @@ -65,6 +65,7 @@ typedef struct xfs_inode {
> > > > uint16_t i_flushiter; /* incremented on flush */
> > > > };
> > > > uint8_t i_forkoff; /* attr fork offset >> 3 */
> > > > + enum xfs_metafile_type i_metatype; /* XFS_METAFILE_* */
> > > > uint16_t i_diflags; /* XFS_DIFLAG_... */
> > > > uint64_t i_diflags2; /* XFS_DIFLAG2_... */
> > > > struct timespec64 i_crtime; /* time created */
> > > > @@ -276,10 +277,23 @@ static inline bool xfs_is_reflink_inode(const struct xfs_inode *ip)
> > > > return ip->i_diflags2 & XFS_DIFLAG2_REFLINK;
> > > > }
> > > >
> > > > +static inline bool xfs_is_metadir_inode(const struct xfs_inode *ip)
> > > > +{
> > > > + return ip->i_diflags2 & XFS_DIFLAG2_METADATA;
> > > > +}
> > > > +
> > > > static inline bool xfs_is_metadata_inode(const struct xfs_inode *ip)
> > >
> > > Oh, that's going to get confusing. "is_metadir" checks the inode
> > > METADATA flag, and is "is_metadata" checks the superblock METADIR
> > > flag....
> > >
> > > Can we change this to higher level function to
> > > xfs_inode_is_internal() or something else that is not easily
> > > confused with checking an inode flag?
> >
> > But there's already xfs_internal_inum, which only covers sb-rooted
> > metadata inodes. I guess first we have to rename that to xfs_is_sb_inum
> > in a separate patch, and then this one can add xfs_inode_is_internal.
>
> Fine by me.
>
> > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > > index 457c2d70968d9a..59953278964de9 100644
> > > > --- a/fs/xfs/xfs_super.c
> > > > +++ b/fs/xfs/xfs_super.c
> > > > @@ -1733,6 +1733,10 @@ xfs_fs_fill_super(
> > > > mp->m_features &= ~XFS_FEAT_DISCARD;
> > > > }
> > > >
> > > > + if (xfs_has_metadir(mp))
> > > > + xfs_warn(mp,
> > > > +"EXPERIMENTAL metadata directory feature in use. Use at your own risk!");
> > > > +
> > >
> > > We really need a 'xfs_mark_experimental(mp, "Metadata directory")'
> > > function to format all these experimental feature warnings the same
> > > way....
> >
> > We already have xfs_warn_mount for functionality that isn't sb feature
> > bits. Maybe xfs_warn_feat?
>
> xfs_warn_mount() is only used for experimental warnings, so maybe we
> should simply rename that xfs_mark_experiental(). Then we can use
> it's inherent "warn once" behaviour for all the places where we
> issue an experimental warning regardless of how the experimental
> feature is enabled/detected.
>
> This means we'd have a single location that formats all experimental
> feature warnings the same way. Having a single function explicitly
> for this makes it trivial to audit and manage all the experimental
> features supported by a given kernel version because we are no
> longer reliant on grepping for custom format strings to find
> experimental features.
>
> It also means that adding a kernel taint flag indicating that the
> kernel is running experimental code is trivial to do...
...and I guess this means you can discover which forbidden features are
turned on from crash dumps. Ok, sounds good to me.
Do you want it to return an int so that you (as a distributor, not you
personally) can decide that nobody gets to use the experimental
features?
--D
> -Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
