Re: some help for improvement in btrfs

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

On 2022/9/7 16:24, hmsjwzb wrote:
>
[...]
>> That's the idea to fix the destructive RMW, aka when doing sub-stripe
>> write, we need to:
>>
>> 1. Collected needed data to do the recovery
>>     For data, it should be all csum inside the full stripe.
>>     For metadata, although the csum is inlined, we still need to find out
>>     which range has metadata, and this can be a little tricky.
>
> Hi Qu,
>
>    As for fix solution, I think the following method don't need a on-disk format.

I never mentioned we need a on-disk format change.

I just mentioned some pitfalls you need to look after.

>
>      For Data:
>        When we write to disk, we do checksum for the writing data.
>
>          dev1 |...|DDUUUUUUUUUU|...|
>          dev2 |...|UUUUUUUUUUUU|...|
>          dev3 |...|PPPPPPPPPPPP|...|
>
> 	D:data Space:unused P:parity  U:unused
>
>        The checksum block will look like the following.
>          dev1 |...|CC          |...|
>          dev2 |...|            |...|
>          dev3 |...|            |...|
>
>          C:Checksum
>
>        So if data is corrupted in the area we write, we can know it. But when the corruption happened in
>        the unused block. We can do nothing about it.

We don't need to bother corruption happened in unused space at all.

We only need to ensure our data and parity is correct.

So if the data sectors are fine, although parity mismatches, it's not a
problem, we just do the regular RMW, and correct parity will be
re-calculated and write back to that disk.

>        The checksum of blocks marked with C will be calculated and inserted into checksum tree.

Why insert? There is no insert needed at all.

And furthermore, data csum insert happens way later, RAID56 layer should
not bother to insert the csum.

If you're talking about writing the first two sectors, they should not
have csum at all, as data COW ensured we can only write data into unused
space.

Please make it clear what you're really wanting to do, using W for
blocks to write, U for unused, C for old data which has csum.

(Don't bother NODATASUM case for now).

Thanks,
Qu

>
>        But what if we do the checksum of the full stripe.
>                   |<--stripe1->|
>          dev1 |...|CCCCCCCCCCCC|...|
>                   |<--stripe2->|
>          dev2 |...|CCCCCCCCCCCC|...|
>          dev3 |...|            |...|
>
>        So in the rmw process, we can do the following operation.
>          1.Calculate the checksum of blocks in stripe1 and compare them with the checksum in checksum tree.
>            If mismatch then stripe1 is corrupted, otherwise stripe1 is good.
>          2.We can do the same process for stripe2. So we can tell whether stripe2 is corrupted or good.
> 	3.In this condition, if parity check failed, we can know which stripe is corrupted and use the good
>            data to recover the bad.
>
> Thanks,
> Flint
>
>> 2. Read out all data and stripe, including the range we're writing into
>>     Currently we skip the range we're going to write, but since we may
>>     need to do a recovery, we need the full stripe anyway.
>>
>> 3. Do full stripe verification before doing RMW.
>>     That's the core recovery, thankfully we should have very similiar
>>     code existing already.
>>
>>>
>>> [question]
>>>      I have noticed this patch.
>>>
>>>          [PATCH PoC 0/9] btrfs: scrub: introduce a new family of ioctl, scrub_fs
>>>          Hi Qu,
>>>              Is some part of this patch aim to solve this problem?
>>
>> Nope, that's just to improve scrub for RAID56, nothing related to this
>> destructive RMW thing at all.
>>
>> Thanks,
>> Qu
>>>
>>> Thanks,
>>> Flint
>>>
>>>
>>> On 8/16/22 01:38, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2022/8/16 10:47, hmsjwzb wrote:
>>>>> Hi Qu,
>>>>>
>>>>> Sorry for interrupt you so many times.
>>>>>
>>>>> As for
>>>>>       scrub level checks at RAID56 substripe write time.
>>>>>
>>>>> Is this feature available in latest linux-next branch?
>>>>
>>>> Nope, no one is working on that, thus no patches at all.
>>>>
>>>>> Or may I need to get patches from mail list.
>>>>> What is the core function of this feature ?
>>>>
>>>> The following small script would explain it pretty well:
>>>>
>>>>     mkfs.btrfs -f -m raid5 -d raid5 -b 1G $dev1 $dev2 $dev3
>>>>     mount $dev1 $mnt
>>>>
>>>>     xfs_io -f -c "pwrite -S 0xee 0 64K" $mnt/file1
>>>>     sync
>>>>     umount $mnt
>>>>
>>>>     # Currupt data stripe 1 of full stripe of above 64K write
>>>>     xfs_io -f -c "pwrite -S 0xff 119865344 64K" $dev1
>>>>
>>>>     mount $dev1 $mnt
>>>>
>>>>     # Do a new write into data stripe 2,
>>>>     # We will trigger a RMW, which will use on-disk (corrupted) data to
>>>>     # generate new P/Q.
>>>>     xfs_io -f -c "pwrite -S 0xee 0 64K" -c sync $mnt/file2
>>>>
>>>>     # Now we can no longer read file1, as its data is corrupted, and
>>>>     # above write generated new P/Q using corrupted data stripe 1,
>>>>     # preventing us to recover the data stripe 1.
>>>>     cat $mnt/file1 > /dev/null
>>>>     umount $mnt
>>>>
>>>> Above script is the best way to demonstrate the "destructive RMW".
>>>> Although this is not btrfs specific (other RAID56 is also affected),
>>>> it's definitely a real problem.
>>>>
>>>> There are several different directions to solve it:
>>>>
>>>> - A way to add CSUM for P/Q stripes
>>>>     In theory this should be the easiest way implementation wise.
>>>>     We can easily know if a P/Q stripe is correct, then before doing
>>>>     RMW, we verify the result of P/Q.
>>>>     If the result doesn't match, we know some data stripe(s) are
>>>>     corrupted, then rebuild the data first before write.
>>>>
>>>>     Unfortunately, this needs a on-disk format.
>>>>
>>>> - Full stripe verification before writes
>>>>     This means, before we submit sub-stripe writes, we use some scrub like
>>>>     method to verify all data stripes first.
>>>>     Then we can do recovery if needed, then do writes.
>>>>
>>>>     Unfortunately, scrub-like checks has quite some limitations.
>>>>     Regular scrub only works on RO block groups, thus extent tree and csum
>>>>     tree are consistent.
>>>>     But for RAID56 writes, we have no such luxury, I'm not 100% sure if
>>>>     this can even pass stress tests.
>>>>
>>>> Thanks,
>>>> Qu
>>>>
>>>>>
>>>>> I think I may use qemu and gdb to get basic understanding about this feature.
>>>>>
>>>>> Thanks,
>>>>> Flint
>>>>>
>>>>> On 8/15/22 04:54, Qu Wenruo wrote:
>>>>>> scrub level checks at RAID56 substripe write time.