Re: [PATCH 03/42] xfs: block reservation too large for minleft allocation

Allison Henderson
2023-01-19T20:38:55Z

On Thu, 2023-01-19 at 09:44 +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> When we enter xfs_bmbt_alloc_block() without having first allocated
> a data extent (i.e. tp->t_firstblock == NULLFSBLOCK) because we
> are doing something like unwritten extent conversion, the transaction
> block reservation is used as the minleft value.
>
> This works for operations like unwritten extent conversion, but it
> assumes that the block reservation is only for a BMBT split. THis is
> not always true, and sometimes results in larger than necessary
> minleft values being set. We only actually need enough space for a
> btree split, something we already handle correctly in
> xfs_bmapi_write() via the xfs_bmapi_minleft() calculation.
>
> We should use xfs_bmapi_minleft() in xfs_bmbt_alloc_block() to
> calculate the number of blocks a BMBT split on this inode is going to
> require, not use the transaction block reservation that contains the
> maximum number of blocks this transaction may consume in it...
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
Ok, makes sense
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>  fs/xfs/libxfs/xfs_bmap.c       |  2 +-
>  fs/xfs/libxfs/xfs_bmap.h       |  2 ++
>  fs/xfs/libxfs/xfs_bmap_btree.c | 19 +++++++++----------
>  3 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 018837bd72c8..9dc33cdc2ab9 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4242,7 +4242,7 @@ xfs_bmapi_convert_unwritten(
>         return 0;
>  }
>
> -static inline xfs_extlen_t
> +xfs_extlen_t
>  xfs_bmapi_minleft(
>         struct xfs_trans        *tp,
>         struct xfs_inode        *ip,
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index 16db95b11589..08c16e4edc0f 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -220,6 +220,8 @@ int xfs_bmap_add_extent_unwritten_real(struct
> xfs_trans *tp,
>                 struct xfs_inode *ip, int whichfork,
>                 struct xfs_iext_cursor *icur, struct xfs_btree_cur
> **curp,
>                 struct xfs_bmbt_irec *new, int *logflagsp);
> +xfs_extlen_t xfs_bmapi_minleft(struct xfs_trans *tp, struct
> xfs_inode *ip,
> +               int fork);
>
>  enum xfs_bmap_intent_type {
>         XFS_BMAP_MAP = 1,
> diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c
> b/fs/xfs/libxfs/xfs_bmap_btree.c
> index cfa052d40105..18de4fbfef4e 100644
> --- a/fs/xfs/libxfs/xfs_bmap_btree.c
> +++ b/fs/xfs/libxfs/xfs_bmap_btree.c
> @@ -213,18 +213,16 @@ xfs_bmbt_alloc_block(
>         if (args.fsbno == NULLFSBLOCK) {
>                 args.fsbno = be64_to_cpu(start->l);
>                 args.type = XFS_ALLOCTYPE_START_BNO;
> +
>                 /*
> -                * Make sure there is sufficient room left in the AG
> to
> -                * complete a full tree split for an extent insert.
> If
> -                * we are converting the middle part of an extent
> then
> -                * we may need space for two tree splits.
> -                *
> -                * We are relying on the caller to make the correct
> block
> -                * reservation for this operation to succeed.  If the
> -                * reservation amount is insufficient then we may
> fail a
> -                * block allocation here and corrupt the filesystem.
> +                * If we are coming here from something like
> unwritten extent
> +                * conversion, there has been no data extent
> allocation already
> +                * done, so we have to ensure that we attempt to
> locate the
> +                * entire set of bmbt allocations in the same AG, as
> +                * xfs_bmapi_write() would have reserved.
>                  */
> -               args.minleft = args.tp->t_blk_res;
> +               args.minleft = xfs_bmapi_minleft(cur->bc_tp, cur-
> >bc_ino.ip,
> +                                               cur-
> >bc_ino.whichfork);
>         } else if (cur->bc_tp->t_flags & XFS_TRANS_LOWMODE) {
>                 args.type = XFS_ALLOCTYPE_START_BNO;
>         } else {
> @@ -248,6 +246,7 @@ xfs_bmbt_alloc_block(
>                  * successful activate the lowspace algorithm.
>                  */
>                 args.fsbno = 0;
> +               args.minleft = 0;
>                 args.type = XFS_ALLOCTYPE_FIRST_AG;
>                 error = xfs_alloc_vextent(&args);
>                 if (error)