linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] btrfs: remove skinny extent verbose message
@ 2022-06-23  8:08 Nikolay Borisov
  2022-06-23  8:22 ` Qu Wenruo
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Nikolay Borisov @ 2022-06-23  8:08 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Nikolay Borisov

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>
---
 fs/btrfs/disk-io.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 8c34d08e3c64..0af4c03279df 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3501,9 +3501,6 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 	else if (fs_info->compress_type == BTRFS_COMPRESS_ZSTD)
 		features |= BTRFS_FEATURE_INCOMPAT_COMPRESS_ZSTD;
 
-	if (features & BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA)
-		btrfs_info(fs_info, "has skinny extents");
-
 	/*
 	 * Flag our filesystem as having big metadata blocks if they are bigger
 	 * than the page size.
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] btrfs: remove skinny extent verbose message
  2022-06-23  8:08 [PATCH] btrfs: remove skinny extent verbose message Nikolay Borisov
@ 2022-06-23  8:22 ` Qu Wenruo
  2022-06-23  8:30   ` Nikolay Borisov
  2022-06-23 14:19   ` David Sterba
  2022-06-23  9:30 ` [PATCH] btrfs: remove skinny extent verbose message Nikolay Borisov
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 11+ messages in thread
From: Qu Wenruo @ 2022-06-23  8:22 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs



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.

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.

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.

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

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

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

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

Thanks,
Qu
> ---
>   fs/btrfs/disk-io.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 8c34d08e3c64..0af4c03279df 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3501,9 +3501,6 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>   	else if (fs_info->compress_type == BTRFS_COMPRESS_ZSTD)
>   		features |= BTRFS_FEATURE_INCOMPAT_COMPRESS_ZSTD;
>
> -	if (features & BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA)
> -		btrfs_info(fs_info, "has skinny extents");
> -
>   	/*
>   	 * Flag our filesystem as having big metadata blocks if they are bigger
>   	 * than the page size.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] btrfs: remove skinny extent verbose message
  2022-06-23  8:22 ` Qu Wenruo
@ 2022-06-23  8:30   ` Nikolay Borisov
  2022-06-23 14:19   ` David Sterba
  1 sibling, 0 replies; 11+ messages in thread
From: Nikolay Borisov @ 2022-06-23  8:30 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs



On 23.06.22 г. 11:22 ч., 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.
> 
> 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.

Already sent a patch for this one.

> 
> 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.
> 
> For "SSD", it's a good thing to output.
> 
> For "UUID" tree, it's definitely not useful, even for developers.

This is predicated on whether we need to check the UUID tree not on 
whether a flag is set, but I agree it could be removed.

> 
> For skinny metadata it's the one you're cleaning.
> 
> So I guess you can clean up more unnecessary mount messages then?

Yes

