[PATCH v2 0/2] btrfs: fix the length of reserved qgroup to free

[PATCH v2 0/2] btrfs: fix the length of reserved qgroup to free

[iamhswang]

From: Haisu Wang <haisuwang@tencent.com>

This patch set fixes the inconsistent region size of qgroup data.

The first patch ("btrfs: fix the length of reserved qgroup to free")
is enough to work together with the fix of CVE-2024-46733 to port
to all effected stable release branches.
The second patch is aim to make the reserved/alloced region more clear
to ease the error handling clean up. The start mark no longer advanced
in error handling, also the cur_alloc_size can represent the ram size
and dealloc area.

I am able to run fstest generic/475 for hundred times with quota enabled,
half of the tests modified by removing sleep time. About one tenth of
the tests are enter to the error handling process due to fail to reserve
extent. Though I didin't find a proper reproducer to enter all possible
error conditions to simulate alloc/checksum failure.

[CHANGELOG]
V2:
- Clear the alloc and error handling path and keep the start unchanged
- Patch ("btrfs: fix the length of reserved qgroup to free") unchanged
  to make CVE-2024-46733 related fix as simple as possible

V1:
Adjust the length of untouch region to free.
https://lore.kernel.org/linux-btrfs/20241008064849.1814829-1-haisuwang@tencent.com/T/#u

Haisu Wang (2):
  btrfs: fix the length of reserved qgroup to free
  btrfs: simplify regions mark and keep start unchanged in err handling

 fs/btrfs/inode.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

-- 
2.43.5

[PATCH 1/2] btrfs: fix the length of reserved qgroup to free

[iamhswang]

From: Haisu Wang <haisuwang@tencent.com>

The dealloc flag may be cleared and the extent won't reach the disk
in cow_file_range when errors path. The reserved qgroup space is
freed in commit 30479f31d44d ("btrfs: fix qgroup reserve leaks in
cow_file_range"). However, the length of untouched region to free
need to be adjusted with the region size.

Fixes: 30479f31d44d ("btrfs: fix qgroup reserve leaks in cow_file_range")
Reviewed-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Haisu Wang <haisuwang@tencent.com>
---
 fs/btrfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 5618ca02934a..3646734a7e59 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1618,7 +1618,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 		clear_bits |= EXTENT_CLEAR_DATA_RESV;
 		extent_clear_unlock_delalloc(inode, start, end, locked_folio,
 					     &cached, clear_bits, page_ops);
-		btrfs_qgroup_free_data(inode, NULL, start, cur_alloc_size, NULL);
+		btrfs_qgroup_free_data(inode, NULL, start, end - start + 1, NULL);
 	}
 	return ret;
 }
-- 
2.43.5

[PATCH 2/2] btrfs: simplify regions mark and keep start unchanged in err handling

[iamhswang]

From: Haisu Wang <haisuwang@tencent.com>

Simplify the regions mark by using cur_alloc_size only to present
the reserved but may failed to alloced extent. Remove the ram_size
as well since it is always consistent to the cur_alloc_size in the
context. Advanced the start mark in normal path until extent succeed
alloced and keep the start unchanged in error handling path.

PASSed the fstest generic/475 test for a hundred times with quota
enabled. And a modified generic/475 test by removing the sleep time
for a hundred times. About one tenth of the tests do enter the error
handling path due to fail to reserve extent.

Suggested-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Haisu Wang <haisuwang@tencent.com>
---
 fs/btrfs/inode.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3646734a7e59..7e67a6d50be2 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1359,7 +1359,6 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 	u64 alloc_hint = 0;
 	u64 orig_start = start;
 	u64 num_bytes;
-	unsigned long ram_size;
 	u64 cur_alloc_size = 0;
 	u64 min_alloc_size;
 	u64 blocksize = fs_info->sectorsize;
@@ -1367,7 +1366,6 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 	struct extent_map *em;
 	unsigned clear_bits;
 	unsigned long page_ops;
