Re: [PATCH v3 08/13] btrfs: introduce btrfs_space_info sub-group

[Josef Bacik <josef@toxicpanda.com>]

On Wed, Apr 16, 2025 at 11:28:13PM +0900, Naohiro Aota wrote:
> Current code assumes we have only one space_info for each block group type
> (DATA, METADATA, and SYSTEM). We sometime needs multiple space_info to
> manage special block groups.
> 
> One example is handling the data relocation block group for the zoned mode.
> That block group is dedicated for writing relocated data and we cannot
> allocate any regular extent from that block group, which is implemented in
> the zoned extent allocator. That block group still belongs to the normal
> data space_info. So, when all the normal data block groups are full and
> there are some free space in the dedicated block group, the space_info
> looks to have some free space, while it cannot allocate normal extent
> anymore. That results in a strange ENOSPC error. We need to have a
> space_info for the relocation data block group to represent the situation
> properly.
> 
> This commit adds a basic infrastructure for having a "sub-group" of a
> space_info: creation and removing. A sub-group space_info belongs to one of
> the primary space_infos and has the same flags as its parent.
> 
> This commit first introduces the relocation data sub-space_info, and the
> next commit will introduce tree-log sub-space_info. In the future, it could
> be useful to implement tiered storage for btrfs e.g, by implementing a
> sub-group space_info for block groups resides on a fast storage.
> 
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
>  fs/btrfs/block-group.c | 11 +++++++++++
>  fs/btrfs/space-info.c  | 38 +++++++++++++++++++++++++++++++++++---
>  fs/btrfs/space-info.h  |  8 ++++++++
>  fs/btrfs/sysfs.c       | 16 +++++++++++++---
>  4 files changed, 67 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 846c9737ff5a..475353b0b32c 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -4411,6 +4411,17 @@ static void check_removing_space_info(struct btrfs_space_info *space_info)
>  {
>  	struct btrfs_fs_info *info = space_info->fs_info;
>  
> +	if (space_info->subgroup_id == SUB_GROUP_PRIMARY) {
> +		/* This is a top space_info, proceeds its children first. */
> +		for (int i = 0; i < BTRFS_SPACE_INFO_SUB_GROUP_MAX; i++) {
> +			if (space_info->sub_group[i]) {
> +				check_removing_space_info(space_info->sub_group[i]);
> +				kfree(space_info->sub_group[i]);
> +				space_info->sub_group[i] = NULL;
> +			}
> +		}
> +	}
> +
>  	/*
>  	 * Do not hide this behind enospc_debug, this is actually
>  	 * important and indicates a real bug if this happens.
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index 2489c2a16123..37e55298c082 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -249,16 +249,38 @@ static void init_space_info(struct btrfs_fs_info *info,
>  	INIT_LIST_HEAD(&space_info->priority_tickets);
>  	space_info->clamp = 1;
>  	btrfs_update_space_info_chunk_size(space_info, calc_chunk_size(info, flags));
> +	space_info->subgroup_id = SUB_GROUP_PRIMARY;
>  
>  	if (btrfs_is_zoned(info))
>  		space_info->bg_reclaim_threshold = BTRFS_DEFAULT_ZONED_RECLAIM_THRESH;
>  }
>  
> +static int create_space_info_sub_group(struct btrfs_space_info *parent, u64 flags,
> +				       enum btrfs_space_info_sub_group id)
> +{
> +	struct btrfs_fs_info *fs_info = parent->fs_info;
> +	struct btrfs_space_info *sub_space;
> +
> +	ASSERT(parent->subgroup_id == SUB_GROUP_PRIMARY);
> +	ASSERT(id != SUB_GROUP_PRIMARY);
> +
> +	sub_space = kzalloc(sizeof(*sub_space), GFP_NOFS);
> +	if (!sub_space)
> +		return -ENOMEM;
> +
> +	init_space_info(fs_info, sub_space, flags);
> +	parent->sub_group[id] = sub_space;
> +	sub_space->parent = parent;
> +	sub_space->subgroup_id = id;
> +
> +	return btrfs_sysfs_add_space_info_type(fs_info, sub_space);
> +}
> +
>  static int create_space_info(struct btrfs_fs_info *info, u64 flags)
>  {
>  
>  	struct btrfs_space_info *space_info;
> -	int ret;
> +	int ret = 0;
>  
>  	space_info = kzalloc(sizeof(*space_info), GFP_NOFS);
>  	if (!space_info)
> @@ -266,6 +288,15 @@ static int create_space_info(struct btrfs_fs_info *info, u64 flags)
>  
>  	init_space_info(info, space_info, flags);
>  
> +	if (btrfs_is_zoned(info)) {
> +		if (flags & BTRFS_BLOCK_GROUP_DATA)
> +			ret = create_space_info_sub_group(space_info, flags,
> +							  SUB_GROUP_DATA_RELOC);
> +		if (ret == -ENOMEM)
> +			return ret;
> +		ASSERT(!ret);
> +	}
> +
>  	ret = btrfs_sysfs_add_space_info_type(info, space_info);
>  	if (ret)
>  		return ret;
> @@ -561,8 +592,9 @@ static void __btrfs_dump_space_info(const struct btrfs_fs_info *fs_info,
>  	lockdep_assert_held(&info->lock);
>  
>  	/* The free space could be negative in case of overcommit */
> -	btrfs_info(fs_info, "space_info %s has %lld free, is %sfull",
> -		   flag_str,
> +	btrfs_info(fs_info,
> +		   "space_info %s (sub-group id %d) has %lld free, is %sfull",
> +		   flag_str, info->subgroup_id,
>  		   (s64)(info->total_bytes - btrfs_space_info_used(info, true)),
>  		   info->full ? "" : "not ");
>  	btrfs_info(fs_info,
> diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h
> index 7459b4eb99cd..64641885babd 100644
> --- a/fs/btrfs/space-info.h
> +++ b/fs/btrfs/space-info.h
> @@ -98,8 +98,16 @@ enum btrfs_flush_state {
>  	RESET_ZONES		= 12,
>  };
>  
> +enum btrfs_space_info_sub_group {
> +	SUB_GROUP_DATA_RELOC = 0,
> +	SUB_GROUP_PRIMARY = -1,
> +};
> +#define BTRFS_SPACE_INFO_SUB_GROUP_MAX 1

We want to avoid namespace pollution, so rename these to have a btrfs prefix,
and do something like

enum btrfs_space_info_sub_group {
	BTRFS_SUB_GROUP_DATA_RELOC = 0,
	BTRFS_SUB_GROUP_MAX,
	BTRFS_SUB_GROUP_PRIMARY = -1,
};

And then you can remove the define.  Thanks,

Josef