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."
> > +static const struct xfs_rtginode_ops xfs_rtginode_ops[XFS_RTGI_MAX] = {
> > +};
> > +
> > +/* Return the shortname of this rtgroup inode. */
> > +const char *
> > +xfs_rtginode_name(
> > + enum xfs_rtg_inodes type)
> > +{
> > + return xfs_rtginode_ops[type].name;
> > +}
> > +
> > +/* Should this rtgroup inode be present? */
> > +bool
> > +xfs_rtginode_enabled(
> > + struct xfs_rtgroup *rtg,
> > + enum xfs_rtg_inodes type)
> > +{
> > + const struct xfs_rtginode_ops *ops = &xfs_rtginode_ops[type];
> > +
> > + if (!ops->enabled)
> > + return true;
> > + return ops->enabled(rtg->rtg_mount);
> > +}
> > +
> > +/* Load and existing rtgroup inode into the rtgroup structure. */
> > +int
> > +xfs_rtginode_load(
> > + struct xfs_rtgroup *rtg,
> > + enum xfs_rtg_inodes type,
> > + struct xfs_trans *tp)
> > +{
> > + struct xfs_mount *mp = tp->t_mountp;
> > + const char *path;
> > + struct xfs_inode *ip;
> > + const struct xfs_rtginode_ops *ops = &xfs_rtginode_ops[type];
> > + int error;
> > +
> > + if (!xfs_rtginode_enabled(rtg, type))
> > + return 0;
> > +
> > + if (!mp->m_rtdirip)
> > + return -EFSCORRUPTED;
> > +
> > + 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.
> > + if (XFS_IS_CORRUPT(mp, ip->i_projid != rtg->rtg_rgno)) {
> > + xfs_irele(ip);
> > + return -EFSCORRUPTED;
> > + }
> > +
> > + xfs_rtginode_lockdep_setup(ip, rtg->rtg_rgno, type);
> > + rtg->rtg_inodes[type] = ip;
> > + return 0;
> > +}
> > +
> > +/* Release an rtgroup metadata inode. */
> > +void
> > +xfs_rtginode_irele(
> > + struct xfs_inode **ipp)
> > +{
> > + if (*ipp)
> > + xfs_irele(*ipp);
> > + *ipp = NULL;
> > +}
> > +
> > +/* Add a metadata inode for a realtime rmap btree. */
> > +int
> > +xfs_rtginode_create(
> > + struct xfs_rtgroup *rtg,
> > + enum xfs_rtg_inodes type,
> > + bool init)
>
> This doesn't seem to belong in this patchset...
>
> ....
>
> > +/* Create the parent directory for all rtgroup inodes and load it. */
> > +int
> > +xfs_rtginode_mkdir_parent(
> > + struct xfs_mount *mp)
>
> Or this...
>
> -Dave.
>
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
