[PATCH 0/2] btrfs: more RAID stripe tree updates

[PATCH 0/2] btrfs: more RAID stripe tree updates

[Johannes Thumshirn]

Two further RST updates targeted for 6.11 (hopefully).

The first one is a reworked version of the scrub vs dev-replace deadlock
fix. It does have reviews from Josef and Qu but I'd love to head Filipe's
take on it.

The second one updates a stripe extent in case a write to a already
present logical address happens.

---
Johannes Thumshirn (2):
      btrfs: don't hold dev_replace rwsem over whole of btrfs_map_block
      btrfs: replace stripe extents

 fs/btrfs/raid-stripe-tree.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/volumes.c          | 28 +++++++++++++++----------
 2 files changed, 68 insertions(+), 11 deletions(-)
---
base-commit: 584df860cac6e35e364ada101ccd13495b954644
change-id: 20240709-b4-rst-updates-bb9c0e49cd5b

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

[PATCH 1/2] btrfs: don't hold dev_replace rwsem over whole of btrfs_map_block

[Johannes Thumshirn]

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

Don't hold the dev_replace rwsem for the entirety of btrfs_map_block().

It is only needed to protect
a) calls to find_live_mirror() and
b) calling into handle_ops_on_dev_replace().

But there is no need to hold the rwsem for any kind of set_io_stripe()
calls.

So relax taking the dev_replace rwsem to only protect both cases and check
if the device replace status has changed in the meantime, for which we have
to re-do the find_live_mirror() calls.

This fixes a deadlock on raid-stripe-tree where device replace performs a
scrub operation, which in turn calls into btrfs_map_block() to find the
physical location of the block.

Cc: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/volumes.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index fcedc43ef291..4209419244a1 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6650,14 +6650,9 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 	max_len = btrfs_max_io_len(map, map_offset, &io_geom);
 	*length = min_t(u64, map->chunk_len - map_offset, max_len);
 
+again:
 	down_read(&dev_replace->rwsem);
 	dev_replace_is_ongoing = btrfs_dev_replace_is_ongoing(dev_replace);
-	/*
-	 * Hold the semaphore for read during the whole operation, write is
-	 * requested at commit time but must wait.
-	 */
-	if (!dev_replace_is_ongoing)
-		up_read(&dev_replace->rwsem);
 
 	switch (map->type & BTRFS_BLOCK_GROUP_PROFILE_MASK) {
 	case BTRFS_BLOCK_GROUP_RAID0:
@@ -6695,6 +6690,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 			   "stripe index math went horribly wrong, got stripe_index=%u, num_stripes=%u",
 			   io_geom.stripe_index, map->num_stripes);
 		ret = -EINVAL;
+		up_read(&dev_replace->rwsem);
 		goto out;
 	}
 
@@ -6710,6 +6706,8 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 		 */
 		num_alloc_stripes += 2;
 
+	up_read(&dev_replace->rwsem);
+
 	/*
 	 * If this I/O maps to a single device, try to return the device and
 	 * physical block information on the stack instead of allocating an
@@ -6782,6 +6780,18 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 		goto out;
 	}
 
+	/*
+	 * Check if something changed the dev_replace state since
+	 * we've checked it for the last time and if redo the whole
+	 * mapping operation.
+	 */
+	down_read(&dev_replace->rwsem);
+	if (dev_replace_is_ongoing !=
+	    btrfs_dev_replace_is_ongoing(dev_replace)) {
+		up_read(&dev_replace->rwsem);
+		goto again;
+	}
+
 	if (op != BTRFS_MAP_READ)
 		io_geom.max_errors = btrfs_chunk_max_errors(map);
 
@@ -6789,6 +6799,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 	    op != BTRFS_MAP_READ) {
 		handle_ops_on_dev_replace(bioc, dev_replace, logical, &io_geom);
 	}
+	up_read(&dev_replace->rwsem);
 
 	*bioc_ret = bioc;
 	bioc->num_stripes = io_geom.num_stripes;
@@ -6796,11 +6807,6 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 	bioc->mirror_num = io_geom.mirror_num;
 
 out:
