Re: [PATCH v3 03/17] btrfs: allow remapped chunks to have zero stripes

[Boris Burkov <boris@bur.io>]

On Thu, Oct 09, 2025 at 12:27:58PM +0100, Mark Harmstone wrote:
> When a chunk has been fully remapped, we are going to set its
> num_stripes to 0, as it will no longer represent a physical location on
> disk.
> 
> Change tree-checker to allow for this, and fix a couple of
> divide-by-zeroes seen elsewhere.
> 
> Signed-off-by: Mark Harmstone <mark@harmstone.com>

This one feels close to me but I still have a few reservations. Happy to
discuss the details a bit more, of course.

> ---
>  fs/btrfs/tree-checker.c | 65 ++++++++++++++++++++++++++++-------------
>  fs/btrfs/volumes.c      | 13 ++++++++-
>  2 files changed, 57 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index 62f35338a555..59b1393e5018 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -816,6 +816,41 @@ static void chunk_err(const struct btrfs_fs_info *fs_info,
>  	va_end(args);
>  }
>  
> +static bool valid_stripe_count(u64 profile, u16 num_stripes,
> +			       u16 sub_stripes)
> +{
> +	switch (profile) {
> +	case BTRFS_BLOCK_GROUP_RAID0:
> +		return true;
> +	case BTRFS_BLOCK_GROUP_RAID10:
> +		return sub_stripes ==
> +			btrfs_raid_array[BTRFS_RAID_RAID10].sub_stripes;
> +	case BTRFS_BLOCK_GROUP_RAID1:
> +		return num_stripes ==
> +			btrfs_raid_array[BTRFS_RAID_RAID1].devs_min;
> +	case BTRFS_BLOCK_GROUP_RAID1C3:
> +		return num_stripes ==
> +			btrfs_raid_array[BTRFS_RAID_RAID1C3].devs_min;
> +	case BTRFS_BLOCK_GROUP_RAID1C4:
> +		return num_stripes ==
> +			btrfs_raid_array[BTRFS_RAID_RAID1C4].devs_min;
> +	case BTRFS_BLOCK_GROUP_RAID5:
> +		return num_stripes >=
> +			btrfs_raid_array[BTRFS_RAID_RAID5].devs_min;
> +	case BTRFS_BLOCK_GROUP_RAID6:
> +		return num_stripes >=
> +			btrfs_raid_array[BTRFS_RAID_RAID6].devs_min;
> +	case BTRFS_BLOCK_GROUP_DUP:
> +		return num_stripes ==
> +			btrfs_raid_array[BTRFS_RAID_DUP].dev_stripes;
> +	case 0: /* SINGLE */
> +		return num_stripes ==
> +			btrfs_raid_array[BTRFS_RAID_SINGLE].dev_stripes;
> +	default:
> +		BUG();
> +	}
> +}
> +
>  /*
>   * The common chunk check which could also work on super block sys chunk array.
>   *
> @@ -839,6 +874,7 @@ int btrfs_check_chunk_valid(const struct btrfs_fs_info *fs_info,
>  	u64 features;
>  	u32 chunk_sector_size;
>  	bool mixed = false;
> +	bool remapped;
>  	int raid_index;
>  	int nparity;
>  	int ncopies;
> @@ -862,12 +898,14 @@ int btrfs_check_chunk_valid(const struct btrfs_fs_info *fs_info,
>  	ncopies = btrfs_raid_array[raid_index].ncopies;
>  	nparity = btrfs_raid_array[raid_index].nparity;
>  
> -	if (unlikely(!num_stripes)) {
> +	remapped = type & BTRFS_BLOCK_GROUP_REMAPPED;
> +
> +	if (unlikely(!remapped && !num_stripes)) {
>  		chunk_err(fs_info, leaf, chunk, logical,
>  			  "invalid chunk num_stripes, have %u", num_stripes);
>  		return -EUCLEAN;
>  	}
> -	if (unlikely(num_stripes < ncopies)) {
> +	if (unlikely(num_stripes != 0 && num_stripes < ncopies)) {
>  		chunk_err(fs_info, leaf, chunk, logical,
>  			  "invalid chunk num_stripes < ncopies, have %u < %d",
>  			  num_stripes, ncopies);
> @@ -965,22 +1003,9 @@ int btrfs_check_chunk_valid(const struct btrfs_fs_info *fs_info,
>  		}
>  	}
>  
> -	if (unlikely((type & BTRFS_BLOCK_GROUP_RAID10 &&
> -		      sub_stripes != btrfs_raid_array[BTRFS_RAID_RAID10].sub_stripes) ||
> -		     (type & BTRFS_BLOCK_GROUP_RAID1 &&
> -		      num_stripes != btrfs_raid_array[BTRFS_RAID_RAID1].devs_min) ||
> -		     (type & BTRFS_BLOCK_GROUP_RAID1C3 &&
> -		      num_stripes != btrfs_raid_array[BTRFS_RAID_RAID1C3].devs_min) ||
> -		     (type & BTRFS_BLOCK_GROUP_RAID1C4 &&
> -		      num_stripes != btrfs_raid_array[BTRFS_RAID_RAID1C4].devs_min) ||
> -		     (type & BTRFS_BLOCK_GROUP_RAID5 &&
> -		      num_stripes < btrfs_raid_array[BTRFS_RAID_RAID5].devs_min) ||
> -		     (type & BTRFS_BLOCK_GROUP_RAID6 &&
> -		      num_stripes < btrfs_raid_array[BTRFS_RAID_RAID6].devs_min) ||
> -		     (type & BTRFS_BLOCK_GROUP_DUP &&
> -		      num_stripes != btrfs_raid_array[BTRFS_RAID_DUP].dev_stripes) ||
> -		     ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 &&
> -		      num_stripes != btrfs_raid_array[BTRFS_RAID_SINGLE].dev_stripes))) {
> +	if (!remapped &&
> +	    !valid_stripe_count(type & BTRFS_BLOCK_GROUP_PROFILE_MASK,
> +				num_stripes, sub_stripes)) {

Is num_stripes == 0 valid for raid 0 aside from remapped?

>  		chunk_err(fs_info, leaf, chunk, logical,
>  			"invalid num_stripes:sub_stripes %u:%u for profile %llu",
>  			num_stripes, sub_stripes,
> @@ -1004,11 +1029,11 @@ static int check_leaf_chunk_item(struct extent_buffer *leaf,
>  	struct btrfs_fs_info *fs_info = leaf->fs_info;
>  	int num_stripes;
>  
> -	if (unlikely(btrfs_item_size(leaf, slot) < sizeof(struct btrfs_chunk))) {
> +	if (unlikely(btrfs_item_size(leaf, slot) < offsetof(struct btrfs_chunk, stripe))) {

Does this need a remapped parameter as part of the check too?

>  		chunk_err(fs_info, leaf, chunk, key->offset,
>  			"invalid chunk item size: have %u expect [%zu, %u)",
>  			btrfs_item_size(leaf, slot),
> -			sizeof(struct btrfs_chunk),
> +			offsetof(struct btrfs_chunk, stripe),
>  			BTRFS_LEAF_DATA_SIZE(fs_info));
>  		return -EUCLEAN;
>  	}
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e14b86e2996b..6750f6f0af59 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6145,6 +6145,12 @@ struct btrfs_discard_stripe *btrfs_map_discard(struct btrfs_fs_info *fs_info,
>  		goto out_free_map;
>  	}
>  
> +	/* avoid divide by zero on fully-remapped chunks */
> +	if (map->num_stripes == 0) {
> +		ret = -EOPNOTSUPP;
> +		goto out_free_map;
> +	}
> +

I'll reconsider this when I read the discard stuff more closely, but
aren't you intending to discard fully remapped chunks? So this seems
counter-intuitive at best. Since you tested it, I assume it must work,
just saying since it stuck out to me..

>  	offset = logical - map->start;
>  	length = min_t(u64, map->start + map->chunk_len - logical, length);
>  	*length_ret = length;
> @@ -7088,7 +7094,12 @@ static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf,
>  	 */
>  	map->sub_stripes = btrfs_raid_array[index].sub_stripes;
>  	map->verified_stripes = 0;
> -	map->stripe_size = btrfs_calc_stripe_length(map);
> +
> +	if (num_stripes > 0)
> +		map->stripe_size = btrfs_calc_stripe_length(map);
> +	else
> +		map->stripe_size = 0;
> +
>  	for (i = 0; i < num_stripes; i++) {
>  		map->stripes[i].physical =
>  			btrfs_stripe_offset_nr(leaf, chunk, i);
> -- 
> 2.49.1
>