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

Josef Bacik
2024-07-01T14:07:11Z

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,

Josef