Re: [PATCH v3 0/4] btrfs: make read time repair to be only submitted for each corrupted sector

[Qu Wenruo <wqu@suse.com>]

On 2021/5/11 上午4:41, David Sterba wrote:
> On Mon, May 03, 2021 at 10:08:52AM +0800, Qu Wenruo wrote:
>> Btrfs read time repair has to handle two different cases when a corruption
>> or read failure is hit:
>> - The failed bio contains only one sector
>>    Then it only need to find a good copy
>>
>> - The failed bio contains several sectors
>>    Then it needs to find which sectors really need to be repaired
>>
>> But this different behaviors are not really needed, as we can teach btrfs
>> to only submit read repair for each corrupted sector.
>> By this, we only need to handle the one-sector corruption case.
>>
>> This not only makes the code smaller and simpler, but also benefits subpage,
>> allow subpage case to use the same infrastructure.
>>
>> For current subpage code, we hacked the read repair code to make full
>> bvec read repair, which has less granularity compared to regular sector
>> size.
>>
>> The code is still based on subpage branch, but can be forward ported to
>> non-subpage code basis with minor conflicts.
>>
>> Changelog:
>> v2:
>> - Split the original patch
>>    Now we have two preparation patches, then the core change.
>>    And finally a cleanup.
>>
>> - Fix the uninitialize @error_bitmap when the bio read fails.
>>
>> v3:
>> - Fix the return value type mismatch in repair_one_sector()
>>    An error happens in v2 patch split, which can lead to hang when
>>    we can't repair the error.
> 
> Patchset added to for-next. The cleanups and simplifications look good
> to me, thanks.
> 
I'm afraid there is a bug in the patchset.

If we had a data read for 16 sectors in one page, one sector is bad and 
can't be repaired, we will under flow subage::readers number.

The cause is there are two call sites calling end_page_read().

One in btrfs_submit_read_repair(), one in end_bio_extent_readpage().
The former one is just calling end_page_read() for the good copy, while 
the latter one is calling end_page_read() for the full range.

The direct fix is to make btrfs_submit_read_repair() to handle both 
cases, and skip the call in end_bio_extent_readpage().

So I need to update the patchset to include a proper fix for it.

But on the other hand, I'm also wondering should we use 
btrfs_subpage::readers as an atomic.
For a more idiot proof way, we can also go 16bit map for reader/writer 
accounting, by that even we call end_page_read() twice for the same 
range, it won't cause anything.

Any advice on btrfs_subpage::readers implementation?
Should it be really idiot (me) proof, or just current atomic way to 
catch more idiots like me?

Thanks,
Qu