Re: [PATCH 9/9] xfs_repair: track blocks lost during btree construction via extents

[Brian Foster <bfoster@redhat.com>]

On Tue, May 19, 2020 at 06:51:40PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Use extent records (not just raw fsbs) to track blocks that were lost
> during btree construction.  This makes it somewhat more efficient.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  repair/phase5.c |   61 ++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 35 insertions(+), 26 deletions(-)
> 
> 
> diff --git a/repair/phase5.c b/repair/phase5.c
> index 72c6908a..22007275 100644
> --- a/repair/phase5.c
> +++ b/repair/phase5.c
> @@ -45,6 +45,12 @@ struct bt_rebuild {
>  	};
>  };
>  
> +struct lost_fsb {
> +	xfs_fsblock_t		fsbno;
> +	xfs_extlen_t		len;
> +};
> +

Looks reasonable at a glance, but could we call the above structure an
extent and rename lost_fsbs to lost_extents or some such?

Brian

> +
>  /*
>   * extra metadata for the agi
>   */
> @@ -314,21 +320,24 @@ static void
>  finish_rebuild(
>  	struct xfs_mount	*mp,
>  	struct bt_rebuild	*btr,
> -	struct xfs_slab		*lost_fsb)
> +	struct xfs_slab		*lost_fsbs)
>  {
>  	struct xrep_newbt_resv	*resv, *n;
>  
>  	for_each_xrep_newbt_reservation(&btr->newbt, resv, n) {
> -		while (resv->used < resv->len) {
> -			xfs_fsblock_t	fsb = resv->fsbno + resv->used;
> -			int		error;
> +		struct lost_fsb	lost;
> +		int		error;
>  
> -			error = slab_add(lost_fsb, &fsb);
> -			if (error)
> -				do_error(
> +		if (resv->used == resv->len)
> +			continue;
> +
> +		lost.fsbno = resv->fsbno + resv->used;
> +		lost.len = resv->len - resv->used;
> +		error = slab_add(lost_fsbs, &lost);
> +		if (error)
> +			do_error(
>  _("Insufficient memory saving lost blocks.\n"));
> -			resv->used++;
> -		}
> +		resv->used = resv->len;
>  	}
>  
>  	xrep_newbt_destroy(&btr->newbt, 0);
> @@ -1036,7 +1045,7 @@ build_agf_agfl(
>  	int			lostblocks,	/* # blocks that will be lost */
>  	struct bt_rebuild	*btr_rmap,
>  	struct bt_rebuild	*btr_refc,
> -	struct xfs_slab		*lost_fsb)
> +	struct xfs_slab		*lost_fsbs)
>  {
>  	struct extent_tree_node	*ext_ptr;
>  	struct xfs_buf		*agf_buf, *agfl_buf;
> @@ -1253,7 +1262,7 @@ static void
>  phase5_func(
>  	struct xfs_mount	*mp,
>  	xfs_agnumber_t		agno,
> -	struct xfs_slab		*lost_fsb)
> +	struct xfs_slab		*lost_fsbs)
>  {
>  	struct repair_ctx	sc = { .mp = mp, };
>  	struct agi_stat		agi_stat = {0,};
> @@ -1372,7 +1381,7 @@ _("unable to rebuild AG %u.  Not enough free space in on-disk AG.\n"),
>  	 * set up agf and agfl
>  	 */
>  	build_agf_agfl(mp, agno, &btr_bno, &btr_cnt, freeblks1, extra_blocks,
> -			&btr_rmap, &btr_refc, lost_fsb);
> +			&btr_rmap, &btr_refc, lost_fsbs);
>  
>  	/*
>  	 * build inode allocation trees.
> @@ -1387,15 +1396,15 @@ _("unable to rebuild AG %u.  Not enough free space in on-disk AG.\n"),
>  	/*
>  	 * tear down cursors
>  	 */
> -	finish_rebuild(mp, &btr_bno, lost_fsb);
> -	finish_rebuild(mp, &btr_cnt, lost_fsb);
> -	finish_rebuild(mp, &btr_ino, lost_fsb);
> +	finish_rebuild(mp, &btr_bno, lost_fsbs);
> +	finish_rebuild(mp, &btr_cnt, lost_fsbs);
> +	finish_rebuild(mp, &btr_ino, lost_fsbs);
>  	if (xfs_sb_version_hasfinobt(&mp->m_sb))
> -		finish_rebuild(mp, &btr_fino, lost_fsb);
> +		finish_rebuild(mp, &btr_fino, lost_fsbs);
>  	if (xfs_sb_version_hasrmapbt(&mp->m_sb))
> -		finish_rebuild(mp, &btr_rmap, lost_fsb);
> +		finish_rebuild(mp, &btr_rmap, lost_fsbs);
>  	if (xfs_sb_version_hasreflink(&mp->m_sb))
> -		finish_rebuild(mp, &btr_refc, lost_fsb);
> +		finish_rebuild(mp, &btr_refc, lost_fsbs);
>  
>  	/*
>  	 * release the incore per-AG bno/bcnt trees so the extent nodes
> @@ -1414,19 +1423,19 @@ inject_lost_blocks(
>  {
>  	struct xfs_trans	*tp = NULL;
>  	struct xfs_slab_cursor	*cur = NULL;
> -	xfs_fsblock_t		*fsb;
> +	struct lost_fsb		*lost;
>  	int			error;
>  
>  	error = init_slab_cursor(lost_fsbs, NULL, &cur);
>  	if (error)
>  		return error;
>  
> -	while ((fsb = pop_slab_cursor(cur)) != NULL) {
> +	while ((lost = pop_slab_cursor(cur)) != NULL) {
>  		error = -libxfs_trans_alloc_rollable(mp, 16, &tp);
>  		if (error)
>  			goto out_cancel;
>  
> -		error = -libxfs_free_extent(tp, *fsb, 1,
> +		error = -libxfs_free_extent(tp, lost->fsbno, lost->len,
>  				&XFS_RMAP_OINFO_ANY_OWNER, XFS_AG_RESV_NONE);
>  		if (error)
>  			goto out_cancel;
> @@ -1447,7 +1456,7 @@ inject_lost_blocks(
>  void
>  phase5(xfs_mount_t *mp)
>  {
> -	struct xfs_slab		*lost_fsb;
> +	struct xfs_slab		*lost_fsbs;
>  	xfs_agnumber_t		agno;
>  	int			error;
>  
> @@ -1490,12 +1499,12 @@ phase5(xfs_mount_t *mp)
>  	if (sb_fdblocks_ag == NULL)
>  		do_error(_("cannot alloc sb_fdblocks_ag buffers\n"));
>  
> -	error = init_slab(&lost_fsb, sizeof(xfs_fsblock_t));
> +	error = init_slab(&lost_fsbs, sizeof(struct lost_fsb));
>  	if (error)
>  		do_error(_("cannot alloc lost block slab\n"));
>  
>  	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++)
> -		phase5_func(mp, agno, lost_fsb);
> +		phase5_func(mp, agno, lost_fsbs);
>  
>  	print_final_rpt();
>  
> @@ -1538,10 +1547,10 @@ _("unable to add AG %u reverse-mapping data to btree.\n"), agno);
>  	 * Put blocks that were unnecessarily reserved for btree
>  	 * reconstruction back into the filesystem free space data.
>  	 */
> -	error = inject_lost_blocks(mp, lost_fsb);
> +	error = inject_lost_blocks(mp, lost_fsbs);
>  	if (error)
>  		do_error(_("Unable to reinsert lost blocks into filesystem.\n"));
> -	free_slab(&lost_fsb);
> +	free_slab(&lost_fsbs);
>  
>  	bad_ino_btree = 0;
>  
>