Re: [PATCH 3/5] xfs: repair free space btrees

[Christoph Hellwig <hch@infradead.org>]

A highlevel question on how blocks are (re)used here.

 - xrep_abt_find_freespace accounts the old allocbt blocks as freespace
   per the comment, although as far as I understand  the code in
   xrep_abt_walk_rmap the allocbt blocks aren't actually added to the
   free_records xfarray, but recorded into the old_allocbt_blocks
   bitmap (btw, why are we using different data structures for them?)
 - xrep_abt_reserve_space seems to allocate space for the new alloc
   btrees by popping the first entry of ->free_records until it has
   enough space.
 - what happens if the AG is so full and fragmented that we do not
   have space to create a second set of allocbts without tearing down
   the old ones first?

I've also got a whole bunch of nitpicks that mostly don't require an
immediate action and/or about existing code that just grows more users
here:

> +int
> +xrep_allocbt(
> +	struct xfs_scrub	*sc)
> +{
> +	struct xrep_abt		*ra;
> +	struct xfs_mount	*mp = sc->mp;
> +	char			*descr;
> +	int			error;
> +
> +	/* We require the rmapbt to rebuild anything. */
> +	if (!xfs_has_rmapbt(mp))
> +		return -EOPNOTSUPP;

Shoudn't this be checked by the .has callback in struct xchk_meta_ops?

> +	/* Set up enough storage to handle maximally fragmented free space. */
> +	descr = xchk_xfile_ag_descr(sc, "free space records");
> +	error = xfarray_create(descr, mp->m_sb.sb_agblocks / 2,
> +			sizeof(struct xfs_alloc_rec_incore),
> +			&ra->free_records);
> +	kfree(descr);

Commenting on a new user of old code here, but why doesn't
xfarray_create simply take a format string so that we don't need the
separate allocatiom and kasprintf here?

> +	/*
> +	 * We must update sm_type temporarily so that the tree-to-tree cross
> +	 * reference checks will work in the correct direction, and also so
> +	 * that tracing will report correctly if there are more errors.
> +	 */
> +	sc->sm->sm_type = XFS_SCRUB_TYPE_BNOBT;
> +	error = xchk_bnobt(sc);

So xchk_bnobt is a tiny wrapper around xchk_allocbt, which is a small
wrapper around xchk_btree that basіally de-multiplex the argument
pass in by xchk_bnobt again.  This is existing code not newly added,
but the call chain looks a bit odd to me.


> +/*
> + * 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
> +xrep_newbt_add_extent(
> +	struct xrep_newbt	*xnr,
> +	struct xfs_perag	*pag,
> +	xfs_agblock_t		agbno,
> +	xfs_extlen_t		len)
> +{
> +	struct xfs_mount	*mp = xnr->sc->mp;
> +	struct xfs_alloc_arg	args = {
> +		.tp		= NULL, /* no autoreap */
> +		.oinfo		= xnr->oinfo,
> +		.fsbno		= XFS_AGB_TO_FSB(mp, pag->pag_agno, agbno),
> +		.len		= len,
> +		.resv		= xnr->resv,
> +	};
> +
> +	return xrep_newbt_add_blocks(xnr, pag, &args);
> +}

I don't quite understand what this helper adds, and the _blocks vs
_extent naming is a bit confusing.


> +#define for_each_xrep_newbt_reservation(xnr, resv, n)	\
> +	list_for_each_entry_safe((resv), (n), &(xnr)->resv_list, list)

I have to admit that I find the open code list_for_each_entry_safe
easier to follow than such wrappers.


> +/* Initialize all the btree cursors for an AG repair. */
> +void
> +xrep_ag_btcur_init(
> +	struct xfs_scrub	*sc,
> +	struct xchk_ag		*sa)
> +{

As far as I can tell this basically sets up cursors for all the
btrees except the one that we want to repair, and the one that
goes along with for the alloc and ialloc pairs?  Maybe just spell
that out for clarity.