Re: [PATCH 12/15] btrfs: add new read repair infrastructure

[Nikolay Borisov <nborisov@suse.com>]

On 19.05.22 г. 13:41 ч., Qu Wenruo wrote:
> 
> 
> On 2022/5/19 17:36, Christoph Hellwig wrote:
>> On Wed, May 18, 2022 at 07:04:22AM +0800, Qu Wenruo wrote:
>>> Function btrfs_repair_read_bio() will only return true if all of its
>>> data matches csum.
>>>
>>> Consider the following case:
>>>
>>> Profile RAID1C3, 2 sectors to read, the initial mirror is 1.
>>>
>>> Mirror 1:    |X|X|
>>> Mirror 2:    |X| |
>>> Mirror 3:    | |X|
>>>
>>> Now we will got -EIO, but in reality, we can repair the read by using
>>> the first sector from mirror 3 and the 2nd sector from mirror 2.
>>
>> I tried to write a test case for this by copying btrfs/140 and then
>> as a first step extending it to three mirrors unsing the raid1c1
>> profile.  But it seems that the tricks used there don't work,
>> as the code in btrfs/140 relies on the fact that the btrfs logic
>> address repored by file frag is reported by dump-tree as the item
>> "index" ĭn this line:
>>
>> item 4 key (FIRST_CHUNK_TREE CHUNK_ITEM 137756672) itemoff 15751 itemsiz
>>
>> but for the raid1c3 profile that line reports something entirely
>> different.
>>
>> for raid1:
>>
>> logical: 137756672
>> item 4 key (FIRST_CHUNK_TREE CHUNK_ITEM 137756672) itemoff 15751 
>> itemsize 112
>>
>> for raid1c3:
>>
>> logical: 343998464
>> item 5 key (FIRST_CHUNK_TREE CHUNK_ITEM 298844160) itemoff 15621 
>> itemsize 144
>>
>> any idea how to find physical sectors to corrupt for raid1c1?
>>
> 
> I also recently hit weird cases why extent allocator no longer puts the 
> first data extent at the beginning of a chunk.
> 
> So in that case, the best solution is to use "btrfs-map-logical -l 
> 343998464", which will directly return the physical offset of the wanted 
> logical on each involved devices.

Any reason why this is kept as a separate tool and not simply integrated 
into btrfs-progs as a separate command?

> 
> Although we need to note:
> 
> - btrfs-map-logical may not always be shipped in progs in the future
>    This tool really looks like a debug tool. I'm not sure if we will keep
>    shipping it (personally I really hope to)
> 
> - btrfs-map-logical only return data stripes
>    Thus it doesn't work for RAID56 just in case you want to use it.
> 
> Despite the weird extent logical bytenr, everything should be fine with 
> btrfs-map-logical.
> 
> I'll take some time looking into the weird behavior change.
> 
> Thanks,
> Qu
>