diff mbox series

[v3,3/9] xfs: make xfs_bmapi_convert_delalloc() to allocate the target offset

Message ID 20240319011102.2929635-4-yi.zhang@huaweicloud.com (mailing list archive)
State New
Headers show
Series xfs/iomap: fix non-atomic clone operation and don't update size when zeroing range post eof | expand

Commit Message

Zhang Yi March 19, 2024, 1:10 a.m. UTC 
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(-)

Comments

Darrick J. Wong March 19, 2024, 8:45 p.m. UTC | #1 
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?

> +__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
> 
>
Christoph Hellwig March 19, 2024, 10:54 p.m. UTC | #2 
> > -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.
Zhang Yi March 20, 2024, 1:51 a.m. UTC | #3 
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.
Christoph Hellwig March 20, 2024, 1:57 a.m. UTC | #4 
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.
diff mbox series

Patch

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