[PATCH v2 0/2] btrfs: zoned: activate new BG only from extent allocation context

[PATCH v2 0/2] btrfs: zoned: activate new BG only from extent allocation context

[Naohiro Aota]

In btrfs_make_block_group(), we activate the allocated block group,
expecting that the block group is soon used for the extent allocation.

With a lot of IOs, btrfs_async_reclaim_data_space() tries to prepare
for them by pre-allocating data block groups. That preallocation can
consume all the active zone counting. It is OK if they are soon
written and filled. However, that's not the case. As a result, an
allocation of non-data block groups fails due to the lack of an active
zone resource.

This series fixes the issue by activating a new block group only when
it's called from find_free_extent(). This series introduces
CHUNK_ALLOC_FORCE_FOR_EXTENT in btrfs_chunk_alloc_enum to distinguish
the context.

--
Changes
- v2
  - Fix a flipped condition

Naohiro Aota (2):
  btrfs: return allocated block group from do_chunk_alloc()
  btrfs: zoned: activate block group only for extent allocation

 fs/btrfs/block-group.c | 36 +++++++++++++++++++++++++++---------
 fs/btrfs/block-group.h |  1 +
 fs/btrfs/extent-tree.c |  2 +-
 3 files changed, 29 insertions(+), 10 deletions(-)

-- 
2.35.1

[PATCH v2 1/2] btrfs: return allocated block group from do_chunk_alloc()

[Naohiro Aota]

Return the allocated block group from do_chunk_alloc(). This is a
preparation patch for the next patch.

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 fs/btrfs/block-group.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 59f18a10fd5f..d4ac1c76f539 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -3433,7 +3433,7 @@ int btrfs_force_chunk_alloc(struct btrfs_trans_handle *trans, u64 type)
 	return btrfs_chunk_alloc(trans, alloc_flags, CHUNK_ALLOC_FORCE);
 }
 
-static int do_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags)
+static struct btrfs_block_group *do_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags)
 {
 	struct btrfs_block_group *bg;
 	int ret;
@@ -3520,7 +3520,11 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags)
 out:
 	btrfs_trans_release_chunk_metadata(trans);
 
