Re: [PATCH 14/24] xfs: support caching rtgroup metadata inodes

[Dave Chinner <david@fromorbit.com>]

On Thu, Aug 22, 2024 at 05:18:18PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Create the necessary per-rtgroup infrastructure that we need to load
> metadata inodes into memory.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  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....


> +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?

> +	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@fromorbit.com