Re: [PATCH 10/21] xfs: add helper routines for the repair code

[Dave Chinner <david@fromorbit.com>]

On Mon, Apr 02, 2018 at 12:57:18PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add some helper functions for repair functions that will help us to
> allocate and initialize new metadata blocks for btrees that we're
> rebuilding.

This is more than "helper functions" - my replay is almost 700 lines
long!

i.e. This is a stack of extent invalidation, freeing and rmap/free
space rebuild code. Needs a lot more description and context than
"helper functions".

.....
> @@ -574,7 +575,10 @@ xfs_scrub_setup_fs(
>  	struct xfs_scrub_context	*sc,
>  	struct xfs_inode		*ip)
>  {
> -	return xfs_scrub_trans_alloc(sc->sm, sc->mp, &sc->tp);
> +	uint				resblks;
> +
> +	resblks = xfs_repair_calc_ag_resblks(sc);
> +	return xfs_scrub_trans_alloc(sc->sm, sc->mp, resblks, &sc->tp);
>  }

What's the block reservation for?

> +/*
> + * Roll a transaction, keeping the AG headers locked and reinitializing
> + * the btree cursors.
> + */
> +int
> +xfs_repair_roll_ag_trans(
> +	struct xfs_scrub_context	*sc)
> +{
> +	struct xfs_trans		*tp;
> +	int				error;
> +
> +	/* Keep the AG header buffers locked so we can keep going. */
> +	xfs_trans_bhold(sc->tp, sc->sa.agi_bp);
> +	xfs_trans_bhold(sc->tp, sc->sa.agf_bp);
> +	xfs_trans_bhold(sc->tp, sc->sa.agfl_bp);
> +
> +	/* Roll the transaction. */
> +	tp = sc->tp;
> +	error = xfs_trans_roll(&sc->tp);
> +	if (error)
> +		return error;

Who releases those buffers if we get an error here?

> +
> +	/* Join the buffer to the new transaction or release the hold. */
> +	if (sc->tp != tp) {

When does xfs_trans_roll() return successfully without allocating a
new transaction?

> +		xfs_trans_bjoin(sc->tp, sc->sa.agi_bp);
> +		xfs_trans_bjoin(sc->tp, sc->sa.agf_bp);
> +		xfs_trans_bjoin(sc->tp, sc->sa.agfl_bp);
> +	} else {
> +		xfs_trans_bhold_release(sc->tp, sc->sa.agi_bp);
> +		xfs_trans_bhold_release(sc->tp, sc->sa.agf_bp);
> +		xfs_trans_bhold_release(sc->tp, sc->sa.agfl_bp);
> +	}
> +
> +	return error;
> +}
.....

