On Fri, Sep 06, 2024 at 02:11:16 PM -0700, Catherine Hoang wrote:
> From: Christoph Hellwig <hch@xxxxxx>
>
> commit d69bee6a35d3c5e4873b9e164dd1a9711351a97c upstream.
>
> [backport: resolve conflict due to xfs_mod_freecounter refactor]
>
> xfs_bmap_add_extent_delay_real takes parts or all of a delalloc extent
> and converts them to a real extent. It is written to deal with any
> potential overlap of the to be converted range with the delalloc extent,
> but it turns out that currently only converting the entire extents, or a
> part starting at the beginning is actually exercised, as the only caller
> always tries to convert the entire delalloc extent, and either succeeds
> or at least progresses partially from the start.
>
> If it only converts a tiny part of a delalloc extent, the indirect block
> calculation for the new delalloc extent (da_new) might be equivalent to that
> of the existing delalloc extent (da_old). If this extent conversion now
> requires allocating an indirect block that gets accounted into da_new,
> leading to the assert that da_new must be smaller or equal to da_new
> unless we split the extent to trigger.
>
> Except for the assert that case is actually handled by just trying to
> allocate more space, as that already handled for the split case (which
> currently can't be reached at all), so just reusing it should be fine.
> Except that without dipping into the reserved block pool that would make
> it a bit too easy to trigger a fs shutdown due to ENOSPC. So in addition
> to adjusting the assert, also dip into the reserved block pool.
>
> Note that I could only reproduce the assert with a change to only convert
> the actually asked range instead of the full delalloc extent from
> xfs_bmapi_write.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Reviewed-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>
> Signed-off-by: Chandan Babu R <chandanbabu@xxxxxxxxxx>
> Signed-off-by: Catherine Hoang <catherine.hoang@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 97f575e21f86..0ac533a18065 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -1549,6 +1549,7 @@ xfs_bmap_add_extent_delay_real(
> if (error)
> goto done;
> }
> + ASSERT(da_new <= da_old);
> break;
>
> case BMAP_LEFT_FILLING | BMAP_RIGHT_FILLING | BMAP_LEFT_CONTIG:
> @@ -1578,6 +1579,7 @@ xfs_bmap_add_extent_delay_real(
> if (error)
> goto done;
> }
> + ASSERT(da_new <= da_old);
> break;
>
> case BMAP_LEFT_FILLING | BMAP_RIGHT_FILLING | BMAP_RIGHT_CONTIG:
> @@ -1611,6 +1613,7 @@ xfs_bmap_add_extent_delay_real(
> if (error)
> goto done;
> }
> + ASSERT(da_new <= da_old);
> break;
>
> case BMAP_LEFT_FILLING | BMAP_RIGHT_FILLING:
> @@ -1643,6 +1646,7 @@ xfs_bmap_add_extent_delay_real(
> goto done;
> }
> }
> + ASSERT(da_new <= da_old);
> break;
>
> case BMAP_LEFT_FILLING | BMAP_LEFT_CONTIG:
> @@ -1680,6 +1684,7 @@ xfs_bmap_add_extent_delay_real(
> if (error)
> goto done;
> }
> + ASSERT(da_new <= da_old);
> break;
>
> case BMAP_LEFT_FILLING:
> @@ -1767,6 +1772,7 @@ xfs_bmap_add_extent_delay_real(
> xfs_iext_update_extent(bma->ip, state, &bma->icur, &PREV);
> xfs_iext_next(ifp, &bma->icur);
> xfs_iext_update_extent(bma->ip, state, &bma->icur, &RIGHT);
> + ASSERT(da_new <= da_old);
> break;
>
> case BMAP_RIGHT_FILLING:
> @@ -1814,6 +1820,7 @@ xfs_bmap_add_extent_delay_real(
> PREV.br_blockcount = temp;
> xfs_iext_insert(bma->ip, &bma->icur, &PREV, state);
> xfs_iext_next(ifp, &bma->icur);
> + ASSERT(da_new <= da_old);
> break;
>
> case 0:
> @@ -1934,11 +1941,9 @@ xfs_bmap_add_extent_delay_real(
> }
>
> /* adjust for changes in reserved delayed indirect blocks */
> - if (da_new != da_old) {
> - ASSERT(state == 0 || da_new < da_old);
> + if (da_new != da_old)
> error = xfs_mod_fdblocks(mp, (int64_t)(da_old - da_new),
> false);
I think we need to allow space to be allocated from the reservation pool when
da_new > da_old. Hence, the last argument to xfs_mod_fdblocks() should
probably be true.
> - }
>
> xfs_bmap_check_leaf_extents(bma->cur, bma->ip, whichfork);
> done:
--
Chandan
