Re: [PATCH 12/16] xfs: add a generic group pointer to the btree cursor

Dave Chinner
2024-10-15T03:05:29Z

On Thu, Oct 10, 2024 at 05:47:20PM -0700, Darrick J. Wong wrote:
> From: Christoph Hellwig <hch@lst.de>
>
> Replace the pag pointers in the type specific union with a generic
> xfs_group pointer. This prepares for adding realtime group support.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> fs/xfs/libxfs/xfs_alloc.c | 8 ++++----
> fs/xfs/libxfs/xfs_alloc_btree.c | 28 ++++++++++++++--------------
> fs/xfs/libxfs/xfs_btree.c | 35 ++++++++++++-----------------------
> fs/xfs/libxfs/xfs_btree.h | 3 +--
> fs/xfs/libxfs/xfs_btree_mem.c | 6 ++----
> fs/xfs/libxfs/xfs_ialloc.c | 12 +++++++-----
> fs/xfs/libxfs/xfs_ialloc_btree.c | 15 ++++++++-------
> fs/xfs/libxfs/xfs_refcount.c | 17 +++++++++--------
> fs/xfs/libxfs/xfs_refcount_btree.c | 10 +++++-----
> fs/xfs/libxfs/xfs_rmap.c | 8 +++-----
> fs/xfs/libxfs/xfs_rmap_btree.c | 19 ++++++++++---------
> fs/xfs/scrub/alloc.c | 2 +-
> fs/xfs/scrub/bmap.c | 3 ++-
> fs/xfs/scrub/bmap_repair.c | 4 ++--
> fs/xfs/scrub/cow_repair.c | 9 ++++++---
> fs/xfs/scrub/health.c | 2 +-
> fs/xfs/scrub/ialloc.c | 14 +++++++-------
> fs/xfs/scrub/refcount.c | 3 ++-
> fs/xfs/scrub/rmap.c | 2 +-
> fs/xfs/scrub/rmap_repair.c | 2 +-
> fs/xfs/xfs_fsmap.c | 6 ++++--
> fs/xfs/xfs_health.c | 23 ++++++-----------------
> fs/xfs/xfs_trace.h | 28 ++++++++++++++--------------
> 23 files changed, 122 insertions(+), 137 deletions(-)
>
>
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 820ffa6ea6bd75..db25c8ad104206 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -275,7 +275,7 @@ xfs_alloc_complain_bad_rec(
>
> xfs_warn(mp,
> "%sbt record corruption in AG %d detected at %pS!",
> - cur->bc_ops->name, pag_agno(cur->bc_ag.pag), fa);
> + cur->bc_ops->name, cur->bc_group->xg_index, fa);
^^^^^^^^^^^^^^^^^^^^^^^

Reading through this patch, I keep wanting to this to read as "group
number" as a replacement for AG number. i.e. pag_agno(pag) ->
group_num(grp) as the nice, short helper function.

We're kinda used to this with terminology with agno, agbno, fsbno,
ino, agino, etc all refering to the "number" associated with an
object type. Hence it seems kinda natural to refer to these as
group numbers rather than an index into something....

Just an observation, up to you whether you think it's worthwhile or
not.

> diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
> index 843174a5903658..22a65d09e647a3 100644
> --- a/fs/xfs/libxfs/xfs_alloc_btree.c
> +++ b/fs/xfs/libxfs/xfs_alloc_btree.c
> @@ -28,7 +28,7 @@ xfs_bnobt_dup_cursor(
> struct xfs_btree_cur *cur)
> {
> return xfs_bnobt_init_cursor(cur->bc_mp, cur->bc_tp, cur->bc_ag.agbp,
> - cur->bc_ag.pag);
> + to_perag(cur->bc_group));
> }
>
> STATIC struct xfs_btree_cur *
> @@ -36,29 +36,29 @@ xfs_cntbt_dup_cursor(
> struct xfs_btree_cur *cur)
> {
> return xfs_cntbt_init_cursor(cur->bc_mp, cur->bc_tp, cur->bc_ag.agbp,
> - cur->bc_ag.pag);
> + to_perag(cur->bc_group));
> }

Huh. Less than ideal code will be generated for these (group on old
cursor -> perag -> back to group in new cursor) code, but converting
every single bit of the btree cursor code over to groups doesn't
need to be done here...

-Dave.
--
Dave Chinner
david@fromorbit.com