Re: [PATCH 0/5] btrfs: raid56: destructive RMW fix for RAID5 data profiles

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2022/11/15 21:24, David Sterba wrote:
> On Mon, Nov 14, 2022 at 08:26:29AM +0800, Qu Wenruo wrote:
>> [DESTRUCTIVE RMW]
>> Btrfs (and generic) RAID56 is always vulnerable against destructive RMW.
>>
>> Such destructive RMW can happen when one of the data stripe is
>> corrupted, then a sub-stripe write into other data stripes happens, like
>> the following:
>>
>>
>>    Disk 1: data 1 |0x000000000000| <- Corrupted
>>    Disk 2: data 2 |0x000000000000|
>>    Disk 2: parity |0xffffffffffff|
>>
>> In above case, if we write data into disk 2, we will got something like
>> this:
>>
>>    Disk 1: data 1 |0x000000000000| <- Corrupted
>>    Disk 2: data 2 |0x000000000000| <- New '0x00' writes
>>    Disk 2: parity |0x000000000000| <- New Parity.
>>
>> Since the new parity is calculated using the new writes and the
>> corrupted data, we lost the chance to recovery data stripe 1, and that
>> corruption will forever be there.
> ...
>> [TODO]
>> - Iterate all RAID6 combinations
>>    Currently we don't try all combinations of RAID6 during the repair.
>>    Thus for RAID6 we treat it just like RAID5 in RMW.
>>
>>    Currently the RAID6 recovery combination is only exhausted during
>>    recovery path, relying on the caller the increase the mirror number.
>>
>>    Can be implemented later for RMW path.
>>
>> - Write back the repaired sectors
>>    Currently we don't write back the repaired sectors, thus if we read
>>    the corrupted sectors, we rely on the recover path, and since the new
>>    parity is calculated using the recovered sectors, we can get the
>>    correct data without any problem.
>>
>>    But if we write back the repaired sectors during RMW, we can save the
>>    reader some time without going into recovery path at all.
>>
>>    This is just a performance optimization, thus I believe it can be
>>    implemented later.
> 
> Even with the todo and potential performance drop due to mandatory
> stripe caching I think this is worth merging at this point, so patches
> are in misc-next now.
> 
> Regarding the perf drop, I'll try to get some results besides functional
> testing but if anybody with a decent setup for raid5 can do some
> destructive tests it would be most welcome.
> 
> Fallback plan is to revert the last patch if it turns out to be too
> problematic.

If you're concerned about the fallback plan, I'd say we need to at least 
drop the last two patches.

Sure, dropping the last one will remove the verification and the extra 
reads, but unfortunately we will still fetch the csum for RMW cycles.
Although the csum fetch would at most cost 2 leaves, the cost is still 
there.

Thus if needed, I can do more refactoring of the series, to separate the 
final patch.
Although I hope we don't need to go that path.

Thanks,
Qu