On Mon, Oct 09, 2023 at 12:30:20PM +0200, Christoph Hellwig wrote:
> If xfs_bmapi_write finds a delalloc extent at the requested range, it
> tries to convert the entire delalloc extent to a real allocation.
> But if the allocator then can't find an AG with enough space to at least
> cover the start block of the requested range, xfs_bmapi_write will return
> 0 but leave *nimaps set to 0.
> In that case we simply need to keep looping with the same startoffset_fsb.
>
> Note that this could affect any caller of xfs_bmapi_write that covers
> an existing delayed allocation. As far as I can tell we do not have
> any other such caller, though - the regular writeback path uses
> xfs_bmapi_convert_delalloc to convert delayed allocations to real ones,
> and direct I/O invalidates the page cache first.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/xfs_bmap_util.c | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index d85580b101ad8a..556f57f757f33e 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -814,12 +814,10 @@ xfs_alloc_file_space(
> {
> xfs_mount_t *mp = ip->i_mount;
> xfs_off_t count;
> - xfs_filblks_t allocated_fsb;
> xfs_filblks_t allocatesize_fsb;
> xfs_extlen_t extsz, temp;
> xfs_fileoff_t startoffset_fsb;
> xfs_fileoff_t endoffset_fsb;
> - int nimaps;
> int rt;
> xfs_trans_t *tp;
> xfs_bmbt_irec_t imaps[1], *imapp;
> @@ -842,7 +840,6 @@ xfs_alloc_file_space(
>
> count = len;
> imapp = &imaps[0];
> - nimaps = 1;
> startoffset_fsb = XFS_B_TO_FSBT(mp, offset);
> endoffset_fsb = XFS_B_TO_FSB(mp, offset + count);
> allocatesize_fsb = endoffset_fsb - startoffset_fsb;
> @@ -853,6 +850,7 @@ xfs_alloc_file_space(
> while (allocatesize_fsb && !error) {
> xfs_fileoff_t s, e;
> unsigned int dblocks, rblocks, resblks;
> + int nimaps = 1;
>
> /*
> * Determine space reservations for data/realtime.
> @@ -918,15 +916,20 @@ xfs_alloc_file_space(
> if (error)
> break;
>
> - allocated_fsb = imapp->br_blockcount;
> -
> - if (nimaps == 0) {
> - error = -ENOSPC;
> - break;
> + /*
> + * If xfs_bmapi_write finds a delalloc extent at the requested
> + * range, it tries to convert the entire delalloc extent to a
> + * real allocation.
> + * But if the allocator then can't find an AG with enough space
> + * to at least cover the start block of the requested range,
Hmm. Given that you said this was done in the context of delalloc on
the realtime volume, I don't think there are AGs in play here? Unless
the AG actually ran out of space allocating a bmbt block?
My hunch here is that free space on the rt volume is fragmented, but
there were still enough free rtextents to create a large delalloc
reservation. Conversion of the reservation to an unwritten extent
managed to map one free rtextent into the file, but not enough to
convert the file mapping all the way to @startoffset_fsb. Hence the
bmapi_write call succeeds, but returns @nmaps == 0.
If that's true, I suggest changing the second sentence of the comment to
read:
"If the allocator cannot find a single free extent large enough to
cover the start block of the requested range, xfs_bmapi_write will
return 0 but leave *nimaps set to 0."
I agree with the fix -- calling bmapi_write again with the same
startoffset_fsb will return the mapping of the space that /did/ get
allocated, which enables us to push @startoffset_fsb forward.
--D
> + * xfs_bmapi_write will return 0 but leave *nimaps set to 0.
> + * In that case we simply need to keep looping with the same
> + * startoffset_fsb.
> + */
> + if (nimaps) {
> + startoffset_fsb += imapp->br_blockcount;
> + allocatesize_fsb -= imapp->br_blockcount;
> }
> -
> - startoffset_fsb += allocated_fsb;
> - allocatesize_fsb -= allocated_fsb;
> }
>
> return error;
> --
> 2.39.2
>
