Re: [PATCH 6/7] btrfs: scrub: unify and shorten the error message

[Qu Wenruo <wqu@suse.com>]

在 2024/3/15 04:10, Filipe Manana 写道:
> On Thu, Mar 14, 2024 at 9:51 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> Currently the scrub error report is pretty long:
>>
>>   BTRFS error (device dm-2): unable to fixup (regular) error at logical 13647872 on dev /dev/mapper/test-scratch1 physical 13647872
>>   BTRFS warning (device dm-2): checksum error at logical 13647872 on dev /dev/mapper/test-scratch1, physical 13647872, root 5, inode 257, offset 16384, length 4096, links 1 (path: file1)
>>
>> Since we have so many things to output, it's not a surprise we got long
>> error lines.
>>
>> To make the lines a little shorter, and make important info more
>> obvious, change the unify output to something like this:
>>
>>   BTRFS error (device dm-2): unable to fixup (regular) error at logical 13647872(1) physical 1(/dev/mapper/test-scratch1)13647872
>>   BTRFS warning (device dm-2): checksum error at inode 5/257(file1) fileoff 16384, logical 13647872(1) physical 1(/dev/mapper/test-scratch1)13647872
> 
> I find that hard to read because:
> 
> 1) Please leave spaces before opening parenthesis.
>     That makes things less cluttered and easier to the eyes, more
> consistent with what we generally do and it's the formal way to do
> things in English (see
> https://www.scribens.com/grammar-rules/typography/spacing.html);

Yep, I can do that, but I also want to keep the involved members 
together thus closer.

Not sure if adding spaces would still keep the close relationships 
between the values.

E.g: inode 5/256 (file1) fileoff 16384, logical 123456 (1) physical 1 
(scratch1) 123456

It makes it a little harder to indicate that "(1)" is related to logical 
address (thus mirror number).

> 
> 2) Instead of "inode 5/257(file1)", something a lot easier to
> understand like "inode 5 root 257 path \"file1\"", which leaves no
> margin for doubt about what each number is;
> 
> 3) Same with "logical 13647872(1)" - what is the 1? That will be the
> question anyone reading such a log message will likely have.
>      Something like "logical 13647872 mirror 1" makes it clear;
> 
> 4) Same with "physical 1(/dev/mapper/test-scratch1)13647872".
>      Something like "physical 13647872 device ID 1 device path
> \"/dev/mapper/test-scratch1\"", makes it clear what each number is and
> easier to read.

I totally understand the original output format is much easier to read 
on its own.

However if we have hundreds lines of similar output, it's a different 
story, a small change in any of the value is much harder to catch, and 
the extra helpful prompt is in fact a burden other than a bless.

(That's why things like spreadsheet is much easier to reader for 
multiple similarly structured data, and in that case it's much better to 
craft a script to convert the lines into a csv)

Unfortunately we don't have the benefit (at least yet) to structure all 
these info into a structured output.


But what I'm doing here is to reduce the prompts to minimal (at most 4 
prompts, "inode", "fileoff", "logical", "physical"), so less duplicated 
strings for multiple lines of similar error messages.

The patchset is in the middle ground between fully detailed output (the 
old one, focusing on single line) and the fully script orient output (no 
prompt at all, just all numbers/strings, focusing on CSV like output).


Furthermore, I would also argue that, for entry level end users, they 
can still catch the critical info (like file path and device path) 
without much difficult, and those info is enough for them to take action.
(E.g. deleting the file, or replace the disk)

Yes, they would get confused on the devid or rootid, but that's fine and 
everyone can learn something new.

For experienced users who understand basics of btrfs, or us developers, 
the split in values are already arranged in a way similar sized values 
are never close together (aka, string/large/small value split).

Thanks,
Qu

> 
> Thanks.
>