On Mon, Apr 29, 2024 at 08:15:27AM +0200, Christoph Hellwig wrote:
> 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>
Looks good now,
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
> ---
> fs/xfs/libxfs/xfs_bmap.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 472c795beb8add..42c5a2efa656a5 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -1570,6 +1570,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:
> @@ -1600,6 +1601,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:
> @@ -1634,6 +1636,7 @@ xfs_bmap_add_extent_delay_real(
> if (error)
> goto done;
> }
> + ASSERT(da_new <= da_old);
> break;
>
> case BMAP_LEFT_FILLING | BMAP_RIGHT_FILLING:
> @@ -1668,6 +1671,7 @@ xfs_bmap_add_extent_delay_real(
> goto done;
> }
> }
> + ASSERT(da_new <= da_old);
> break;
>
> case BMAP_LEFT_FILLING | BMAP_LEFT_CONTIG:
> @@ -1706,6 +1710,7 @@ xfs_bmap_add_extent_delay_real(
> if (error)
> goto done;
> }
> + ASSERT(da_new <= da_old);
> break;
>
> case BMAP_LEFT_FILLING:
> @@ -1796,6 +1801,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:
> @@ -1845,6 +1851,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:
> @@ -1967,12 +1974,10 @@ xfs_bmap_add_extent_delay_real(
> }
>
> /* adjust for changes in reserved delayed indirect blocks */
> - if (da_new < da_old) {
> + if (da_new < da_old)
> xfs_add_fdblocks(mp, da_old - da_new);
> - } else if (da_new > da_old) {
> - ASSERT(state == 0);
> - error = xfs_dec_fdblocks(mp, da_new - da_old, false);
> - }
> + else if (da_new > da_old)
> + error = xfs_dec_fdblocks(mp, da_new - da_old, true);
>
> xfs_bmap_check_leaf_extents(bma->cur, bma->ip, whichfork);
> done:
> --
> 2.39.2
>
>
