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

Johannes Thumshirn
2024-07-03T15:47:40Z

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 :/