| Message ID | 20220623080858.1433010-1-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: remove skinny extent verbose message | expand |
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.
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.
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.
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.
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
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
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
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,
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
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.
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.
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(-)