> +/* Allocate a block in an AG. */
> +int
> +xfs_repair_alloc_ag_block(
> +	struct xfs_scrub_context	*sc,
> +	struct xfs_owner_info		*oinfo,
> +	xfs_fsblock_t			*fsbno,
> +	enum xfs_ag_resv_type		resv)
> +{
> +	struct xfs_alloc_arg		args = {0};
> +	xfs_agblock_t			bno;
> +	int				error;
> +
> +	if (resv == XFS_AG_RESV_AGFL) {
> +		error = xfs_alloc_get_freelist(sc->tp, sc->sa.agf_bp, &bno, 1);
> +		if (error)
> +			return error;
> +		if (bno == NULLAGBLOCK)
> +			return -ENOSPC;
> +		xfs_extent_busy_reuse(sc->mp, sc->sa.agno, bno,
> +				1, false);
> +		*fsbno = XFS_AGB_TO_FSB(sc->mp, sc->sa.agno, bno);
> +		return 0;
> +	}
> +
> +	args.tp = sc->tp;
> +	args.mp = sc->mp;
> +	args.oinfo = *oinfo;
> +	args.fsbno = XFS_AGB_TO_FSB(args.mp, sc->sa.agno, 0);

So our target BNO is the start of the AG.

> +	args.minlen = 1;
> +	args.maxlen = 1;
> +	args.prod = 1;
> +	args.type = XFS_ALLOCTYPE_NEAR_BNO;

And we do a "near" search" for a single block. i.e. we are looking
for a single block near to the start of the AG. Basically, we are
looking for the extent at the left edge of the by-bno tree, which
may not be a single block.

Would it be better to do a XFS_ALLOCTYPE_THIS_AG allocation here so
we look up the by-size btree for a single block extent? The left
edge of that tree will always be the smallest free extent closest to
the start of the AG, and so using THIS_AG will tend to fill
small freespace holes rather than tear up larger free spaces for
single block allocations.....

> +/* Put a block back on the AGFL. */
> +int
> +xfs_repair_put_freelist(
> +	struct xfs_scrub_context	*sc,
> +	xfs_agblock_t			agbno)
> +{
> +	struct xfs_owner_info		oinfo;
> +	int				error;
> +
> +	/*
> +	 * Since we're "freeing" a lost block onto the AGFL, we have to
> +	 * create an rmap for the block prior to merging it or else other
> +	 * parts will break.
> +	 */
> +	xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_AG);
> +	error = xfs_rmap_alloc(sc->tp, sc->sa.agf_bp, sc->sa.agno, agbno, 1,
> +			&oinfo);
> +	if (error)
> +		return error;
> +
> +	/* Put the block on the AGFL. */
> +	error = xfs_alloc_put_freelist(sc->tp, sc->sa.agf_bp, sc->sa.agfl_bp,
> +			agbno, 0);

At what point do we check there's actually room in the AGFL for this
block?

> +	if (error)
> +		return error;
> +	xfs_extent_busy_insert(sc->tp, sc->sa.agno, agbno, 1,
> +			XFS_EXTENT_BUSY_SKIP_DISCARD);
> +
> +	/* Make sure the AGFL doesn't overfill. */
> +	return xfs_repair_fix_freelist(sc, true);

i.e. shouldn't this be done first?

> +}
> +
> +/*
> + * For a given metadata extent and owner, delete the associated rmap.
> + * If the block has no other owners, free it.
> + */
> +STATIC int
> +xfs_repair_free_or_unmap_extent(
> +	struct xfs_scrub_context	*sc,
> +	xfs_fsblock_t			fsbno,
> +	xfs_extlen_t			len,
> +	struct xfs_owner_info		*oinfo,
> +	enum xfs_ag_resv_type		resv)
> +{
> +	struct xfs_mount		*mp = sc->mp;
> +	struct xfs_btree_cur		*rmap_cur;
> +	struct xfs_buf			*agf_bp = NULL;
> +	xfs_agnumber_t			agno;
> +	xfs_agblock_t			agbno;
> +	bool				has_other_rmap;
> +	int				error = 0;
> +
> +	ASSERT(xfs_sb_version_hasrmapbt(&mp->m_sb));
> +	agno = XFS_FSB_TO_AGNO(mp, fsbno);
> +	agbno = XFS_FSB_TO_AGBNO(mp, fsbno);
> +
> +	trace_xfs_repair_free_or_unmap_extent(mp, agno, agbno, len);
> +
> +	for (; len > 0 && !error; len--, agbno++, fsbno++) {
> +		ASSERT(sc->ip != NULL || agno == sc->sa.agno);
> +
> +		/* Can we find any other rmappings? */
> +		if (sc->ip) {
> +			error = xfs_alloc_read_agf(mp, sc->tp, agno, 0,
> +					&agf_bp);
> +			if (error)
> +				break;
> +			if (!agf_bp) {
> +				error = -ENOMEM;
> +				break;
> +			}
> +		}
> +		rmap_cur = xfs_rmapbt_init_cursor(mp, sc->tp,
> +				agf_bp ? agf_bp : sc->sa.agf_bp, agno);

Why is the inode case treated differently here - why do we have a
special reference to agf_bp here rather than using sc->sa.agf_bp?


> +		error = xfs_rmap_has_other_keys(rmap_cur, agbno, 1, oinfo,
> +				&has_other_rmap);
> +		if (error)
> +			goto out_cur;
> +		xfs_btree_del_cursor(rmap_cur, XFS_BTREE_NOERROR);
> +		if (agf_bp)
> +			xfs_trans_brelse(sc->tp, agf_bp);

agf_bp released here.

> +
> +		/*
> +		 * If there are other rmappings, this block is cross
> +		 * linked and must not be freed.  Remove the reverse
> +		 * mapping and move on.  Otherwise, we were the only
> +		 * owner of the block, so free the extent, which will
> +		 * also remove the rmap.
> +		 */
> +		if (has_other_rmap)
> +			error = xfs_rmap_free(sc->tp, agf_bp, agno, agbno, 1,
> +					oinfo);

And then used here. Use-after-free?

> +		else if (resv == XFS_AG_RESV_AGFL)
> +			error = xfs_repair_put_freelist(sc, agbno);
> +		else
> +			error = xfs_free_extent(sc->tp, fsbno, 1, oinfo, resv);
> +		if (error)
> +			break;
> +
> +		if (sc->ip)
> +			error = xfs_trans_roll_inode(&sc->tp, sc->ip);
> +		else
> +			error = xfs_repair_roll_ag_trans(sc);

why don't we break on error here?

> +	}
> +
> +	return error;
> +out_cur:
> +	xfs_btree_del_cursor(rmap_cur, XFS_BTREE_ERROR);
> +	if (agf_bp)
> +		xfs_trans_brelse(sc->tp, agf_bp);
> +	return error;

Can you put a blank line preceeding the out label so there is clear
separation between the normal return code and error handling stack?

> +}
> +
> +/* Collect a dead btree extent for later disposal. */
> +int
> +xfs_repair_collect_btree_extent(
> +	struct xfs_scrub_context	*sc,
> +	struct xfs_repair_extent_list	*exlist,
> +	xfs_fsblock_t			fsbno,
> +	xfs_extlen_t			len)
> +{
> +	struct xfs_repair_extent	*rae;

What's "rae" short for? "rex" I'd understand, but I can't work out
what the "a" in rae means :/

> +
> +	trace_xfs_repair_collect_btree_extent(sc->mp,
> +			XFS_FSB_TO_AGNO(sc->mp, fsbno),
> +			XFS_FSB_TO_AGBNO(sc->mp, fsbno), len);
> +
> +	rae = kmem_alloc(sizeof(struct xfs_repair_extent),
> +			KM_MAYFAIL | KM_NOFS);

Why KM_NOFS?

> +	if (!rae)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&rae->list);
> +	rae->fsbno = fsbno;
> +	rae->len = len;
> +	list_add_tail(&rae->list, &exlist->list);
> +
> +	return 0;
> +}
> +
> +/* Invalidate buffers for blocks we're dumping. */
> +int
> +xfs_repair_invalidate_blocks(
> +	struct xfs_scrub_context	*sc,
> +	struct xfs_repair_extent_list	*exlist)
> +{
> +	struct xfs_repair_extent	*rae;
> +	struct xfs_repair_extent	*n;
> +	struct xfs_buf			*bp;
> +	xfs_agnumber_t			agno;
> +	xfs_agblock_t			agbno;
> +	xfs_agblock_t			i;
> +
> +	for_each_xfs_repair_extent_safe(rae, n, exlist) {
> +		agno = XFS_FSB_TO_AGNO(sc->mp, rae->fsbno);
> +		agbno = XFS_FSB_TO_AGBNO(sc->mp, rae->fsbno);
> +		for (i = 0; i < rae->len; i++) {
> +			bp = xfs_btree_get_bufs(sc->mp, sc->tp, agno,
> +					agbno + i, 0);
> +			xfs_trans_binval(sc->tp, bp);
> +		}
> +	}

What if they are data blocks we are dumping? xfs_trans_binval is
fine for metadata blocks, but creating a stale metadata buffer and
logging a buffer cancel for user data blocks doesn't make any sense
to me....

> +/* Dispose of dead btree extents.  If oinfo is NULL, just delete the list. */
> +int
> +xfs_repair_reap_btree_extents(
> +	struct xfs_scrub_context	*sc,
> +	struct xfs_repair_extent_list	*exlist,
> +	struct xfs_owner_info		*oinfo,
> +	enum xfs_ag_resv_type		type)
> +{
> +	struct xfs_repair_extent	*rae;
> +	struct xfs_repair_extent	*n;
> +	int				error = 0;
> +
> +	for_each_xfs_repair_extent_safe(rae, n, exlist) {
> +		if (oinfo) {
> +			error = xfs_repair_free_or_unmap_extent(sc, rae->fsbno,
> +					rae->len, oinfo, type);
> +			if (error)
> +				oinfo = NULL;
> +		}
> +		list_del(&rae->list);
> +		kmem_free(rae);
> +	}
> +
> +	return error;
> +}

So does this happen before or after the extent list invalidation?

> +/* Errors happened, just delete the dead btree extent list. */
> +void
> +xfs_repair_cancel_btree_extents(
> +	struct xfs_scrub_context	*sc,
> +	struct xfs_repair_extent_list	*exlist)
> +{
> +	xfs_repair_reap_btree_extents(sc, exlist, NULL, XFS_AG_RESV_NONE);
> +}

So if an error happened durign rebuild, we just trash whatever we
were trying to rebuild?

> +/* Compare two btree extents. */
> +static int
> +xfs_repair_btree_extent_cmp(
> +	void				*priv,
> +	struct list_head		*a,
> +	struct list_head		*b)
> +{
> +	struct xfs_repair_extent	*ap;
> +	struct xfs_repair_extent	*bp;
> +
> +	ap = container_of(a, struct xfs_repair_extent, list);
> +	bp = container_of(b, struct xfs_repair_extent, list);
> +
> +	if (ap->fsbno > bp->fsbno)
> +		return 1;
> +	else if (ap->fsbno < bp->fsbno)
> +		return -1;
> +	return 0;
> +}
> +
> +/* Remove all the blocks in sublist from exlist. */
> +#define LEFT_CONTIG	(1 << 0)
> +#define RIGHT_CONTIG	(1 << 1)

Namespace, please.

> +int
> +xfs_repair_subtract_extents(
> +	struct xfs_scrub_context	*sc,
> +	struct xfs_repair_extent_list	*exlist,
> +	struct xfs_repair_extent_list	*sublist)

What is this function supposed to do? I can't really review the code
(looks complex) because I don't know what it's function is supposed
to be.

> +struct xfs_repair_find_ag_btree_roots_info {
> +	struct xfs_buf			*agfl_bp;
> +	struct xfs_repair_find_ag_btree	*btree_info;
> +};
> +
> +/* Is this an OWN_AG block in the AGFL? */
> +STATIC bool
> +xfs_repair_is_block_in_agfl(
> +	struct xfs_mount		*mp,
> +	uint64_t			rmap_owner,
> +	xfs_agblock_t			agbno,
> +	struct xfs_buf			*agf_bp,
> +	struct xfs_buf			*agfl_bp)
> +{
> +	struct xfs_agf			*agf;
> +	__be32				*agfl_bno;
> +	unsigned int			flfirst;
> +	unsigned int			fllast;
> +	int				i;
> +
> +	if (rmap_owner != XFS_RMAP_OWN_AG)
> +		return false;
> +
> +	agf = XFS_BUF_TO_AGF(agf_bp);
> +	agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agfl_bp);
> +	flfirst = be32_to_cpu(agf->agf_flfirst);
> +	fllast = be32_to_cpu(agf->agf_fllast);
> +
> +	/* Skip an empty AGFL. */
> +	if (agf->agf_flcount == cpu_to_be32(0))
> +		return false;
> +
> +	/* first to last is a consecutive list. */
> +	if (fllast >= flfirst) {
> +		for (i = flfirst; i <= fllast; i++) {
> +			if (be32_to_cpu(agfl_bno[i]) == agbno)
> +				return true;
> +		}
> +
> +		return false;
> +	}
> +
> +	/* first to the end */
> +	for (i = flfirst; i < xfs_agfl_size(mp); i++) {
> +		if (be32_to_cpu(agfl_bno[i]) == agbno)
> +			return true;
> +	}
> +
> +	/* the start to last. */
> +	for (i = 0; i <= fllast; i++) {
> +		if (be32_to_cpu(agfl_bno[i]) == agbno)
> +			return true;
> +	}
> +
> +	return false;

Didn't you just create a generic agfl walk function for this?

> +}
> +
> +/* Find btree roots from the AGF. */
> +STATIC int
> +xfs_repair_find_ag_btree_roots_helper(
> +	struct xfs_btree_cur		*cur,
> +	struct xfs_rmap_irec		*rec,
> +	void				*priv)

