On Tue, Mar 19, 2024 at 09:10:56AM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
>
> Since xfs_bmapi_convert_delalloc() only attempts to allocate the entire
> delalloc extent and require multiple invocations to allocate the target
> offset. So xfs_convert_blocks() add a loop to do this job and we call it
> in the write back path, but xfs_convert_blocks() isn't a common helper.
> Let's do it in xfs_bmapi_convert_delalloc() and drop
> xfs_convert_blocks(), preparing for the post EOF delalloc blocks
> converting in the buffered write begin path.
>
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 34 +++++++++++++++++++++++--
> fs/xfs/xfs_aops.c | 54 +++++++++++-----------------------------
> 2 files changed, 46 insertions(+), 42 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 07dc35de8ce5..042e8d3ab0ba 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4516,8 +4516,8 @@ xfs_bmapi_write(
> * invocations to allocate the target offset if a large enough physical extent
> * is not available.
> */
> -int
> -xfs_bmapi_convert_delalloc(
> +static int
static inline?
> +__xfs_bmapi_convert_delalloc(
Double underscore prefixes read to me like "do this without grabbing
a lock or a resource", not just one step in a loop.
Would you mind changing it to xfs_bmapi_convert_one_delalloc() ?
Then the callsite looks like:
xfs_bmapi_convert_delalloc(...)
{
...
do {
error = xfs_bmapi_convert_one_delalloc(ip, whichfork, offset,
iomap, seq);
if (error)
return error;
} while (iomap->offset + iomap->length <= offset);
}
With that renamed,
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
> struct xfs_inode *ip,
> int whichfork,
> xfs_off_t offset,
> @@ -4648,6 +4648,36 @@ xfs_bmapi_convert_delalloc(
> return error;
> }
>
> +/*
> + * Pass in a dellalloc extent and convert it to real extents, return the real
> + * extent that maps offset_fsb in iomap.
> + */
> +int
> +xfs_bmapi_convert_delalloc(
> + struct xfs_inode *ip,
> + int whichfork,
> + loff_t offset,
> + struct iomap *iomap,
> + unsigned int *seq)
> +{
> + int error;
> +
> + /*
> + * Attempt to allocate whatever delalloc extent currently backs offset
> + * and put the result into iomap. Allocate in a loop because it may
> + * take several attempts to allocate real blocks for a contiguous
> + * delalloc extent if free space is sufficiently fragmented.
> + */
> + do {
> + error = __xfs_bmapi_convert_delalloc(ip, whichfork, offset,
> + iomap, seq);
> + if (error)
> + return error;
> + } while (iomap->offset + iomap->length <= offset);
> +
> + return 0;
> +}
> +
> int
> xfs_bmapi_remap(
> struct xfs_trans *tp,
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 813f85156b0c..6479e0dac69d 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -233,45 +233,6 @@ xfs_imap_valid(
> return true;
> }
>
> -/*
> - * Pass in a dellalloc extent and convert it to real extents, return the real
> - * extent that maps offset_fsb in wpc->iomap.
> - *
> - * The current page is held locked so nothing could have removed the block
> - * backing offset_fsb, although it could have moved from the COW to the data
> - * fork by another thread.
> - */
> -static int
> -xfs_convert_blocks(
> - struct iomap_writepage_ctx *wpc,
> - struct xfs_inode *ip,
> - int whichfork,
> - loff_t offset)
> -{
> - int error;
> - unsigned *seq;
> -
> - if (whichfork == XFS_COW_FORK)
> - seq = &XFS_WPC(wpc)->cow_seq;
> - else
> - seq = &XFS_WPC(wpc)->data_seq;
> -
> - /*
> - * Attempt to allocate whatever delalloc extent currently backs offset
> - * and put the result into wpc->iomap. Allocate in a loop because it
> - * may take several attempts to allocate real blocks for a contiguous
> - * delalloc extent if free space is sufficiently fragmented.
> - */
> - do {
> - error = xfs_bmapi_convert_delalloc(ip, whichfork, offset,
> - &wpc->iomap, seq);
> - if (error)
> - return error;
> - } while (wpc->iomap.offset + wpc->iomap.length <= offset);
> -
> - return 0;
> -}
> -
> static int
> xfs_map_blocks(
> struct iomap_writepage_ctx *wpc,
> @@ -289,6 +250,7 @@ xfs_map_blocks(
> struct xfs_iext_cursor icur;
> int retries = 0;
> int error = 0;
> + unsigned int *seq;
>
> if (xfs_is_shutdown(mp))
> return -EIO;
> @@ -386,7 +348,19 @@ xfs_map_blocks(
> trace_xfs_map_blocks_found(ip, offset, count, whichfork, &imap);
> return 0;
> allocate_blocks:
> - error = xfs_convert_blocks(wpc, ip, whichfork, offset);
> + /*
> + * Convert a dellalloc extent to a real one. The current page is held
> + * locked so nothing could have removed the block backing offset_fsb,
> + * although it could have moved from the COW to the data fork by another
> + * thread.
> + */
> + if (whichfork == XFS_COW_FORK)
> + seq = &XFS_WPC(wpc)->cow_seq;
> + else
> + seq = &XFS_WPC(wpc)->data_seq;
> +
> + error = xfs_bmapi_convert_delalloc(ip, whichfork, offset,
> + &wpc->iomap, seq);
> if (error) {
> /*
> * If we failed to find the extent in the COW fork we might have
> --
> 2.39.2
>
>
