From: Zhang Yi <yi.zhang@huawei.com>
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@huawei.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
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
+__xfs_bmapi_convert_delalloc(
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.2On Tue, Mar 19, 2024 at 09:10:56AM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
>
> 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@huawei.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> 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@kernel.org>
--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
>
>
On 2024/3/20 4:45, Darrick J. Wong wrote:
> On Tue, Mar 19, 2024 at 09:10:56AM +0800, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> 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@huawei.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> ---
>> 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?
>
I'd suggest to leave that to the compiler too.
>> +__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);
> }
>
Thanks for your suggestions, all subsequent improvements in this series are
also looks fine by me, I will revise them in my next iteration. Christoph,
I will keep your review tag, please let me know if you have different
opinion.
Thanks,
Yi.
On Wed, Mar 20, 2024 at 09:51:26AM +0800, Zhang Yi wrote: > Thanks for your suggestions, all subsequent improvements in this series are > also looks fine by me, I will revise them in my next iteration. Christoph, > I will keep your review tag, please let me know if you have different > opinion. Yes, please keep them. All changes so far remain pretty trivial.
> > -xfs_bmapi_convert_delalloc( > > +static int > > static inline? I'd just leave that to the compiler, no need to second guess all the decisions. > 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: Fine with me.
© 2016 - 2026 Red Hat, Inc.