Re: [PATCH] btrfs: remove skinny extent verbose message

[David Sterba <dsterba@suse.cz>]

On Thu, Jun 23, 2022 at 04:22:27PM +0800, Qu Wenruo wrote:
> 
> 
> On 2022/6/23 16:08, Nikolay Borisov wrote:
> > Skinny extents have been a default mkfs feature since version 3.18 i
> > (introduced in btrfs-progs commit 6715de04d9a7 ("btrfs-progs: mkfs:
> > make skinny-metadata default") ). It really doesn't bring any value to
> > users to simply remove it.
> >
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> 
> Looks fine to me.
> 
> But this means we need to define the level of (in)compat flags we want
> to show in the dmesg.

Yes and we haven't done that so far so we should have some guidelines.
> 
> By default, we have the following lines:
> 
>   BTRFS info (device loop0): flagging fs with big metadata feature
>   BTRFS info (device loop0): using free space tree
>   BTRFS info (device loop0): has skinny extents
>   BTRFS info (device loop0): enabling ssd optimizations
>   BTRFS info (device loop0): checking UUID tree
> 
> For "big metadata" it's even less meaningful, and it doesn't even have
> sysfs entry for it.

There's an entry in the global features but 'big_metadata' does not
appear in the per-filesystem directory.

> For "free space tree" it may be helpful, but if one is really concerning
> about the cache version we're using, it's better to go sysfs other than
> checking the kernel dmesg.

The logged messages are a bit different as they could be stored and then
used for analysis. For new features it makes more sense to log them, I
think eg. the free space tree messages have been useful when debugging
the online conversion that took a few patches to get right.

> For "SSD", it's a good thing to output.

Agreed.

> For "UUID" tree, it's definitely not useful, even for developers.

Agreed.

> For skinny metadata it's the one you're cleaning.

Though I've sent patch to make it only debug I agree that this has
little value and don't object to removing it completely.

> So I guess you can clean up more unnecessary mount messages then?

The criteria I'd use for adding/removing the messages are vaguely like
that:

- does the user want to know a particular feature is in use? this is
  namely when we're introducing something and want to verify what's
  happening

- can the option have an impact on the filesystem behaviour?  eg. like
  ssd, but we tend to log almost all mount options already

- if there's a value for developer, the level should be debug, otherwise
  info

- remove messages if a feature is on by default for a long time and does
  not bring any other value, eg. the mixed_backref, big_metadata or
  skinny extents;
  the respective sysfs files may need to stay as they could be used for
  runtime detection even after a long time, eg. in some testsuite
  collecting testcases over time but likely not updating them, removal
  should be done on case-by-case basis