On Mon, Jun 01, 2020 at 09:27:57PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> Use the btree bulk loading functions to rebuild the refcount btrees
> and drop the open-coded implementation.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> libxfs/libxfs_api_defs.h | 1
> repair/agbtree.c | 71 ++++++++++
> repair/agbtree.h | 5 +
> repair/phase5.c | 341 ++--------------------------------------------
> 4 files changed, 93 insertions(+), 325 deletions(-)
>
>
...
> diff --git a/repair/phase5.c b/repair/phase5.c
> index 1c6448f4..ad009416 100644
> --- a/repair/phase5.c
> +++ b/repair/phase5.c
...
> @@ -817,10 +510,14 @@ build_agf_agfl(
> cpu_to_be32(btr_rmap->newbt.afake.af_blocks);
> }
>
> - agf->agf_refcount_root = cpu_to_be32(refcnt_bt->root);
> - agf->agf_refcount_level = cpu_to_be32(refcnt_bt->num_levels);
> - agf->agf_refcount_blocks = cpu_to_be32(refcnt_bt->num_tot_blocks -
> - refcnt_bt->num_free_blocks);
> + if (xfs_sb_version_hasreflink(&mp->m_sb)) {
> + agf->agf_refcount_root =
> + cpu_to_be32(btr_refc->newbt.afake.af_root);
> + agf->agf_refcount_level =
> + cpu_to_be32(btr_refc->newbt.afake.af_levels);
> + agf->agf_refcount_blocks =
> + cpu_to_be32(btr_refc->newbt.afake.af_blocks);
> + }
It looks like the previous cursor variant (refcnt_bt) would be zeroed
out if the feature isn't enabled (causing this to zero out the agf
fields on disk), whereas now we only write the fields when the feature
is enabled. Any concern over removing that zeroing behavior? Also note
that an assert further down unconditionally reads the
->agf_refcount_root field.
BTW, I suppose the same question may apply to the previous patch as
well...
Brian
>
> /*
> * Count and record the number of btree blocks consumed if required.
> @@ -981,7 +678,7 @@ phase5_func(
> struct bt_rebuild btr_ino;
> struct bt_rebuild btr_fino;
> struct bt_rebuild btr_rmap;
> - bt_status_t refcnt_btree_curs;
> + struct bt_rebuild btr_refc;
> int extra_blocks = 0;
> uint num_freeblocks;
> xfs_agblock_t num_extents;
> @@ -1017,11 +714,7 @@ _("unable to rebuild AG %u. Not enough free space in on-disk AG.\n"),
>
> init_rmapbt_cursor(&sc, agno, num_freeblocks, &btr_rmap);
>
> - /*
> - * Set up the btree cursors for the on-disk refcount btrees,
> - * which includes pre-allocating all required blocks.
> - */
> - init_refc_cursor(mp, agno, &refcnt_btree_curs);
> + init_refc_cursor(&sc, agno, num_freeblocks, &btr_refc);
>
> num_extents = count_bno_extents_blocks(agno, &num_freeblocks);
> /*
> @@ -1085,16 +778,14 @@ _("unable to rebuild AG %u. Not enough free space in on-disk AG.\n"),
> sb_fdblocks_ag[agno] += btr_rmap.newbt.afake.af_blocks - 1;
> }
>
> - if (xfs_sb_version_hasreflink(&mp->m_sb)) {
> - build_refcount_tree(mp, agno, &refcnt_btree_curs);
> - write_cursor(&refcnt_btree_curs);
> - }
> + if (xfs_sb_version_hasreflink(&mp->m_sb))
> + build_refcount_tree(&sc, agno, &btr_refc);
>
> /*
> * set up agf and agfl
> */
> - build_agf_agfl(mp, agno, &btr_bno, &btr_cnt, &btr_rmap,
> - &refcnt_btree_curs, lost_fsb);
> + build_agf_agfl(mp, agno, &btr_bno, &btr_cnt, &btr_rmap, &btr_refc,
> + lost_fsb);
>
> build_inode_btrees(&sc, agno, &btr_ino, &btr_fino);
>
> @@ -1112,7 +803,7 @@ _("unable to rebuild AG %u. Not enough free space in on-disk AG.\n"),
> if (xfs_sb_version_hasrmapbt(&mp->m_sb))
> finish_rebuild(mp, &btr_rmap, lost_fsb);
> if (xfs_sb_version_hasreflink(&mp->m_sb))
> - finish_cursor(&refcnt_btree_curs);
> + finish_rebuild(mp, &btr_refc, lost_fsb);
>
> /*
> * release the incore per-AG bno/bcnt trees so the extent nodes
>
