Re: [PATCH 0/6] btrfs: scrub: refine the error messages

[Boris Burkov <boris@bur.io>]

On Thu, Mar 14, 2024 at 08:20:13PM +1030, Qu Wenruo wrote:
> During some support sessions, I found older kernels are always report
> very unuseful scrub error messages like:
> 
>  BTRFS error (device dm-0): bdev /dev/mapper/sys-rootlv errs: wr 0, rd 0, flush 0, corrupt 2823, gen 0
>  BTRFS error (device dm-0): unable to fixup (regular) error at logical 1563504640 on dev /dev/mapper/sys-rootlv
>  BTRFS error (device dm-0): bdev /dev/mapper/sys-rootlv errs: wr 0, rd 0, flush 0, corrupt 2824, gen 0
>  BTRFS error (device dm-0): unable to fixup (regular) error at logical 1579016192 on dev /dev/mapper/sys-rootlv
> 
> There are two problems:
> - No proper details about the corruption
>   No metadata or data indication, nor the filename or the tree id.
>   Even the involved kernel (and newer kernels after the scrub rework)
>   has the ability to do backref walk and find out the file path or the
>   tree backref.
> 
>   My guess is, for data sometimes the corrupted sector is no longer
>   referred by any data extent.
> 
> - Too noisy and useless error message from
>   btrfs_dev_stat_inc_and_print()
>   I'd argue we should not output any error message just for
>   btrfs_dev_stat_inc_and_print().
> 
> After the series, the error message would look like this:
> 
>  BTRFS warning (device dm-2): checksum error at inode 5/257(file1) fileoff 16384, logical 13647872(1) physical 1(/dev/mapper/test-scratch1)13647872

Stoked on this series, thanks for doing it!

qq: would it be helpful to also include the actual/expected csum? I
think it's particularly helpful when one or the other is either zeros or
the checksum of the zero block.

> 
> This involves the following enhancement:
> 
> - Single line
>   And we ensure we output at least one line for the error we hit.
>   No more unrelated btrfs_dev_stat_inc_and_print() output.
> 
> - Proper fall backs
>   We have the following different fall back options
>   * Repaired
>     Just a line of something repaired for logical/physical address.
> 
>   * Detailed data info
>     Including the following elements (if possible), and if higher
>     priority ones can not be fetched, it would be skipped and try
>     lower priority items:
>     + file path (can have multiple ones)
>     + root/inode number and offset
>     + logical/physical address (always output)
> 
>   * Detaile metadata info
>     The priority list is:
>     + root ref/level
>     + logical/physical address (always output)
> 
>   For the worst case of data corruption, we would still have some like:
> 
>    BTRFS warning (device dm-2): checksum error at data, logical 13647872(1) physical 1(/dev/mapper/test-scratch1)13647872
> 
>   And similar ones for metadata:
> 
>    BTRFS warning (device dm-2): checksum error at meta, logical 13647872(1) physical 1(/dev/mapper/test-scratch1)13647872
> 
> The first patch is fixing a regression in the error message, which leads
> to bad logical/physical bytenr.
> The second one would reduce the log level for
> btrfs_dev_stat_inc_and_print().
> 
> Path 3~4 are cleanups to remove unnecessary parameters.
> 
> The remaining reworks the format and refine the error message frequency.
> 
> Qu Wenruo (7):
>   btrfs: scrub: fix incorrectly reported logical/physical address
>   btrfs: reduce the log level for btrfs_dev_stat_inc_and_print()
>   btrfs: scrub: remove unused is_super parameter from
>     scrub_print_common_warning()
>   btrfs: scrub: remove unnecessary dev/physical lookup for
>     scrub_stripe_report_errors()
>   btrfs: scrub: simplify the inode iteration output
>   btrfs: scrub: unify and shorten the error message
>   btrfs: scrub: fix the frequency of error messages
> 
>  fs/btrfs/scrub.c   | 185 ++++++++++++++++-----------------------------
>  fs/btrfs/volumes.c |   2 +-
>  2 files changed, 66 insertions(+), 121 deletions(-)
> 
> -- 
> 2.44.0
>