Re: some help for improvement in btrfs

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

On 2022/9/8 17:21, hmsjwzb wrote:
> Hi Qu,
>
> Thanks for your reply. Sorry for the ambiguous and inaccuracy in last mail.
> This email is intend to express my idea in detail.
>
> [Main idea]
>
>      Calculate the checksum of all blocks in a full stripe. Use these checksums to
>      tell which stripe is corrupted.

That means a on-disk format change first.

>
>
> Let's go back to the destructive RMW.
>
> [test case]
>
>      mkfs.btrfs -f -m raid5 -d raid5 -b 1G /dev/vda /dev/vdb /dev/vdc
>      mount /dev/vda /mnt
>      xfs_io -f -c "pwrite -S 0xee 0 64k" /mnt/file1
>      sync
>      umount /dev/vda
>      xfs_io -f -c "pwrite -S 0xff 119865344 64k" /dev/vda
>      mount /dev/vda /mnt
>      xfs_io -f -c "pwrite -S 0xee 0 64k" -c sync /mnt/file2
>
>          At this point, the layout of devices is as follows.
>
>                  |<---stripe1---->|
>          vda |...|CCCCCCCCCCCCCCCC|...|
>
>                  |<---stripe2---->|
>          vdb |...|UUUUUUUUUUUUUUUU|...|
>
>                  |<---parity----->|
>          vdc |...|PPPPPPPPPPPPPPPP|...|
>
> 		C:corrupted    U:unused    P:parity
>
>          Before the data of file2 written to the stripe2 of vdb, we can still
>          recover the data of stripe1 by stripe2 and parity.
>          But Here is the problem.
>
>          How can we know which stripe is corrupted?
>            We need checksum for whole stripe.

That's unnecessary.

If we read that file1, we know it's corrupted and will trigger recovery.

So before writing the 2nd data stripe, if we search for checksum of the
full stripe (which would only find the csum for the data stripe 1).

And read out all stripes (data stripe 1, data stripe 2, parity stripe),
then we can compare the checksum against what we found (even csum only
covers data stripe 1).

Then for the range we have csum, we can verify if the csum matches.
If not, try recovery using data stripe 2 and parity, and check again.

>
>
>
>          But If we calculate the checksum of all blocks of a stripe, we can also tell which stripe is corrupted.
>          As for our test case, Here is my plan.

I see no obvious benefit.

You need a on-disk format change first, then you also need extra
metadata updates to handle the full stripe update.

Note that, parity is updated more frequently, thus the metadata update
is not that a small thing.

>
>          1. If we use some part or all of stripe1, then we calculate the checksum of stripe1 and stripe2.
>
>          When the write request of file2 comes, we can calculate the checksum of stripe1 and stripe2 block by block
>          in the RMW process. If some block in stripe1 mismatch, then stripe1 is corrupted. If some block in stripe2 mismatch
>          then stripe2 is corrupted.
>
>          In this case, we know stripe1 is corrupted, so we can use stripe2 and parity to recover stripe1.
>
>          In my opinion, the checksum stored in checksum tree is by byte number. So we can calculate the checksum of stripe2
>          during the writing process and store it to csum tree in later end_io process.
>
>      cat /mnt/file1 > /dev/null
>      umount /mnt
>
> Thanks,
> Flint
>
> On 9/7/22 04:46, Qu Wenruo wrote:
>>
>>
>> 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.