Re: misc-next and for-next: kernel BUG at fs/btrfs/extent_io.c:2350! during raid5 recovery

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

On 2022/8/10 16:08, Goffredo Baroncelli wrote:
> On 09/08/2022 23.50, Qu Wenruo wrote:
>>>> n 2022/8/9 11:31, Zygo Blaxell wrote:
>>>>> Test case is:
>>>>>
>>>>>     - start with a -draid5 -mraid1 filesystem on 2 disks
>>>>>
>>>>>     - run assorted IO with a mix of reads and writes (randomly
>>>>>     run rsync, bees, snapshot create/delete, balance, scrub, start
>>>>>     replacing one of the disks...)
>>>>>
>>>>>     - cat /dev/zero > /dev/vdb (device 1) in the VM guest, or run
>>>>>     blkdiscard on the underlying SSD in the VM host, to simulate
>>>>>     single-disk data corruption
>>>>
>>>> One thing to mention is, this is going to cause destructive RMW to
>>>> happen.
>>>>
>>>> As currently substripe write will not verify if the on-disk data stripe
>>>> matches its csum.
>>>>
>>>> Thus if the wipeout happens while above workload is still running, it's
>>>> going to corrupt data eventually.
>>>
>>> That would be a btrfs raid5 design bug,
>>
>> That's something all RAID5 design would have the problem, not just btrfs.
>>
>> Any P/Q based profile will have the problem.
>
>
> Hi Qu,
>
> I looked at your description of 'destructive RMW' cyle:
>
> ----
> Test case btrfs/125 (and above workload) always has its trouble with
> the destructive read-modify-write (RMW) cycle:
>
>          0       32K     64K
> Data1:  | Good  | Good  |
> Data2:  | Bad   | Bad   |
> Parity: | Good  | Good  |
>
> In above case, if we trigger any write into Data1, we will use the bad
> data in Data2 to re-generate parity, killing the only chance to recovery
> Data2, thus Data2 is lost forever.
> ----
>
> What I don't understood if we have a "implementation problem" or an
> intrinsic problem of raid56...
>
> To calculate parity we need to know:
>      - data1 (in ram)
>      - data2 (not cached, bad on disk)
>
> So, first, we need to "read data2" then to calculate the parity and then
> to write data1.

First thing first, the idea of RAID5/6 itself doesn't involve how to
verify the data is correct or not.

Btrfs is better because it has extra csum ability.

Thus for bad on-disk data case, it's even worse for all the other
RAID5/6 implementations which don't have csum for its data.

>
> The key factor is "read data", where we can face three cases:
> 1) the data is referenced and has a checksum: we can check against the
> checksum and if the checksum doesn't match we should perform a recover
> (on the basis of the data stored on the disk)

Then let's talk about the detail problems in the btrfs specific areas.

Yes, it's possible that we can try to check the csums when doing RMW for
sub-stripe writes.

But there are problems:

=== csum tree race ===

1) Data writes (with csum) into some data 1 stripes finished

2) By somehow the just written data is corrupted

3) RAID56 RMW triggered for write into data 2

4) We search csum tree, and found nothing, as the csum insert hasn't
    happen yet

5) Csum insert happens for data write in 1)

Then we still have the destructive RMW.
Yes we can argue that 2) is so rare that we shouldn't really bother, but
in theory this can still happen, all just because the csum tree search
and csum insert has race window.

=== performance ===

This means, every time we do a RMW, we have to do a full csum tree and
extent tree search.
And this also mean, we have to read the full stripe, including the range
we're going to write data into.

AKA, this is full scrub level check.

This is definitely not a small cost, which will further slowdown the
RAID56 write.

Thus even if we're going to implement such scrub level check, I'm not
going to make it the default option, but make it opt-in.

Although recently since I'm working on RAID56, unless there are some
possible dead-lock, I believe it's possible to implement.

Maybe in the near future we may see some prototypes to try to address
this problem.

Thanks,
Qu


> 2) the data is referenced but doesn't have a checksum (nocow): we cannot
> ensure the corruption of the data if checksum is not enabled. We can
> only ensure the availability of the data (which may be corrupted)
> 3) the data is not referenced: so the data is good.
>
> So in effect for the case 2) the data may be corrupted and not
> recoverable (but this is true in any case); but for the case 1) from a
> theoretical point of view it seems recoverable. Of course this has a
> cost: you need to read the stripe and their checksum (doing a recovery
> if needed) before updating any part of the stripe itself, maintaining a
> strict order between the read and the writing.
>
>