Re: [PATCH v3 06/13] btrfs: introduce space_info argument to btrfs_chunk_alloc

[Josef Bacik <josef@toxicpanda.com>]

On Wed, Apr 16, 2025 at 11:28:11PM +0900, Naohiro Aota wrote:
> Take an optional btrfs_space_info argument in btrfs_chunk_alloc(). If
> specified, btrfs_chunk_alloc() works on the space_info. If not, the default
> space_info is used as the same as before.
> 
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

For consistency sake I'd prefer if you'd just update the callers to lookup the
space_info and pass that in as appropriate.  In fact a lot of these callers
already have the block group or space_info available, so we could avoid the
extra overhead of doing a lookup

> ---
>  fs/btrfs/block-group.c | 19 ++++++++++++-------
>  fs/btrfs/block-group.h |  3 ++-
>  fs/btrfs/extent-tree.c |  2 +-
>  fs/btrfs/space-info.c  |  2 +-
>  fs/btrfs/transaction.c |  2 +-
>  5 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index b700d80089d3..12cc9069d4bb 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -3018,7 +3018,7 @@ int btrfs_inc_block_group_ro(struct btrfs_block_group *cache,
>  		 */
>  		alloc_flags = btrfs_get_alloc_profile(fs_info, cache->flags);
>  		if (alloc_flags != cache->flags) {
> -			ret = btrfs_chunk_alloc(trans, alloc_flags,
> +			ret = btrfs_chunk_alloc(trans, NULL, alloc_flags,
>  						CHUNK_ALLOC_FORCE);

Here we just do cache->space_info;

>  			/*
>  			 * ENOSPC is allowed here, we may have enough space
> @@ -3047,7 +3047,7 @@ int btrfs_inc_block_group_ro(struct btrfs_block_group *cache,
>  		goto unlock_out;
>  
>  	alloc_flags = btrfs_get_alloc_profile(fs_info, cache->space_info->flags);
> -	ret = btrfs_chunk_alloc(trans, alloc_flags, CHUNK_ALLOC_FORCE);
> +	ret = btrfs_chunk_alloc(trans, NULL, alloc_flags, CHUNK_ALLOC_FORCE);

Same here.

>  	if (ret < 0)
>  		goto out;
>  	/*
> @@ -3899,7 +3899,7 @@ int btrfs_force_chunk_alloc(struct btrfs_trans_handle *trans, u64 type)
>  {
>  	u64 alloc_flags = btrfs_get_alloc_profile(trans->fs_info, type);
>  
> -	return btrfs_chunk_alloc(trans, alloc_flags, CHUNK_ALLOC_FORCE);
> +	return btrfs_chunk_alloc(trans, NULL, alloc_flags, CHUNK_ALLOC_FORCE);

Here we'd have to lookup.

>  }
>  
>  static struct btrfs_block_group *do_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags)
> @@ -4102,12 +4102,15 @@ static struct btrfs_block_group *do_chunk_alloc(struct btrfs_trans_handle *trans
>   *    - return 0 if it doesn't need to allocate a new chunk,
>   *    - return 1 if it successfully allocates a chunk,
>   *    - return errors including -ENOSPC otherwise.
> + *
> + * @space_info can optionally be specified to make a new chunk belong to it. If
> + * it is NULL, it is set automatically.
>   */
> -int btrfs_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags,
> +int btrfs_chunk_alloc(struct btrfs_trans_handle *trans,
> +		      struct btrfs_space_info *space_info, u64 flags,
>  		      enum btrfs_chunk_alloc_enum force)
>  {
>  	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;
> @@ -4146,8 +4149,10 @@ int btrfs_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags,
>  	if (flags & BTRFS_BLOCK_GROUP_SYSTEM)
>  		return -ENOSPC;
>  
> -	space_info = btrfs_find_space_info(fs_info, flags);
> -	ASSERT(space_info);
> +	if (!space_info) {
> +		space_info = btrfs_find_space_info(fs_info, flags);
> +		ASSERT(space_info);
> +	}
>  
>  	do {
>  		spin_lock(&space_info->lock);
> diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
> index 36937eeab9b8..c01f3af726a1 100644
> --- a/fs/btrfs/block-group.h
> +++ b/fs/btrfs/block-group.h
> @@ -342,7 +342,8 @@ int btrfs_add_reserved_bytes(struct btrfs_block_group *cache,
>  			     bool force_wrong_size_class);
>  void btrfs_free_reserved_bytes(struct btrfs_block_group *cache,
>  			       u64 num_bytes, int delalloc);
> -int btrfs_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags,
> +int btrfs_chunk_alloc(struct btrfs_trans_handle *trans,
> +		      struct btrfs_space_info *space_info, u64 flags,
>  		      enum btrfs_chunk_alloc_enum force);
>  int btrfs_force_chunk_alloc(struct btrfs_trans_handle *trans, u64 type);
>  void check_system_chunk(struct btrfs_trans_handle *trans, const u64 type);
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index a68a8a07caff..1dad1a42c9c1 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -4159,7 +4159,7 @@ static int find_free_extent_update_loop(struct btrfs_fs_info *fs_info,
>  				return ret;
>  			}
>  
> -			ret = btrfs_chunk_alloc(trans, ffe_ctl->flags,
> +			ret = btrfs_chunk_alloc(trans, NULL, ffe_ctl->flags,
>  						CHUNK_ALLOC_FORCE_FOR_EXTENT);

We'd have to look up here.

>  
>  			/* Do not bail out on ENOSPC since we can do more. */
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index d6d33ab754ba..2489c2a16123 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -817,7 +817,7 @@ static void flush_space(struct btrfs_fs_info *fs_info,
>  			ret = PTR_ERR(trans);
>  			break;
>  		}
> -		ret = btrfs_chunk_alloc(trans,
> +		ret = btrfs_chunk_alloc(trans, space_info,
>  				btrfs_get_alloc_profile(fs_info, space_info->flags),
>  				(state == ALLOC_CHUNK) ? CHUNK_ALLOC_NO_FORCE :
>  					CHUNK_ALLOC_FORCE);
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 39e48bf610a1..670e0527996c 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -763,7 +763,7 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
>  	if (do_chunk_alloc && num_bytes) {
>  		u64 flags = h->block_rsv->space_info->flags;
>  
> -		btrfs_chunk_alloc(h, btrfs_get_alloc_profile(fs_info, flags),
> +		btrfs_chunk_alloc(h, NULL, btrfs_get_alloc_profile(fs_info, flags),
>  				  CHUNK_ALLOC_NO_FORCE);

Here we just do h->block_rsv->space_info.  Thanks,

Josef