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 |
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 >
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 > >
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'.
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.
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*
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 --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;