Re: [PATCH v3 3/4] btrfs: submit read time repair only for each corrupted sector

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

On 2021/5/11 上午4:32, David Sterba wrote:
> On Mon, May 03, 2021 at 10:08:55AM +0800, Qu Wenruo wrote:
>> Currently btrfs_submit_read_repair() has some extra check on whether the
>> failed bio needs extra validation for repair.
>>
>> But we can avoid all these extra mechanism if we submit the repair for
>> each sector.
>>
>> By this, each read repair can be easily handled without the need to
>> verify which sector is corrupted.
>>
>> This will also benefit subpage, as one subpage bvec can contain several
>> sectors, making the extra verification more complex.
>>
>> So this patch will:
>> - Introduce repair_one_sector()
>>    The main code submitting repair, which is more or less the same as old
>>    btrfs_submit_read_repair().
>>    But this time, it only repair one sector.
>>
>> - Make btrfs_submit_read_repair() to handle sectors differently
>>    For sectors without csum error, just release them like what we did
>>    in end_bio_extent_readpage().
>>    Although in this context we don't have process_extent structure, thus
>>    we have to do extent tree operations sector by sector.
>>    This is slower, but since it's only in csum mismatch path, it should
>>    be fine.
>>
>>    For sectors with csum error, we submit repair for each sector.
>>
>> This patch will focus on the change on the repair path, the extra
>> validation code is still kept as is, and will be cleaned up later.
>
> This leaves btrfs_io_needs_validation unused and compiler warns about
> that but it gets removed in the next patch so that's ok.
>
> I did some minor style fixups
>
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2706,7 +2706,7 @@ static void end_page_read(struct page *page, bool uptodate, u64 start, u32 len)
>          struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
>
>          ASSERT(page_offset(page) <= start &&
> -               start + len <= page_offset(page) + PAGE_SIZE);
> +              start + len <= page_offset(page) + PAGE_SIZE);
>
>          if (uptodate) {
>                  btrfs_page_set_uptodate(fs_info, page, start, len);
> @@ -2734,7 +2734,7 @@ blk_status_t btrfs_submit_read_repair(struct inode *inode,
>   {
>          struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>          const u32 sectorsize = fs_info->sectorsize;
> -       int nr_bits = (end + 1 - start) / sectorsize;
> +       const int nr_bits = (end + 1 - start) / fs_info->sectorsize_bits;

It should be >> fs_info->sectorsize_bits;

Anyway, I'll submit a proper updated version, with your update and
proper test.

Thanks,
Qu

>          int i;
>
>          BUG_ON(bio_op(failed_bio) == REQ_OP_WRITE);
> @@ -2747,10 +2747,10 @@ blk_status_t btrfs_submit_read_repair(struct inode *inode,
>                  int ret;
>                  unsigned int offset = i * sectorsize;
>
> -               if (!(error_bitmap & (1 << i))) {
> +               if (!(error_bitmap & (1U << i))) {
>                          struct extent_state *cached = NULL;
>
> -                       /* This sector has no error, just finish the read. */
> +                       /* This sector has no error, just finish the read */
>                          end_page_read(page, true, start + offset, sectorsize);
>                          set_extent_uptodate(&BTRFS_I(inode)->io_tree,
>                                          start + offset,
> ---
>
> The division can be replaced by shift as we have it in fs_info and "1U"
> in shifts is for clarity that it's performed on unsigned type.
>