diff mbox series

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

Message ID 20250107-rst-delete-fixes-v2-1-0c7b14c0aac2@kernel.org (mailing list archive)
State New, archived
Headers show
Series btrfs: more RST delete fixes | expand

Commit Message

Johannes Thumshirn Jan. 7, 2025, 12:47 p.m. UTC
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(-)

Comments

David Sterba Jan. 9, 2025, 10:37 a.m. UTC | #1
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
>
Filipe Manana Jan. 9, 2025, 12:35 p.m. UTC | #2
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
>
>
Johannes Thumshirn Jan. 9, 2025, 2:39 p.m. UTC | #3
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'.
Filipe Manana Jan. 9, 2025, 3:14 p.m. UTC | #4
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.
Johannes Thumshirn Jan. 9, 2025, 3:27 p.m. UTC | #5
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*
Johannes Thumshirn Jan. 10, 2025, 11:29 a.m. UTC | #6
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.
diff mbox series

Patch

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;