[PATCH v2 00/14] btrfs: more RST delete fixes

[PATCH v2 00/14] btrfs: more RST delete fixes

[Johannes Thumshirn]

Here's another set of fixes for the delete path on RAID stripe-tree backed
filesystems.

Josef's CI system started tripping over a bad key order due to the usage
of btrfs_set_item_key_safe() in btrfs_partially_delete_raid_extent() and
while investigating what is happening there I found more bugs and not
handled corner cases, which resulted in more fixes and test-cases.

Unfortunately I couldn't fix the bad key order problem and had to resort
to re-creating the item in btrfs_partially_delete_raid_extent() and insert
the new one after deleting the old.

Fstests btrfs/06* are extremely good in exhibiting these failures and
btrfs/060 has been extensively run while developing this series.

A full CI run of v1 can be found here:
https://github.com/btrfs/linux/actions/runs/12291668397

Changes to v1:
- Handle extent_map lookup failure in 1/14
- Don't use key.offset = -1 for initial search in 3/14
- Don't break before calling btrfs_previous_item if we're on slot 0 in
  6/14
- Remove btrfs_mark_buffer_dirty calls
- Remove line breaks at 80 chars if we're just a bit over
- Fix multiple issues on comment styling

Link to v1:
https://lore.kernel.org/linux-btrfs/cover.1733989299.git.jth@kernel.org

Note:
I did not copy the implementation of btrfs_drop_extents() as I'd like to
have feedback on this variant first, before putting the time and energy in
a "completely new" implementation.

---
Johannes Thumshirn (14):
      btrfs: don't try to delete RAID stripe-extents if we don't need to
      btrfs: assert RAID stripe-extent length is always greater than 0
      btrfs: fix search when deleting a RAID stripe-extent
      btrfs: fix front delete range calculation for RAID stripe extents
      btrfs: fix tail delete of RAID stripe-extents
      btrfs: fix deletion of a range spanning parts two RAID stripe extents
      btrfs: implement hole punching for RAID stripe extents
      btrfs: don't use btrfs_set_item_key_safe on RAID stripe-extents
      btrfs: selftests: check for correct return value of failed lookup
      btrfs: selftests: don't split RAID extents in half
      btrfs: selftests: test RAID stripe-tree deletion spanning two items
      btrfs: selftests: add selftest for punching holes into the RAID stripe extents
      btrfs: selftests: add test for punching a hole into 3 RAID stripe-extents
      btrfs: selftests: add a selftest for deleting two out of three extents

 fs/btrfs/ctree.c                        |   1 +
 fs/btrfs/raid-stripe-tree.c             | 146 ++++++-
 fs/btrfs/tests/raid-stripe-tree-tests.c | 660 +++++++++++++++++++++++++++++++-
 3 files changed, 776 insertions(+), 31 deletions(-)
---
base-commit: 86e936bc54aa920fa4249f3fe96b4420964901f4
change-id: 20241218-rst-delete-fixes-f2659047f627

Best regards,
-- 
Johannes Thumshirn <jth@kernel.org>

[PATCH v2 01/14] btrfs: don't try to delete RAID stripe-extents if we don't need to

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Don't try to delete RAID stripe-extents if we don't need to.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/raid-stripe-tree.c             | 15 ++++++++++++++-
 fs/btrfs/tests/raid-stripe-tree-tests.c |  3 ++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
index 45b823a0913aea5fdaab91a80e79d253a66bb700..757e9c681f6c49f2d0295c1b3b2de56aad3c94a6 100644
--- a/fs/btrfs/raid-stripe-tree.c
+++ b/fs/btrfs/raid-stripe-tree.c
@@ -59,9 +59,22 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
 	int slot;
 	int ret;
 
-	if (!stripe_root)
+	if (!btrfs_fs_incompat(fs_info, RAID_STRIPE_TREE) || !stripe_root)
 		return 0;
 
+	if (!btrfs_is_testing(fs_info)) {
+		struct btrfs_chunk_map *map;
+		bool use_rst;
+
+		map = btrfs_find_chunk_map(fs_info, start, length);
+		if (!map)
+			return -EINVAL;
+		use_rst = btrfs_need_stripe_tree_update(fs_info, map->type);
+		btrfs_free_chunk_map(map);
+		if (!use_rst)
+			return 0;
+	}
+
 	path = btrfs_alloc_path();
 	if (!path)
 		return -ENOMEM;
diff --git a/fs/btrfs/tests/raid-stripe-tree-tests.c b/fs/btrfs/tests/raid-stripe-tree-tests.c
index 30f17eb7b6a8a1dfa9f66ed5508da42a70db1fa3..f060c04c7f76357e6d2c6ba78a8ba981e35645bd 100644
--- a/fs/btrfs/tests/raid-stripe-tree-tests.c
+++ b/fs/btrfs/tests/raid-stripe-tree-tests.c
@@ -478,8 +478,9 @@ static int run_test(test_func_t test, u32 sectorsize, u32 nodesize)
 		ret = PTR_ERR(root);
 		goto out;
 	}
-	btrfs_set_super_compat_ro_flags(root->fs_info->super_copy,
+	btrfs_set_super_incompat_flags(root->fs_info->super_copy,
 					BTRFS_FEATURE_INCOMPAT_RAID_STRIPE_TREE);
+	btrfs_set_fs_incompat(root->fs_info, RAID_STRIPE_TREE);
 	root->root_key.objectid = BTRFS_RAID_STRIPE_TREE_OBJECTID;
 	root->root_key.type = BTRFS_ROOT_ITEM_KEY;
 	root->root_key.offset = 0;

-- 
2.43.0

[PATCH v2 02/14] btrfs: assert RAID stripe-extent length is always greater than 0

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

When modifying a RAID stripe-extent, ASSERT() that the length of the new
RAID stripe-extent is always greater than 0.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/raid-stripe-tree.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
index 757e9c681f6c49f2d0295c1b3b2de56aad3c94a6..5c6224ed3eda53a11a41bffdf6c789fbd6d3a503 100644
--- a/fs/btrfs/raid-stripe-tree.c
+++ b/fs/btrfs/raid-stripe-tree.c
@@ -28,6 +28,7 @@ static void btrfs_partially_delete_raid_extent(struct btrfs_trans_handle *trans,
 		.offset = newlen,
 	};
 
+	ASSERT(newlen > 0);
 	ASSERT(oldkey->type == BTRFS_RAID_STRIPE_KEY);
 
 	leaf = path->nodes[0];

-- 
2.43.0

[PATCH v2 03/14] btrfs: fix search when deleting a RAID stripe-extent

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Only pick the previous slot, when btrfs_search_slot() returned '1'.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/raid-stripe-tree.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
index 5c6224ed3eda53a11a41bffdf6c789fbd6d3a503..0c4d218a99d4aaea5da6c39624e20e77758a89d3 100644
--- a/fs/btrfs/raid-stripe-tree.c
+++ b/fs/btrfs/raid-stripe-tree.c
@@ -89,8 +89,12 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
 		if (ret < 0)
 			break;
 
-		if (path->slots[0] == btrfs_header_nritems(path->nodes[0]))
-			path->slots[0]--;
+		if (ret == 1) {
+			ret = 0;
+			if (path->slots[0] ==
+			    btrfs_header_nritems(path->nodes[0]))
+				path->slots[0]--;
+		}
 
 		leaf = path->nodes[0];
 		slot = path->slots[0];

-- 
2.43.0

[PATCH v2 04/14] btrfs: fix front delete range calculation for RAID stripe extents

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

When deleting the front of a RAID stripe-extent the delete code
miscalculates the size on how much to pad the remaining extent part in the
front.

Fix the calculation so we're always having the sizes we expect.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/raid-stripe-tree.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
index 0c4d218a99d4aaea5da6c39624e20e77758a89d3..7fc6ef214f87d480df27023816dd800610d7dcf0 100644
--- a/fs/btrfs/raid-stripe-tree.c
+++ b/fs/btrfs/raid-stripe-tree.c
@@ -140,10 +140,12 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
 		 * length to the new size and then re-insert the item.
 		 */
 		if (found_end > end) {
-			u64 diff = found_end - end;
+			u64 diff_end = found_end - end;
 
 			btrfs_partially_delete_raid_extent(trans, path, &key,
-							   diff, diff);
+							   key.offset - length,
+							   length);
+			ASSERT(key.offset - diff_end == length);
 			break;
 		}
 

-- 
2.43.0

[PATCH v2 05/14] btrfs: fix tail delete of RAID stripe-extents

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Fix tail delete of RAID stripe-extents, if there is a range to be deleted
as well after the tail delete of the extent.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/raid-stripe-tree.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
index 7fc6ef214f87d480df27023816dd800610d7dcf0..79f8f692aaa8f6df2c9482fbd7777c2812528f65 100644
--- a/fs/btrfs/raid-stripe-tree.c
+++ b/fs/btrfs/raid-stripe-tree.c
@@ -123,11 +123,18 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
 		 * length to the new size and then re-insert the item.
 		 */
 		if (found_start < start) {
-			u64 diff = start - found_start;
+			u64 diff_start = start - found_start;
 
 			btrfs_partially_delete_raid_extent(trans, path, &key,
-							   diff, 0);
-			break;
+							   diff_start, 0);
+
+			start += (key.offset - diff_start);
+			length -= (key.offset - diff_start);
+			if (length == 0)
+				break;
+
+			btrfs_release_path(path);
+			continue;
 		}
 
 		/*

-- 
2.43.0

[PATCH v2 06/14] btrfs: fix deletion of a range spanning parts two RAID stripe extents

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

When a user requests the deletion of a range that spans multiple stripe
extents and btrfs_search_slot() returns us the second RAID stripe extent,
we need to pick the previous item and truncate it, if there's still a
range to delete left, move on to the next item.

The following diagram illustrates the operation:

 |--- RAID Stripe Extent ---||--- RAID Stripe Extent ---|
        |--- keep  ---|--- drop ---|

While at it, comment the trivial case of a whole item delete as well.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/raid-stripe-tree.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
index 79f8f692aaa8f6df2c9482fbd7777c2812528f65..893d963951315abfc734e1ca232b3087b7889431 100644
--- a/fs/btrfs/raid-stripe-tree.c
+++ b/fs/btrfs/raid-stripe-tree.c
@@ -103,6 +103,31 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
 		found_end = found_start + key.offset;
 		ret = 0;
 
+		/*
+		 * The stripe extent starts before the range we want to delete,
+		 * but the range spans more than one stripe extent:
+		 *
+		 * |--- RAID Stripe Extent ---||--- RAID Stripe Extent ---|
+		 *        |--- keep  ---|--- drop ---|
+		 *
+		 * This means we have to get the previous item, truncate its
+		 * length and then restart the search.
+		 */
+		if (found_start > start) {
+
+			ret = btrfs_previous_item(stripe_root, path, start,
+						  BTRFS_RAID_STRIPE_KEY);
+			if (ret < 0)
+				break;
+			ret = 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;
+		}
+
 		if (key.type != BTRFS_RAID_STRIPE_KEY)
 			break;
 
@@ -156,6 +181,9 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
 			break;
 		}
 
+		/*
+		 * Finally we can delete the whole item, no more special cases.
+		 */
 		ret = btrfs_del_item(trans, stripe_root, path);
 		if (ret)
 			break;

-- 
2.43.0

[PATCH v2 07/14] btrfs: implement hole punching for RAID stripe extents

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

If the stripe extent we want to delete starts before the range we want to
delete and ends after the range we want to delete we're punching a
hole in the stripe extent:

  |--- RAID Stripe Extent ---|
  | keep |--- drop ---| keep |

This means we need to a) truncate the existing item and b)
create a second item for the remaining range.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/ctree.c            |  1 +
 fs/btrfs/raid-stripe-tree.c | 49 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index c93f52a30a16028470594de1d1256dbec5c7899c..92071ca0655f0f1920eb841e77d3444a0e0d8834 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -3833,6 +3833,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 893d963951315abfc734e1ca232b3087b7889431..d15df49c61a86a4188b822b05453428e444920b5 100644
--- a/fs/btrfs/raid-stripe-tree.c
+++ b/fs/btrfs/raid-stripe-tree.c
@@ -138,6 +138,55 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
 		trace_btrfs_raid_extent_delete(fs_info, start, end,
 					       found_start, found_end);
 
