linux-kernel - Re: [PATCH 2/2] btrfs: replace stripe extents

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <97d6c743-5828-4183-a39c-f9c9004674b4@wdc.com>
Date: Tue, 9 Jul 2024 08:44:39 +0000
From: Johannes Thumshirn <Johannes.Thumshirn@....com>
To: Qu Wenruo <wqu@...e.com>, Qu Wenruo <quwenruo.btrfs@....com>, Johannes
 Thumshirn <jth@...nel.org>, Chris Mason <clm@...com>, Josef Bacik
	<josef@...icpanda.com>, David Sterba <dsterba@...e.com>
CC: "linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Filipe Manana
	<fdmanana@...e.com>
Subject: Re: [PATCH 2/2] btrfs: replace stripe extents

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