> 
> Thanks,
> Qu
>> ---
>>   fs/btrfs/disk-io.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 8c34d08e3c64..0af4c03279df 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -3501,9 +3501,6 @@ int __cold open_ctree(struct super_block *sb, 
>> struct btrfs_fs_devices *fs_device
>>       else if (fs_info->compress_type == BTRFS_COMPRESS_ZSTD)
>>           features |= BTRFS_FEATURE_INCOMPAT_COMPRESS_ZSTD;
>>
>> -    if (features & BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA)
>> -        btrfs_info(fs_info, "has skinny extents");
>> -
>>       /*
>>        * Flag our filesystem as having big metadata blocks if they are 
>> bigger
>>        * than the page size.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] btrfs: remove skinny extent verbose message
  2022-06-23  8:08 [PATCH] btrfs: remove skinny extent verbose message Nikolay Borisov
  2022-06-23  8:22 ` Qu Wenruo
@ 2022-06-23  9:30 ` Nikolay Borisov
  2022-06-23  9:52 ` Qu Wenruo
  2022-06-24 16:21 ` David Sterba
  3 siblings, 0 replies; 11+ messages in thread
From: Nikolay Borisov @ 2022-06-23  9:30 UTC (permalink / raw)
  To: linux-btrfs



On 23.06.22 г. 11: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>

Disregard this, I see you've sent a patch demoting it to debug level.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] btrfs: remove skinny extent verbose message
  2022-06-23  8:08 [PATCH] btrfs: remove skinny extent verbose message Nikolay Borisov
  2022-06-23  8:22 ` Qu Wenruo
  2022-06-23  9:30 ` [PATCH] btrfs: remove skinny extent verbose message Nikolay Borisov
@ 2022-06-23  9:52 ` Qu Wenruo
  2022-06-24 16:21 ` David Sterba
  3 siblings, 0 replies; 11+ messages in thread
From: Qu Wenruo @ 2022-06-23  9:52 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs



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>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   fs/btrfs/disk-io.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 8c34d08e3c64..0af4c03279df 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3501,9 +3501,6 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>   	else if (fs_info->compress_type == BTRFS_COMPRESS_ZSTD)
>   		features |= BTRFS_FEATURE_INCOMPAT_COMPRESS_ZSTD;
>
> -	if (features & BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA)
> -		btrfs_info(fs_info, "has skinny extents");
> -
>   	/*
>   	 * Flag our filesystem as having big metadata blocks if they are bigger
>   	 * than the page size.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] btrfs: remove skinny extent verbose message
  2022-06-23  8:22 ` Qu Wenruo
  2022-06-23  8:30   ` Nikolay Borisov
@ 2022-06-23 14:19   ` David Sterba
  2022-06-23 22:46     ` Criteria for mount messages. (was "Re: [PATCH] btrfs: remove skinny extent verbose message") Qu Wenruo
  1 sibling, 1 reply; 11+ messages in thread
From: David Sterba @ 2022-06-23 14:19 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: Nikolay Borisov, linux-btrfs

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Criteria for mount messages. (was "Re: [PATCH] btrfs: remove skinny extent verbose message")
  2022-06-23 14:19   ` David Sterba
@ 2022-06-23 22:46     ` Qu Wenruo
  2022-06-24  9:10       ` Forza
  2022-06-24 10:52       ` Martin Steigerwald
  0 siblings, 2 replies; 11+ messages in thread
From: Qu Wenruo @ 2022-06-23 22:46 UTC (permalink / raw)
  To: dsterba, Nikolay Borisov, linux-btrfs



On 2022/6/23 22:19, David Sterba wrote:
> 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

I'd say this is not that important.

In fact, there is a pretty bad example from the past, BIG_METADATA.

It's just we're supporting larger nodesize, it doesn't even bother users
that much, nor really need a incompat flag at all.

Another example is from recent subpage feature, it doesn't need any
incompat/compat RO flags at all, the only reason we're outputting
message for subpage is, it's not tested as much as native page size.

If we can ensure enough test coverage (already getting better and better
coverage) that experimental message would definitely go away.


Thus my idea criteria would be:

- Would this feature bring bad impact to end users?
   If the feature is only bringing good impact, it should not be output.

   Thus in this way, v2 cache nowadays should also be skipped, as it's
   already well tested, and no real disadvantage at all.

>
> - 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

I'd say, considering how hard to enable debug messages, it doesn't
really make much sense for developers.

Thus unfortunately such debugging info would still better to be at info
level, just in case it's something like dying message and/or the user
can not easily reproduce it.

But we may want a much shorter (even it means less human readable), less
noisy output.

Thus I'd say, we may want to output hex incompat/compat RO/compat flags
just in one line during mount, instead of current one feature per-line
behavior.

By this, it provides more debug info, but still way shorter, and way
more expandable.

Thanks,
Qu
>
> - 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Criteria for mount messages. (was "Re: [PATCH] btrfs: remove skinny extent verbose message")
  2022-06-23 22:46     ` Criteria for mount messages. (was "Re: [PATCH] btrfs: remove skinny extent verbose message") Qu Wenruo
@ 2022-06-24  9:10       ` Forza
  2022-06-24 10:52       ` Martin Steigerwald
  1 sibling, 0 replies; 11+ messages in thread
From: Forza @ 2022-06-24  9:10 UTC (permalink / raw)
  To: Qu Wenruo, dsterba, Nikolay Borisov, linux-btrfs



On 6/24/22 00:46, Qu Wenruo wrote:
> 
> 
> On 2022/6/23 22:19, David Sterba wrote:
>> 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

I think this is a good thought.

> 
> I'd say this is not that important.
> 
> In fact, there is a pretty bad example from the past, BIG_METADATA.
> 
> It's just we're supporting larger nodesize, it doesn't even bother users
> that much, nor really need a incompat flag at all.
> 
> Another example is from recent subpage feature, it doesn't need any
> incompat/compat RO flags at all, the only reason we're outputting
> message for subpage is, it's not tested as much as native page size.
> 
> If we can ensure enough test coverage (already getting better and better
> coverage) that experimental message would definitely go away.
> 
> 
> Thus my idea criteria would be:
> 
> - Would this feature bring bad impact to end users?
>    If the feature is only bringing good impact, it should not be output.
> 
>    Thus in this way, v2 cache nowadays should also be skipped, as it's
>    already well tested, and no real disadvantage at all.
> 
Even if v2 is default, lots of users are on older kernel/progs and would 
still benefit from seeing these messages.

>>
>> - 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
> 
> I'd say, considering how hard to enable debug messages, it doesn't
> really make much sense for developers.
> 
> Thus unfortunately such debugging info would still better to be at info
> level, just in case it's something like dying message and/or the user
> can not easily reproduce it.
> 
> But we may want a much shorter (even it means less human readable), less
> noisy output.
> 
> Thus I'd say, we may want to output hex incompat/compat RO/compat flags
> just in one line during mount, instead of current one feature per-line
> behavior.
> 
> By this, it provides more debug info, but still way shorter, and way
> more expandable.
> 

The idea of having a short feature flag code is good. As an end-user I 
do want to have the human readable form too. Especially during 
transition periods where lots of distros still use older kernels that 
may not have the new features enabled. For example a lot of users still 
use Ubuntu with a 5.4 kernel.

Of course, we should teach users to use /sys/ information or use btrfs 
inspect-internal tools, but I think this will take a long time.


> Thanks,
> Qu
>>
>> - 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

The sysfs files could stay always? Is that a problem?

Thanks,
Forza

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Criteria for mount messages. (was "Re: [PATCH] btrfs: remove skinny extent verbose message")
  2022-06-23 22:46     ` Criteria for mount messages. (was "Re: [PATCH] btrfs: remove skinny extent verbose message") Qu Wenruo
  2022-06-24  9:10       ` Forza
@ 2022-06-24 10:52       ` Martin Steigerwald
  2022-06-24 13:10         ` Forza
  1 sibling, 1 reply; 11+ messages in thread
From: Martin Steigerwald @ 2022-06-24 10:52 UTC (permalink / raw)
  To: dsterba, Nikolay Borisov, linux-btrfs, Qu Wenruo

Qu Wenruo - 24.06.22, 00:46:02 CEST:
> Thus my idea criteria would be:
> 
> - Would this feature bring bad impact to end users?
>    If the feature is only bringing good impact, it should not be
> output.
> 
>    Thus in this way, v2 cache nowadays should also be skipped, as it's
> already well tested, and no real disadvantage at all.

There is one aspect where I find those messages helpful:

To confirm that I successfully enabled a feature (like space cache v2).

However, for that there does not really need to be a kernel message on 
each mounting, as long as I have a way to confirm the currently used 
features of BTRFS like:

- which checksum algorithm?
- space cache v2
- big metadata
- which compression method configured as standard (I am aware that does 
not say anything about which files are compressed by which method)

and all other things like that.

Preferably in the output of just one command instead of being scattered 
around several different outputs or not even available at all. For 
example I am not aware of a command that confirms whether or not a 
filesystem uses "xxhash" instead of "crc32c" as checksum algorithm.

Something a bit similar to "tune2fs -l" or "xfs_info".

Is there such a something? I believe there is some way to dump a 
superblock however is there something that gives a structured output on 
what features are in use on a given filesystem?

Best,
-- 
Martin



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Criteria for mount messages. (was "Re: [PATCH] btrfs: remove skinny extent verbose message")
  2022-06-24 10:52       ` Martin Steigerwald
@ 2022-06-24 13:10         ` Forza
  0 siblings, 0 replies; 11+ messages in thread
From: Forza @ 2022-06-24 13:10 UTC (permalink / raw)
  To: Martin Steigerwald, dsterba, Nikolay Borisov, linux-btrfs, Qu Wenruo



On 6/24/22 12:52, Martin Steigerwald wrote:
> Qu Wenruo - 24.06.22, 00:46:02 CEST:
>> Thus my idea criteria would be:
>>
>> - Would this feature bring bad impact to end users?
>>     If the feature is only bringing good impact, it should not be
>> output.
>>
>>     Thus in this way, v2 cache nowadays should also be skipped, as it's
>> already well tested, and no real disadvantage at all.
> 
> There is one aspect where I find those messages helpful:
> 
> To confirm that I successfully enabled a feature (like space cache v2).
> 
> However, for that there does not really need to be a kernel message on
> each mounting, as long as I have a way to confirm the currently used
> features of BTRFS like:
> 
> - which checksum algorithm?
> - space cache v2
> - big metadata
> - which compression method configured as standard (I am aware that does
> not say anything about which files are compressed by which method)
> 
> and all other things like that.
> 
> Preferably in the output of just one command instead of being scattered
> around several different outputs or not even available at all. For
> example I am not aware of a command that confirms whether or not a
> filesystem uses "xxhash" instead of "crc32c" as checksum algorithm.
> 
> Something a bit similar to "tune2fs -l" or "xfs_info".
> 
> Is there such a something? I believe there is some way to dump a
> superblock however is there something that gives a structured output on
> what features are in use on a given filesystem?
> 

This is perhaps the better choice? A "btrfs info" or "btrfs filesystem 
info" command could output all these things in a user friendly and 
machine readable way. Possibly with added machine parsable output with 
an optional flag.

Currently we have "btrfs inspect-internal dump-super" which has a lot of 
the infomation already. I might be mistaken, but I think it only reads 
the superblocks directly off the block device, and it may not reflect 
what options the kernel currently uses. For example it cannot tell what 
kernel implementation of the checksum algorithm is used (crc32c-intel, 
sha256-generic, etc).


  # btrfs inspect-internal dump-super /dev/sdc2
superblock: bytenr=65536, device=/dev/sdc2
---------------------------------------------------------
csum_type               1 (xxhash64)
csum_size               8
csum                    0x46c8eb9318dc87eb [match]
bytenr                  65536
flags                   0x1
                         ( WRITTEN )
magic                   _BHRfS_M [match]
fsid                    aa358efb-ce43-498c-9997-0d35ba13261f
metadata_uuid           aa358efb-ce43-498c-9997-0d35ba13261f
label                   btrfs_backup_2T
generation              30096
root                    927851757568
sys_array_size          129
chunk_root_generation   30092
root_level              1
chunk_root              23003136
chunk_root_level        1
log_root                0
log_root_transid        0
log_root_level          0
total_bytes             2996496760832
bytes_used              1756499472384
sectorsize              4096
nodesize                16384
leafsize (deprecated)   16384
stripesize              4096
root_dir                6
num_devices             1
compat_flags            0x0
compat_ro_flags         0x3
                         ( FREE_SPACE_TREE |
                           FREE_SPACE_TREE_VALID )
incompat_flags          0x371
                         ( MIXED_BACKREF |
                           COMPRESS_ZSTD |
                           BIG_METADATA |
                           EXTENDED_IREF |
                           SKINNY_METADATA |
                           NO_HOLES )
cache_generation        0
uuid_tree_generation    30096
block_group_root        0
block_group_root_generation     0
block_group_root_level  0
dev_item.uuid           d97d96ae-8f9c-4d32-a326-632700d29fa2
dev_item.fsid           aa358efb-ce43-498c-9997-0d35ba13261f [match]
dev_item.type           0
dev_item.total_bytes    2996496760832
dev_item.bytes_used     1803903041536
dev_item.io_align       4096
dev_item.io_width       4096
dev_item.sector_size    4096
dev_item.devid          1
dev_item.dev_group      0
dev_item.seek_speed     0
dev_item.bandwidth      0
dev_item.generation     0


Thanks,
Forza

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] btrfs: remove skinny extent verbose message
  2022-06-23  8:08 [PATCH] btrfs: remove skinny extent verbose message Nikolay Borisov
                   ` (2 preceding siblings ...)
  2022-06-23  9:52 ` Qu Wenruo
@ 2022-06-24 16:21 ` David Sterba
  3 siblings, 0 replies; 11+ messages in thread
From: David Sterba @ 2022-06-24 16:21 UTC (permalink / raw)
  To: Nikolay Borisov; +Cc: linux-btrfs

On Thu, Jun 23, 2022 at 11:08:58AM +0300, 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>
> ---

Added to misc-next, thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-06-24 16:26 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-23  8:08 [PATCH] btrfs: remove skinny extent verbose message Nikolay Borisov
2022-06-23  8:22 ` Qu Wenruo
2022-06-23  8:30   ` Nikolay Borisov
2022-06-23 14:19   ` David Sterba
2022-06-23 22:46     ` Criteria for mount messages. (was "Re: [PATCH] btrfs: remove skinny extent verbose message") Qu Wenruo
2022-06-24  9:10       ` Forza
2022-06-24 10:52       ` Martin Steigerwald
2022-06-24 13:10         ` Forza
2022-06-23  9:30 ` [PATCH] btrfs: remove skinny extent verbose message Nikolay Borisov
2022-06-23  9:52 ` Qu Wenruo
2022-06-24 16:21 ` David Sterba

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).