Re: [PATCH] btrfs: scrub: expand scrub block size for data range scrub

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

On 2022/11/13 23:35, Li Zhang wrote:

A small description here on the problem would help on the context.

> [implement]
> 1. Add the member checksum_error to the scrub_sector,
> which indicates the checksum error of the sector
> 
> 2. Use scrub_find_btrfs_ordered_sum to find the desired

Please don't put "_btrfs_" in the middle.

Just "scrub_find_ordered_sum()" is good enough.

> btrfs_ordered_sum containing address logic, in
> scrub_sectors_for_parity and scrub_sectors, call
> Scrub_find_btrfs_ordered_sum finds the
> btrfs_ordered_sum containing the current logical address, and
> Calculate the exact checksum later.
> 
> 3. In the scrub_checksum_data function,
> we should check all sectors in the scrub_block.
> 
> 4. The job of the scrub_handle_errored_block
> function is to count the number of error and
> repair sectors if they can be repaired.

The idea is good.

And I hope it can also unify the error accounting of data and metadata.

Currently for metadata csum mismatch, we only report one csum error even 
if the metadata is 4 sectors.
While for data, we always report the number of corrupted sectors.

> 
> The function enters the wrong scrub_block, and
> the overall process is as follows
> 
> 1) Check the scrub_block again, check again if the error is gone.
> 
> 2) Check the corresponding mirror scrub_block, if there is no error,
> Fix bad sblocks with mirror scrub_block.
> 
> 3) If no error-free scrub_block is found, repair it sector by sector.
> 
> One difficulty with this function is rechecking the scrub_block.
> 
> Imagine this situation, if a sector is checked the first time
> without errors, butthe recheck returns an error.

If you mean the interleaved corrupted sectors like the following:
             0 1 2 3
  Mirror 1: |X| |X| |
  Mirror 2: | |X| |X|

Then it's pretty simple, when doing re-check, we should only bother the 
one with errors.
You do not always treat the scrub_block all together.

Thus if you're handling mirror 1, then you should only need to fix 
sector 0 and sector 2, not bothering fixing the good sectors (1 and 3).


> What should
> we do, this patch only fixes the bug that the sector first
> appeared (As in the case where the scrub_block
> contains only one scrub_sector).
> 
> Another reason to only handle the first error is,
> If the device goes bad, the recheck function will report more and
> more errors,if we want to deal with the errors in the recheck,
> you need to recheck again and again, which may lead to
> Stuck in scrub_handle_errored_block for a long time.

Taking longer time is not a problem, compared to corrupted data.

Although I totally understand that the existing scrub is complex in its 
design, that's exactly why I'm trying to implement a better scrub_fs 
interface:

https://lwn.net/Articles/907058/

RAID56 has a similiar problem until recent big refactor, changing it to 
a more streamlined flow.

But the per-sector repair is still there, you can not avoid that, no 
matter if scrub_block contains single or multiple sectors.
(Although single sector scrub_block make is much easier)

[...]
> @@ -1054,7 +1056,8 @@ static int scrub_handle_errored_block(struct scrub_block *sblock_to_check)
>   		if (ret == -ENOMEM)
>   			sctx->stat.malloc_errors++;
>   		sctx->stat.read_errors++;
> -		sctx->stat.uncorrectable_errors++;
> +		sctx->stat.uncorrectable_errors += scrub_get_sblock_checksum_error(sblock_to_check);
> +		sctx->stat.uncorrectable_errors += sblock_to_check->header_error;

Do we double accout the header_error for metadata?

Thanks,
Qu