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

[Mark Harmstone <mark@harmstone.com>]

On 13/06/2025 10.41 pm, Boris Burkov wrote:

...snip...

 >> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
>> index 0505f8d76581..fd83df06e3fb 100644
>> --- a/fs/btrfs/tree-checker.c
>> +++ b/fs/btrfs/tree-checker.c
>> @@ -829,7 +829,7 @@ int btrfs_check_chunk_valid(const struct btrfs_fs_info *fs_info,
>>   	u64 type;
>>   	u64 features;
>>   	u32 chunk_sector_size;
>> -	bool mixed = false;
>> +	bool mixed = false, remapped;
> 
> nit:
> I *personally* don't like such declarations. I think they are lightly if
> not decisively discouraged in the Linux Coding Style document as well
> (at least to my reading). In our code, I found some examples where they
> are used where both values are related and get assigned. But I haven't
> seen any like this for unrelated variables with mixed assignment.
> 
>>   	int raid_index;
>>   	int nparity;
>>   	int ncopies;
>> @@ -853,12 +853,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(!remapped && num_stripes < ncopies)) {
> 
> I think remapped only permits you exactly num_stripes == 0, not
> num_stripes = 2 if ncopies = 3, right? Though it makes the logic less
> neat, I would make the check as precise as possible on the invariants.

I'm changing the check here to num_stripes != 0 && num_stripes < ncopies,
which I hope makes things a little clearer. It also means that we have an
error if something goes wrong with a partially remapped chunk.

The current logic is that we keep the RAID flags as they are when a chunk
is remapped, and ncopies is derived from the RAID flags. I'm not entirely
sure this is the right thing to do, but it hopefully might make it easier
to discover what happened if something goes wrong. Similarly with
sub_stripes etc.

>>   		chunk_err(fs_info, leaf, chunk, logical,
>>   			  "invalid chunk num_stripes < ncopies, have %u < %d",
>>   			  num_stripes, ncopies);
>> @@ -960,7 +962,7 @@ int btrfs_check_chunk_valid(const struct btrfs_fs_info *fs_info,
>>   		}
>>   	}
>>   
>> -	if (unlikely((type & BTRFS_BLOCK_GROUP_RAID10 &&
>> +	if (unlikely(!remapped && ((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) ||
>> @@ -975,7 +977,7 @@ int btrfs_check_chunk_valid(const struct btrfs_fs_info *fs_info,
>>   		     (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))) {
>> +		      num_stripes != btrfs_raid_array[BTRFS_RAID_SINGLE].dev_stripes)))) {
>>   		chunk_err(fs_info, leaf, chunk, logical,
>>   			"invalid num_stripes:sub_stripes %u:%u for profile %llu",
>>   			num_stripes, sub_stripes,
>> @@ -999,11 +1001,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))) {
>>   		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;
> 
> Same complaint as above for nstripes < ncopies. Is there some way to
> more generically bypass stripe checking if we detect the case we care
> about: (remapped && num_stripes == 0) ?

I'm presuming this comment is for the if beginning on line 964. I'm moving this whole
bit to a helper function, it's too confusing as it is.

> 
>>   	}
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index e7c467b6af46..9159d11cb143 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -6133,6 +6133,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;
>> +	}
>> +
> 
> This seems kinda sketchy to me. Presumably once we have remapped a block
> group, we do want to discard it. But this makes that impossible? Does
> the discarding happen before we set num_stripes to 0? Even with
> discard=async?
> 
>>   	offset = logical - map->start;
>>   	length = min_t(u64, map->start + map->chunk_len - logical, length);
>>   	*length_ret = length;
>> @@ -6953,7 +6959,7 @@ u64 btrfs_calc_stripe_length(const struct btrfs_chunk_map *map)
>>   {
>>   	const int data_stripes = calc_data_stripes(map->type, map->num_stripes);
>>   
>> -	return div_u64(map->chunk_len, data_stripes);
>> +	return data_stripes ? div_u64(map->chunk_len, data_stripes) : 0;
> 
> Rather than making 0 a special value meaning remapped, would it be
> useful/clearer to also include a remapped flag on the btrfs_chunk_map?
> This function would have to behave the same way, presumably, but it
> would be less implicit, and we could make assertions on chunk maps that
> they have 0 stripes iff remapped at various places where we use the
> stripe length. That would raise confidence that all the uses of stripe
> logic account for remapped chunks.

It's less that 0 is a special value, more that btrfs_calc_stripe_length()
gets called from read_one_chunk(), and will divide-by-zero otherwise. I could
have put a check in read_one_chunk() instead, but this seems less footgunny.
If a chunk is fully remapped all I/O to it will be redirected elsewhere, so its
stripe length won't actually be used.

The BTRFS_BLOCK_GROUP_REMAPPED flag already gets set in the btrfs_chunk_map,
as does the num_stripes == 0 meaning fully remapped.

> 
>>   }
>>   
>>   #if BITS_PER_LONG == 32
>> -- 
>> 2.49.0
>>
>