linux-kernel - Re: [PATCH v4 3/7] btrfs: split RAID stripes on deletion

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <e3927e86-d85e-4003-9ce5-e9e88741afa3@wdc.com>
Date: Mon, 8 Jul 2024 04:56:55 +0000
From: Johannes Thumshirn <Johannes.Thumshirn@....com>
To: 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>
Subject: Re: [PATCH v4 3/7] btrfs: split RAID stripes on deletion

On 06.07.24 01:26, Qu Wenruo wrote:
> 
> 
> 在 2024/7/6 00:43, Johannes Thumshirn 写道:
>> From: Johannes Thumshirn <johannes.thumshirn@....com>
>>
>> The current RAID stripe code assumes, that we will always remove a
>> whole stripe entry.
>>
>> But if we're only removing a part of a RAID stripe we're hitting the
>> ASSERT()ion checking for this condition.
>>
>> Instead of assuming the complete deletion of a RAID stripe, split the
>> stripe if we need to.
> 
> Sorry to be so critical, but if I understand correctly,
> btrfs_insert_one_raid_extent() does not do any merge of stripe extent.

No problem at all. I want to solve bugs, not increase my patch count ;).

> 
> Thus one stripe extent always means part of a data extent.
> 
> In that case a removal of a data extent should always remove all its
> stripe extents.
> 
> Furthermore due to the COW nature on zoned/rst devices, the space of a
> deleted data extent should not be re-allocated until a transaction
> commitment.
> 
> Thus I'm wonder if this split is masking some bigger problems.

Hmm now that you're saying it. The reason I wrote this path is, that I 
did hit the following ASSERT() in my testing:

>> -		ASSERT(found_start >= start && found_end <= end);

This indicates a partial delete of a stripe extent. But I agree as 
stripe extents are tied to extent items, this shouldn't really happen.

So maybe most of this series (apart from the deadlock fix) masks problems?

I'm back to the drawing board :(.