-	if (dev_replace_is_ongoing) {
-		lockdep_assert_held(&dev_replace->rwsem);
-		/* Unlock and let waiting writers proceed */
-		up_read(&dev_replace->rwsem);
-	}
 	btrfs_free_chunk_map(map);
 	return ret;
 }

-- 
2.43.0

[PATCH 2/2] btrfs: replace stripe extents

[Johannes Thumshirn]

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

Update stripe extents in case a write to an already existing address
incoming.

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

diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
index e6f7a234b8f6..fd56535b2289 100644
--- a/fs/btrfs/raid-stripe-tree.c
+++ b/fs/btrfs/raid-stripe-tree.c
@@ -73,6 +73,55 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
 	return ret;
 }
 
+static int update_raid_extent_item(struct btrfs_trans_handle *trans,
+				   struct btrfs_key *key,
+				   struct btrfs_io_context *bioc)
+{
+	struct btrfs_path *path;
+	struct extent_buffer *leaf;
+	struct btrfs_stripe_extent *stripe_extent;
+	int num_stripes;
+	int ret;
+	int slot;
+
+	path = btrfs_alloc_path();
+	if (!path)
+		return -ENOMEM;
+
+	ret = btrfs_search_slot(trans, trans->fs_info->stripe_root, key, path,
+				0, 1);
+	if (ret)
+		return ret == 1 ? ret : -EINVAL;
+
+	leaf = path->nodes[0];
+	slot = path->slots[0];
+
+	btrfs_item_key_to_cpu(leaf, key, slot);
+	num_stripes = btrfs_num_raid_stripes(btrfs_item_size(leaf, slot));
+	stripe_extent = btrfs_item_ptr(leaf, slot, struct btrfs_stripe_extent);
+
+	ASSERT(key->offset == bioc->size);
+
+	for (int i = 0; i < num_stripes; i++) {
+		u64 devid = bioc->stripes[i].dev->devid;
+		u64 physical = bioc->stripes[i].physical;
+		u64 length = bioc->stripes[i].length;
+		struct btrfs_raid_stride *raid_stride =
+			&stripe_extent->strides[i];
+
+		if (length == 0)
+			length = bioc->size;
+
+		btrfs_set_raid_stride_devid(leaf, raid_stride, devid);
+		btrfs_set_raid_stride_physical(leaf, raid_stride, physical);
+	}
+
+	btrfs_mark_buffer_dirty(trans, leaf);
+	btrfs_free_path(path);
+
+	return ret;
+}
+
 static int btrfs_insert_one_raid_extent(struct btrfs_trans_handle *trans,
 					struct btrfs_io_context *bioc)
 {
@@ -112,6 +161,8 @@ static int btrfs_insert_one_raid_extent(struct btrfs_trans_handle *trans,
 
 	ret = btrfs_insert_item(trans, stripe_root, &stripe_key, stripe_extent,
 				item_size);
+	if (ret == -EEXIST)
+		ret = update_raid_extent_item(trans, &stripe_key, bioc);
 	if (ret)
 		btrfs_abort_transaction(trans, ret);
 

-- 
2.43.0

Re: [PATCH 2/2] btrfs: replace stripe extents

[Qu Wenruo]

在 2024/7/9 16:02, Johannes Thumshirn 写道:
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
> Update stripe extents in case a write to an already existing address
> incoming.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Looks good to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

But still as I mentioned in the original thread, I'm wondering why
dev-replace of RST needs to update RST entry.

I'd prefer to do a dev-extent level copy so that no RST/chunk needs to
be updated, just like what we did for non-RST cases.

But so far the change should be good enough for us to continue the testing.

Thanks,
Qu

> ---
>   fs/btrfs/raid-stripe-tree.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 51 insertions(+)
>
> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
> index e6f7a234b8f6..fd56535b2289 100644
> --- a/fs/btrfs/raid-stripe-tree.c
> +++ b/fs/btrfs/raid-stripe-tree.c
> @@ -73,6 +73,55 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
>   	return ret;
>   }
>
> +static int update_raid_extent_item(struct btrfs_trans_handle *trans,
> +				   struct btrfs_key *key,
> +				   struct btrfs_io_context *bioc)
> +{
> +	struct btrfs_path *path;
> +	struct extent_buffer *leaf;
> +	struct btrfs_stripe_extent *stripe_extent;
> +	int num_stripes;
> +	int ret;
> +	int slot;
> +
> +	path = btrfs_alloc_path();
> +	if (!path)
> +		return -ENOMEM;
> +
> +	ret = btrfs_search_slot(trans, trans->fs_info->stripe_root, key, path,
> +				0, 1);
> +	if (ret)
> +		return ret == 1 ? ret : -EINVAL;
> +
> +	leaf = path->nodes[0];
> +	slot = path->slots[0];
> +
> +	btrfs_item_key_to_cpu(leaf, key, slot);
> +	num_stripes = btrfs_num_raid_stripes(btrfs_item_size(leaf, slot));
> +	stripe_extent = btrfs_item_ptr(leaf, slot, struct btrfs_stripe_extent);
> +
> +	ASSERT(key->offset == bioc->size);
> +
> +	for (int i = 0; i < num_stripes; i++) {
> +		u64 devid = bioc->stripes[i].dev->devid;
> +		u64 physical = bioc->stripes[i].physical;
> +		u64 length = bioc->stripes[i].length;
> +		struct btrfs_raid_stride *raid_stride =
> +			&stripe_extent->strides[i];
> +
> +		if (length == 0)
> +			length = bioc->size;
> +
> +		btrfs_set_raid_stride_devid(leaf, raid_stride, devid);
> +		btrfs_set_raid_stride_physical(leaf, raid_stride, physical);
> +	}
> +
> +	btrfs_mark_buffer_dirty(trans, leaf);
> +	btrfs_free_path(path);
> +
> +	return ret;
> +}
> +
>   static int btrfs_insert_one_raid_extent(struct btrfs_trans_handle *trans,
>   					struct btrfs_io_context *bioc)
>   {
> @@ -112,6 +161,8 @@ static int btrfs_insert_one_raid_extent(struct btrfs_trans_handle *trans,
>
>   	ret = btrfs_insert_item(trans, stripe_root, &stripe_key, stripe_extent,
>   				item_size);
> +	if (ret == -EEXIST)
> +		ret = update_raid_extent_item(trans, &stripe_key, bioc);
>   	if (ret)
>   		btrfs_abort_transaction(trans, ret);
>
>

Re: [PATCH 2/2] btrfs: replace stripe extents

[Johannes Thumshirn]

On 09.07.24 09:18, Qu Wenruo wrote:
> 
> 
> 在 2024/7/9 16:02, Johannes Thumshirn 写道:
>> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>
>> Update stripe extents in case a write to an already existing address
>> incoming.
>>
>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> Looks good to me.
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> But still as I mentioned in the original thread, I'm wondering why
> dev-replace of RST needs to update RST entry.
> 
> I'd prefer to do a dev-extent level copy so that no RST/chunk needs to
> be updated, just like what we did for non-RST cases.
> 
> But so far the change should be good enough for us to continue the testing.

I /think/ I have a fix for the ASSERT() as well. It survived btrfs/060 
once already (which it hasn't before) and it's trivial and I feel stupid 
for it:

diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
index fd56535b2289..6b1c6004f94c 100644
--- a/fs/btrfs/raid-stripe-tree.c
+++ b/fs/btrfs/raid-stripe-tree.c
@@ -57,6 +57,9 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle 
*trans, u64 start, u64 le
                 /* That stripe ends before we start, we're done. */
                 if (found_end <= start)
                         break;
+               /* That stripe starts after we end, we're done as well */
+               if (found_start >= end)
+                       break;

                 trace_btrfs_raid_extent_delete(fs_info, start, end,
                                                found_start, found_end);

Re: [PATCH 2/2] btrfs: replace stripe extents

[Qu Wenruo]

在 2024/7/9 17:00, Johannes Thumshirn 写道:
> On 09.07.24 09:18, Qu Wenruo wrote:
>>
>>
>> 在 2024/7/9 16:02, Johannes Thumshirn 写道:
>>> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>>
>>> Update stripe extents in case a write to an already existing address
>>> incoming.
>>>
>>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>
>> Looks good to me.
>>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>
>> But still as I mentioned in the original thread, I'm wondering why
>> dev-replace of RST needs to update RST entry.
>>
>> I'd prefer to do a dev-extent level copy so that no RST/chunk needs to
>> be updated, just like what we did for non-RST cases.
>>
>> But so far the change should be good enough for us to continue the testing.
> 
> I /think/ I have a fix for the ASSERT() as well. It survived btrfs/060
> once already (which it hasn't before) and it's trivial and I feel stupid
> for it:

Wow, it's indeed a little embarrassing, but I'm still a little confused.

> 
> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
> index fd56535b2289..6b1c6004f94c 100644
> --- a/fs/btrfs/raid-stripe-tree.c
> +++ b/fs/btrfs/raid-stripe-tree.c
> @@ -57,6 +57,9 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle
> *trans, u64 start, u64 le
>                   /* That stripe ends before we start, we're done. */

Didn't all the btrfs_delete_raid_extent() callers expects to delete 
exact the range? Thus I though we should always hit 0 from 
btrfs_search_slot().

>                   if (found_end <= start)
>                           break;
> +               /* That stripe starts after we end, we're done as well */
> +               if (found_start >= end)
> +                       break;

Another thing is, just to be safer, you may want to do the RST entry 
search using key.offset = 0 or key.offset = -1, instead of an exact search.

The key.offset == 0 search example can be found in scrub_enumerate_chunk().
And the key.offset == -1 search example can be found in 
btrfs_free_dev_extent().

And do extra length check to ensure we always hit an exact match.

Thanks,
Qu

> 
>                   trace_btrfs_raid_extent_delete(fs_info, start, end,
>                                                  found_start, found_end);
> 

Re: [PATCH 2/2] btrfs: replace stripe extents

[Johannes Thumshirn]

On 09.07.24 09:46, Qu Wenruo wrote:
> 
> 
> 在 2024/7/9 17:00, Johannes Thumshirn 写道:
>> On 09.07.24 09:18, Qu Wenruo wrote:
>>>
>>>
>>> 在 2024/7/9 16:02, Johannes Thumshirn 写道:
>>>> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>>>
>>>> Update stripe extents in case a write to an already existing address
>>>> incoming.
>>>>
>>>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>>
>>> Looks good to me.
>>>
>>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>>
>>> But still as I mentioned in the original thread, I'm wondering why
>>> dev-replace of RST needs to update RST entry.
>>>
>>> I'd prefer to do a dev-extent level copy so that no RST/chunk needs to
>>> be updated, just like what we did for non-RST cases.
>>>
>>> But so far the change should be good enough for us to continue the testing.
>>
>> I /think/ I have a fix for the ASSERT() as well. It survived btrfs/060
>> once already (which it hasn't before) and it's trivial and I feel stupid
>> for it:
> 
> Wow, it's indeed a little embarrassing, but I'm still a little confused.
> 
>>
>> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
>> index fd56535b2289..6b1c6004f94c 100644
>> --- a/fs/btrfs/raid-stripe-tree.c
>> +++ b/fs/btrfs/raid-stripe-tree.c
>> @@ -57,6 +57,9 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle
>> *trans, u64 start, u64 le
>>                    /* That stripe ends before we start, we're done. */
> 
> Didn't all the btrfs_delete_raid_extent() callers expects to delete
> exact the range? Thus I though we should always hit 0 from
> btrfs_search_slot().
> 
>>                    if (found_end <= start)
>>                            break;
>> +               /* That stripe starts after we end, we're done as well */
>> +               if (found_start >= end)
>> +                       break;
> 
> Another thing is, just to be safer, you may want to do the RST entry
> search using key.offset = 0 or key.offset = -1, instead of an exact search.
> 
> The key.offset == 0 search example can be found in scrub_enumerate_chunk().
> And the key.offset == -1 search example can be found in
> btrfs_free_dev_extent().
> 
> And do extra length check to ensure we always hit an exact match.

Ah I didn't know about that one, thanks :).

Currently the above is running through CI and once it completes I'll 
give it a 2nd try with the key.offset = 0 or -1 variant.