On Thu, 2023-01-19 at 09:44 +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
Ok, makes sense
Reviewed-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> ---
> 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)