-	return ret;
+	if (ret)
+		return ERR_PTR(ret);
+
+	btrfs_get_block_group(bg);
+	return bg;
 }
 
 /*
@@ -3635,6 +3639,7 @@ int btrfs_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags,
 {
 	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct btrfs_space_info *space_info;
+	struct btrfs_block_group *ret_bg;
 	bool wait_for_alloc = false;
 	bool should_alloc = false;
 	int ret = 0;
@@ -3728,9 +3733,14 @@ int btrfs_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags,
 			force_metadata_allocation(fs_info);
 	}
 
-	ret = do_chunk_alloc(trans, flags);
+	ret_bg = do_chunk_alloc(trans, flags);
 	trans->allocating_chunk = false;
 
+	if (IS_ERR(ret_bg))
+		ret = PTR_ERR(ret_bg);
+	else
+		btrfs_put_block_group(ret_bg);
+
 	spin_lock(&space_info->lock);
 	if (ret < 0) {
 		if (ret == -ENOSPC)
-- 
2.35.1

[PATCH v2 2/2] btrfs: zoned: activate block group only for extent allocation

[Naohiro Aota]

In btrfs_make_block_group(), we activate the allocated block group,
expecting that the block group is soon used for allocation. However, the
chunk allocation from flush_space() context broke the assumption. There can
be a large time gap between the chunk allocation time and the extent
allocation time from the chunk.

Activating the empty block groups pre-allocated from flush_space() context
can exhaust the active zone counter of a device. Once we use all the active
zone counts for empty pre-allocated BGs, we cannot activate new BG for the
other things: metadata, tree-log, or data relocation BG. That failure
results in a fake -ENOSPC.

This patch introduces CHUNK_ALLOC_FORCE_FOR_EXTENT to distinguish the chunk
allocation from find_free_extent(). Now, the new block group is activated
only in that context.

Fixes: eb66a010d518 ("btrfs: zoned: activate new block group")
Cc: stable@vger.kernel.org # 5.16+: c7d1b9109dd0: btrfs: return allocated block group from do_chunk_alloc()
Cc: stable@vger.kernel.org # 5.16+
Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 fs/btrfs/block-group.c | 24 ++++++++++++++++--------
 fs/btrfs/block-group.h |  1 +
 fs/btrfs/extent-tree.c |  2 +-
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index d4ac1c76f539..84c97d76de92 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -2481,12 +2481,6 @@ struct btrfs_block_group *btrfs_make_block_group(struct btrfs_trans_handle *tran
 		return ERR_PTR(ret);
 	}
 
-	/*
-	 * New block group is likely to be used soon. Try to activate it now.
-	 * Failure is OK for now.
-	 */
-	btrfs_zone_activate(cache);
-
 	ret = exclude_super_stripes(cache);
 	if (ret) {
 		/* We may have excluded something, so call this just in case */
@@ -3642,8 +3636,14 @@ int btrfs_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags,
 	struct btrfs_block_group *ret_bg;
 	bool wait_for_alloc = false;
 	bool should_alloc = false;
+	bool from_extent_allocation = false;
 	int ret = 0;
 
+	if (force == CHUNK_ALLOC_FORCE_FOR_EXTENT) {
+		from_extent_allocation = true;
+		force = CHUNK_ALLOC_FORCE;
+	}
+
 	/* Don't re-enter if we're already allocating a chunk */
 	if (trans->allocating_chunk)
 		return -ENOSPC;
@@ -3736,9 +3736,17 @@ int btrfs_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags,
 	ret_bg = do_chunk_alloc(trans, flags);
 	trans->allocating_chunk = false;
 
-	if (IS_ERR(ret_bg))
+	if (IS_ERR(ret_bg)) {
 		ret = PTR_ERR(ret_bg);
-	else
+	} else if (from_extent_allocation) {
+		/*
+		 * New block group is likely to be used soon. Try to activate
+		 * it now. Failure is OK for now.
+		 */
+		btrfs_zone_activate(ret_bg);
+	}
+
+	if (!ret)
 		btrfs_put_block_group(ret_bg);
 
 	spin_lock(&space_info->lock);
diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
index 93aabc68bb6a..9c822367c432 100644
--- a/fs/btrfs/block-group.h
+++ b/fs/btrfs/block-group.h
@@ -40,6 +40,7 @@ enum btrfs_chunk_alloc_enum {
 	CHUNK_ALLOC_NO_FORCE,
 	CHUNK_ALLOC_LIMITED,
 	CHUNK_ALLOC_FORCE,
+	CHUNK_ALLOC_FORCE_FOR_EXTENT,
 };
 
 struct btrfs_caching_control {
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index f477035a2ac2..6aa92f84f465 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4082,7 +4082,7 @@ static int find_free_extent_update_loop(struct btrfs_fs_info *fs_info,
 			}
 
 			ret = btrfs_chunk_alloc(trans, ffe_ctl->flags,
-						CHUNK_ALLOC_FORCE);
+						CHUNK_ALLOC_FORCE_FOR_EXTENT);
 
 			/* Do not bail out on ENOSPC since we can do more. */
 			if (ret == -ENOSPC)
-- 
2.35.1

Re: [PATCH v2 2/2] btrfs: zoned: activate block group only for extent allocation

[David Sterba]

On Tue, Mar 22, 2022 at 06:11:34PM +0900, Naohiro Aota wrote:
> In btrfs_make_block_group(), we activate the allocated block group,
> expecting that the block group is soon used for allocation. However, the
> chunk allocation from flush_space() context broke the assumption. There can
> be a large time gap between the chunk allocation time and the extent
> allocation time from the chunk.
> 
> Activating the empty block groups pre-allocated from flush_space() context
> can exhaust the active zone counter of a device. Once we use all the active
> zone counts for empty pre-allocated BGs, we cannot activate new BG for the
> other things: metadata, tree-log, or data relocation BG. That failure
> results in a fake -ENOSPC.
> 
> This patch introduces CHUNK_ALLOC_FORCE_FOR_EXTENT to distinguish the chunk
> allocation from find_free_extent(). Now, the new block group is activated
> only in that context.
> 
> Fixes: eb66a010d518 ("btrfs: zoned: activate new block group")
> Cc: stable@vger.kernel.org # 5.16+: c7d1b9109dd0: btrfs: return allocated block group from do_chunk_alloc()
> Cc: stable@vger.kernel.org # 5.16+
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
>  fs/btrfs/block-group.c | 24 ++++++++++++++++--------
>  fs/btrfs/block-group.h |  1 +
>  fs/btrfs/extent-tree.c |  2 +-
>  3 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index d4ac1c76f539..84c97d76de92 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -2481,12 +2481,6 @@ struct btrfs_block_group *btrfs_make_block_group(struct btrfs_trans_handle *tran
>  		return ERR_PTR(ret);
>  	}
>  
> -	/*
> -	 * New block group is likely to be used soon. Try to activate it now.
> -	 * Failure is OK for now.
> -	 */
> -	btrfs_zone_activate(cache);
> -
>  	ret = exclude_super_stripes(cache);
>  	if (ret) {
>  		/* We may have excluded something, so call this just in case */
> @@ -3642,8 +3636,14 @@ int btrfs_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags,
>  	struct btrfs_block_group *ret_bg;
>  	bool wait_for_alloc = false;
>  	bool should_alloc = false;
> +	bool from_extent_allocation = false;
>  	int ret = 0;
>  
> +	if (force == CHUNK_ALLOC_FORCE_FOR_EXTENT) {
> +		from_extent_allocation = true;
> +		force = CHUNK_ALLOC_FORCE;
> +	}
> +
>  	/* Don't re-enter if we're already allocating a chunk */
>  	if (trans->allocating_chunk)
>  		return -ENOSPC;
> @@ -3736,9 +3736,17 @@ int btrfs_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags,
>  	ret_bg = do_chunk_alloc(trans, flags);
>  	trans->allocating_chunk = false;
>  
> -	if (IS_ERR(ret_bg))
> +	if (IS_ERR(ret_bg)) {
>  		ret = PTR_ERR(ret_bg);
> -	else
> +	} else if (from_extent_allocation) {
> +		/*
> +		 * New block group is likely to be used soon. Try to activate
> +		 * it now. Failure is OK for now.
> +		 */
> +		btrfs_zone_activate(ret_bg);
> +	}
> +
> +	if (!ret)
>  		btrfs_put_block_group(ret_bg);
>  
>  	spin_lock(&space_info->lock);
> diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
> index 93aabc68bb6a..9c822367c432 100644
> --- a/fs/btrfs/block-group.h
> +++ b/fs/btrfs/block-group.h
> @@ -40,6 +40,7 @@ enum btrfs_chunk_alloc_enum {
>  	CHUNK_ALLOC_NO_FORCE,
>  	CHUNK_ALLOC_LIMITED,
>  	CHUNK_ALLOC_FORCE,
> +	CHUNK_ALLOC_FORCE_FOR_EXTENT,

There's a comment documenting what it means, I've written

	CHUNK_ALLOC_FORCE_FOR_EXTENT like CHUNK_ALLOC_FORCE but called from
	find_free_extent() that also activaes the zone

based on the changelog.

Re: [PATCH v2 0/2] btrfs: zoned: activate new BG only from extent allocation context

[Johannes Thumshirn]

On 22/03/2022 10:11, Naohiro Aota wrote:
> In btrfs_make_block_group(), we activate the allocated block group,
> expecting that the block group is soon used for the extent allocation.
> 
> With a lot of IOs, btrfs_async_reclaim_data_space() tries to prepare
> for them by pre-allocating data block groups. That preallocation can
> consume all the active zone counting. It is OK if they are soon
> written and filled. However, that's not the case. As a result, an
> allocation of non-data block groups fails due to the lack of an active
> zone resource.
> 
> This series fixes the issue by activating a new block group only when
> it's called from find_free_extent(). This series introduces
> CHUNK_ALLOC_FORCE_FOR_EXTENT in btrfs_chunk_alloc_enum to distinguish
> the context.
> 
> --
> Changes
> - v2
>   - Fix a flipped condition
> 
> Naohiro Aota (2):
>   btrfs: return allocated block group from do_chunk_alloc()
>   btrfs: zoned: activate block group only for extent allocation
> 
>  fs/btrfs/block-group.c | 36 +++++++++++++++++++++++++++---------
>  fs/btrfs/block-group.h |  1 +
>  fs/btrfs/extent-tree.c |  2 +-
>  3 files changed, 29 insertions(+), 10 deletions(-)
> 


For the series,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Tested-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v2 0/2] btrfs: zoned: activate new BG only from extent allocation context

[David Sterba]

On Tue, Mar 22, 2022 at 06:11:32PM +0900, Naohiro Aota wrote:
> In btrfs_make_block_group(), we activate the allocated block group,
> expecting that the block group is soon used for the extent allocation.
> 
> With a lot of IOs, btrfs_async_reclaim_data_space() tries to prepare
> for them by pre-allocating data block groups. That preallocation can
> consume all the active zone counting. It is OK if they are soon
> written and filled. However, that's not the case. As a result, an
> allocation of non-data block groups fails due to the lack of an active
> zone resource.
> 
> This series fixes the issue by activating a new block group only when
> it's called from find_free_extent(). This series introduces
> CHUNK_ALLOC_FORCE_FOR_EXTENT in btrfs_chunk_alloc_enum to distinguish
> the context.
> 
> --
> Changes
> - v2
>   - Fix a flipped condition
> 
> Naohiro Aota (2):
>   btrfs: return allocated block group from do_chunk_alloc()
>   btrfs: zoned: activate block group only for extent allocation

Added to misc-next, thanks.