Again, I'm not sure exactly what this function is doing. It look
slike it's trying to tell if a freed block is a btree block,
but I'm not sure of that, nor the context in which we'd be searching
free blocks for a specific metadata contents.

> +/* Find the roots of the given btrees from the rmap info. */
> +int
> +xfs_repair_find_ag_btree_roots(
> +	struct xfs_scrub_context	*sc,
> +	struct xfs_buf			*agf_bp,
> +	struct xfs_repair_find_ag_btree	*btree_info,
> +	struct xfs_buf			*agfl_bp)
> +{
> +	struct xfs_mount		*mp = sc->mp;
> +	struct xfs_repair_find_ag_btree_roots_info	ri;
> +	struct xfs_repair_find_ag_btree	*fab;
> +	struct xfs_btree_cur		*cur;
> +	int				error;
> +
> +	ri.btree_info = btree_info;
> +	ri.agfl_bp = agfl_bp;
> +	for (fab = btree_info; fab->buf_ops; fab++) {
> +		ASSERT(agfl_bp || fab->rmap_owner != XFS_RMAP_OWN_AG);
> +		fab->root = NULLAGBLOCK;
> +		fab->level = 0;
> +	}
> +
> +	cur = xfs_rmapbt_init_cursor(mp, sc->tp, agf_bp, sc->sa.agno);
> +	error = xfs_rmap_query_all(cur, xfs_repair_find_ag_btree_roots_helper,
> +			&ri);
> +	xfs_btree_del_cursor(cur, error ? XFS_BTREE_ERROR : XFS_BTREE_NOERROR);
> +
> +	for (fab = btree_info; !error && fab->buf_ops; fab++)
> +		if (fab->root != NULLAGBLOCK)
> +			fab->level++;
> +
> +	return error;
> +}
> +
> +/* Reset the superblock counters from the AGF/AGI. */
> +int
> +xfs_repair_reset_counters(
> +	struct xfs_mount	*mp)
> +{
> +	struct xfs_trans	*tp;
> +	struct xfs_buf		*agi_bp;
> +	struct xfs_buf		*agf_bp;
> +	struct xfs_agi		*agi;
> +	struct xfs_agf		*agf;
> +	xfs_agnumber_t		agno;
> +	xfs_ino_t		icount = 0;
> +	xfs_ino_t		ifree = 0;
> +	xfs_filblks_t		fdblocks = 0;
> +	int64_t			delta_icount;
> +	int64_t			delta_ifree;
> +	int64_t			delta_fdblocks;
> +	int			error;
> +
> +	trace_xfs_repair_reset_counters(mp);
> +
> +	error = xfs_trans_alloc_empty(mp, &tp);
> +	if (error)
> +		return error;
> +
> +	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
> +		/* Count all the inodes... */
> +		error = xfs_ialloc_read_agi(mp, tp, agno, &agi_bp);
> +		if (error)
> +			goto out;
> +		agi = XFS_BUF_TO_AGI(agi_bp);
> +		icount += be32_to_cpu(agi->agi_count);
> +		ifree += be32_to_cpu(agi->agi_freecount);
> +
> +		/* Add up the free/freelist/bnobt/cntbt blocks... */
> +		error = xfs_alloc_read_agf(mp, tp, agno, 0, &agf_bp);
> +		if (error)
> +			goto out;
> +		if (!agf_bp) {
> +			error = -ENOMEM;
> +			goto out;
> +		}
> +		agf = XFS_BUF_TO_AGF(agf_bp);
> +		fdblocks += be32_to_cpu(agf->agf_freeblks);
> +		fdblocks += be32_to_cpu(agf->agf_flcount);
> +		fdblocks += be32_to_cpu(agf->agf_btreeblks);
> +	}
> +
> +	/*
> +	 * Reinitialize the counters.  The on-disk and in-core counters
> +	 * differ by the number of inodes/blocks reserved by the admin,
> +	 * the per-AG reservation, and any transactions in progress, so
> +	 * we have to account for that.
> +	 */
> +	spin_lock(&mp->m_sb_lock);
> +	delta_icount = (int64_t)mp->m_sb.sb_icount - icount;
> +	delta_ifree = (int64_t)mp->m_sb.sb_ifree - ifree;
> +	delta_fdblocks = (int64_t)mp->m_sb.sb_fdblocks - fdblocks;
> +	mp->m_sb.sb_icount = icount;
> +	mp->m_sb.sb_ifree = ifree;
> +	mp->m_sb.sb_fdblocks = fdblocks;
> +	spin_unlock(&mp->m_sb_lock);

