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

[David Sterba <dsterba@suse.cz>]

On Tue, May 11, 2021 at 09:07:26AM +0800, Qu Wenruo wrote:
> 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?

At this point do what you think would work safely even if the
performance would not be great, eg. using a spinlock around the bitmap.
We'll have to optimize all the bitmaps anyway but not before the
subpage support is finished.