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...
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