-	bool extent_reserved = false;
 	int ret = 0;
 
 	if (btrfs_is_free_space_inode(inode)) {
@@ -1421,8 +1419,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 		struct btrfs_ordered_extent *ordered;
 		struct btrfs_file_extent file_extent;
 
-		cur_alloc_size = num_bytes;
-		ret = btrfs_reserve_extent(root, cur_alloc_size, cur_alloc_size,
+		ret = btrfs_reserve_extent(root, num_bytes, num_bytes,
 					   min_alloc_size, 0, alloc_hint,
 					   &ins, 1, 1);
 		if (ret == -EAGAIN) {
@@ -1453,9 +1450,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 		if (ret < 0)
 			goto out_unlock;
 		cur_alloc_size = ins.offset;
-		extent_reserved = true;
 
-		ram_size = ins.offset;
 		file_extent.disk_bytenr = ins.objectid;
 		file_extent.disk_num_bytes = ins.offset;
 		file_extent.num_bytes = ins.offset;
@@ -1463,14 +1458,14 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 		file_extent.offset = 0;
 		file_extent.compression = BTRFS_COMPRESS_NONE;
 
-		lock_extent(&inode->io_tree, start, start + ram_size - 1,
+		lock_extent(&inode->io_tree, start, start + cur_alloc_size - 1,
 			    &cached);
 
 		em = btrfs_create_io_em(inode, start, &file_extent,
 					BTRFS_ORDERED_REGULAR);
 		if (IS_ERR(em)) {
 			unlock_extent(&inode->io_tree, start,
-				      start + ram_size - 1, &cached);
+				      start + cur_alloc_size - 1, &cached);
 			ret = PTR_ERR(em);
 			goto out_reserve;
 		}
@@ -1480,7 +1475,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 						     1 << BTRFS_ORDERED_REGULAR);
 		if (IS_ERR(ordered)) {
 			unlock_extent(&inode->io_tree, start,
-				      start + ram_size - 1, &cached);
+				      start + cur_alloc_size - 1, &cached);
 			ret = PTR_ERR(ordered);
 			goto out_drop_extent_cache;
 		}
@@ -1501,7 +1496,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 			 */
 			if (ret)
 				btrfs_drop_extent_map_range(inode, start,
-							    start + ram_size - 1,
+							    start + cur_alloc_size - 1,
 							    false);
 		}
 		btrfs_put_ordered_extent(ordered);
@@ -1519,7 +1514,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 		page_ops = (keep_locked ? 0 : PAGE_UNLOCK);
 		page_ops |= PAGE_SET_ORDERED;
 
