Re: [PATCH 5/7] xfs: implement block reservation accounting for btrees we're staging

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Oct 05, 2023 at 03:53:10PM +1100, Dave Chinner wrote:
> On Tue, Sep 26, 2023 at 04:32:25PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Create a new xrep_newbt structure to encapsulate a fake root for
> > creating a staged btree cursor as well as to track all the blocks that
> > we need to reserve in order to build that btree.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/Makefile                   |    1 
> >  fs/xfs/libxfs/xfs_btree_staging.h |    7 -
> >  fs/xfs/scrub/agheader_repair.c    |    1 
> >  fs/xfs/scrub/common.c             |    1 
> >  fs/xfs/scrub/newbt.c              |  492 +++++++++++++++++++++++++++++++++++++
> >  fs/xfs/scrub/newbt.h              |   62 +++++
> >  fs/xfs/scrub/scrub.c              |    2 
> >  fs/xfs/scrub/trace.h              |   37 +++
> >  8 files changed, 598 insertions(+), 5 deletions(-)
> >  create mode 100644 fs/xfs/scrub/newbt.c
> >  create mode 100644 fs/xfs/scrub/newbt.h
> 
> Looks reasonable to me. It all makes sense and nothing is obviously
> wrong.
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

Thanks!

> 
> Some notes on the extent allocation API bits - the rework of the
> high level allocation primitives I just posted intersects with this
> code in some interesting ways....
> 
> > +
> > +/* Allocate disk space for a new per-AG btree. */
> > +STATIC int
> > +xrep_newbt_alloc_ag_blocks(
> > +	struct xrep_newbt	*xnr,
> > +	uint64_t		nr_blocks)
> > +{
> > +	struct xfs_scrub	*sc = xnr->sc;
> > +	struct xfs_mount	*mp = sc->mp;
> > +	int			error = 0;
> > +
> > +	ASSERT(sc->sa.pag != NULL);
> > +
> > +	while (nr_blocks > 0) {
> > +		struct xfs_alloc_arg	args = {
> > +			.tp		= sc->tp,
> > +			.mp		= mp,
> > +			.oinfo		= xnr->oinfo,
> > +			.minlen		= 1,
> > +			.maxlen		= nr_blocks,
> > +			.prod		= 1,
> > +			.resv		= xnr->resv,
> > +		};
> > +		xfs_agnumber_t		agno;
> > +
> > +		xrep_newbt_validate_ag_alloc_hint(xnr);
> > +
> > +		error = xfs_alloc_vextent_near_bno(&args, xnr->alloc_hint);
> 
> This would require a perag to be held by the caller (sc->sa.pag)
> and attached to the args. The target also changes to an agbno
> (IIRC).

<nod> Pretty straightforward.

> > +		if (error)
> > +			return error;
> > +		if (args.fsbno == NULLFSBLOCK)
> > +			return -ENOSPC;
> 
> This will need to change to handling ENOSPC as the error directly on
> failure.

<nod>

> > +
> > +		agno = XFS_FSB_TO_AGNO(mp, args.fsbno);
> > +
> > +		trace_xrep_newbt_alloc_ag_blocks(mp, agno,
> > +				XFS_FSB_TO_AGBNO(mp, args.fsbno), args.len,
> > +				xnr->oinfo.oi_owner);
> > +
> > +		if (agno != sc->sa.pag->pag_agno) {
> > +			ASSERT(agno == sc->sa.pag->pag_agno);
> > +			return -EFSCORRUPTED;
> > +		}
> 
> This can go away, because it simply isn't possible - it will
> allocate a block in sc->sa.pag or fail with ENOSPC.
> 
> Hence this will probably simplify down a bit.

Yessssssss

> > +
> > +		error = xrep_newbt_add_blocks(xnr, sc->sa.pag, &args);
> > +		if (error)
> > +			return error;
> > +
> > +		nr_blocks -= args.len;
> > +		xnr->alloc_hint = args.fsbno + args.len;
> > +
> > +		error = xrep_defer_finish(sc);
> > +		if (error)
> > +			return error;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/* Don't let our allocation hint take us beyond EOFS */
> > +static inline void
> > +xrep_newbt_validate_file_alloc_hint(
> > +	struct xrep_newbt	*xnr)
> > +{
> > +	struct xfs_scrub	*sc = xnr->sc;
> > +
> > +	if (xfs_verify_fsbno(sc->mp, xnr->alloc_hint))
> > +		return;
> > +
> > +	xnr->alloc_hint = XFS_AGB_TO_FSB(sc->mp, 0, XFS_AGFL_BLOCK(sc->mp) + 1);
> > +}
> > +
> > +/* Allocate disk space for our new file-based btree. */
> > +STATIC int
> > +xrep_newbt_alloc_file_blocks(
> > +	struct xrep_newbt	*xnr,
> > +	uint64_t		nr_blocks)
> > +{
> > +	struct xfs_scrub	*sc = xnr->sc;
> > +	struct xfs_mount	*mp = sc->mp;
> > +	int			error = 0;
> > +
> > +	while (nr_blocks > 0) {
> > +		struct xfs_alloc_arg	args = {
> > +			.tp		= sc->tp,
> > +			.mp		= mp,
> > +			.oinfo		= xnr->oinfo,
> > +			.minlen		= 1,
> > +			.maxlen		= nr_blocks,
> > +			.prod		= 1,
> > +			.resv		= xnr->resv,
> > +		};
> > +		struct xfs_perag	*pag;
> > +		xfs_agnumber_t		agno;
> > +
> > +		xrep_newbt_validate_file_alloc_hint(xnr);
> > +
> > +		error = xfs_alloc_vextent_start_ag(&args, xnr->alloc_hint);
> > +		if (error)
> > +			return error;
> > +		if (args.fsbno == NULLFSBLOCK)
> > +			return -ENOSPC;
> 
> Similar target/errno changes will be needed here, and ....
> > +
> > +		agno = XFS_FSB_TO_AGNO(mp, args.fsbno);
> > +
> > +		trace_xrep_newbt_alloc_file_blocks(mp, agno,
> > +				XFS_FSB_TO_AGBNO(mp, args.fsbno), args.len,
> > +				xnr->oinfo.oi_owner);
> > +
> > +		pag = xfs_perag_get(mp, agno);
> > +		if (!pag) {
> > +			ASSERT(0);
> > +			return -EFSCORRUPTED;
> > +		}
> > +
> > +		error = xrep_newbt_add_blocks(xnr, pag, &args);
> > +		xfs_perag_put(pag);
> > +		if (error)
> > +			return error;
> 
> I suspect it might be useful to have xfs_alloc_vextent_start_ag() be
> able to return the referenced perag that the allocation occurred in
> rather than having to split the result and look it up again....

Yeah, I think it's reasonable to return an active(?) reference to the
perag that we picked and the space allocated from that AG.

> Hust a heads up for now, thought, we can deal with these issues when
> merging for one or the other happens...

Ok.

--D

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com