On Tue, Nov 28, 2023 at 01:13:58PM -0800, Darrick J. Wong wrote: > > - 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?) > > The old free space btree blocks are tracked via old_allocbt_blocks so > that we can reap the space after committing the new btree roots. Yes, I got that from the code. But I'm a bit confused about the comment treating the old allocbt blocks as free space, while the code doesn't. Or I guess the confusion is that we really have two slightly different notions of "free space": 1) the space we try to build the trees for 2) the space used as free space to us for the trees The old allocbt blocks are part of 1 but not of 2. > > - 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? > > xrep_abt_reserve_space returns -ENOSPC, so we throw away all the incore > records and throw the errno out to userspace. Across all the btree > rebuilding code, the block reservation step happens as the very last > thing before we start writing btree blocks, so it's still possible to > back out cleanly. But that means we can't repair the allocation btrees for this case. > > > + /* 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? > > No. Checking doesn't require the rmapbt because all we do is walk the > bnobt/cntbt records and cross-reference them with whatever metadata we > can find. Oh, and .has applies to both checking and repairing. Gt it. > This wrapper simplifes the interface to xrep_newbt_add_blocks so that > external callers don't have to know which magic values of xfs_alloc_arg > are actually used by xrep_newbt_add_blocks and therefore need to be set. > > For all the other repair functions, they have to allocate space from the > free space btree, so xrep_newbt_add_blocks is passed the full > _alloc_args as returned by the allocator to xrep_newbt_alloc_ag_blocks. Ok. Maybe throw in a comment that it just is a convenience wrapper?
