Re: [PATCH v3 2/5] btrfs: split RAID stripes on deletion

[Johannes Thumshirn <Johannes.Thumshirn@wdc.com>]

On 01.07.24 16:07, Josef Bacik wrote:
> On Mon, Jul 01, 2024 at 12:25:16PM +0200, Johannes Thumshirn wrote:
>> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>
>> The current RAID stripe code assumes, that we will always remove a
>> whole stripe entry.
>>
>> But if we're only removing a part of a RAID stripe we're hitting the
>> ASSERT()ion checking for this condition.
>>
>> Instead of assuming the complete deletion of a RAID stripe, split the
>> stripe if we need to.
>>
>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>> ---
>>   fs/btrfs/ctree.c            |   1 +
>>   fs/btrfs/raid-stripe-tree.c | 100 +++++++++++++++++++++++++++++++++-----------
>>   2 files changed, 77 insertions(+), 24 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>> index e33f9f5a228d..16f9cf6360a4 100644
>> --- a/fs/btrfs/ctree.c
>> +++ b/fs/btrfs/ctree.c
>> @@ -3863,6 +3863,7 @@ static noinline int setup_leaf_for_split(struct btrfs_trans_handle *trans,
>>   	btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
>>   
>>   	BUG_ON(key.type != BTRFS_EXTENT_DATA_KEY &&
>> +	       key.type != BTRFS_RAID_STRIPE_KEY &&
>>   	       key.type != BTRFS_EXTENT_CSUM_KEY);
>>   
>>   	if (btrfs_leaf_free_space(leaf) >= ins_len)
>> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
>> index 3020820dd6e2..64e36b46cbab 100644
>> --- a/fs/btrfs/raid-stripe-tree.c
>> +++ b/fs/btrfs/raid-stripe-tree.c
>> @@ -33,42 +33,94 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
>>   	if (!path)
>>   		return -ENOMEM;
>>   
>> -	while (1) {
>> -		key.objectid = start;
>> -		key.type = BTRFS_RAID_STRIPE_KEY;
>> -		key.offset = length;
>> +again:
>> +	key.objectid = start;
>> +	key.type = BTRFS_RAID_STRIPE_KEY;
>> +	key.offset = length;
>>   
>> -		ret = btrfs_search_slot(trans, stripe_root, &key, path, -1, 1);
>> -		if (ret < 0)
>> -			break;
>> -		if (ret > 0) {
>> -			ret = 0;
>> -			if (path->slots[0] == 0)
>> -				break;
>> -			path->slots[0]--;
>> -		}
>> +	ret = btrfs_search_slot(trans, stripe_root, &key, path, -1, 1);
>> +	if (ret < 0)
>> +		goto out;
>> +	if (ret > 0) {
>> +		ret = 0;
>> +		if (path->slots[0] == 0)
>> +			goto out;
>> +		path->slots[0]--;
>> +	}
>> +
>> +	leaf = path->nodes[0];
>> +	slot = path->slots[0];
>> +	btrfs_item_key_to_cpu(leaf, &key, slot);
>> +	found_start = key.objectid;
>> +	found_end = found_start + key.offset;
>> +
>> +	/* That stripe ends before we start, we're done. */
>> +	if (found_end <= start)
>> +		goto out;
>> +
>> +	trace_btrfs_raid_extent_delete(fs_info, start, end,
>> +				       found_start, found_end);
>> +
>> +	if (found_start < start) {
>> +		u64 diff = start - found_start;
>> +		struct btrfs_key new_key;
>> +		int num_stripes;
>> +		struct btrfs_stripe_extent *stripe_extent;
>> +
>> +		new_key.objectid = start;
>> +		new_key.type = BTRFS_RAID_STRIPE_KEY;
>> +		new_key.offset = length - diff;
>> +
>> +		ret = btrfs_duplicate_item(trans, stripe_root, path,
>> +					   &new_key);
>> +		if (ret)
>> +			goto out;
>>   
>>   		leaf = path->nodes[0];
>>   		slot = path->slots[0];
>> -		btrfs_item_key_to_cpu(leaf, &key, slot);
>> -		found_start = key.objectid;
>> -		found_end = found_start + key.offset;
>>   
>> -		/* That stripe ends before we start, we're done. */
>> -		if (found_end <= start)
>> -			break;
>> +		num_stripes =
>> +			btrfs_num_raid_stripes(btrfs_item_size(leaf, slot));
>> +		stripe_extent =
>> +			btrfs_item_ptr(leaf, slot, struct btrfs_stripe_extent);
>> +
>> +		for (int i = 0; i < num_stripes; i++) {
>> +			struct btrfs_raid_stride *raid_stride =
>> +				&stripe_extent->strides[i];
>> +			u64 physical =
>> +				btrfs_raid_stride_physical(leaf, raid_stride);
>> +
>> +			btrfs_set_raid_stride_physical(leaf, raid_stride,
>> +							     physical + diff);
>> +		}
>> +
>> +		btrfs_mark_buffer_dirty(trans, leaf);
>> +		btrfs_release_path(path);
>> +		goto again;
>> +	}
>> +
>> +	if (found_end > end) {
>> +		u64 diff = found_end - end;
>> +		struct btrfs_key new_key;
>>   
>> -		trace_btrfs_raid_extent_delete(fs_info, start, end,
>> -					       found_start, found_end);
>> +		new_key.objectid = found_start;
>> +		new_key.type = BTRFS_RAID_STRIPE_KEY;
>> +		new_key.offset = length - diff;
>>   
>> -		ASSERT(found_start >= start && found_end <= end);
>> -		ret = btrfs_del_item(trans, stripe_root, path);
>> +		ret = btrfs_duplicate_item(trans, stripe_root, path,
>> +					   &new_key);
> 
> This seems incorrect to me.  If we have [0, 1MiB) and we're deleting [0,512KiB)
> then the tree at this point is
> 
> [0, BTRFS_RAID_STRIPE_KEY, 512KiB]
> [0, BTRFS_RAID_STRIPE_KEY, 1MiB]
> 
> which is valid as far as key ordering goes, but is a violation of the raid
> stripe tree design correct?  And then you do goto again, and then you'll delete
> 
> [0, BTRFS_RAID_STRIPE_KEY, 512KiB]
> 
> but leave the old one in place, correct?  Thanks,

Oh right, jumping back to again removes the wrong one :/