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

From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 5/7] xfs: implement block reservation accounting for btrees we're staging
Date: Thu, 5 Oct 2023 15:53:10 +1100	[thread overview]
Message-ID: <ZR5BNt6BfBcpp1c+@dread.disaster.area> (raw)
In-Reply-To: <169577059224.3312911.3596538645136769266.stgit@frogsfrogsfrogs>

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>

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).

> +		if (error)
> +			return error;
> +		if (args.fsbno == NULLFSBLOCK)
> +			return -ENOSPC;

This will need to change to handling ENOSPC as the error directly on
failure.

> +
> +		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.

> +
> +		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....

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

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