This is largely a copy and paste of the code run by mount after log
recovery on an unclean mount. Can you factor this into a single
function?

> +
> +	if (delta_icount) {
> +		error = xfs_mod_icount(mp, delta_icount);
> +		if (error)
> +			goto out;
> +	}
> +	if (delta_ifree) {
> +		error = xfs_mod_ifree(mp, delta_ifree);
> +		if (error)
> +			goto out;
> +	}
> +	if (delta_fdblocks) {
> +		error = xfs_mod_fdblocks(mp, delta_fdblocks, false);
> +		if (error)
> +			goto out;
> +	}
> +
> +out:
> +	xfs_trans_cancel(tp);
> +	return error;

Ummmm - why do we do all this superblock modification work in a
transaction context, and then just cancel it?

> +}
> +
> +/* Figure out how many blocks to reserve for an AG repair. */
> +xfs_extlen_t
> +xfs_repair_calc_ag_resblks(
> +	struct xfs_scrub_context	*sc)
> +{
> +	struct xfs_mount		*mp = sc->mp;
> +	struct xfs_scrub_metadata	*sm = sc->sm;
> +	struct xfs_agi			*agi;
> +	struct xfs_agf			*agf;
> +	struct xfs_buf			*bp;
> +	xfs_agino_t			icount;
> +	xfs_extlen_t			aglen;
> +	xfs_extlen_t			usedlen;
> +	xfs_extlen_t			freelen;
> +	xfs_extlen_t			bnobt_sz;
> +	xfs_extlen_t			inobt_sz;
> +	xfs_extlen_t			rmapbt_sz;
> +	xfs_extlen_t			refcbt_sz;
> +	int				error;
> +
> +	if (!(sm->sm_flags & XFS_SCRUB_IFLAG_REPAIR))
> +		return 0;
> +
> +	/*
> +	 * Try to get the actual counters from disk; if not, make
> +	 * some worst case assumptions.
> +	 */
> +	error = xfs_read_agi(mp, NULL, sm->sm_agno, &bp);
> +	if (!error) {
> +		agi = XFS_BUF_TO_AGI(bp);
> +		icount = be32_to_cpu(agi->agi_count);
> +		xfs_trans_brelse(NULL, bp);

xfs_buf_relse(bp)?

And, well, why not just use pag->pagi_count rather than having to
read it from the on-disk buffer?

> +	} else {
> +		icount = mp->m_sb.sb_agblocks / mp->m_sb.sb_inopblock;
> +	}
> +
> +	error = xfs_alloc_read_agf(mp, NULL, sm->sm_agno, 0, &bp);
> +	if (!error && bp) {
> +		agf = XFS_BUF_TO_AGF(bp);
> +		aglen = be32_to_cpu(agf->agf_length);
> +		freelen = be32_to_cpu(agf->agf_freeblks);
> +		usedlen = aglen - freelen;
> +		xfs_trans_brelse(NULL, bp);
> +	} else {
> +		aglen = mp->m_sb.sb_agblocks;
> +		freelen = aglen;
> +		usedlen = aglen;
> +	}

ditto for there - it's all in the xfs_perag, right?

> +
> +	trace_xfs_repair_calc_ag_resblks(mp, sm->sm_agno, icount, aglen,
> +			freelen, usedlen);
> +
> +	/*
> +	 * Figure out how many blocks we'd need worst case to rebuild
> +	 * each type of btree.  Note that we can only rebuild the
> +	 * bnobt/cntbt or inobt/finobt as pairs.
> +	 */
> +	bnobt_sz = 2 * xfs_allocbt_calc_size(mp, freelen);
> +	if (xfs_sb_version_hassparseinodes(&mp->m_sb))
> +		inobt_sz = xfs_iallocbt_calc_size(mp, icount /
> +				XFS_INODES_PER_HOLEMASK_BIT);
> +	else
> +		inobt_sz = xfs_iallocbt_calc_size(mp, icount /
> +				XFS_INODES_PER_CHUNK);
> +	if (xfs_sb_version_hasfinobt(&mp->m_sb))
> +		inobt_sz *= 2;
> +	if (xfs_sb_version_hasreflink(&mp->m_sb)) {
> +		rmapbt_sz = xfs_rmapbt_calc_size(mp, aglen);
> +		refcbt_sz = xfs_refcountbt_calc_size(mp, usedlen);
> +	} else {
> +		rmapbt_sz = xfs_rmapbt_calc_size(mp, usedlen);
> +		refcbt_sz = 0;
> +	}
> +	if (!xfs_sb_version_hasrmapbt(&mp->m_sb))
> +		rmapbt_sz = 0;
> +
> +	trace_xfs_repair_calc_ag_resblks_btsize(mp, sm->sm_agno, bnobt_sz,
> +			inobt_sz, rmapbt_sz, refcbt_sz);
> +
> +	return max(max(bnobt_sz, inobt_sz), max(rmapbt_sz, refcbt_sz));

Why are we only returning the resblks for a single tree rebuild?
Shouldn't it return the total blocks require to rebuild all all the
trees?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com