+		/*
+		 * The stripe extent starts before the range we want to delete
+		 * and ends after the range we want to delete, i.e. we're
+		 * punching a hole in the stripe extent:
+		 *
+		 *  |--- RAID Stripe Extent ---|
+		 *  | keep |--- drop ---| keep |
+		 *
+		 * This means we need to a) truncate the existing item and b)
+		 * create a second item for the remaining range.
+		 */
+		if (found_start < start && found_end > end) {
+			size_t item_size;
+			u64 diff_start = start - found_start;
+			u64 diff_end = found_end - end;
+			struct btrfs_stripe_extent *extent;
+			struct btrfs_key newkey = {
+				.objectid = end,
+				.type = BTRFS_RAID_STRIPE_KEY,
+				.offset = diff_end,
+			};
+
+			/* "right" item */
+			ret = btrfs_duplicate_item(trans, stripe_root, path,
+						   &newkey);
+			if (ret)
+				break;
+
+			item_size = btrfs_item_size(leaf, path->slots[0]);
+			extent = btrfs_item_ptr(leaf, path->slots[0],
+						struct btrfs_stripe_extent);
+
+			for (int i = 0; i < btrfs_num_raid_stripes(item_size); i++) {
+				struct btrfs_raid_stride *stride = &extent->strides[i];
+				u64 phys;
+
+				phys = btrfs_raid_stride_physical(leaf, stride);
+				phys += diff_start + length;
+				btrfs_set_raid_stride_physical(leaf, stride, phys);
+			}
+
+			/* "left" item */
+			path->slots[0]--;
+			btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
+			btrfs_partially_delete_raid_extent(trans, path, &key,
+							   diff_start, 0);
+			break;
+		}
+
 		/*
 		 * The stripe extent starts before the range we want to delete:
 		 *

-- 
2.43.0

[PATCH v2 08/14] btrfs: don't use btrfs_set_item_key_safe on RAID stripe-extents

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Don't use btrfs_set_item_key_safe() to modify the keys in the RAID
stripe-tree as this can lead to corruption of the tree, which is caught by
the checks in btrfs_set_item_key_safe():

 BTRFS info (device nvme1n1): leaf 49168384 gen 15 total ptrs 194 free space 8329 owner 12
 BTRFS info (device nvme1n1): refs 2 lock_owner 1030 current 1030
  [ snip ]
  item 105 key (354549760 230 20480) itemoff 14587 itemsize 16
                  stride 0 devid 5 physical 67502080
  item 106 key (354631680 230 4096) itemoff 14571 itemsize 16
                  stride 0 devid 1 physical 88559616
  item 107 key (354631680 230 32768) itemoff 14555 itemsize 16
                  stride 0 devid 1 physical 88555520
  item 108 key (354717696 230 28672) itemoff 14539 itemsize 16
                  stride 0 devid 2 physical 67604480
  [ snip ]
 BTRFS critical (device nvme1n1): slot 106 key (354631680 230 32768) new key (354635776 230 4096)
 ------------[ cut here ]------------
 kernel BUG at fs/btrfs/ctree.c:2602!
 Oops: invalid opcode: 0000 [#1] PREEMPT SMP PTI
 CPU: 1 UID: 0 PID: 1055 Comm: fsstress Not tainted 6.13.0-rc1+ #1464
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-3-gd478f380-rebuilt.opensuse.org 04/01/2014
 RIP: 0010:btrfs_set_item_key_safe+0xf7/0x270
 Code: <snip>
 RSP: 0018:ffffc90001337ab0 EFLAGS: 00010287
 RAX: 0000000000000000 RBX: ffff8881115fd000 RCX: 0000000000000000
 RDX: 0000000000000001 RSI: 0000000000000001 RDI: 00000000ffffffff
 RBP: ffff888110ed6f50 R08: 00000000ffffefff R09: ffffffff8244c500
 R10: 00000000ffffefff R11: 00000000ffffffff R12: ffff888100586000
 R13: 00000000000000c9 R14: ffffc90001337b1f R15: ffff888110f23b58
 FS:  00007f7d75c72740(0000) GS:ffff88813bd00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00007fa811652c60 CR3: 0000000111398001 CR4: 0000000000370eb0
 Call Trace:
  <TASK>
  ? __die_body.cold+0x14/0x1a
  ? die+0x2e/0x50
  ? do_trap+0xca/0x110
  ? do_error_trap+0x65/0x80
  ? btrfs_set_item_key_safe+0xf7/0x270
  ? exc_invalid_op+0x50/0x70
  ? btrfs_set_item_key_safe+0xf7/0x270
  ? asm_exc_invalid_op+0x1a/0x20
  ? btrfs_set_item_key_safe+0xf7/0x270
  btrfs_partially_delete_raid_extent+0xc4/0xe0
  btrfs_delete_raid_extent+0x227/0x240
  __btrfs_free_extent.isra.0+0x57f/0x9c0
  ? exc_coproc_segment_overrun+0x40/0x40
  __btrfs_run_delayed_refs+0x2fa/0xe80
  btrfs_run_delayed_refs+0x81/0xe0
  btrfs_commit_transaction+0x2dd/0xbe0
  ? preempt_count_add+0x52/0xb0
  btrfs_sync_file+0x375/0x4c0
  do_fsync+0x39/0x70
  __x64_sys_fsync+0x13/0x20
  do_syscall_64+0x54/0x110
  entry_SYSCALL_64_after_hwframe+0x76/0x7e
 RIP: 0033:0x7f7d7550ef90
 Code: <snip>
 RSP: 002b:00007ffd70237248 EFLAGS: 00000202 ORIG_RAX: 000000000000004a
 RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007f7d7550ef90
 RDX: 000000000000013a RSI: 000000000040eb28 RDI: 0000000000000004
 RBP: 000000000000001b R08: 0000000000000078 R09: 00007ffd7023725c
 R10: 00007f7d75400390 R11: 0000000000000202 R12: 028f5c28f5c28f5c
 R13: 8f5c28f5c28f5c29 R14: 000000000040b520 R15: 00007f7d75c726c8
  </TASK>

Instead copy the item, adjust the key and per-device physical addresses
and re-insert it into the tree.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/raid-stripe-tree.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
index d15df49c61a86a4188b822b05453428e444920b5..a4225ad043216e5d7035a71eab6bcc49b242836f 100644
--- a/fs/btrfs/raid-stripe-tree.c
+++ b/fs/btrfs/raid-stripe-tree.c
@@ -13,12 +13,13 @@
 #include "volumes.h"
 #include "print-tree.h"
 
-static void btrfs_partially_delete_raid_extent(struct btrfs_trans_handle *trans,
+static int btrfs_partially_delete_raid_extent(struct btrfs_trans_handle *trans,
 					       struct btrfs_path *path,
 					       const struct btrfs_key *oldkey,
 					       u64 newlen, u64 frontpad)
 {
-	struct btrfs_stripe_extent *extent;
+	struct btrfs_root *stripe_root = trans->fs_info->stripe_root;
+	struct btrfs_stripe_extent *extent, *new;
 	struct extent_buffer *leaf;
 	int slot;
 	size_t item_size;
@@ -27,6 +28,7 @@ static void btrfs_partially_delete_raid_extent(struct btrfs_trans_handle *trans,
 		.type = BTRFS_RAID_STRIPE_KEY,
 		.offset = newlen,
 	};
+	int ret;
 
 	ASSERT(newlen > 0);
 	ASSERT(oldkey->type == BTRFS_RAID_STRIPE_KEY);
@@ -34,17 +36,31 @@ static void btrfs_partially_delete_raid_extent(struct btrfs_trans_handle *trans,
 	leaf = path->nodes[0];
 	slot = path->slots[0];
 	item_size = btrfs_item_size(leaf, slot);
+
+	new = kzalloc(item_size, GFP_NOFS);
+	if (!new)
+		return -ENOMEM;
+
 	extent = btrfs_item_ptr(leaf, slot, struct btrfs_stripe_extent);
 
 	for (int i = 0; i < btrfs_num_raid_stripes(item_size); i++) {
 		struct btrfs_raid_stride *stride = &extent->strides[i];
 		u64 phys;
 
-		phys = btrfs_raid_stride_physical(leaf, stride);
-		btrfs_set_raid_stride_physical(leaf, stride, phys + frontpad);
+		phys = btrfs_raid_stride_physical(leaf, stride) + frontpad;
+		btrfs_set_stack_raid_stride_physical(&new->strides[i], phys);
 	}
 
-	btrfs_set_item_key_safe(trans, path, &newkey);
+	ret = btrfs_del_item(trans, stripe_root, path);
+	if (ret)
+		goto out;
+
+	btrfs_release_path(path);
+	ret = btrfs_insert_item(trans, stripe_root, &newkey, new, item_size);
+
+out:
+	kfree(new);
+	return ret;
 }
 
 int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 length)

-- 
2.43.0

[PATCH v2 09/14] btrfs: selftests: check for correct return value of failed lookup

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Commit 5e72aabc1fff ("btrfs: return ENODATA in case RST lookup fails")
changed btrfs_get_raid_extent_offset()'s return value to ENODATA in case
the RAID stripe-tree lookup failed.

Adjust the test cases which check for absence of a given range to check
for ENODATA as return value in this case.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/tests/raid-stripe-tree-tests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/tests/raid-stripe-tree-tests.c b/fs/btrfs/tests/raid-stripe-tree-tests.c
index f060c04c7f76357e6d2c6ba78a8ba981e35645bd..19f6147a38a54f6fe581264a971542840bc61180 100644
--- a/fs/btrfs/tests/raid-stripe-tree-tests.c
+++ b/fs/btrfs/tests/raid-stripe-tree-tests.c
@@ -125,7 +125,7 @@ static int test_front_delete(struct btrfs_trans_handle *trans)
 	}
 
 	ret = btrfs_get_raid_extent_offset(fs_info, logical, &len, map_type, 0, &io_stripe);
-	if (!ret) {
+	if (ret != -ENODATA) {
 		ret = -EINVAL;
 		test_err("lookup of RAID extent [%llu, %llu] succeeded, should fail",
 			 logical, logical + SZ_32K);

-- 
2.43.0

[PATCH v2 10/14] btrfs: selftests: don't split RAID extents in half

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

The selftests for partially deleting the start or tail of RAID
stripe-extents split these extents in half.

This can hide errors in the calculation, so don't split the RAID
stripe-extents in half but delete the first or last 16K of the 64K
extents.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/tests/raid-stripe-tree-tests.c | 44 +++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/tests/raid-stripe-tree-tests.c b/fs/btrfs/tests/raid-stripe-tree-tests.c
index 19f6147a38a54f6fe581264a971542840bc61180..12f3dbb23a6423c4d224c5b087b35862b8d643db 100644
--- a/fs/btrfs/tests/raid-stripe-tree-tests.c
+++ b/fs/btrfs/tests/raid-stripe-tree-tests.c
@@ -14,6 +14,8 @@
 #define RST_TEST_NUM_DEVICES	(2)
 #define RST_TEST_RAID1_TYPE	(BTRFS_BLOCK_GROUP_DATA | BTRFS_BLOCK_GROUP_RAID1)
 
+#define SZ_48K (SZ_32K + SZ_16K)
+
 typedef int (*test_func_t)(struct btrfs_trans_handle *trans);
 
 static struct btrfs_device *btrfs_device_by_devid(struct btrfs_fs_devices *fs_devices,
@@ -94,32 +96,32 @@ static int test_front_delete(struct btrfs_trans_handle *trans)
 		goto out;
 	}
 
-	ret = btrfs_delete_raid_extent(trans, logical, SZ_32K);
+	ret = btrfs_delete_raid_extent(trans, logical, SZ_16K);
 	if (ret) {
 		test_err("deleting RAID extent [%llu, %llu] failed", logical,
-			 logical + SZ_32K);
+			 logical + SZ_16K);
 		goto out;
 	}
 
-	len = SZ_32K;
-	ret = btrfs_get_raid_extent_offset(fs_info, logical + SZ_32K, &len,
+	len -= SZ_16K;
+	ret = btrfs_get_raid_extent_offset(fs_info, logical + SZ_16K, &len,
 					   map_type, 0, &io_stripe);
 	if (ret) {
 		test_err("lookup of RAID extent [%llu, %llu] failed",
-			 logical + SZ_32K, logical + SZ_32K + len);
+			 logical + SZ_16K, logical + SZ_64K);
 		goto out;
 	}
 
-	if (io_stripe.physical != logical + SZ_32K) {
+	if (io_stripe.physical != logical + SZ_16K) {
 		test_err("invalid physical address, expected %llu, got %llu",
-			 logical + SZ_32K, io_stripe.physical);
+			 logical + SZ_16K, io_stripe.physical);
 		ret = -EINVAL;
 		goto out;
 	}
 
-	if (len != SZ_32K) {
+	if (len != SZ_48K) {
 		test_err("invalid stripe length, expected %llu, got %llu",
-			 (u64)SZ_32K, len);
+			 (u64)SZ_48K, len);
 		ret = -EINVAL;
 		goto out;
 	}
@@ -128,11 +130,11 @@ static int test_front_delete(struct btrfs_trans_handle *trans)
 	if (ret != -ENODATA) {
 		ret = -EINVAL;
 		test_err("lookup of RAID extent [%llu, %llu] succeeded, should fail",
-			 logical, logical + SZ_32K);
+			 logical, logical + SZ_16K);
 		goto out;
 	}
 
-	ret = btrfs_delete_raid_extent(trans, logical + SZ_32K, SZ_32K);
+	ret = btrfs_delete_raid_extent(trans, logical + SZ_16K, SZ_48K);
 out:
 	btrfs_put_bioc(bioc);
 	return ret;
@@ -209,14 +211,14 @@ static int test_tail_delete(struct btrfs_trans_handle *trans)
 		goto out;
 	}
 
-	ret = btrfs_delete_raid_extent(trans, logical + SZ_32K, SZ_32K);
+	ret = btrfs_delete_raid_extent(trans, logical + SZ_48K, SZ_16K);
 	if (ret) {
 		test_err("deleting RAID extent [%llu, %llu] failed",
-			 logical + SZ_32K, logical + SZ_64K);
+			 logical + SZ_48K, logical + SZ_64K);
 		goto out;
 	}
 
-	len = SZ_32K;
+	len = SZ_48K;
 	ret = btrfs_get_raid_extent_offset(fs_info, logical, &len, map_type, 0, &io_stripe);
 	if (ret) {
 		test_err("lookup of RAID extent [%llu, %llu] failed", logical,
@@ -231,9 +233,19 @@ static int test_tail_delete(struct btrfs_trans_handle *trans)
 		goto out;
 	}
 
-	if (len != SZ_32K) {
+	if (len != SZ_48K) {
 		test_err("invalid stripe length, expected %llu, got %llu",
-			 (u64)SZ_32K, len);
+			 (u64)SZ_48K, len);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	len = SZ_16K;
+	ret = btrfs_get_raid_extent_offset(fs_info, logical + SZ_48K, &len,
+					   map_type, 0, &io_stripe);
+	if (ret != -ENODATA) {
+		test_err("lookup of RAID extent [%llu, %llu] succeeded should fail",
+			 logical + SZ_48K, logical + SZ_64K);
 		ret = -EINVAL;
 		goto out;
 	}

-- 
2.43.0

[PATCH v2 11/14] btrfs: selftests: test RAID stripe-tree deletion spanning two items

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Add a selftest for RAID stripe-tree deletion with a delete range spanning
two items, so that we're punching a hole into two adjacent RAID stripe
extents truncating the first and "moving" the second to the right.

The following diagram illustrates the operation:

 |--- RAID Stripe Extent ---||--- RAID Stripe Extent ---|
 |-----  keep  -----|--- drop ---|-----  keep  ----|

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/tests/raid-stripe-tree-tests.c | 144 ++++++++++++++++++++++++++++++++
 1 file changed, 144 insertions(+)

diff --git a/fs/btrfs/tests/raid-stripe-tree-tests.c b/fs/btrfs/tests/raid-stripe-tree-tests.c
index 12f3dbb23a6423c4d224c5b087b35862b8d643db..a815fc5c4dd32e9b10844ad6df34f418c2e88ce7 100644
--- a/fs/btrfs/tests/raid-stripe-tree-tests.c
+++ b/fs/btrfs/tests/raid-stripe-tree-tests.c
@@ -31,6 +31,149 @@ static struct btrfs_device *btrfs_device_by_devid(struct btrfs_fs_devices *fs_de
 	return NULL;
 }
 
+/*
+ * Test a 1M RST write that spans two adjecent RST items on disk and then
+ * delete a portion starting in the first item and spanning into the second
+ * item. This is similar to test_front_delete(), but spanning multiple items.
+ */
+static int test_front_delete_prev_item(struct btrfs_trans_handle *trans)
+{
+	struct btrfs_fs_info *fs_info = trans->fs_info;
+	struct btrfs_io_context *bioc;
+	struct btrfs_io_stripe io_stripe = { 0 };
+	u64 map_type = RST_TEST_RAID1_TYPE;
+	u64 logical1 = SZ_1M;
+	u64 logical2 = SZ_2M;
+	u64 len = SZ_1M;
+	int ret;
+
+	bioc = alloc_btrfs_io_context(fs_info, logical1, RST_TEST_NUM_DEVICES);
+	if (!bioc) {
+		test_std_err(TEST_ALLOC_IO_CONTEXT);
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	io_stripe.dev = btrfs_device_by_devid(fs_info->fs_devices, 0);
+	bioc->map_type = map_type;
+	bioc->size = len;
+
+	/* insert RAID extent 1. */
+	for (int i = 0; i < RST_TEST_NUM_DEVICES; i++) {
+		struct btrfs_io_stripe *stripe = &bioc->stripes[i];
+
+		stripe->dev = btrfs_device_by_devid(fs_info->fs_devices, i);
+		if (!stripe->dev) {
+			test_err("cannot find device with devid %d", i);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		stripe->physical = logical1 + i * SZ_1G;
+	}
+
+	ret = btrfs_insert_one_raid_extent(trans, bioc);
+	if (ret) {
+		test_err("inserting RAID extent failed: %d", ret);
+		goto out;
+	}
+
+	bioc->logical = logical2;
+	/* Insert RAID extent 2, directly adjacent to it. */
+	for (int i = 0; i < RST_TEST_NUM_DEVICES; i++) {
+		struct btrfs_io_stripe *stripe = &bioc->stripes[i];
+
+		stripe->dev = btrfs_device_by_devid(fs_info->fs_devices, i);
+		if (!stripe->dev) {
+			test_err("cannot find device with devid %d", i);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		stripe->physical = logical2 + i * SZ_1G;
+	}
+
+	ret = btrfs_insert_one_raid_extent(trans, bioc);
+	if (ret) {
+		test_err("inserting RAID extent failed: %d", ret);
+		goto out;
+	}
+
+	ret = btrfs_delete_raid_extent(trans, logical1 + SZ_512K, SZ_1M);
+	if (ret) {
+		test_err("deleting RAID extent [%llu, %llu] failed",
+			 logical1 + SZ_512K, (u64)SZ_1M);
+		goto out;
+	}
+
+	/* Verify item 1 is truncated to 512K. */
+	ret = btrfs_get_raid_extent_offset(fs_info, logical1, &len, map_type, 0,
+					   &io_stripe);
+	if (ret) {
+		test_err("lookup of RAID extent [%llu, %llu] failed", logical1,
+			 logical1 + len);
+		goto out;
+	}
+
+	if (io_stripe.physical != logical1) {
+		test_err("invalid physical address, expected %llu got %llu",
+			 logical1, io_stripe.physical);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (len != SZ_512K) {
+		test_err("invalid stripe length, expected %llu got %llu",
+			 (u64)SZ_512K, len);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Verify item 2's start is moved by 512K. */
+	ret = btrfs_get_raid_extent_offset(fs_info, logical2 + SZ_512K, &len,
+					   map_type, 0, &io_stripe);
+	if (ret) {
+		test_err("lookup of RAID extent [%llu, %llu] failed",
+			 logical2 + SZ_512K, logical2 + len);
+		goto out;
+	}
+
+	if (io_stripe.physical != logical2 + SZ_512K) {
+		test_err("invalid physical address, expected %llu got %llu",
+			 logical2 + SZ_512K, io_stripe.physical);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (len != SZ_512K) {
+		test_err("invalid stripe length, expected %llu got %llu",
+			 (u64)SZ_512K, len);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Verify there's a hole at [1M+512K, 2M+512K] */
+	len = SZ_1M;
+	ret = btrfs_get_raid_extent_offset(fs_info, logical1 + SZ_512K, &len,
+					   map_type, 0, &io_stripe);
+	if (ret != -ENODATA) {
+		test_err("lookup of RAID [%llu, %llu] succeeded, should fail",
+			 logical1 + SZ_512K, logical1 + SZ_512K + len);
+		goto out;
+	}
+
+	/* Clean up after us. */
+	ret = btrfs_delete_raid_extent(trans, logical1, SZ_512K);
+	if (ret)
+		goto out;
+
+	ret = btrfs_delete_raid_extent(trans, logical2 + SZ_512K, SZ_512K);
+
+out:
+	btrfs_put_bioc(bioc);
+	return ret;
+}
+
 /*
  * Test a 64K RST write on a 2 disk RAID1 at a logical address of 1M and then
  * delete the 1st 32K, making the new start address 1M+32K.
@@ -468,6 +611,7 @@ static const test_func_t tests[] = {
 	test_create_update_delete,
 	test_tail_delete,
 	test_front_delete,
+	test_front_delete_prev_item,
 };
 
 static int run_test(test_func_t test, u32 sectorsize, u32 nodesize)

-- 
2.43.0

[PATCH v2 12/14] btrfs: selftests: add selftest for punching holes into the RAID stripe extents

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Add a selftest for punching a hole into a RAID stripe extent. The test
create an 1M extent and punches a 64k bytes long hole at offset of 32k from
the start of the extent.

Afterwards it verifies the start and length of both resulting new extents
"left" and "right" as well as the absence of the hole.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/tests/raid-stripe-tree-tests.c | 140 ++++++++++++++++++++++++++++++++
 1 file changed, 140 insertions(+)

diff --git a/fs/btrfs/tests/raid-stripe-tree-tests.c b/fs/btrfs/tests/raid-stripe-tree-tests.c
index a815fc5c4dd32e9b10844ad6df34f418c2e88ce7..c7e44e944f5ecc37ffb937237cb81fefbafbaf9a 100644
--- a/fs/btrfs/tests/raid-stripe-tree-tests.c
+++ b/fs/btrfs/tests/raid-stripe-tree-tests.c
@@ -31,6 +31,145 @@ static struct btrfs_device *btrfs_device_by_devid(struct btrfs_fs_devices *fs_de
 	return NULL;
 }
 
+/* Test punching a hole into a single RAID stripe-extent. */
+static int test_punch_hole(struct btrfs_trans_handle *trans)
+{
+	struct btrfs_fs_info *fs_info = trans->fs_info;
+	struct btrfs_io_context *bioc;
+	struct btrfs_io_stripe io_stripe = { 0 };
+	u64 map_type = RST_TEST_RAID1_TYPE;
+	u64 logical1 = SZ_1M;
+	u64 hole_start = logical1 + SZ_32K;
+	u64 hole_len = SZ_64K;
+	u64 logical2 = hole_start + hole_len;
+	u64 len = SZ_1M;
+	u64 len1 = SZ_32K;
+	u64 len2 = len - len1 - hole_len;
+	int ret;
+
+	bioc = alloc_btrfs_io_context(fs_info, logical1, RST_TEST_NUM_DEVICES);
+	if (!bioc) {
+		test_std_err(TEST_ALLOC_IO_CONTEXT);
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	io_stripe.dev = btrfs_device_by_devid(fs_info->fs_devices, 0);
+	bioc->map_type = map_type;
+	bioc->size = len;
+
+	for (int i = 0; i < RST_TEST_NUM_DEVICES; i++) {
+		struct btrfs_io_stripe *stripe = &bioc->stripes[i];
+
+		stripe->dev = btrfs_device_by_devid(fs_info->fs_devices, i);
+		if (!stripe->dev) {
+			test_err("cannot find device with devid %d", i);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		stripe->physical = logical1 + i * SZ_1G;
+	}
+
+	ret = btrfs_insert_one_raid_extent(trans, bioc);
+	if (ret) {
+		test_err("inserting RAID extent failed: %d", ret);
+		goto out;
+	}
+
+	ret = btrfs_get_raid_extent_offset(fs_info, logical1, &len, map_type, 0,
+					   &io_stripe);
+	if (ret) {
+		test_err("lookup of RAID extent [%llu, %llu] failed", logical1,
+			 logical1 + len);
+		goto out;
+	}
+
+	if (io_stripe.physical != logical1) {
+		test_err("invalid physical address, expected %llu got %llu",
+			 logical1, io_stripe.physical);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (len != SZ_1M) {
+		test_err("invalid stripe length, expected %llu got %llu",
+			 (u64)SZ_1M, len);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = btrfs_delete_raid_extent(trans, hole_start, hole_len);
+	if (ret) {
+		test_err("deleting RAID extent [%llu, %llu] failed",
+			 hole_start, hole_start + hole_len);
+		goto out;
+	}
+
+	ret = btrfs_get_raid_extent_offset(fs_info, logical1, &len1, map_type,
+					   0, &io_stripe);
+	if (ret) {
+		test_err("lookup of RAID extent [%llu, %llu] failed",
+			 logical1, logical1 + len1);
+		goto out;
+	}
+
+	if (io_stripe.physical != logical1) {
+		test_err("invalid physical address, expected %llu, got %llu",
+			 logical1, io_stripe.physical);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (len1 != SZ_32K) {
+		test_err("invalid stripe length, expected %llu, got %llu",
+			 (u64)SZ_32K, len1);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = btrfs_get_raid_extent_offset(fs_info, logical2, &len2, map_type,
+					   0, &io_stripe);
+	if (ret) {
+		test_err("lookup of RAID extent [%llu, %llu] failed", logical2,
+			 logical2 + len2);
+		goto out;
+	}
+
+	if (io_stripe.physical != logical2) {
+		test_err("invalid physical address, expected %llu, got %llu",
+			 logical2, io_stripe.physical);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (len2 != len - len1 - hole_len) {
+		test_err("invalid length, expected %llu, got %llu",
+			 len - len1 - hole_len, len2);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Check for the absence of the hole. */
+	ret = btrfs_get_raid_extent_offset(fs_info, hole_start, &hole_len,
+					   map_type, 0, &io_stripe);
+	if (ret != -ENODATA) {
+		ret = -EINVAL;
+		test_err("lookup of RAID extent [%llu, %llu] succeeded, should fail",
+			 hole_start, hole_start + SZ_64K);
+		goto out;
+	}
+
+	ret = btrfs_delete_raid_extent(trans, logical1, len1);
+	if (ret)
+		goto out;
+
+	ret = btrfs_delete_raid_extent(trans, logical2, len2);
+out:
+	btrfs_put_bioc(bioc);
+	return ret;
+}
+
 /*
  * Test a 1M RST write that spans two adjecent RST items on disk and then
  * delete a portion starting in the first item and spanning into the second
@@ -612,6 +751,7 @@ static const test_func_t tests[] = {
 	test_tail_delete,
 	test_front_delete,
 	test_front_delete_prev_item,
+	test_punch_hole,
 };
 
 static int run_test(test_func_t test, u32 sectorsize, u32 nodesize)

-- 
2.43.0

[PATCH v2 13/14] btrfs: selftests: add test for punching a hole into 3 RAID stripe-extents

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Test creating a range of three RAID stripe-extents and then punch a hole
in the middle, deleting all of the middle extents and partially deleting
the "book ends".

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/tests/raid-stripe-tree-tests.c | 183 ++++++++++++++++++++++++++++++++
 1 file changed, 183 insertions(+)

diff --git a/fs/btrfs/tests/raid-stripe-tree-tests.c b/fs/btrfs/tests/raid-stripe-tree-tests.c
index c7e44e944f5ecc37ffb937237cb81fefbafbaf9a..e12b6abbfd2be66170d33ab13b1e0971444c0f9a 100644
--- a/fs/btrfs/tests/raid-stripe-tree-tests.c
+++ b/fs/btrfs/tests/raid-stripe-tree-tests.c
@@ -31,6 +31,188 @@ static struct btrfs_device *btrfs_device_by_devid(struct btrfs_fs_devices *fs_de
 	return NULL;
 }
 
+/*
+ * Test creating a range of three extents and then punch a hole in the middle,
+ * deleting all of the middle extents and partially deleting the "book ends"
+ */
+static int test_punch_hole_3extents(struct btrfs_trans_handle *trans)
+{
+	struct btrfs_fs_info *fs_info = trans->fs_info;
+	struct btrfs_io_context *bioc;
+	struct btrfs_io_stripe io_stripe = { 0 };
+	u64 map_type = RST_TEST_RAID1_TYPE;
+	u64 logical1 = SZ_1M;
+	u64 len1 = SZ_1M;
+	u64 logical2 = logical1 + len1;
+	u64 len2 = SZ_1M;
+	u64 logical3 = logical2 + len2;
+	u64 len3 = SZ_1M;
+	u64 hole_start = logical1 + SZ_256K;
+	u64 hole_len = SZ_2M;
+	int ret;
+
+	bioc = alloc_btrfs_io_context(fs_info, logical1, RST_TEST_NUM_DEVICES);
+	if (!bioc) {
+		test_std_err(TEST_ALLOC_IO_CONTEXT);
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	io_stripe.dev = btrfs_device_by_devid(fs_info->fs_devices, 0);
+
+	/* Prepare for the test, 1st create 3 x 1M extents. */
+	bioc->map_type = map_type;
+	bioc->size = len1;
+
+	for (int i = 0; i < RST_TEST_NUM_DEVICES; i++) {
+		struct btrfs_io_stripe *stripe = &bioc->stripes[i];
+
+		stripe->dev = btrfs_device_by_devid(fs_info->fs_devices, i);
+		if (!stripe->dev) {
+			test_err("cannot find device with devid %d", i);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		stripe->physical = logical1 + i * SZ_1G;
+	}
+
+	ret = btrfs_insert_one_raid_extent(trans, bioc);
+	if (ret) {
+		test_err("inserting RAID extent failed: %d", ret);
+		goto out;
+	}
+
+	bioc->logical = logical2;
+	bioc->size = len2;
+	for (int i = 0; i < RST_TEST_NUM_DEVICES; i++) {
+		struct btrfs_io_stripe *stripe = &bioc->stripes[i];
+
+		stripe->dev = btrfs_device_by_devid(fs_info->fs_devices, i);
+		if (!stripe->dev) {
+			test_err("cannot find device with devid %d", i);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		stripe->physical = logical2 + i * SZ_1G;
+	}
+
+	ret = btrfs_insert_one_raid_extent(trans, bioc);
+	if (ret) {
+		test_err("inserting RAID extent failed: %d", ret);
+		goto out;
+	}
+
+	bioc->logical = logical3;
+	bioc->size = len3;
+	for (int i = 0; i < RST_TEST_NUM_DEVICES; i++) {
+		struct btrfs_io_stripe *stripe = &bioc->stripes[i];
+
+		stripe->dev = btrfs_device_by_devid(fs_info->fs_devices, i);
+		if (!stripe->dev) {
+			test_err("cannot find device with devid %d", i);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		stripe->physical = logical3 + i * SZ_1G;
+	}
+
+	ret = btrfs_insert_one_raid_extent(trans, bioc);
+	if (ret) {
+		test_err("inserting RAID extent failed: %d", ret);
+		goto out;
+	}
+
+	/*
+	 * Delete a range starting at logical1 + 256K and 2M in length. Extent
+	 * 1 is truncated to 256k length, extent 2 is completely dropped and
+	 * extent 3 is moved 256K to the right.
+	 */
+	ret = btrfs_delete_raid_extent(trans, hole_start, hole_len);
+	if (ret) {
+		test_err("deleting RAID extent [%llu, %llu] failed",
+			 hole_start, hole_start + hole_len);
+		goto out;
+	}
+
+	/* Get the first extent and check its size. */
+	ret = btrfs_get_raid_extent_offset(fs_info, logical1, &len1, map_type,
+					   0, &io_stripe);
+	if (ret) {
+		test_err("lookup of RAID extent [%llu, %llu] failed",
+			 logical1, logical1 + len1);
+		goto out;
+	}
+
+	if (io_stripe.physical != logical1) {
+		test_err("invalid physical address, expected %llu, got %llu",
+			 logical1, io_stripe.physical);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (len1 != SZ_256K) {
+		test_err("invalid stripe length, expected %llu, got %llu",
+			 (u64)SZ_256K, len1);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Get the second extent and check it's absent. */
+	ret = btrfs_get_raid_extent_offset(fs_info, logical2, &len2, map_type,
+					   0, &io_stripe);
+	if (ret != -ENODATA) {
+		test_err("lookup of RAID extent [%llu, %llu] succeeded should fail",
+			 logical2, logical2 + len2);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Get the third extent and check its size. */
+	logical3 += SZ_256K;
+	ret = btrfs_get_raid_extent_offset(fs_info, logical3, &len3, map_type,
+					   0, &io_stripe);
+	if (ret) {
+		test_err("lookup of RAID extent [%llu, %llu] failed",
+			 logical3, logical3 + len3);
+		goto out;
+	}
+
+	if (io_stripe.physical != logical3) {
+		test_err("invalid physical address, expected %llu, got %llu",
+			 logical3 + SZ_256K, io_stripe.physical);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (len3 != SZ_1M - SZ_256K) {
+		test_err("invalid stripe length, expected %llu, got %llu",
+			 (u64)SZ_1M - SZ_256K, len3);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = btrfs_delete_raid_extent(trans, logical1, len1);
+	if (ret) {
+		test_err("deleting RAID extent [%llu, %llu] failed",
+			 logical1, logical1 + len1);
+		goto out;
+	}
+
+	ret = btrfs_delete_raid_extent(trans, logical3, len3);
+	if (ret) {
+		test_err("deleting RAID extent [%llu, %llu] failed",
+			 logical1, logical1 + len1);
+		goto out;
+	}
+
+out:
+	btrfs_put_bioc(bioc);
+	return ret;
+}
+
 /* Test punching a hole into a single RAID stripe-extent. */
 static int test_punch_hole(struct btrfs_trans_handle *trans)
 {
@@ -752,6 +934,7 @@ static const test_func_t tests[] = {
 	test_front_delete,
 	test_front_delete_prev_item,
 	test_punch_hole,
+	test_punch_hole_3extents,
 };
 
 static int run_test(test_func_t test, u32 sectorsize, u32 nodesize)

-- 
2.43.0

[PATCH v2 14/14] btrfs: selftests: add a selftest for deleting two out of three extents

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Add a selftest creating three extents and then deleting two out of the
three extents.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/tests/raid-stripe-tree-tests.c | 144 ++++++++++++++++++++++++++++++++
 1 file changed, 144 insertions(+)

diff --git a/fs/btrfs/tests/raid-stripe-tree-tests.c b/fs/btrfs/tests/raid-stripe-tree-tests.c
index e12b6abbfd2be66170d33ab13b1e0971444c0f9a..6c7e561e55641472a63bb15c8c2213273111ad08 100644
--- a/fs/btrfs/tests/raid-stripe-tree-tests.c
+++ b/fs/btrfs/tests/raid-stripe-tree-tests.c
@@ -213,6 +213,149 @@ static int test_punch_hole_3extents(struct btrfs_trans_handle *trans)
 	return ret;
 }
 
+static int test_delete_two_extents(struct btrfs_trans_handle *trans)
+{
+	struct btrfs_fs_info *fs_info = trans->fs_info;
+	struct btrfs_io_context *bioc;
+	struct btrfs_io_stripe io_stripe = { 0 };
+	u64 map_type = RST_TEST_RAID1_TYPE;
+	u64 logical1 = SZ_1M;
+	u64 len1 = SZ_1M;
+	u64 logical2 = logical1 + len1;
+	u64 len2 = SZ_1M;
+	u64 logical3 = logical2 + len2;
+	u64 len3 = SZ_1M;
+	int ret;
+
+	bioc = alloc_btrfs_io_context(fs_info, logical1, RST_TEST_NUM_DEVICES);
+	if (!bioc) {
+		test_std_err(TEST_ALLOC_IO_CONTEXT);
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	io_stripe.dev = btrfs_device_by_devid(fs_info->fs_devices, 0);
+
+	/* Prepare for the test, 1st create 3 x 1M extents. */
+	bioc->map_type = map_type;
+	bioc->size = len1;
+
+	for (int i = 0; i < RST_TEST_NUM_DEVICES; i++) {
+		struct btrfs_io_stripe *stripe = &bioc->stripes[i];
+
+		stripe->dev = btrfs_device_by_devid(fs_info->fs_devices, i);
+		if (!stripe->dev) {
+			test_err("cannot find device with devid %d", i);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		stripe->physical = logical1 + i * SZ_1G;
+	}
+
+	ret = btrfs_insert_one_raid_extent(trans, bioc);
+	if (ret) {
+		test_err("inserting RAID extent failed: %d", ret);
+		goto out;
+	}
+
+	bioc->logical = logical2;
+	bioc->size = len2;
+	for (int i = 0; i < RST_TEST_NUM_DEVICES; i++) {
+		struct btrfs_io_stripe *stripe = &bioc->stripes[i];
+
+		stripe->dev = btrfs_device_by_devid(fs_info->fs_devices, i);
+		if (!stripe->dev) {
+			test_err("cannot find device with devid %d", i);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		stripe->physical = logical2 + i * SZ_1G;
+	}
+
+	ret = btrfs_insert_one_raid_extent(trans, bioc);
+	if (ret) {
+		test_err("inserting RAID extent failed: %d", ret);
+		goto out;
+	}
+
+	bioc->logical = logical3;
+	bioc->size = len3;
+	for (int i = 0; i < RST_TEST_NUM_DEVICES; i++) {
+		struct btrfs_io_stripe *stripe = &bioc->stripes[i];
+
+		stripe->dev = btrfs_device_by_devid(fs_info->fs_devices, i);
+		if (!stripe->dev) {
+			test_err("cannot find device with devid %d", i);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		stripe->physical = logical3 + i * SZ_1G;
+	}
+
+	ret = btrfs_insert_one_raid_extent(trans, bioc);
+	if (ret) {
+		test_err("inserting RAID extent failed: %d", ret);
+		goto out;
+	}
+
+	/*
+	 * Delete a range starting at logical1 and 2M in length. Extents 1
+	 * and 2 are dropped and extent 3 is kept as is.
+	 */
+	ret = btrfs_delete_raid_extent(trans, logical1, len1 + len2);
+	if (ret) {
+		test_err("deleting RAID extent [%llu, %llu] failed",
+			 logical1, logical1 + len1 + len2);
+		goto out;
+	}
+
+	ret = btrfs_get_raid_extent_offset(fs_info, logical1, &len1, map_type,
+					   0, &io_stripe);
+	if (ret != -ENODATA) {
+		test_err("lookup of RAID extent [%llu, %llu] suceeded, should fail\n",
+			 logical1, len1);
+		goto out;
+	}
+
+	ret = btrfs_get_raid_extent_offset(fs_info, logical2, &len2, map_type,
+					   0, &io_stripe);
+	if (ret != -ENODATA) {
+		test_err("lookup of RAID extent [%llu, %llu] suceeded, should fail\n",
+			 logical2, len2);
+		goto out;
+	}
+
+	ret = btrfs_get_raid_extent_offset(fs_info, logical3, &len3, map_type,
+					   0, &io_stripe);
+	if (ret) {
+		test_err("lookup of RAID extent [%llu, %llu] failed\n",
+			 logical3, len3);
+		goto out;
+	}
+
+	if (io_stripe.physical != logical3) {
+		test_err("invalid physical address, expected %llu, got %llu",
+			 logical3, io_stripe.physical);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (len3 != SZ_1M) {
+		test_err("invalid stripe length, expected %llu, got %llu",
+			 (u64)SZ_1M, len3);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = btrfs_delete_raid_extent(trans, logical3, len3);
+out:
+	btrfs_put_bioc(bioc);
+	return ret;
+}
+
 /* Test punching a hole into a single RAID stripe-extent. */
 static int test_punch_hole(struct btrfs_trans_handle *trans)
 {
@@ -935,6 +1078,7 @@ static const test_func_t tests[] = {
 	test_front_delete_prev_item,
 	test_punch_hole,
 	test_punch_hole_3extents,
+	test_delete_two_extents,
 };
 
 static int run_test(test_func_t test, u32 sectorsize, u32 nodesize)

-- 
2.43.0

Re: [PATCH v2 00/14] btrfs: more RST delete fixes

[David Sterba]

On Tue, Jan 07, 2025 at 01:47:30PM +0100, Johannes Thumshirn wrote:
> Here's another set of fixes for the delete path on RAID stripe-tree backed
> filesystems.
> 
> Josef's CI system started tripping over a bad key order due to the usage
> of btrfs_set_item_key_safe() in btrfs_partially_delete_raid_extent() and
> while investigating what is happening there I found more bugs and not
> handled corner cases, which resulted in more fixes and test-cases.
> 
> Unfortunately I couldn't fix the bad key order problem and had to resort
> to re-creating the item in btrfs_partially_delete_raid_extent() and insert
> the new one after deleting the old.
> 
> Fstests btrfs/06* are extremely good in exhibiting these failures and
> btrfs/060 has been extensively run while developing this series.
> 
> A full CI run of v1 can be found here:
> https://github.com/btrfs/linux/actions/runs/12291668397
> 
> Changes to v1:
> - Handle extent_map lookup failure in 1/14
> - Don't use key.offset = -1 for initial search in 3/14
> - Don't break before calling btrfs_previous_item if we're on slot 0 in
>   6/14
> - Remove btrfs_mark_buffer_dirty calls
> - Remove line breaks at 80 chars if we're just a bit over
> - Fix multiple issues on comment styling
> 
> Link to v1:
> https://lore.kernel.org/linux-btrfs/cover.1733989299.git.jth@kernel.org
> 
> Note:
> I did not copy the implementation of btrfs_drop_extents() as I'd like to
> have feedback on this variant first, before putting the time and energy in
> a "completely new" implementation.
> 
> ---
> Johannes Thumshirn (14):
>       btrfs: don't try to delete RAID stripe-extents if we don't need to
>       btrfs: assert RAID stripe-extent length is always greater than 0
>       btrfs: fix search when deleting a RAID stripe-extent
>       btrfs: fix front delete range calculation for RAID stripe extents
>       btrfs: fix tail delete of RAID stripe-extents
>       btrfs: fix deletion of a range spanning parts two RAID stripe extents
>       btrfs: implement hole punching for RAID stripe extents
>       btrfs: don't use btrfs_set_item_key_safe on RAID stripe-extents
>       btrfs: selftests: check for correct return value of failed lookup
>       btrfs: selftests: don't split RAID extents in half
>       btrfs: selftests: test RAID stripe-tree deletion spanning two items
>       btrfs: selftests: add selftest for punching holes into the RAID stripe extents
>       btrfs: selftests: add test for punching a hole into 3 RAID stripe-extents
>       btrfs: selftests: add a selftest for deleting two out of three extents
> 
>  fs/btrfs/ctree.c                        |   1 +
>  fs/btrfs/raid-stripe-tree.c             | 146 ++++++-
>  fs/btrfs/tests/raid-stripe-tree-tests.c | 660 +++++++++++++++++++++++++++++++-
>  3 files changed, 776 insertions(+), 31 deletions(-)

As this is completely in RST I'm considering it safe for late merge
(ideally by the end of this week before rc7 is out).

Re: [PATCH v2 01/14] btrfs: don't try to delete RAID stripe-extents if we don't need to

[David Sterba]

On Tue, Jan 07, 2025 at 01:47:31PM +0100, Johannes Thumshirn wrote:
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> Don't try to delete RAID stripe-extents if we don't need to.

Please add why it's not needed.

> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  fs/btrfs/raid-stripe-tree.c             | 15 ++++++++++++++-
>  fs/btrfs/tests/raid-stripe-tree-tests.c |  3 ++-
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
> index 45b823a0913aea5fdaab91a80e79d253a66bb700..757e9c681f6c49f2d0295c1b3b2de56aad3c94a6 100644
> --- a/fs/btrfs/raid-stripe-tree.c
> +++ b/fs/btrfs/raid-stripe-tree.c
> @@ -59,9 +59,22 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
>  	int slot;
>  	int ret;
>  
> -	if (!stripe_root)
> +	if (!btrfs_fs_incompat(fs_info, RAID_STRIPE_TREE) || !stripe_root)
>  		return 0;
>  
> +	if (!btrfs_is_testing(fs_info)) {
> +		struct btrfs_chunk_map *map;
> +		bool use_rst;
> +
> +		map = btrfs_find_chunk_map(fs_info, start, length);
> +		if (!map)
> +			return -EINVAL;
> +		use_rst = btrfs_need_stripe_tree_update(fs_info, map->type);
> +		btrfs_free_chunk_map(map);
> +		if (!use_rst)
> +			return 0;
> +	}
> +
>  	path = btrfs_alloc_path();
>  	if (!path)
>  		return -ENOMEM;
> diff --git a/fs/btrfs/tests/raid-stripe-tree-tests.c b/fs/btrfs/tests/raid-stripe-tree-tests.c
> index 30f17eb7b6a8a1dfa9f66ed5508da42a70db1fa3..f060c04c7f76357e6d2c6ba78a8ba981e35645bd 100644
> --- a/fs/btrfs/tests/raid-stripe-tree-tests.c
> +++ b/fs/btrfs/tests/raid-stripe-tree-tests.c
> @@ -478,8 +478,9 @@ static int run_test(test_func_t test, u32 sectorsize, u32 nodesize)
>  		ret = PTR_ERR(root);
>  		goto out;
>  	}
> -	btrfs_set_super_compat_ro_flags(root->fs_info->super_copy,
> +	btrfs_set_super_incompat_flags(root->fs_info->super_copy,
>  					BTRFS_FEATURE_INCOMPAT_RAID_STRIPE_TREE);
> +	btrfs_set_fs_incompat(root->fs_info, RAID_STRIPE_TREE);
>  	root->root_key.objectid = BTRFS_RAID_STRIPE_TREE_OBJECTID;
>  	root->root_key.type = BTRFS_ROOT_ITEM_KEY;
>  	root->root_key.offset = 0;
> 
> -- 
> 2.43.0
> 

Re: [PATCH v2 03/14] btrfs: fix search when deleting a RAID stripe-extent

[David Sterba]

On Tue, Jan 07, 2025 at 01:47:33PM +0100, Johannes Thumshirn wrote:
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> Only pick the previous slot, when btrfs_search_slot() returned '1'.

"which means we did not find the key."

> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  fs/btrfs/raid-stripe-tree.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
> index 5c6224ed3eda53a11a41bffdf6c789fbd6d3a503..0c4d218a99d4aaea5da6c39624e20e77758a89d3 100644
> --- a/fs/btrfs/raid-stripe-tree.c
> +++ b/fs/btrfs/raid-stripe-tree.c
> @@ -89,8 +89,12 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
>  		if (ret < 0)
>  			break;
>  
> -		if (path->slots[0] == btrfs_header_nritems(path->nodes[0]))
> -			path->slots[0]--;
> +		if (ret == 1) {
> +			ret = 0;
> +			if (path->slots[0] ==
> +			    btrfs_header_nritems(path->nodes[0]))
> +				path->slots[0]--;
> +		}
>  
>  		leaf = path->nodes[0];
>  		slot = path->slots[0];
> 
> -- 
> 2.43.0
> 

Re: [PATCH v2 08/14] btrfs: don't use btrfs_set_item_key_safe on RAID stripe-extents

[David Sterba]

On Tue, Jan 07, 2025 at 01:47:38PM +0100, Johannes Thumshirn wrote:
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> Don't use btrfs_set_item_key_safe() to modify the keys in the RAID
> stripe-tree as this can lead to corruption of the tree, which is caught by
> the checks in btrfs_set_item_key_safe():
> 
>  BTRFS info (device nvme1n1): leaf 49168384 gen 15 total ptrs 194 free space 8329 owner 12
>  BTRFS info (device nvme1n1): refs 2 lock_owner 1030 current 1030
>   [ snip ]
>   item 105 key (354549760 230 20480) itemoff 14587 itemsize 16
>                   stride 0 devid 5 physical 67502080
>   item 106 key (354631680 230 4096) itemoff 14571 itemsize 16
>                   stride 0 devid 1 physical 88559616
>   item 107 key (354631680 230 32768) itemoff 14555 itemsize 16
>                   stride 0 devid 1 physical 88555520
>   item 108 key (354717696 230 28672) itemoff 14539 itemsize 16
>                   stride 0 devid 2 physical 67604480
>   [ snip ]
>  BTRFS critical (device nvme1n1): slot 106 key (354631680 230 32768) new key (354635776 230 4096)
>  ------------[ cut here ]------------
>  kernel BUG at fs/btrfs/ctree.c:2602!
>  Oops: invalid opcode: 0000 [#1] PREEMPT SMP PTI
>  CPU: 1 UID: 0 PID: 1055 Comm: fsstress Not tainted 6.13.0-rc1+ #1464
>  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-3-gd478f380-rebuilt.opensuse.org 04/01/2014
>  RIP: 0010:btrfs_set_item_key_safe+0xf7/0x270
>  Code: <snip>
>  RSP: 0018:ffffc90001337ab0 EFLAGS: 00010287
>  RAX: 0000000000000000 RBX: ffff8881115fd000 RCX: 0000000000000000
>  RDX: 0000000000000001 RSI: 0000000000000001 RDI: 00000000ffffffff
>  RBP: ffff888110ed6f50 R08: 00000000ffffefff R09: ffffffff8244c500
>  R10: 00000000ffffefff R11: 00000000ffffffff R12: ffff888100586000
>  R13: 00000000000000c9 R14: ffffc90001337b1f R15: ffff888110f23b58
>  FS:  00007f7d75c72740(0000) GS:ffff88813bd00000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 00007fa811652c60 CR3: 0000000111398001 CR4: 0000000000370eb0
>  Call Trace:
>   <TASK>
>   ? __die_body.cold+0x14/0x1a
>   ? die+0x2e/0x50
>   ? do_trap+0xca/0x110
>   ? do_error_trap+0x65/0x80
>   ? btrfs_set_item_key_safe+0xf7/0x270
>   ? exc_invalid_op+0x50/0x70
>   ? btrfs_set_item_key_safe+0xf7/0x270
>   ? asm_exc_invalid_op+0x1a/0x20
>   ? btrfs_set_item_key_safe+0xf7/0x270
>   btrfs_partially_delete_raid_extent+0xc4/0xe0
>   btrfs_delete_raid_extent+0x227/0x240
>   __btrfs_free_extent.isra.0+0x57f/0x9c0
>   ? exc_coproc_segment_overrun+0x40/0x40
>   __btrfs_run_delayed_refs+0x2fa/0xe80
>   btrfs_run_delayed_refs+0x81/0xe0
>   btrfs_commit_transaction+0x2dd/0xbe0
>   ? preempt_count_add+0x52/0xb0
>   btrfs_sync_file+0x375/0x4c0
>   do_fsync+0x39/0x70
>   __x64_sys_fsync+0x13/0x20
>   do_syscall_64+0x54/0x110
>   entry_SYSCALL_64_after_hwframe+0x76/0x7e
>  RIP: 0033:0x7f7d7550ef90
>  Code: <snip>
>  RSP: 002b:00007ffd70237248 EFLAGS: 00000202 ORIG_RAX: 000000000000004a
>  RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007f7d7550ef90
>  RDX: 000000000000013a RSI: 000000000040eb28 RDI: 0000000000000004
>  RBP: 000000000000001b R08: 0000000000000078 R09: 00007ffd7023725c
>  R10: 00007f7d75400390 R11: 0000000000000202 R12: 028f5c28f5c28f5c
>  R13: 8f5c28f5c28f5c29 R14: 000000000040b520 R15: 00007f7d75c726c8
>   </TASK>
> 
> Instead copy the item, adjust the key and per-device physical addresses
> and re-insert it into the tree.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  fs/btrfs/raid-stripe-tree.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
> index d15df49c61a86a4188b822b05453428e444920b5..a4225ad043216e5d7035a71eab6bcc49b242836f 100644
> --- a/fs/btrfs/raid-stripe-tree.c
> +++ b/fs/btrfs/raid-stripe-tree.c
> @@ -13,12 +13,13 @@
>  #include "volumes.h"
>  #include "print-tree.h"
>  
> -static void btrfs_partially_delete_raid_extent(struct btrfs_trans_handle *trans,
> +static int btrfs_partially_delete_raid_extent(struct btrfs_trans_handle *trans,
>  					       struct btrfs_path *path,
>  					       const struct btrfs_key *oldkey,
>  					       u64 newlen, u64 frontpad)
>  {
> -	struct btrfs_stripe_extent *extent;
> +	struct btrfs_root *stripe_root = trans->fs_info->stripe_root;
> +	struct btrfs_stripe_extent *extent, *new;

Maybe call it 'newitem' so it's clear that it's related to newlen and
newkey.

>  	struct extent_buffer *leaf;
>  	int slot;
>  	size_t item_size;

Re: [PATCH v2 01/14] btrfs: don't try to delete RAID stripe-extents if we don't need to

[Filipe Manana]

On Tue, Jan 7, 2025 at 12:48 PM Johannes Thumshirn <jth@kernel.org> wrote:
>
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
> Don't try to delete RAID stripe-extents if we don't need to.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  fs/btrfs/raid-stripe-tree.c             | 15 ++++++++++++++-
>  fs/btrfs/tests/raid-stripe-tree-tests.c |  3 ++-
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
> index 45b823a0913aea5fdaab91a80e79d253a66bb700..757e9c681f6c49f2d0295c1b3b2de56aad3c94a6 100644
> --- a/fs/btrfs/raid-stripe-tree.c
> +++ b/fs/btrfs/raid-stripe-tree.c
> @@ -59,9 +59,22 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
>         int slot;
>         int ret;
>
> -       if (!stripe_root)
> +       if (!btrfs_fs_incompat(fs_info, RAID_STRIPE_TREE) || !stripe_root)
>                 return 0;
>
> +       if (!btrfs_is_testing(fs_info)) {
> +               struct btrfs_chunk_map *map;
> +               bool use_rst;
> +
> +               map = btrfs_find_chunk_map(fs_info, start, length);
> +               if (!map)
> +                       return -EINVAL;
> +               use_rst = btrfs_need_stripe_tree_update(fs_info, map->type);
> +               btrfs_free_chunk_map(map);
> +               if (!use_rst)
> +                       return 0;
> +       }
> +
>         path = btrfs_alloc_path();
>         if (!path)
>                 return -ENOMEM;
> diff --git a/fs/btrfs/tests/raid-stripe-tree-tests.c b/fs/btrfs/tests/raid-stripe-tree-tests.c
> index 30f17eb7b6a8a1dfa9f66ed5508da42a70db1fa3..f060c04c7f76357e6d2c6ba78a8ba981e35645bd 100644
> --- a/fs/btrfs/tests/raid-stripe-tree-tests.c
> +++ b/fs/btrfs/tests/raid-stripe-tree-tests.c
> @@ -478,8 +478,9 @@ static int run_test(test_func_t test, u32 sectorsize, u32 nodesize)
>                 ret = PTR_ERR(root);
>                 goto out;
>         }
> -       btrfs_set_super_compat_ro_flags(root->fs_info->super_copy,
> +       btrfs_set_super_incompat_flags(root->fs_info->super_copy,
>                                         BTRFS_FEATURE_INCOMPAT_RAID_STRIPE_TREE);

This hunk seems unrelated to the rest of the patch, could be fixed in
a different patch in case it actually solves any problem (probably
not, but it's an incompat feature so it should be changed anyway).

I agree the changelog should be more clear, just say we don't need to
attempt the delete if the rst feature is not enabled.

Anyway:

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

> +       btrfs_set_fs_incompat(root->fs_info, RAID_STRIPE_TREE);
>         root->root_key.objectid = BTRFS_RAID_STRIPE_TREE_OBJECTID;
>         root->root_key.type = BTRFS_ROOT_ITEM_KEY;
>         root->root_key.offset = 0;
>
> --
> 2.43.0
>
>

Re: [PATCH v2 03/14] btrfs: fix search when deleting a RAID stripe-extent

[Filipe Manana]

On Tue, Jan 7, 2025 at 12:48 PM Johannes Thumshirn <jth@kernel.org> wrote:
>
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
> Only pick the previous slot, when btrfs_search_slot() returned '1'.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  fs/btrfs/raid-stripe-tree.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
> index 5c6224ed3eda53a11a41bffdf6c789fbd6d3a503..0c4d218a99d4aaea5da6c39624e20e77758a89d3 100644
> --- a/fs/btrfs/raid-stripe-tree.c
> +++ b/fs/btrfs/raid-stripe-tree.c
> @@ -89,8 +89,12 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
>                 if (ret < 0)
>                         break;
>
> -               if (path->slots[0] == btrfs_header_nritems(path->nodes[0]))
> -                       path->slots[0]--;
> +               if (ret == 1) {
> +                       ret = 0;
> +                       if (path->slots[0] ==
> +                           btrfs_header_nritems(path->nodes[0]))

Btw this can fit in a single line, it stays at 83 characters which is
acceptable nowadays, making things a bit more readable.
I've commented on this many times before in other patches.

Can you explain what bug is this patch fixing?
The changelog doesn't provide any information about that.

path->slots[0] should only match btrfs_header_nritems(path->nodes[0])
when the key wasn't found, that is, when ret == 1.
So I don't see what this patch is trying to fix or improve.

Also, the 'ret = 0' is pointless, as we do it shortly after this code.

Thanks.


> +                               path->slots[0]--;
> +               }
>
>                 leaf = path->nodes[0];
>                 slot = path->slots[0];
>
> --
> 2.43.0
>
>

Re: [PATCH v2 05/14] btrfs: fix tail delete of RAID stripe-extents

[Filipe Manana]

On Tue, Jan 7, 2025 at 12:49 PM Johannes Thumshirn <jth@kernel.org> wrote:
>
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
> Fix tail delete of RAID stripe-extents, if there is a range to be deleted
> as well after the tail delete of the extent.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/raid-stripe-tree.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
> index 7fc6ef214f87d480df27023816dd800610d7dcf0..79f8f692aaa8f6df2c9482fbd7777c2812528f65 100644
> --- a/fs/btrfs/raid-stripe-tree.c
> +++ b/fs/btrfs/raid-stripe-tree.c
> @@ -123,11 +123,18 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
>                  * length to the new size and then re-insert the item.
>                  */
>                 if (found_start < start) {
> -                       u64 diff = start - found_start;
> +                       u64 diff_start = start - found_start;
>
>                         btrfs_partially_delete_raid_extent(trans, path, &key,
> -                                                          diff, 0);
> -                       break;
> +                                                          diff_start, 0);
> +
> +                       start += (key.offset - diff_start);
> +                       length -= (key.offset - diff_start);
> +                       if (length == 0)
> +                               break;
> +
> +                       btrfs_release_path(path);
> +                       continue;
>                 }
>
>                 /*
>
> --
> 2.43.0
>
>

Re: [PATCH v2 03/14] btrfs: fix search when deleting a RAID stripe-extent

[Johannes Thumshirn]

On 09.01.25 13:42, Filipe Manana wrote:
> On Tue, Jan 7, 2025 at 12:48 PM Johannes Thumshirn <jth@kernel.org> wrote:
>>
>> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>
>> Only pick the previous slot, when btrfs_search_slot() returned '1'.
>>
>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>> ---
>>   fs/btrfs/raid-stripe-tree.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
>> index 5c6224ed3eda53a11a41bffdf6c789fbd6d3a503..0c4d218a99d4aaea5da6c39624e20e77758a89d3 100644
>> --- a/fs/btrfs/raid-stripe-tree.c
>> +++ b/fs/btrfs/raid-stripe-tree.c
>> @@ -89,8 +89,12 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
>>                  if (ret < 0)
>>                          break;
>>
>> -               if (path->slots[0] == btrfs_header_nritems(path->nodes[0]))
>> -                       path->slots[0]--;
>> +               if (ret == 1) {
>> +                       ret = 0;
>> +                       if (path->slots[0] ==
>> +                           btrfs_header_nritems(path->nodes[0]))
> 
> Btw this can fit in a single line, it stays at 83 characters which is
> acceptable nowadays, making things a bit more readable.
> I've commented on this many times before in other patches.
> 
> Can you explain what bug is this patch fixing?
> The changelog doesn't provide any information about that.
> 
> path->slots[0] should only match btrfs_header_nritems(path->nodes[0])
> when the key wasn't found, that is, when ret == 1.
> So I don't see what this patch is trying to fix or improve.
> 
> Also, the 'ret = 0' is pointless, as we do it shortly after this code.
> 

You're right, that patch is BS. I'll drop it.

Re: [PATCH v2 01/14] btrfs: don't try to delete RAID stripe-extents if we don't need to

[Johannes Thumshirn]

On 09.01.25 13:35, Filipe Manana wrote:
>> diff --git a/fs/btrfs/tests/raid-stripe-tree-tests.c b/fs/btrfs/tests/raid-stripe-tree-tests.c
>> index 30f17eb7b6a8a1dfa9f66ed5508da42a70db1fa3..f060c04c7f76357e6d2c6ba78a8ba981e35645bd 100644
>> --- a/fs/btrfs/tests/raid-stripe-tree-tests.c
>> +++ b/fs/btrfs/tests/raid-stripe-tree-tests.c
>> @@ -478,8 +478,9 @@ static int run_test(test_func_t test, u32 sectorsize, u32 nodesize)
>>                  ret = PTR_ERR(root);
>>                  goto out;
>>          }
>> -       btrfs_set_super_compat_ro_flags(root->fs_info->super_copy,
>> +       btrfs_set_super_incompat_flags(root->fs_info->super_copy,
>>                                          BTRFS_FEATURE_INCOMPAT_RAID_STRIPE_TREE);
> This hunk seems unrelated to the rest of the patch, could be fixed in
> a different patch in case it actually solves any problem (probably
> not, but it's an incompat feature so it should be changed anyway).

I'll make it a separate patch. RST is an incompat feature not a compat one.

With this patch btrfs_delete_raid_extent() starts checking the incompat 
bit so it is fixing a 'problem'.

Re: [PATCH v2 01/14] btrfs: don't try to delete RAID stripe-extents if we don't need to

[Filipe Manana]

On Thu, Jan 9, 2025 at 2:39 PM Johannes Thumshirn
<Johannes.Thumshirn@wdc.com> wrote:
>
> On 09.01.25 13:35, Filipe Manana wrote:
> >> diff --git a/fs/btrfs/tests/raid-stripe-tree-tests.c b/fs/btrfs/tests/raid-stripe-tree-tests.c
> >> index 30f17eb7b6a8a1dfa9f66ed5508da42a70db1fa3..f060c04c7f76357e6d2c6ba78a8ba981e35645bd 100644
> >> --- a/fs/btrfs/tests/raid-stripe-tree-tests.c
> >> +++ b/fs/btrfs/tests/raid-stripe-tree-tests.c
> >> @@ -478,8 +478,9 @@ static int run_test(test_func_t test, u32 sectorsize, u32 nodesize)
> >>                  ret = PTR_ERR(root);
> >>                  goto out;
> >>          }
> >> -       btrfs_set_super_compat_ro_flags(root->fs_info->super_copy,
> >> +       btrfs_set_super_incompat_flags(root->fs_info->super_copy,
> >>                                          BTRFS_FEATURE_INCOMPAT_RAID_STRIPE_TREE);
> > This hunk seems unrelated to the rest of the patch, could be fixed in
> > a different patch in case it actually solves any problem (probably
> > not, but it's an incompat feature so it should be changed anyway).
>
> I'll make it a separate patch. RST is an incompat feature not a compat one.
>
> With this patch btrfs_delete_raid_extent() starts checking the incompat
> bit so it is fixing a 'problem'.

Yes, but for that all that's needed is this call:

btrfs_set_fs_incompat(root->fs_info, RAID_STRIPE_TREE);

Right?

Replacing the btrfs_set_super_compat_ro_flags() call with a call to
btrfs_set_super_incompat_flags() shouldn't be needed for this patch.
That's what I was referring to.

Re: [PATCH v2 06/14] btrfs: fix deletion of a range spanning parts two RAID stripe extents

[Filipe Manana]

On Tue, Jan 7, 2025 at 12:50 PM Johannes Thumshirn <jth@kernel.org> wrote:
>
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
> When a user requests the deletion of a range that spans multiple stripe
> extents and btrfs_search_slot() returns us the second RAID stripe extent,
> we need to pick the previous item and truncate it, if there's still a
> range to delete left, move on to the next item.
>
> The following diagram illustrates the operation:
>
>  |--- RAID Stripe Extent ---||--- RAID Stripe Extent ---|
>         |--- keep  ---|--- drop ---|
>
> While at it, comment the trivial case of a whole item delete as well.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  fs/btrfs/raid-stripe-tree.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
> index 79f8f692aaa8f6df2c9482fbd7777c2812528f65..893d963951315abfc734e1ca232b3087b7889431 100644
> --- a/fs/btrfs/raid-stripe-tree.c
> +++ b/fs/btrfs/raid-stripe-tree.c
> @@ -103,6 +103,31 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
>                 found_end = found_start + key.offset;
>                 ret = 0;
>
> +               /*
> +                * The stripe extent starts before the range we want to delete,
> +                * but the range spans more than one stripe extent:
> +                *
> +                * |--- RAID Stripe Extent ---||--- RAID Stripe Extent ---|
> +                *        |--- keep  ---|--- drop ---|
> +                *
> +                * This means we have to get the previous item, truncate its
> +                * length and then restart the search.
> +                */
> +               if (found_start > start) {
> +
> +                       ret = btrfs_previous_item(stripe_root, path, start,
> +                                                 BTRFS_RAID_STRIPE_KEY);
> +                       if (ret < 0)
> +                               break;
> +                       ret = 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;

Hum, this isn't safe, ignoring the case where btrfs_previous_item()
returns 1, meaning there's no previous item.

In that case previous_item() returns pointing to the same leaf and
slot, and then below we delete the item instead of trimming it
(increasing its range start and decreasing its length).

Thanks.

> +               }
> +
>                 if (key.type != BTRFS_RAID_STRIPE_KEY)
>                         break;
>
> @@ -156,6 +181,9 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
>                         break;
>                 }
>
> +               /*
> +                * Finally we can delete the whole item, no more special cases.
> +                */
>                 ret = btrfs_del_item(trans, stripe_root, path);
>                 if (ret)
>                         break;
>
> --
> 2.43.0
>
>

Re: [PATCH v2 01/14] btrfs: don't try to delete RAID stripe-extents if we don't need to

[Johannes Thumshirn]

On 09.01.25 16:15, Filipe Manana wrote:
> On Thu, Jan 9, 2025 at 2:39 PM Johannes Thumshirn
> <Johannes.Thumshirn@wdc.com> wrote:
>>
>> On 09.01.25 13:35, Filipe Manana wrote:
>>>> diff --git a/fs/btrfs/tests/raid-stripe-tree-tests.c b/fs/btrfs/tests/raid-stripe-tree-tests.c
>>>> index 30f17eb7b6a8a1dfa9f66ed5508da42a70db1fa3..f060c04c7f76357e6d2c6ba78a8ba981e35645bd 100644
>>>> --- a/fs/btrfs/tests/raid-stripe-tree-tests.c
>>>> +++ b/fs/btrfs/tests/raid-stripe-tree-tests.c
>>>> @@ -478,8 +478,9 @@ static int run_test(test_func_t test, u32 sectorsize, u32 nodesize)
>>>>                   ret = PTR_ERR(root);
>>>>                   goto out;
>>>>           }
>>>> -       btrfs_set_super_compat_ro_flags(root->fs_info->super_copy,
>>>> +       btrfs_set_super_incompat_flags(root->fs_info->super_copy,
>>>>                                           BTRFS_FEATURE_INCOMPAT_RAID_STRIPE_TREE);
>>> This hunk seems unrelated to the rest of the patch, could be fixed in
>>> a different patch in case it actually solves any problem (probably
>>> not, but it's an incompat feature so it should be changed anyway).
>>
>> I'll make it a separate patch. RST is an incompat feature not a compat one.
>>
>> With this patch btrfs_delete_raid_extent() starts checking the incompat
>> bit so it is fixing a 'problem'.
> 
> Yes, but for that all that's needed is this call:
> 
> btrfs_set_fs_incompat(root->fs_info, RAID_STRIPE_TREE);
> 
> Right?
> 
> Replacing the btrfs_set_super_compat_ro_flags() call with a call to
> btrfs_set_super_incompat_flags() shouldn't be needed for this patch.
> That's what I was referring to.
> 

Ah now I see the problem. I used btrfs_set_super_incompat_flags() 
instead of btrfs_set_fs_incompat() *facepalm*

Re: [PATCH v2 07/14] btrfs: implement hole punching for RAID stripe extents

[Filipe Manana]

On Tue, Jan 7, 2025 at 12:59 PM Johannes Thumshirn <jth@kernel.org> wrote:
>
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
> If the stripe extent we want to delete starts before the range we want to
> delete and ends after the range we want to delete we're punching a
> hole in the stripe extent:
>
>   |--- RAID Stripe Extent ---|
>   | keep |--- drop ---| keep |
>
> This means we need to a) truncate the existing item and b)
> create a second item for the remaining range.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/ctree.c            |  1 +
>  fs/btrfs/raid-stripe-tree.c | 49 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index c93f52a30a16028470594de1d1256dbec5c7899c..92071ca0655f0f1920eb841e77d3444a0e0d8834 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -3833,6 +3833,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 893d963951315abfc734e1ca232b3087b7889431..d15df49c61a86a4188b822b05453428e444920b5 100644
> --- a/fs/btrfs/raid-stripe-tree.c
> +++ b/fs/btrfs/raid-stripe-tree.c
> @@ -138,6 +138,55 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
>                 trace_btrfs_raid_extent_delete(fs_info, start, end,
>                                                found_start, found_end);
>
> +               /*
> +                * The stripe extent starts before the range we want to delete
> +                * and ends after the range we want to delete, i.e. we're
> +                * punching a hole in the stripe extent:
> +                *
> +                *  |--- RAID Stripe Extent ---|
> +                *  | keep |--- drop ---| keep |
> +                *
> +                * This means we need to a) truncate the existing item and b)
> +                * create a second item for the remaining range.
> +                */
> +               if (found_start < start && found_end > end) {
> +                       size_t item_size;
> +                       u64 diff_start = start - found_start;
> +                       u64 diff_end = found_end - end;
> +                       struct btrfs_stripe_extent *extent;
> +                       struct btrfs_key newkey = {
> +                               .objectid = end,
> +                               .type = BTRFS_RAID_STRIPE_KEY,
> +                               .offset = diff_end,
> +                       };
> +
> +                       /* "right" item */
> +                       ret = btrfs_duplicate_item(trans, stripe_root, path,
> +                                                  &newkey);
> +                       if (ret)
> +                               break;
> +
> +                       item_size = btrfs_item_size(leaf, path->slots[0]);
> +                       extent = btrfs_item_ptr(leaf, path->slots[0],
> +                                               struct btrfs_stripe_extent);
> +
> +                       for (int i = 0; i < btrfs_num_raid_stripes(item_size); i++) {
> +                               struct btrfs_raid_stride *stride = &extent->strides[i];
> +                               u64 phys;
> +
> +                               phys = btrfs_raid_stride_physical(leaf, stride);
> +                               phys += diff_start + length;
> +                               btrfs_set_raid_stride_physical(leaf, stride, phys);
> +                       }
> +
> +                       /* "left" item */
> +                       path->slots[0]--;
> +                       btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
> +                       btrfs_partially_delete_raid_extent(trans, path, &key,
> +                                                          diff_start, 0);
> +                       break;
> +               }
> +
>                 /*
>                  * The stripe extent starts before the range we want to delete:
>                  *
>
> --
> 2.43.0
>
>

Re: [PATCH v2 08/14] btrfs: don't use btrfs_set_item_key_safe on RAID stripe-extents

[Filipe Manana]

On Tue, Jan 7, 2025 at 12:59 PM Johannes Thumshirn <jth@kernel.org> wrote:
>
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
> Don't use btrfs_set_item_key_safe() to modify the keys in the RAID
> stripe-tree as this can lead to corruption of the tree, which is caught by
> the checks in btrfs_set_item_key_safe():
>
>  BTRFS info (device nvme1n1): leaf 49168384 gen 15 total ptrs 194 free space 8329 owner 12
>  BTRFS info (device nvme1n1): refs 2 lock_owner 1030 current 1030
>   [ snip ]
>   item 105 key (354549760 230 20480) itemoff 14587 itemsize 16
>                   stride 0 devid 5 physical 67502080
>   item 106 key (354631680 230 4096) itemoff 14571 itemsize 16
>                   stride 0 devid 1 physical 88559616
>   item 107 key (354631680 230 32768) itemoff 14555 itemsize 16
>                   stride 0 devid 1 physical 88555520
>   item 108 key (354717696 230 28672) itemoff 14539 itemsize 16
>                   stride 0 devid 2 physical 67604480
>   [ snip ]
>  BTRFS critical (device nvme1n1): slot 106 key (354631680 230 32768) new key (354635776 230 4096)
>  ------------[ cut here ]------------
>  kernel BUG at fs/btrfs/ctree.c:2602!
>  Oops: invalid opcode: 0000 [#1] PREEMPT SMP PTI
>  CPU: 1 UID: 0 PID: 1055 Comm: fsstress Not tainted 6.13.0-rc1+ #1464
>  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-3-gd478f380-rebuilt.opensuse.org 04/01/2014
>  RIP: 0010:btrfs_set_item_key_safe+0xf7/0x270
>  Code: <snip>
>  RSP: 0018:ffffc90001337ab0 EFLAGS: 00010287
>  RAX: 0000000000000000 RBX: ffff8881115fd000 RCX: 0000000000000000
>  RDX: 0000000000000001 RSI: 0000000000000001 RDI: 00000000ffffffff
>  RBP: ffff888110ed6f50 R08: 00000000ffffefff R09: ffffffff8244c500
>  R10: 00000000ffffefff R11: 00000000ffffffff R12: ffff888100586000
>  R13: 00000000000000c9 R14: ffffc90001337b1f R15: ffff888110f23b58
>  FS:  00007f7d75c72740(0000) GS:ffff88813bd00000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 00007fa811652c60 CR3: 0000000111398001 CR4: 0000000000370eb0
>  Call Trace:
>   <TASK>
>   ? __die_body.cold+0x14/0x1a
>   ? die+0x2e/0x50
>   ? do_trap+0xca/0x110
>   ? do_error_trap+0x65/0x80
>   ? btrfs_set_item_key_safe+0xf7/0x270
>   ? exc_invalid_op+0x50/0x70
>   ? btrfs_set_item_key_safe+0xf7/0x270
>   ? asm_exc_invalid_op+0x1a/0x20
>   ? btrfs_set_item_key_safe+0xf7/0x270
>   btrfs_partially_delete_raid_extent+0xc4/0xe0
>   btrfs_delete_raid_extent+0x227/0x240
>   __btrfs_free_extent.isra.0+0x57f/0x9c0
>   ? exc_coproc_segment_overrun+0x40/0x40
>   __btrfs_run_delayed_refs+0x2fa/0xe80
>   btrfs_run_delayed_refs+0x81/0xe0
>   btrfs_commit_transaction+0x2dd/0xbe0
>   ? preempt_count_add+0x52/0xb0
>   btrfs_sync_file+0x375/0x4c0
>   do_fsync+0x39/0x70
>   __x64_sys_fsync+0x13/0x20
>   do_syscall_64+0x54/0x110
>   entry_SYSCALL_64_after_hwframe+0x76/0x7e
>  RIP: 0033:0x7f7d7550ef90
>  Code: <snip>
>  RSP: 002b:00007ffd70237248 EFLAGS: 00000202 ORIG_RAX: 000000000000004a
>  RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007f7d7550ef90
>  RDX: 000000000000013a RSI: 000000000040eb28 RDI: 0000000000000004
>  RBP: 000000000000001b R08: 0000000000000078 R09: 00007ffd7023725c
>  R10: 00007f7d75400390 R11: 0000000000000202 R12: 028f5c28f5c28f5c
>  R13: 8f5c28f5c28f5c29 R14: 000000000040b520 R15: 00007f7d75c726c8
>   </TASK>
>
> Instead copy the item, adjust the key and per-device physical addresses
> and re-insert it into the tree.

So my comments are basically the same as in the previous version.
Why do we hit this situation, what's the bug in the algorithm that
makes us try to set a key that breaks the key ordering in the leaf?

Did this happen even with all previous fixes applied?

Looking at this change log I'm reading it as "not sure what causes the
bug, so switching to a delete + insert as that always results in a
correct key order".

Thanks.


>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  fs/btrfs/raid-stripe-tree.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
> index d15df49c61a86a4188b822b05453428e444920b5..a4225ad043216e5d7035a71eab6bcc49b242836f 100644
> --- a/fs/btrfs/raid-stripe-tree.c
> +++ b/fs/btrfs/raid-stripe-tree.c
> @@ -13,12 +13,13 @@
>  #include "volumes.h"
>  #include "print-tree.h"
>
> -static void btrfs_partially_delete_raid_extent(struct btrfs_trans_handle *trans,
> +static int btrfs_partially_delete_raid_extent(struct btrfs_trans_handle *trans,
>                                                struct btrfs_path *path,
>                                                const struct btrfs_key *oldkey,
>                                                u64 newlen, u64 frontpad)
>  {
> -       struct btrfs_stripe_extent *extent;
> +       struct btrfs_root *stripe_root = trans->fs_info->stripe_root;
> +       struct btrfs_stripe_extent *extent, *new;
>         struct extent_buffer *leaf;
>         int slot;
>         size_t item_size;
> @@ -27,6 +28,7 @@ static void btrfs_partially_delete_raid_extent(struct btrfs_trans_handle *trans,
>                 .type = BTRFS_RAID_STRIPE_KEY,
>                 .offset = newlen,
>         };
> +       int ret;
>
>         ASSERT(newlen > 0);
>         ASSERT(oldkey->type == BTRFS_RAID_STRIPE_KEY);
> @@ -34,17 +36,31 @@ static void btrfs_partially_delete_raid_extent(struct btrfs_trans_handle *trans,
>         leaf = path->nodes[0];
>         slot = path->slots[0];
>         item_size = btrfs_item_size(leaf, slot);
> +
> +       new = kzalloc(item_size, GFP_NOFS);
> +       if (!new)
> +               return -ENOMEM;
> +
>         extent = btrfs_item_ptr(leaf, slot, struct btrfs_stripe_extent);
>
>         for (int i = 0; i < btrfs_num_raid_stripes(item_size); i++) {
>                 struct btrfs_raid_stride *stride = &extent->strides[i];
>                 u64 phys;
>
> -               phys = btrfs_raid_stride_physical(leaf, stride);
> -               btrfs_set_raid_stride_physical(leaf, stride, phys + frontpad);
> +               phys = btrfs_raid_stride_physical(leaf, stride) + frontpad;
> +               btrfs_set_stack_raid_stride_physical(&new->strides[i], phys);
>         }
>
> -       btrfs_set_item_key_safe(trans, path, &newkey);
> +       ret = btrfs_del_item(trans, stripe_root, path);
> +       if (ret)
> +               goto out;
> +
> +       btrfs_release_path(path);
> +       ret = btrfs_insert_item(trans, stripe_root, &newkey, new, item_size);
> +
> +out:
> +       kfree(new);
> +       return ret;
>  }
>
>  int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 length)
>
> --
> 2.43.0
>
>

Re: [PATCH v2 12/14] btrfs: selftests: add selftest for punching holes into the RAID stripe extents

[Filipe Manana]

On Tue, Jan 7, 2025 at 12:51 PM Johannes Thumshirn <jth@kernel.org> wrote:
>
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
> Add a selftest for punching a hole into a RAID stripe extent. The test
> create an 1M extent and punches a 64k bytes long hole at offset of 32k from
> the start of the extent.
>
> Afterwards it verifies the start and length of both resulting new extents
> "left" and "right" as well as the absence of the hole.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/tests/raid-stripe-tree-tests.c | 140 ++++++++++++++++++++++++++++++++
>  1 file changed, 140 insertions(+)
>
> diff --git a/fs/btrfs/tests/raid-stripe-tree-tests.c b/fs/btrfs/tests/raid-stripe-tree-tests.c
> index a815fc5c4dd32e9b10844ad6df34f418c2e88ce7..c7e44e944f5ecc37ffb937237cb81fefbafbaf9a 100644
> --- a/fs/btrfs/tests/raid-stripe-tree-tests.c
> +++ b/fs/btrfs/tests/raid-stripe-tree-tests.c
> @@ -31,6 +31,145 @@ static struct btrfs_device *btrfs_device_by_devid(struct btrfs_fs_devices *fs_de
>         return NULL;
>  }
>
> +/* Test punching a hole into a single RAID stripe-extent. */
> +static int test_punch_hole(struct btrfs_trans_handle *trans)
> +{
> +       struct btrfs_fs_info *fs_info = trans->fs_info;
> +       struct btrfs_io_context *bioc;
> +       struct btrfs_io_stripe io_stripe = { 0 };
> +       u64 map_type = RST_TEST_RAID1_TYPE;
> +       u64 logical1 = SZ_1M;
> +       u64 hole_start = logical1 + SZ_32K;
> +       u64 hole_len = SZ_64K;
> +       u64 logical2 = hole_start + hole_len;
> +       u64 len = SZ_1M;
> +       u64 len1 = SZ_32K;
> +       u64 len2 = len - len1 - hole_len;
> +       int ret;
> +
> +       bioc = alloc_btrfs_io_context(fs_info, logical1, RST_TEST_NUM_DEVICES);
> +       if (!bioc) {
> +               test_std_err(TEST_ALLOC_IO_CONTEXT);
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +
> +       io_stripe.dev = btrfs_device_by_devid(fs_info->fs_devices, 0);
> +       bioc->map_type = map_type;
> +       bioc->size = len;
> +
> +       for (int i = 0; i < RST_TEST_NUM_DEVICES; i++) {
> +               struct btrfs_io_stripe *stripe = &bioc->stripes[i];
> +
> +               stripe->dev = btrfs_device_by_devid(fs_info->fs_devices, i);
> +               if (!stripe->dev) {
> +                       test_err("cannot find device with devid %d", i);
> +                       ret = -EINVAL;
> +                       goto out;
> +               }
> +
> +               stripe->physical = logical1 + i * SZ_1G;
> +       }
> +
> +       ret = btrfs_insert_one_raid_extent(trans, bioc);
> +       if (ret) {
> +               test_err("inserting RAID extent failed: %d", ret);
> +               goto out;
> +       }
> +
> +       ret = btrfs_get_raid_extent_offset(fs_info, logical1, &len, map_type, 0,
> +                                          &io_stripe);
> +       if (ret) {
> +               test_err("lookup of RAID extent [%llu, %llu] failed", logical1,
> +                        logical1 + len);
> +               goto out;
> +       }
> +
> +       if (io_stripe.physical != logical1) {
> +               test_err("invalid physical address, expected %llu got %llu",
> +                        logical1, io_stripe.physical);
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       if (len != SZ_1M) {
> +               test_err("invalid stripe length, expected %llu got %llu",
> +                        (u64)SZ_1M, len);
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       ret = btrfs_delete_raid_extent(trans, hole_start, hole_len);
> +       if (ret) {
> +               test_err("deleting RAID extent [%llu, %llu] failed",
> +                        hole_start, hole_start + hole_len);
> +               goto out;
> +       }
> +
> +       ret = btrfs_get_raid_extent_offset(fs_info, logical1, &len1, map_type,
> +                                          0, &io_stripe);
> +       if (ret) {
> +               test_err("lookup of RAID extent [%llu, %llu] failed",
> +                        logical1, logical1 + len1);
> +               goto out;
> +       }
> +
> +       if (io_stripe.physical != logical1) {
> +               test_err("invalid physical address, expected %llu, got %llu",
> +                        logical1, io_stripe.physical);
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       if (len1 != SZ_32K) {
> +               test_err("invalid stripe length, expected %llu, got %llu",
> +                        (u64)SZ_32K, len1);
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       ret = btrfs_get_raid_extent_offset(fs_info, logical2, &len2, map_type,
> +                                          0, &io_stripe);
> +       if (ret) {
> +               test_err("lookup of RAID extent [%llu, %llu] failed", logical2,
> +                        logical2 + len2);
> +               goto out;
> +       }
> +
> +       if (io_stripe.physical != logical2) {
> +               test_err("invalid physical address, expected %llu, got %llu",
> +                        logical2, io_stripe.physical);
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       if (len2 != len - len1 - hole_len) {
> +               test_err("invalid length, expected %llu, got %llu",
> +                        len - len1 - hole_len, len2);
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       /* Check for the absence of the hole. */
> +       ret = btrfs_get_raid_extent_offset(fs_info, hole_start, &hole_len,
> +                                          map_type, 0, &io_stripe);
> +       if (ret != -ENODATA) {
> +               ret = -EINVAL;
> +               test_err("lookup of RAID extent [%llu, %llu] succeeded, should fail",
> +                        hole_start, hole_start + SZ_64K);
> +               goto out;
> +       }
> +
> +       ret = btrfs_delete_raid_extent(trans, logical1, len1);
> +       if (ret)
> +               goto out;
> +
> +       ret = btrfs_delete_raid_extent(trans, logical2, len2);
> +out:
> +       btrfs_put_bioc(bioc);
> +       return ret;
> +}
> +
>  /*
>   * Test a 1M RST write that spans two adjecent RST items on disk and then
>   * delete a portion starting in the first item and spanning into the second
> @@ -612,6 +751,7 @@ static const test_func_t tests[] = {
>         test_tail_delete,
>         test_front_delete,
>         test_front_delete_prev_item,
> +       test_punch_hole,
>  };
>
>  static int run_test(test_func_t test, u32 sectorsize, u32 nodesize)
>
> --
> 2.43.0
>
>

Re: [PATCH v2 08/14] btrfs: don't use btrfs_set_item_key_safe on RAID stripe-extents

[Johannes Thumshirn]

On 09.01.25 16:45, Filipe Manana wrote:
> On Tue, Jan 7, 2025 at 12:59 PM Johannes Thumshirn <jth@kernel.org> wrote:
>>
>> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>
>> Don't use btrfs_set_item_key_safe() to modify the keys in the RAID
>> stripe-tree as this can lead to corruption of the tree, which is caught by
>> the checks in btrfs_set_item_key_safe():
>>
>>   BTRFS info (device nvme1n1): leaf 49168384 gen 15 total ptrs 194 free space 8329 owner 12
>>   BTRFS info (device nvme1n1): refs 2 lock_owner 1030 current 1030
>>    [ snip ]
>>    item 105 key (354549760 230 20480) itemoff 14587 itemsize 16
>>                    stride 0 devid 5 physical 67502080
>>    item 106 key (354631680 230 4096) itemoff 14571 itemsize 16
>>                    stride 0 devid 1 physical 88559616
>>    item 107 key (354631680 230 32768) itemoff 14555 itemsize 16
>>                    stride 0 devid 1 physical 88555520
>>    item 108 key (354717696 230 28672) itemoff 14539 itemsize 16
>>                    stride 0 devid 2 physical 67604480
>>    [ snip ]
>>   BTRFS critical (device nvme1n1): slot 106 key (354631680 230 32768) new key (354635776 230 4096)
>>   ------------[ cut here ]------------
>>   kernel BUG at fs/btrfs/ctree.c:2602!
>>   Oops: invalid opcode: 0000 [#1] PREEMPT SMP PTI
>>   CPU: 1 UID: 0 PID: 1055 Comm: fsstress Not tainted 6.13.0-rc1+ #1464
>>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-3-gd478f380-rebuilt.opensuse.org 04/01/2014
>>   RIP: 0010:btrfs_set_item_key_safe+0xf7/0x270
>>   Code: <snip>
>>   RSP: 0018:ffffc90001337ab0 EFLAGS: 00010287
>>   RAX: 0000000000000000 RBX: ffff8881115fd000 RCX: 0000000000000000
>>   RDX: 0000000000000001 RSI: 0000000000000001 RDI: 00000000ffffffff
>>   RBP: ffff888110ed6f50 R08: 00000000ffffefff R09: ffffffff8244c500
>>   R10: 00000000ffffefff R11: 00000000ffffffff R12: ffff888100586000
>>   R13: 00000000000000c9 R14: ffffc90001337b1f R15: ffff888110f23b58
>>   FS:  00007f7d75c72740(0000) GS:ffff88813bd00000(0000) knlGS:0000000000000000
>>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>   CR2: 00007fa811652c60 CR3: 0000000111398001 CR4: 0000000000370eb0
>>   Call Trace:
>>    <TASK>
>>    ? __die_body.cold+0x14/0x1a
>>    ? die+0x2e/0x50
>>    ? do_trap+0xca/0x110
>>    ? do_error_trap+0x65/0x80
>>    ? btrfs_set_item_key_safe+0xf7/0x270
>>    ? exc_invalid_op+0x50/0x70
>>    ? btrfs_set_item_key_safe+0xf7/0x270
>>    ? asm_exc_invalid_op+0x1a/0x20
>>    ? btrfs_set_item_key_safe+0xf7/0x270
>>    btrfs_partially_delete_raid_extent+0xc4/0xe0
>>    btrfs_delete_raid_extent+0x227/0x240
>>    __btrfs_free_extent.isra.0+0x57f/0x9c0
>>    ? exc_coproc_segment_overrun+0x40/0x40
>>    __btrfs_run_delayed_refs+0x2fa/0xe80
>>    btrfs_run_delayed_refs+0x81/0xe0
>>    btrfs_commit_transaction+0x2dd/0xbe0
>>    ? preempt_count_add+0x52/0xb0
>>    btrfs_sync_file+0x375/0x4c0
>>    do_fsync+0x39/0x70
>>    __x64_sys_fsync+0x13/0x20
>>    do_syscall_64+0x54/0x110
>>    entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>   RIP: 0033:0x7f7d7550ef90
>>   Code: <snip>
>>   RSP: 002b:00007ffd70237248 EFLAGS: 00000202 ORIG_RAX: 000000000000004a
>>   RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007f7d7550ef90
>>   RDX: 000000000000013a RSI: 000000000040eb28 RDI: 0000000000000004
>>   RBP: 000000000000001b R08: 0000000000000078 R09: 00007ffd7023725c
>>   R10: 00007f7d75400390 R11: 0000000000000202 R12: 028f5c28f5c28f5c
>>   R13: 8f5c28f5c28f5c29 R14: 000000000040b520 R15: 00007f7d75c726c8
>>    </TASK>
>>
>> Instead copy the item, adjust the key and per-device physical addresses
>> and re-insert it into the tree.
> 
> So my comments are basically the same as in the previous version.
> Why do we hit this situation, what's the bug in the algorithm that
> makes us try to set a key that breaks the key ordering in the leaf?
> 
> Did this happen even with all previous fixes applied?

Yes.

> Looking at this change log I'm reading it as "not sure what causes the
> bug, so switching to a delete + insert as that always results in a
> correct key order".

Correct, also looking at btrfs_drop_extents() it only uses 
btrfs_set_item_key_safe() when truncating an extent, AFAIU.
Otherwise btrfs_drop_extents() uses btrfs_duplicate_item() as well.

Re: [PATCH v2 01/14] btrfs: don't try to delete RAID stripe-extents if we don't need to

[Johannes Thumshirn]

On 09.01.25 16:27, Johannes Thumshirn wrote:
> On 09.01.25 16:15, Filipe Manana wrote:
>> On Thu, Jan 9, 2025 at 2:39 PM Johannes Thumshirn
>> <Johannes.Thumshirn@wdc.com> wrote:
>>>
>>> On 09.01.25 13:35, Filipe Manana wrote:
>>>>> diff --git a/fs/btrfs/tests/raid-stripe-tree-tests.c b/fs/btrfs/tests/raid-stripe-tree-tests.c
>>>>> index 30f17eb7b6a8a1dfa9f66ed5508da42a70db1fa3..f060c04c7f76357e6d2c6ba78a8ba981e35645bd 100644
>>>>> --- a/fs/btrfs/tests/raid-stripe-tree-tests.c
>>>>> +++ b/fs/btrfs/tests/raid-stripe-tree-tests.c
>>>>> @@ -478,8 +478,9 @@ static int run_test(test_func_t test, u32 sectorsize, u32 nodesize)
>>>>>                    ret = PTR_ERR(root);
>>>>>                    goto out;
>>>>>            }
>>>>> -       btrfs_set_super_compat_ro_flags(root->fs_info->super_copy,
>>>>> +       btrfs_set_super_incompat_flags(root->fs_info->super_copy,
>>>>>                                            BTRFS_FEATURE_INCOMPAT_RAID_STRIPE_TREE);
>>>> This hunk seems unrelated to the rest of the patch, could be fixed in
>>>> a different patch in case it actually solves any problem (probably
>>>> not, but it's an incompat feature so it should be changed anyway).
>>>
>>> I'll make it a separate patch. RST is an incompat feature not a compat one.
>>>
>>> With this patch btrfs_delete_raid_extent() starts checking the incompat
>>> bit so it is fixing a 'problem'.
>>
>> Yes, but for that all that's needed is this call:
>>
>> btrfs_set_fs_incompat(root->fs_info, RAID_STRIPE_TREE);
>>
>> Right?
>>
>> Replacing the btrfs_set_super_compat_ro_flags() call with a call to
>> btrfs_set_super_incompat_flags() shouldn't be needed for this patch.
>> That's what I was referring to.
>>
> 
> Ah now I see the problem. I used btrfs_set_super_incompat_flags()
> instead of btrfs_set_fs_incompat() *facepalm*
> 

But when using btrfs_set_fs_incompat() we get the annoying btrfs_info() 
call about setting the flag. Which in case of a selftest is pointless.

Also btrfs_set_fs_incompat() calls btrfs_set_super_incompat_flags() 
internally, so I think using btrfs_set_super_incompat_flags() here is 
the way to go.

Re: [PATCH v2 06/14] btrfs: fix deletion of a range spanning parts two RAID stripe extents

[Johannes Thumshirn]

On 09.01.25 16:24, Filipe Manana wrote:
> On Tue, Jan 7, 2025 at 12:50 PM Johannes Thumshirn <jth@kernel.org> wrote:
>>
>> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>
>> When a user requests the deletion of a range that spans multiple stripe
>> extents and btrfs_search_slot() returns us the second RAID stripe extent,
>> we need to pick the previous item and truncate it, if there's still a
>> range to delete left, move on to the next item.
>>
>> The following diagram illustrates the operation:
>>
>>   |--- RAID Stripe Extent ---||--- RAID Stripe Extent ---|
>>          |--- keep  ---|--- drop ---|
>>
>> While at it, comment the trivial case of a whole item delete as well.
>>
>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>> ---
>>   fs/btrfs/raid-stripe-tree.c | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
>> index 79f8f692aaa8f6df2c9482fbd7777c2812528f65..893d963951315abfc734e1ca232b3087b7889431 100644
>> --- a/fs/btrfs/raid-stripe-tree.c
>> +++ b/fs/btrfs/raid-stripe-tree.c
>> @@ -103,6 +103,31 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
>>                  found_end = found_start + key.offset;
>>                  ret = 0;
>>
>> +               /*
>> +                * The stripe extent starts before the range we want to delete,
>> +                * but the range spans more than one stripe extent:
>> +                *
>> +                * |--- RAID Stripe Extent ---||--- RAID Stripe Extent ---|
>> +                *        |--- keep  ---|--- drop ---|
>> +                *
>> +                * This means we have to get the previous item, truncate its
>> +                * length and then restart the search.
>> +                */
>> +               if (found_start > start) {
>> +
>> +                       ret = btrfs_previous_item(stripe_root, path, start,
>> +                                                 BTRFS_RAID_STRIPE_KEY);
>> +                       if (ret < 0)
>> +                               break;
>> +                       ret = 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;
> 
> Hum, this isn't safe, ignoring the case where btrfs_previous_item()
> returns 1, meaning there's no previous item.
> 
> In that case previous_item() returns pointing to the same leaf and
> slot, and then below we delete the item instead of trimming it
> (increasing its range start and decreasing its length).

Good catch!

But what should we do when we end up in this situation? Doesn't that 
mean that either do_free_extent_accounting() passed in a bogus range or 
btrfs_previous_item() should've done one more call to btrfs_pref_leaf()?

Re: [PATCH v2 06/14] btrfs: fix deletion of a range spanning parts two RAID stripe extents

[Filipe Manana]

On Fri, Jan 10, 2025 at 11:33 AM Johannes Thumshirn
<Johannes.Thumshirn@wdc.com> wrote:
>
> On 09.01.25 16:24, Filipe Manana wrote:
> > On Tue, Jan 7, 2025 at 12:50 PM Johannes Thumshirn <jth@kernel.org> wrote:
> >>
> >> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> >>
> >> When a user requests the deletion of a range that spans multiple stripe
> >> extents and btrfs_search_slot() returns us the second RAID stripe extent,
> >> we need to pick the previous item and truncate it, if there's still a
> >> range to delete left, move on to the next item.
> >>
> >> The following diagram illustrates the operation:
> >>
> >>   |--- RAID Stripe Extent ---||--- RAID Stripe Extent ---|
> >>          |--- keep  ---|--- drop ---|
> >>
> >> While at it, comment the trivial case of a whole item delete as well.
> >>
> >> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> >> ---
> >>   fs/btrfs/raid-stripe-tree.c | 28 ++++++++++++++++++++++++++++
> >>   1 file changed, 28 insertions(+)
> >>
> >> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
> >> index 79f8f692aaa8f6df2c9482fbd7777c2812528f65..893d963951315abfc734e1ca232b3087b7889431 100644
> >> --- a/fs/btrfs/raid-stripe-tree.c
> >> +++ b/fs/btrfs/raid-stripe-tree.c
> >> @@ -103,6 +103,31 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
> >>                  found_end = found_start + key.offset;
> >>                  ret = 0;
> >>
> >> +               /*
> >> +                * The stripe extent starts before the range we want to delete,
> >> +                * but the range spans more than one stripe extent:
> >> +                *
> >> +                * |--- RAID Stripe Extent ---||--- RAID Stripe Extent ---|
> >> +                *        |--- keep  ---|--- drop ---|
> >> +                *
> >> +                * This means we have to get the previous item, truncate its
> >> +                * length and then restart the search.
> >> +                */
> >> +               if (found_start > start) {
> >> +
> >> +                       ret = btrfs_previous_item(stripe_root, path, start,
> >> +                                                 BTRFS_RAID_STRIPE_KEY);
> >> +                       if (ret < 0)
> >> +                               break;
> >> +                       ret = 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;
> >
> > Hum, this isn't safe, ignoring the case where btrfs_previous_item()
> > returns 1, meaning there's no previous item.
> >
> > In that case previous_item() returns pointing to the same leaf and
> > slot, and then below we delete the item instead of trimming it
> > (increasing its range start and decreasing its length).
>
> Good catch!
>
> But what should we do when we end up in this situation? Doesn't that
> mean that either do_free_extent_accounting() passed in a bogus range or
> btrfs_previous_item() should've done one more call to btrfs_pref_leaf()?

When it returns 1 it means there's no previous leaf - the item we
processed was the first in the tree, so any future calls to
btrfs_prev_leaf() will keep returning 1 (unless some other task
inserts smaller keys).
The right thing is probably to log an error telling the target range,
dump the leaf, abort the transaction and return an error.



>