Re: [PATCH 2/3] xfs_repair: sync bulkload data structures with kernel newbt code

Bill O'Donnell
2024-03-14T16:35:35Z

On Tue, Mar 12, 2024 at 07:14:24PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> A lot of the code in repair/bulkload.c was backwardsported from new code
> that eventually turned into newbt.c in online repair. Since the offline
> repair version got merged upstream years before the online repair code,
> we now need to bring the offline version up to date with the kernel
> again.
>
> Right now, the bulkload.c code is just a fancy way to track space
> extents that are fed to it by its callers. The only caller, of course,
> is phase 5, which builds new btrees in AG space that wasn't claimed by
> any other data structure. Hence there's no need to allocate
> reservations out of the bnobt or put them back there.
>
> However, the next patch adds the ability to generate new file-based
> btrees. For that we need to reorganize the code to allocate and free
> space for new file-based btrees. Let's just crib from the kernel
> version. Make each bulkload space reservation hold a reference to an AG
> and track the space reservation in terms of per-AG extents instead of
> fsblock extents.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Reviewed-by: Bill O'Donnell <bodonnel@redhat.com>

> ---
> libxfs/libxfs_api_defs.h | 1 +
> repair/agbtree.c | 22 +++++++++++-----
> repair/bulkload.c | 63 +++++++++++++++++++++++++++++++++-------------
> repair/bulkload.h | 12 +++++----
> repair/phase5.c | 2 +
> 5 files changed, 69 insertions(+), 31 deletions(-)
>
>
> diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> index 36afc5d0234d..28960317ab6b 100644
> --- a/libxfs/libxfs_api_defs.h
> +++ b/libxfs/libxfs_api_defs.h
> @@ -149,6 +149,7 @@
> #define xfs_log_sb libxfs_log_sb
> #define xfs_mode_to_ftype libxfs_mode_to_ftype
> #define xfs_perag_get libxfs_perag_get
> +#define xfs_perag_hold libxfs_perag_hold
> #define xfs_perag_put libxfs_perag_put
> #define xfs_prealloc_blocks libxfs_prealloc_blocks
>
> diff --git a/repair/agbtree.c b/repair/agbtree.c
> index e014e216e0a5..c6f0512fe7de 100644
> --- a/repair/agbtree.c
> +++ b/repair/agbtree.c
> @@ -77,13 +77,17 @@ reserve_agblocks(
> uint32_t nr_blocks)
> {
> struct extent_tree_node *ext_ptr;
> + struct xfs_perag *pag;
> uint32_t blocks_allocated = 0;
> uint32_t len;
> int error;
>
> + pag = libxfs_perag_get(mp, agno);
> + if (!pag)
> + do_error(_("could not open perag structure for agno 0x%x\n"),
> + agno);
> +
> while (blocks_allocated < nr_blocks) {
> - xfs_fsblock_t fsbno;
> -
> /*
> * Grab the smallest extent and use it up, then get the
> * next smallest. This mimics the init_*_cursor code.
> @@ -94,8 +98,8 @@ reserve_agblocks(
>
> /* Use up the extent we've got. */
> len = min(ext_ptr->ex_blockcount, nr_blocks - blocks_allocated);
> - fsbno = XFS_AGB_TO_FSB(mp, agno, ext_ptr->ex_startblock);
> - error = bulkload_add_blocks(&btr->newbt, fsbno, len);
> + error = bulkload_add_extent(&btr->newbt, pag,
> + ext_ptr->ex_startblock, len);
> if (error)
> do_error(_("could not set up btree reservation: %s\n"),
> strerror(-error));
> @@ -113,6 +117,7 @@ reserve_agblocks(
> fprintf(stderr, "blocks_allocated = %d\n",
> blocks_allocated);
> #endif
> + libxfs_perag_put(pag);
> return blocks_allocated == nr_blocks;
> }
>
> @@ -155,18 +160,21 @@ finish_rebuild(
> int error;
>
> for_each_bulkload_reservation(&btr->newbt, resv, n) {
> + xfs_fsblock_t fsbno;
> +
> if (resv->used == resv->len)
> continue;
>
> - error = bitmap_set(lost_blocks, resv->fsbno + resv->used,
> - resv->len - resv->used);
> + fsbno = XFS_AGB_TO_FSB(mp, resv->pag->pag_agno,
> + resv->agbno + resv->used);
> + error = bitmap_set(lost_blocks, fsbno, resv->len - resv->used);
> if (error)
> do_error(
> _("Insufficient memory saving lost blocks, err=%d.\n"), error);
> resv->used = resv->len;
> }
>
> - bulkload_destroy(&btr->newbt, 0);
> + bulkload_commit(&btr->newbt);
> }
>
> /*
> diff --git a/repair/bulkload.c b/repair/bulkload.c
> index 0117f69416cf..18158c397f56 100644
> --- a/repair/bulkload.c
> +++ b/repair/bulkload.c
> @@ -23,39 +23,64 @@ bulkload_init_ag(
> }
>
> /* Designate specific blocks to be used to build our new btree. */
> -int
> +static int
> bulkload_add_blocks(
> - struct bulkload *bkl,
> - xfs_fsblock_t fsbno,
> - xfs_extlen_t len)
> + struct bulkload *bkl,
> + struct xfs_perag *pag,
> + const struct xfs_alloc_arg *args)
> {
> - struct bulkload_resv *resv;
> + struct xfs_mount *mp = bkl->sc->mp;
> + struct bulkload_resv *resv;
>
> - resv = kmem_alloc(sizeof(struct bulkload_resv), KM_MAYFAIL);
> + resv = kmalloc(sizeof(struct bulkload_resv), GFP_KERNEL);
> if (!resv)
> return ENOMEM;
>
> INIT_LIST_HEAD(&resv->list);
> - resv->fsbno = fsbno;
> - resv->len = len;
> + resv->agbno = XFS_FSB_TO_AGBNO(mp, args->fsbno);
> + resv->len = args->len;
> resv->used = 0;
> + resv->pag = libxfs_perag_hold(pag);
> +
> list_add_tail(&resv->list, &bkl->resv_list);
> - bkl->nr_reserved += len;
> -
> + bkl->nr_reserved += args->len;
> return 0;
> }
>
> +/*
> + * Add an extent to the new btree reservation pool. Callers are required to
> + * reap this reservation manually if the repair is cancelled. @pag must be a
> + * passive reference.
> + */
> +int
> +bulkload_add_extent(
> + struct bulkload *bkl,
> + struct xfs_perag *pag,
> + xfs_agblock_t agbno,
> + xfs_extlen_t len)
> +{
> + struct xfs_mount *mp = bkl->sc->mp;
> + struct xfs_alloc_arg args = {
> + .tp = NULL, /* no autoreap */
> + .oinfo = bkl->oinfo,
> + .fsbno = XFS_AGB_TO_FSB(mp, pag->pag_agno, agbno),
> + .len = len,
> + .resv = XFS_AG_RESV_NONE,
> + };
> +
> + return bulkload_add_blocks(bkl, pag, &args);
> +}
> +
> /* Free all the accounting info and disk space we reserved for a new btree. */
> void
> -bulkload_destroy(
> - struct bulkload *bkl,
> - int error)
> +bulkload_commit(
> + struct bulkload *bkl)
> {
> struct bulkload_resv *resv, *n;
>
> list_for_each_entry_safe(resv, n, &bkl->resv_list, list) {
> list_del(&resv->list);
> - kmem_free(resv);
> + kfree(resv);
> }
> }
>
> @@ -67,7 +92,8 @@ bulkload_claim_block(
> union xfs_btree_ptr *ptr)
> {
> struct bulkload_resv *resv;
> - xfs_fsblock_t fsb;
> + struct xfs_mount *mp = cur->bc_mp;
> + xfs_agblock_t agbno;
>
> /*
> * The first item in the list should always have a free block unless
> @@ -84,7 +110,7 @@ bulkload_claim_block(
> * decreasing order, which hopefully results in leaf blocks ending up
> * together.
> */
> - fsb = resv->fsbno + resv->used;
> + agbno = resv->agbno + resv->used;
> resv->used++;
>
> /* If we used all the blocks in this reservation, move it to the end. */
> @@ -92,9 +118,10 @@ bulkload_claim_block(
> list_move_tail(&resv->list, &bkl->resv_list);
>
> if (cur->bc_flags & XFS_BTREE_LONG_PTRS)
> - ptr->l = cpu_to_be64(fsb);
> + ptr->l = cpu_to_be64(XFS_AGB_TO_FSB(mp, resv->pag->pag_agno,
> + agbno));
> else
> - ptr->s = cpu_to_be32(XFS_FSB_TO_AGBNO(cur->bc_mp, fsb));
> + ptr->s = cpu_to_be32(agbno);
> return 0;
> }
>
> diff --git a/repair/bulkload.h b/repair/bulkload.h
> index a84e99b8c892..f4790e3b3de6 100644
> --- a/repair/bulkload.h
> +++ b/repair/bulkload.h
> @@ -17,8 +17,10 @@ struct bulkload_resv {
> /* Link to list of extents that we've reserved. */
> struct list_head list;
>
> - /* FSB of the block we reserved. */
> - xfs_fsblock_t fsbno;
> + struct xfs_perag *pag;
> +
> + /* AG block of the block we reserved. */
> + xfs_agblock_t agbno;
>
> /* Length of the reservation. */
> xfs_extlen_t len;
> @@ -51,11 +53,11 @@ struct bulkload {
>
> void bulkload_init_ag(struct bulkload *bkl, struct repair_ctx *sc,
> const struct xfs_owner_info *oinfo);
> -int bulkload_add_blocks(struct bulkload *bkl, xfs_fsblock_t fsbno,
> - xfs_extlen_t len);
> -void bulkload_destroy(struct bulkload *bkl, int error);
> int bulkload_claim_block(struct xfs_btree_cur *cur, struct bulkload *bkl,
> union xfs_btree_ptr *ptr);
> +int bulkload_add_extent(struct bulkload *bkl, struct xfs_perag *pag,
> + xfs_agblock_t agbno, xfs_extlen_t len);
> +void bulkload_commit(struct bulkload *bkl);
> void bulkload_estimate_ag_slack(struct repair_ctx *sc,
> struct xfs_btree_bload *bload, unsigned int free);
>
> diff --git a/repair/phase5.c b/repair/phase5.c
> index d6b8168ea776..b0e208f95af5 100644
> --- a/repair/phase5.c
> +++ b/repair/phase5.c
> @@ -194,7 +194,7 @@ fill_agfl(
> for_each_bulkload_reservation(&btr->newbt, resv, n) {
> xfs_agblock_t bno;
>
> - bno = XFS_FSB_TO_AGBNO(mp, resv->fsbno + resv->used);
> + bno = resv->agbno + resv->used;
> while (resv->used < resv->len &&
> *agfl_idx < libxfs_agfl_size(mp)) {
> agfl_bnos[(*agfl_idx)++] = cpu_to_be32(bno++);
>
>