-		extent_clear_unlock_delalloc(inode, start, start + ram_size - 1,
+		extent_clear_unlock_delalloc(inode, start, start + cur_alloc_size - 1,
 					     locked_folio, &cached,
 					     EXTENT_LOCKED | EXTENT_DELALLOC,
 					     page_ops);
@@ -1529,7 +1524,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 			num_bytes -= cur_alloc_size;
 		alloc_hint = ins.objectid + ins.offset;
 		start += cur_alloc_size;
-		extent_reserved = false;
+		cur_alloc_size = 0;
 
 		/*
 		 * btrfs_reloc_clone_csums() error, since start is increased
@@ -1545,7 +1540,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 	return ret;
 
 out_drop_extent_cache:
-	btrfs_drop_extent_map_range(inode, start, start + ram_size - 1, false);
+	btrfs_drop_extent_map_range(inode, start, start + cur_alloc_size - 1, false);
 out_reserve:
 	btrfs_dec_block_group_reservations(fs_info, ins.objectid);
 	btrfs_free_reserved_extent(fs_info, ins.objectid, ins.offset, 1);
@@ -1599,13 +1594,12 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 	 * to decrement again the data space_info's bytes_may_use counter,
 	 * therefore we do not pass it the flag EXTENT_CLEAR_DATA_RESV.
 	 */
-	if (extent_reserved) {
+	if (cur_alloc_size) {
 		extent_clear_unlock_delalloc(inode, start,
 					     start + cur_alloc_size - 1,
 					     locked_folio, &cached, clear_bits,
 					     page_ops);
 		btrfs_qgroup_free_data(inode, NULL, start, cur_alloc_size, NULL);
-		start += cur_alloc_size;
 	}
 
 	/*
@@ -1614,11 +1608,13 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
 	 * space_info's bytes_may_use counter, reserved in
 	 * btrfs_check_data_free_space().
 	 */
-	if (start < end) {
+	if (start + cur_alloc_size < end) {
 		clear_bits |= EXTENT_CLEAR_DATA_RESV;
-		extent_clear_unlock_delalloc(inode, start, end, locked_folio,
+		extent_clear_unlock_delalloc(inode, start + cur_alloc_size,
+					     end, locked_folio,
 					     &cached, clear_bits, page_ops);
-		btrfs_qgroup_free_data(inode, NULL, start, end - start + 1, NULL);
+		btrfs_qgroup_free_data(inode, NULL, start + cur_alloc_size,
+				end - start - cur_alloc_size + 1, NULL);
 	}
 	return ret;
 }
-- 
2.43.5

Re: [PATCH v2 0/2] btrfs: fix the length of reserved qgroup to free

[David Sterba]

On Fri, Oct 25, 2024 at 02:54:39PM +0800, iamhswang@gmail.com wrote:
> From: Haisu Wang <haisuwang@tencent.com>
> 
> This patch set fixes the inconsistent region size of qgroup data.
> 
> The first patch ("btrfs: fix the length of reserved qgroup to free")
> is enough to work together with the fix of CVE-2024-46733 to port
> to all effected stable release branches.
> The second patch is aim to make the reserved/alloced region more clear
> to ease the error handling clean up. The start mark no longer advanced
> in error handling, also the cur_alloc_size can represent the ram size
> and dealloc area.
> 
> I am able to run fstest generic/475 for hundred times with quota enabled,
> half of the tests modified by removing sleep time. About one tenth of
> the tests are enter to the error handling process due to fail to reserve
> extent. Though I didin't find a proper reproducer to enter all possible
> error conditions to simulate alloc/checksum failure.
> 
> [CHANGELOG]
> V2:
> - Clear the alloc and error handling path and keep the start unchanged
> - Patch ("btrfs: fix the length of reserved qgroup to free") unchanged
>   to make CVE-2024-46733 related fix as simple as possible
> 
> V1:
> Adjust the length of untouch region to free.
> https://lore.kernel.org/linux-btrfs/20241008064849.1814829-1-haisuwang@tencent.com/T/#u
> 
> Haisu Wang (2):
>   btrfs: fix the length of reserved qgroup to free
>   btrfs: simplify regions mark and keep start unchanged in err handling

Thanks, patches added to for-next, with some minor adjustments.

Re: [PATCH 2/2] btrfs: simplify regions mark and keep start unchanged in err handling

[Qu Wenruo]

在 2024/10/25 17:24, iamhswang@gmail.com 写道:
> From: Haisu Wang <haisuwang@tencent.com>
>
> Simplify the regions mark by using cur_alloc_size only to present
> the reserved but may failed to alloced extent. Remove the ram_size
> as well since it is always consistent to the cur_alloc_size in the
> context. Advanced the start mark in normal path until extent succeed
> alloced and keep the start unchanged in error handling path.
>
> PASSed the fstest generic/475 test for a hundred times with quota
> enabled. And a modified generic/475 test by removing the sleep time
> for a hundred times. About one tenth of the tests do enter the error
> handling path due to fail to reserve extent.
>

Although this patch is already merged into for-next, it looks like the
next patch will again change the error handling, mostly render the this
one useless:

https://lore.kernel.org/linux-btrfs/2a0925f0264daf90741ed0a7ba7ed4b4888cf778.1728725060.git.wqu@suse.com/

The newer patch will change the error handling to a simpler one, so
instead of 3 regions, there will be only 2.

There will be no change needed from your side, I will update my patches
to solve the conflicts, just in case if you find the error handling is
different in the future.

Thanks,
Qu

> Suggested-by: Qu Wenruo <wqu@suse.com>
> Signed-off-by: Haisu Wang <haisuwang@tencent.com>
> ---
>   fs/btrfs/inode.c | 32 ++++++++++++++------------------
>   1 file changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 3646734a7e59..7e67a6d50be2 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1359,7 +1359,6 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
>   	u64 alloc_hint = 0;
>   	u64 orig_start = start;
>   	u64 num_bytes;
> -	unsigned long ram_size;
>   	u64 cur_alloc_size = 0;
>   	u64 min_alloc_size;
>   	u64 blocksize = fs_info->sectorsize;
> @@ -1367,7 +1366,6 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
>   	struct extent_map *em;
>   	unsigned clear_bits;
>   	unsigned long page_ops;
> -	bool extent_reserved = false;
>   	int ret = 0;
>
>   	if (btrfs_is_free_space_inode(inode)) {
> @@ -1421,8 +1419,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
>   		struct btrfs_ordered_extent *ordered;
>   		struct btrfs_file_extent file_extent;
>
> -		cur_alloc_size = num_bytes;
> -		ret = btrfs_reserve_extent(root, cur_alloc_size, cur_alloc_size,
> +		ret = btrfs_reserve_extent(root, num_bytes, num_bytes,
>   					   min_alloc_size, 0, alloc_hint,
>   					   &ins, 1, 1);
>   		if (ret == -EAGAIN) {
> @@ -1453,9 +1450,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
>   		if (ret < 0)
>   			goto out_unlock;
>   		cur_alloc_size = ins.offset;
> -		extent_reserved = true;
>
> -		ram_size = ins.offset;
>   		file_extent.disk_bytenr = ins.objectid;
>   		file_extent.disk_num_bytes = ins.offset;
>   		file_extent.num_bytes = ins.offset;
> @@ -1463,14 +1458,14 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
>   		file_extent.offset = 0;
>   		file_extent.compression = BTRFS_COMPRESS_NONE;
>
> -		lock_extent(&inode->io_tree, start, start + ram_size - 1,
> +		lock_extent(&inode->io_tree, start, start + cur_alloc_size - 1,
>   			    &cached);
>
>   		em = btrfs_create_io_em(inode, start, &file_extent,
>   					BTRFS_ORDERED_REGULAR);
>   		if (IS_ERR(em)) {
>   			unlock_extent(&inode->io_tree, start,
> -				      start + ram_size - 1, &cached);
> +				      start + cur_alloc_size - 1, &cached);
>   			ret = PTR_ERR(em);
>   			goto out_reserve;
>   		}
> @@ -1480,7 +1475,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
>   						     1 << BTRFS_ORDERED_REGULAR);
>   		if (IS_ERR(ordered)) {
>   			unlock_extent(&inode->io_tree, start,
> -				      start + ram_size - 1, &cached);
> +				      start + cur_alloc_size - 1, &cached);
>   			ret = PTR_ERR(ordered);
>   			goto out_drop_extent_cache;
>   		}
> @@ -1501,7 +1496,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
>   			 */
>   			if (ret)
>   				btrfs_drop_extent_map_range(inode, start,
> -							    start + ram_size - 1,
> +							    start + cur_alloc_size - 1,
>   							    false);
>   		}
>   		btrfs_put_ordered_extent(ordered);
> @@ -1519,7 +1514,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
>   		page_ops = (keep_locked ? 0 : PAGE_UNLOCK);
>   		page_ops |= PAGE_SET_ORDERED;
>
> -		extent_clear_unlock_delalloc(inode, start, start + ram_size - 1,
> +		extent_clear_unlock_delalloc(inode, start, start + cur_alloc_size - 1,
>   					     locked_folio, &cached,
>   					     EXTENT_LOCKED | EXTENT_DELALLOC,
>   					     page_ops);
> @@ -1529,7 +1524,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
>   			num_bytes -= cur_alloc_size;
>   		alloc_hint = ins.objectid + ins.offset;
>   		start += cur_alloc_size;
> -		extent_reserved = false;
> +		cur_alloc_size = 0;
>
>   		/*
>   		 * btrfs_reloc_clone_csums() error, since start is increased
> @@ -1545,7 +1540,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
>   	return ret;
>
>   out_drop_extent_cache:
> -	btrfs_drop_extent_map_range(inode, start, start + ram_size - 1, false);
> +	btrfs_drop_extent_map_range(inode, start, start + cur_alloc_size - 1, false);
>   out_reserve:
>   	btrfs_dec_block_group_reservations(fs_info, ins.objectid);
>   	btrfs_free_reserved_extent(fs_info, ins.objectid, ins.offset, 1);
> @@ -1599,13 +1594,12 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
>   	 * to decrement again the data space_info's bytes_may_use counter,
>   	 * therefore we do not pass it the flag EXTENT_CLEAR_DATA_RESV.
>   	 */
> -	if (extent_reserved) {
> +	if (cur_alloc_size) {
>   		extent_clear_unlock_delalloc(inode, start,
>   					     start + cur_alloc_size - 1,
>   					     locked_folio, &cached, clear_bits,
>   					     page_ops);
>   		btrfs_qgroup_free_data(inode, NULL, start, cur_alloc_size, NULL);
> -		start += cur_alloc_size;
>   	}
>
>   	/*
> @@ -1614,11 +1608,13 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
>   	 * space_info's bytes_may_use counter, reserved in
>   	 * btrfs_check_data_free_space().
>   	 */
> -	if (start < end) {
> +	if (start + cur_alloc_size < end) {
>   		clear_bits |= EXTENT_CLEAR_DATA_RESV;
> -		extent_clear_unlock_delalloc(inode, start, end, locked_folio,
> +		extent_clear_unlock_delalloc(inode, start + cur_alloc_size,
> +					     end, locked_folio,
>   					     &cached, clear_bits, page_ops);
> -		btrfs_qgroup_free_data(inode, NULL, start, end - start + 1, NULL);
> +		btrfs_qgroup_free_data(inode, NULL, start + cur_alloc_size,
> +				end - start - cur_alloc_size + 1, NULL);
>   	}
>   	return ret;
>   }

Re: [PATCH 2/2] btrfs: simplify regions mark and keep start unchanged in err handling

[hs wang]

Qu Wenruo <quwenruo.btrfs@gmx.com> 于2024年10月30日周三 11:01写道：
>
>
>
> 在 2024/10/25 17:24, iamhswang@gmail.com 写道:
> > From: Haisu Wang <haisuwang@tencent.com>
> >
> > Simplify the regions mark by using cur_alloc_size only to present
> > the reserved but may failed to alloced extent. Remove the ram_size
> > as well since it is always consistent to the cur_alloc_size in the
> > context. Advanced the start mark in normal path until extent succeed
> > alloced and keep the start unchanged in error handling path.
> >
> > PASSed the fstest generic/475 test for a hundred times with quota
> > enabled. And a modified generic/475 test by removing the sleep time
> > for a hundred times. About one tenth of the tests do enter the error
> > handling path due to fail to reserve extent.
> >
>
> Although this patch is already merged into for-next, it looks like the
> next patch will again change the error handling, mostly render the this
> one useless:
>
> https://lore.kernel.org/linux-btrfs/2a0925f0264daf90741ed0a7ba7ed4b4888cf778.1728725060.git.wqu@suse.com/
>
> The newer patch will change the error handling to a simpler one, so
> instead of 3 regions, there will be only 2.
>
Sounds better and a completely rewrite, i will catch up the details.

> There will be no change needed from your side, I will update my patches
> to solve the conflicts, just in case if you find the error handling is
> different in the future.
>

Thanks. Understanding the changes, may compile and play after the polishment.

Best regards,
Haisu Wang

> Thanks,
> Qu
>
> > Suggested-by: Qu Wenruo <wqu@suse.com>
> > Signed-off-by: Haisu Wang <haisuwang@tencent.com>
> > ---
> >   fs/btrfs/inode.c | 32 ++++++++++++++------------------
> >   1 file changed, 14 insertions(+), 18 deletions(-)
> >
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 3646734a7e59..7e67a6d50be2 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -1359,7 +1359,6 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> >       u64 alloc_hint = 0;
> >       u64 orig_start = start;
> >       u64 num_bytes;
> > -     unsigned long ram_size;
> >       u64 cur_alloc_size = 0;
> >       u64 min_alloc_size;
> >       u64 blocksize = fs_info->sectorsize;
> > @@ -1367,7 +1366,6 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> >       struct extent_map *em;
> >       unsigned clear_bits;
> >       unsigned long page_ops;
> > -     bool extent_reserved = false;
> >       int ret = 0;
> >
> >       if (btrfs_is_free_space_inode(inode)) {
> > @@ -1421,8 +1419,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> >               struct btrfs_ordered_extent *ordered;
> >               struct btrfs_file_extent file_extent;
> >
> > -             cur_alloc_size = num_bytes;
> > -             ret = btrfs_reserve_extent(root, cur_alloc_size, cur_alloc_size,
> > +             ret = btrfs_reserve_extent(root, num_bytes, num_bytes,
> >                                          min_alloc_size, 0, alloc_hint,
> >                                          &ins, 1, 1);
> >               if (ret == -EAGAIN) {
> > @@ -1453,9 +1450,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> >               if (ret < 0)
> >                       goto out_unlock;
> >               cur_alloc_size = ins.offset;
> > -             extent_reserved = true;
> >
> > -             ram_size = ins.offset;
> >               file_extent.disk_bytenr = ins.objectid;
> >               file_extent.disk_num_bytes = ins.offset;
> >               file_extent.num_bytes = ins.offset;
> > @@ -1463,14 +1458,14 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> >               file_extent.offset = 0;
> >               file_extent.compression = BTRFS_COMPRESS_NONE;
> >
> > -             lock_extent(&inode->io_tree, start, start + ram_size - 1,
> > +             lock_extent(&inode->io_tree, start, start + cur_alloc_size - 1,
> >                           &cached);
> >
> >               em = btrfs_create_io_em(inode, start, &file_extent,
> >                                       BTRFS_ORDERED_REGULAR);
> >               if (IS_ERR(em)) {
> >                       unlock_extent(&inode->io_tree, start,
> > -                                   start + ram_size - 1, &cached);
> > +                                   start + cur_alloc_size - 1, &cached);
> >                       ret = PTR_ERR(em);
> >                       goto out_reserve;
> >               }
> > @@ -1480,7 +1475,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> >                                                    1 << BTRFS_ORDERED_REGULAR);
> >               if (IS_ERR(ordered)) {
> >                       unlock_extent(&inode->io_tree, start,
> > -                                   start + ram_size - 1, &cached);
> > +                                   start + cur_alloc_size - 1, &cached);
> >                       ret = PTR_ERR(ordered);
> >                       goto out_drop_extent_cache;
> >               }
> > @@ -1501,7 +1496,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> >                        */
> >                       if (ret)
> >                               btrfs_drop_extent_map_range(inode, start,
> > -                                                         start + ram_size - 1,
> > +                                                         start + cur_alloc_size - 1,
> >                                                           false);
> >               }
> >               btrfs_put_ordered_extent(ordered);
> > @@ -1519,7 +1514,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> >               page_ops = (keep_locked ? 0 : PAGE_UNLOCK);
> >               page_ops |= PAGE_SET_ORDERED;
> >
> > -             extent_clear_unlock_delalloc(inode, start, start + ram_size - 1,
> > +             extent_clear_unlock_delalloc(inode, start, start + cur_alloc_size - 1,
> >                                            locked_folio, &cached,
> >                                            EXTENT_LOCKED | EXTENT_DELALLOC,
> >                                            page_ops);
> > @@ -1529,7 +1524,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> >                       num_bytes -= cur_alloc_size;
> >               alloc_hint = ins.objectid + ins.offset;
> >               start += cur_alloc_size;
> > -             extent_reserved = false;
> > +             cur_alloc_size = 0;
> >
> >               /*
> >                * btrfs_reloc_clone_csums() error, since start is increased
> > @@ -1545,7 +1540,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> >       return ret;
> >
> >   out_drop_extent_cache:
> > -     btrfs_drop_extent_map_range(inode, start, start + ram_size - 1, false);
> > +     btrfs_drop_extent_map_range(inode, start, start + cur_alloc_size - 1, false);
> >   out_reserve:
> >       btrfs_dec_block_group_reservations(fs_info, ins.objectid);
> >       btrfs_free_reserved_extent(fs_info, ins.objectid, ins.offset, 1);
> > @@ -1599,13 +1594,12 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> >        * to decrement again the data space_info's bytes_may_use counter,
> >        * therefore we do not pass it the flag EXTENT_CLEAR_DATA_RESV.
> >        */
> > -     if (extent_reserved) {
> > +     if (cur_alloc_size) {
> >               extent_clear_unlock_delalloc(inode, start,
> >                                            start + cur_alloc_size - 1,
> >                                            locked_folio, &cached, clear_bits,
> >                                            page_ops);
> >               btrfs_qgroup_free_data(inode, NULL, start, cur_alloc_size, NULL);
> > -             start += cur_alloc_size;
> >       }
> >
> >       /*
> > @@ -1614,11 +1608,13 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> >        * space_info's bytes_may_use counter, reserved in
> >        * btrfs_check_data_free_space().
> >        */
> > -     if (start < end) {
> > +     if (start + cur_alloc_size < end) {
> >               clear_bits |= EXTENT_CLEAR_DATA_RESV;
> > -             extent_clear_unlock_delalloc(inode, start, end, locked_folio,
> > +             extent_clear_unlock_delalloc(inode, start + cur_alloc_size,
> > +                                          end, locked_folio,
> >                                            &cached, clear_bits, page_ops);
> > -             btrfs_qgroup_free_data(inode, NULL, start, end - start + 1, NULL);
> > +             btrfs_qgroup_free_data(inode, NULL, start + cur_alloc_size,
> > +                             end - start - cur_alloc_size + 1, NULL);
> >       }
> >       return ret;
> >   }
>

Re: [PATCH 2/2] btrfs: simplify regions mark and keep start unchanged in err handling

[David Sterba]

On Wed, Oct 30, 2024 at 01:31:15PM +1030, Qu Wenruo wrote:
> 
> 
> 在 2024/10/25 17:24, iamhswang@gmail.com 写道:
> > From: Haisu Wang <haisuwang@tencent.com>
> >
> > Simplify the regions mark by using cur_alloc_size only to present
> > the reserved but may failed to alloced extent. Remove the ram_size
> > as well since it is always consistent to the cur_alloc_size in the
> > context. Advanced the start mark in normal path until extent succeed
> > alloced and keep the start unchanged in error handling path.
> >
> > PASSed the fstest generic/475 test for a hundred times with quota
> > enabled. And a modified generic/475 test by removing the sleep time
> > for a hundred times. About one tenth of the tests do enter the error
> > handling path due to fail to reserve extent.
> >
> 
> Although this patch is already merged into for-next, it looks like the
> next patch will again change the error handling, mostly render the this
> one useless:
> 
> https://lore.kernel.org/linux-btrfs/2a0925f0264daf90741ed0a7ba7ed4b4888cf778.1728725060.git.wqu@suse.com/
> 
> The newer patch will change the error handling to a simpler one, so
> instead of 3 regions, there will be only 2.
> 
> There will be no change needed from your side, I will update my patches
> to solve the conflicts, just in case if you find the error handling is
> different in the future.

Please take care of that, the only request I have is that it's done by
the end of this week so we have the code in linux-next and that a fix
should come before a refactoring (due to backports). Update for-next as
you need.

Re: [PATCH 2/2] btrfs: simplify regions mark and keep start unchanged in err handling

[Qu Wenruo]

在 2024/10/31 01:58, David Sterba 写道:
> On Wed, Oct 30, 2024 at 01:31:15PM +1030, Qu Wenruo wrote:
>>
>>
>> 在 2024/10/25 17:24, iamhswang@gmail.com 写道:
>>> From: Haisu Wang <haisuwang@tencent.com>
>>>
>>> Simplify the regions mark by using cur_alloc_size only to present
>>> the reserved but may failed to alloced extent. Remove the ram_size
>>> as well since it is always consistent to the cur_alloc_size in the
>>> context. Advanced the start mark in normal path until extent succeed
>>> alloced and keep the start unchanged in error handling path.
>>>
>>> PASSed the fstest generic/475 test for a hundred times with quota
>>> enabled. And a modified generic/475 test by removing the sleep time
>>> for a hundred times. About one tenth of the tests do enter the error
>>> handling path due to fail to reserve extent.
>>>
>>
>> Although this patch is already merged into for-next, it looks like the
>> next patch will again change the error handling, mostly render the this
>> one useless:
>>
>> https://lore.kernel.org/linux-btrfs/2a0925f0264daf90741ed0a7ba7ed4b4888cf778.1728725060.git.wqu@suse.com/
>>
>> The newer patch will change the error handling to a simpler one, so
>> instead of 3 regions, there will be only 2.
>>
>> There will be no change needed from your side, I will update my patches
>> to solve the conflicts, just in case if you find the error handling is
>> different in the future.
>
> Please take care of that, the only request I have is that it's done by
> the end of this week so we have the code in linux-next and that a fix
> should come before a refactoring (due to backports). Update for-next as
> you need.

Then everything is done.

The patch itself is not touched and already in for-next branch.

The new one is part of the subpage enhancement series now, which is not
that urgent.
The subpage compression write support is already large enough for the
next release cycle.

Thanks,
Qu