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@xxxxxxx>
> > >
> > > Update stripe extents in case a write to an already existing address
> > > incoming.
> > >
> > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@xxxxxxx>
> >
> > Looks good to me.
> >
> > Reviewed-by: Qu Wenruo <wqu@xxxxxxxx>
> >
> > 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);