On Mon, Aug 26, 2024 at 11:37:34AM -0700, Darrick J. Wong wrote:
> On Mon, Aug 26, 2024 at 11:41:19AM +1000, Dave Chinner wrote:
> > On Thu, Aug 22, 2024 at 05:18:18PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> > >
> > > Create the necessary per-rtgroup infrastructure that we need to load
> > > metadata inodes into memory.
> > >
> > > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > ---
> > > fs/xfs/libxfs/xfs_rtgroup.c | 182 +++++++++++++++++++++++++++++++++++++++++++
> > > fs/xfs/libxfs/xfs_rtgroup.h | 28 +++++++
> > > fs/xfs/xfs_mount.h | 1
> > > fs/xfs/xfs_rtalloc.c | 48 +++++++++++
> > > 4 files changed, 258 insertions(+), 1 deletion(-)
> > >
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_rtgroup.c b/fs/xfs/libxfs/xfs_rtgroup.c
> > > index ae6d67c673b1a..50e4a56d749f0 100644
> > > --- a/fs/xfs/libxfs/xfs_rtgroup.c
> > > +++ b/fs/xfs/libxfs/xfs_rtgroup.c
> > > @@ -30,6 +30,8 @@
> > > #include "xfs_icache.h"
> > > #include "xfs_rtgroup.h"
> > > #include "xfs_rtbitmap.h"
> > > +#include "xfs_metafile.h"
> > > +#include "xfs_metadir.h"
> > >
> > > /*
> > > * Passive reference counting access wrappers to the rtgroup structures. If
> > > @@ -295,3 +297,183 @@ xfs_rtginode_lockdep_setup(
> > > #else
> > > #define xfs_rtginode_lockdep_setup(ip, rgno, type) do { } while (0)
> > > #endif /* CONFIG_PROVE_LOCKING */
> > > +
> > > +struct xfs_rtginode_ops {
> > > + const char *name; /* short name */
> > > +
> > > + enum xfs_metafile_type metafile_type;
> > > +
> > > + /* Does the fs have this feature? */
> > > + bool (*enabled)(struct xfs_mount *mp);
> > > +
> > > + /* Create this rtgroup metadata inode and initialize it. */
> > > + int (*create)(struct xfs_rtgroup *rtg,
> > > + struct xfs_inode *ip,
> > > + struct xfs_trans *tp,
> > > + bool init);
> > > +};
> >
> > What's all this for?
> >
> > AFAICT, loading the inodes into the rtgs requires a call to
> > xfs_metadir_load() when initialising the rtg (either at mount or
> > lazily on the first access to the rtg). Hence I'm not really sure
> > what this complexity is needed for, and the commit message is not
> > very informative....
>
> Yes, the creation and mkdir code in here is really to support growfs,
> mkfs, and repair. How about I change the commit message to:
>
> "Create the necessary per-rtgroup infrastructure that we need to load
> metadata inodes into memory and to create directory trees on the fly.
> Loading is needed by the mounting process. Creation is needed by
> growfs, mkfs, and repair."
IMO it would have been nicer to add this with the patch that
adds growfs support for rtgs. That way the initial inode loading
would be much easier to understand and review, and the rest of it
would have enough context to be able to review it sanely. There
isn't enough context in this patch to determine if the creation code
is sane or works correctly....
> > > + path = xfs_rtginode_path(rtg->rtg_rgno, type);
> > > + if (!path)
> > > + return -ENOMEM;
> > > + error = xfs_metadir_load(tp, mp->m_rtdirip, path, ops->metafile_type,
> > > + &ip);
> > > + kfree(path);
> > > +
> > > + if (error)
> > > + return error;
> > > +
> > > + if (XFS_IS_CORRUPT(mp, ip->i_df.if_format != XFS_DINODE_FMT_EXTENTS &&
> > > + ip->i_df.if_format != XFS_DINODE_FMT_BTREE)) {
> > > + xfs_irele(ip);
> > > + return -EFSCORRUPTED;
> > > + }
> >
> > We don't support LOCAL format for any type of regular file inodes,
> > so I'm a little confiused as to why this wouldn't be caught by the
> > verifier on inode read? i.e. What problem is this trying to catch,
> > and why doesn't the inode verifier catch it for us?
>
> This is really more of a placeholder for more refactorings coming down
> the line for the rtrmap patchset, which will create a new
> XFS_DINODE_FMT_RMAP. At that time we'll need to check that an inode
> that we are loading to be the rmap btree actually has that set.
Ok, can you leave a comment to indicate this so I don't have to
remember why this code exists?
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
