[PATCH] btrfs-progs: print-tree: print the checksum of header without tailing zeros

[PATCH] btrfs-progs: print-tree: print the checksum of header without tailing zeros

[Qu Wenruo]

For the default CRC32 checksum, print-tree now prints tons of
unnecessary padding zeros:

  btrfs-progs v5.17
  chunk tree
  leaf 22036480 items 7 free space 15430 generation 6 owner CHUNK_TREE
  leaf 22036480 flags 0x1(WRITTEN) backref revision 1
  checksum stored 0ac1b9fa00000000000000000000000000000000000000000000000000000000
  checksum calced 0ac1b9fa00000000000000000000000000000000000000000000000000000000
  fs uuid 3d95b7e3-3ab6-4927-af56-c58aa634342e

This is caused by commit 1bb6fb896dfc ("btrfs-progs: btrfstune:
experimental, new option to switch csums"), and it looks like most
distros just enable EXPERIMENTAL features by default.
(Which is a good thing to provide much better coverage).

So here we just limit the csum print to the utilized csum size.

Now the output looks like:

  btrfs-progs v5.17
  chunk tree
  leaf 22036480 items 4 free space 15781 generation 6 owner CHUNK_TREE
  leaf 22036480 flags 0x1(WRITTEN) backref revision 1
  checksum stored 676b812f
  checksum calced 676b812f
  fs uuid d11f8799-b6dc-415d-b1ed-cebe6da5f0b7

Fixes: 1bb6fb896dfc ("btrfs-progs: btrfstune: experimental, new option to switch csums")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/print-tree.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/kernel-shared/print-tree.c b/kernel-shared/print-tree.c
index 9c12dfcb4ca5..4f34645cf741 100644
--- a/kernel-shared/print-tree.c
+++ b/kernel-shared/print-tree.c
@@ -1224,7 +1224,7 @@ static void print_header_info(struct extent_buffer *eb, unsigned int mode)
 	u8 backref_rev;
 	char csum_str[2 * BTRFS_CSUM_SIZE + strlen(" csum 0x") + 1];
 	int i;
-	int csum_size;
+	int csum_size = fs_info->csum_size;
 
 	flags = btrfs_header_flags(eb) & ~BTRFS_BACKREF_REV_MASK;
 	backref_rev = btrfs_header_flags(eb) >> BTRFS_BACKREF_REV_SHIFT;
@@ -1249,7 +1249,6 @@ static void print_header_info(struct extent_buffer *eb, unsigned int mode)
 		char *tmp = csum_str;
 		u8 *csum = (u8 *)(eb->data + offsetof(struct btrfs_header, csum));
 
-		csum_size = fs_info->csum_size;
 		strcpy(csum_str, " csum 0x");
 		tmp = csum_str + strlen(csum_str);
 		for (i = 0; i < csum_size; i++) {
@@ -1268,7 +1267,7 @@ static void print_header_info(struct extent_buffer *eb, unsigned int mode)
 
 #ifdef EXPERIMENTAL
 	printf("checksum stored ");
-	for (i = 0; i < BTRFS_CSUM_SIZE; i++)
+	for (i = 0; i < csum_size; i++)
 		printf("%02hhx", (int)(eb->data[i]));
 	printf("\n");
 	memset(csum, 0, sizeof(csum));
@@ -1276,7 +1275,7 @@ static void print_header_info(struct extent_buffer *eb, unsigned int mode)
 			(u8 *)eb->data + BTRFS_CSUM_SIZE,
 			csum, fs_info->nodesize - BTRFS_CSUM_SIZE);
 	printf("checksum calced ");
-	for (i = 0; i < BTRFS_CSUM_SIZE; i++)
+	for (i = 0; i < csum_size; i++)
 		printf("%02hhx", (int)(csum[i]));
 	printf("\n");
 #endif
-- 
2.36.0

Re: [PATCH] btrfs-progs: print-tree: print the checksum of header without tailing zeros

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH] btrfs-progs: print-tree: print the checksum of header without tailing zeros

[David Sterba]

On Tue, May 10, 2022 at 02:03:18PM +0800, Qu Wenruo wrote:
> For the default CRC32 checksum, print-tree now prints tons of
> unnecessary padding zeros:
> 
>   btrfs-progs v5.17
>   chunk tree
>   leaf 22036480 items 7 free space 15430 generation 6 owner CHUNK_TREE
>   leaf 22036480 flags 0x1(WRITTEN) backref revision 1
>   checksum stored 0ac1b9fa00000000000000000000000000000000000000000000000000000000
>   checksum calced 0ac1b9fa00000000000000000000000000000000000000000000000000000000
>   fs uuid 3d95b7e3-3ab6-4927-af56-c58aa634342e
> 
> This is caused by commit 1bb6fb896dfc ("btrfs-progs: btrfstune:
> experimental, new option to switch csums"), and it looks like most
> distros just enable EXPERIMENTAL features by default.

Which distros?

> (Which is a good thing to provide much better coverage).

Well, this depends if the experimental features are used as such. I'll
add a warning in case they get actually used.

> So here we just limit the csum print to the utilized csum size.
> 
> Now the output looks like:
> 
>   btrfs-progs v5.17
>   chunk tree
>   leaf 22036480 items 4 free space 15781 generation 6 owner CHUNK_TREE
>   leaf 22036480 flags 0x1(WRITTEN) backref revision 1
>   checksum stored 676b812f
>   checksum calced 676b812f
>   fs uuid d11f8799-b6dc-415d-b1ed-cebe6da5f0b7
> 
> Fixes: 1bb6fb896dfc ("btrfs-progs: btrfstune: experimental, new option to switch csums")
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Added to devel, thanks.

Re: [PATCH] btrfs-progs: print-tree: print the checksum of header without tailing zeros

[Qu Wenruo]

On 2022/5/10 19:48, David Sterba wrote:
> On Tue, May 10, 2022 at 02:03:18PM +0800, Qu Wenruo wrote:
>> For the default CRC32 checksum, print-tree now prints tons of
>> unnecessary padding zeros:
>>
>>    btrfs-progs v5.17
>>    chunk tree
>>    leaf 22036480 items 7 free space 15430 generation 6 owner CHUNK_TREE
>>    leaf 22036480 flags 0x1(WRITTEN) backref revision 1
>>    checksum stored 0ac1b9fa00000000000000000000000000000000000000000000000000000000
>>    checksum calced 0ac1b9fa00000000000000000000000000000000000000000000000000000000
>>    fs uuid 3d95b7e3-3ab6-4927-af56-c58aa634342e
>>
>> This is caused by commit 1bb6fb896dfc ("btrfs-progs: btrfstune:
>> experimental, new option to switch csums"), and it looks like most
>> distros just enable EXPERIMENTAL features by default.
>
> Which distros?

Archlinux.

Their PKGBUILD can be found here:
https://github.com/archlinux/svntogit-packages/blob/packages/btrfs-progs/trunk/PKGBUILD

Which doesn't enable experimental explicitly, but still got the full
csum output.

Thanks,
Qu
>
>> (Which is a good thing to provide much better coverage).
>
> Well, this depends if the experimental features are used as such. I'll
> add a warning in case they get actually used.
>
>> So here we just limit the csum print to the utilized csum size.
>>
>> Now the output looks like:
>>
>>    btrfs-progs v5.17
>>    chunk tree
>>    leaf 22036480 items 4 free space 15781 generation 6 owner CHUNK_TREE
>>    leaf 22036480 flags 0x1(WRITTEN) backref revision 1
>>    checksum stored 676b812f
>>    checksum calced 676b812f
>>    fs uuid d11f8799-b6dc-415d-b1ed-cebe6da5f0b7
>>
>> Fixes: 1bb6fb896dfc ("btrfs-progs: btrfstune: experimental, new option to switch csums")
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> Added to devel, thanks.

Re: [PATCH] btrfs-progs: print-tree: print the checksum of header without tailing zeros

[Qu Wenruo]

On 2022/5/10 20:02, Qu Wenruo wrote:
>
>
> On 2022/5/10 19:48, David Sterba wrote:
>> On Tue, May 10, 2022 at 02:03:18PM +0800, Qu Wenruo wrote:
>>> For the default CRC32 checksum, print-tree now prints tons of
>>> unnecessary padding zeros:
>>>
>>>    btrfs-progs v5.17
>>>    chunk tree
>>>    leaf 22036480 items 7 free space 15430 generation 6 owner CHUNK_TREE
>>>    leaf 22036480 flags 0x1(WRITTEN) backref revision 1
>>>    checksum stored
>>> 0ac1b9fa00000000000000000000000000000000000000000000000000000000
>>>    checksum calced
>>> 0ac1b9fa00000000000000000000000000000000000000000000000000000000
>>>    fs uuid 3d95b7e3-3ab6-4927-af56-c58aa634342e
>>>
>>> This is caused by commit 1bb6fb896dfc ("btrfs-progs: btrfstune:
>>> experimental, new option to switch csums"), and it looks like most
>>> distros just enable EXPERIMENTAL features by default.
>>
>> Which distros?
>
> Archlinux.
>
> Their PKGBUILD can be found here:
> https://github.com/archlinux/svntogit-packages/blob/packages/btrfs-progs/trunk/PKGBUILD
>
>
> Which doesn't enable experimental explicitly, but still got the full
> csum output.

OK, got the reason why the EXPERIMENTAL thing doesn't work as expected.

In configure, we set $EXPERIMENTAL=0 by default, then
add the line into confdefs.h:

#define EXPERIMENTAL 0

However in print-tree.c, we use

#ifdef EXPERIMENTAL

Then it always get enabled, no matter if it's 0 or 1.

We should either don't define it at all, and use the "#ifdef" way, or we
should go "#if EXPERIMENTAL" instead.

Thanks,
Qu
>
> Thanks,
> Qu
>>
>>> (Which is a good thing to provide much better coverage).
>>
>> Well, this depends if the experimental features are used as such. I'll
>> add a warning in case they get actually used.
>>
>>> So here we just limit the csum print to the utilized csum size.
>>>
>>> Now the output looks like:
>>>
>>>    btrfs-progs v5.17
>>>    chunk tree
>>>    leaf 22036480 items 4 free space 15781 generation 6 owner CHUNK_TREE
>>>    leaf 22036480 flags 0x1(WRITTEN) backref revision 1
>>>    checksum stored 676b812f
>>>    checksum calced 676b812f
>>>    fs uuid d11f8799-b6dc-415d-b1ed-cebe6da5f0b7
>>>
>>> Fixes: 1bb6fb896dfc ("btrfs-progs: btrfstune: experimental, new
>>> option to switch csums")
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>
>> Added to devel, thanks.

Re: [PATCH] btrfs-progs: print-tree: print the checksum of header without tailing zeros

[David Sterba]

On Tue, May 10, 2022 at 08:10:22PM +0800, Qu Wenruo wrote:
> On 2022/5/10 20:02, Qu Wenruo wrote:
> > On 2022/5/10 19:48, David Sterba wrote:
> >> On Tue, May 10, 2022 at 02:03:18PM +0800, Qu Wenruo wrote:
> >>> For the default CRC32 checksum, print-tree now prints tons of
> >>> unnecessary padding zeros:
> >>>
> >>>    btrfs-progs v5.17
> >>>    chunk tree
> >>>    leaf 22036480 items 7 free space 15430 generation 6 owner CHUNK_TREE
> >>>    leaf 22036480 flags 0x1(WRITTEN) backref revision 1
> >>>    checksum stored
> >>> 0ac1b9fa00000000000000000000000000000000000000000000000000000000
> >>>    checksum calced
> >>> 0ac1b9fa00000000000000000000000000000000000000000000000000000000
> >>>    fs uuid 3d95b7e3-3ab6-4927-af56-c58aa634342e
> >>>
> >>> This is caused by commit 1bb6fb896dfc ("btrfs-progs: btrfstune:
> >>> experimental, new option to switch csums"), and it looks like most
> >>> distros just enable EXPERIMENTAL features by default.
> >>
> >> Which distros?
> >
> > Archlinux.
> >
> > Their PKGBUILD can be found here:
> > https://github.com/archlinux/svntogit-packages/blob/packages/btrfs-progs/trunk/PKGBUILD
> >
> >
> > Which doesn't enable experimental explicitly, but still got the full
> > csum output.
> 
> OK, got the reason why the EXPERIMENTAL thing doesn't work as expected.
> 
> In configure, we set $EXPERIMENTAL=0 by default, then
> add the line into confdefs.h:
> 
> #define EXPERIMENTAL 0
> 
> However in print-tree.c, we use
> 
> #ifdef EXPERIMENTAL
> 
> Then it always get enabled, no matter if it's 0 or 1.
> 
> We should either don't define it at all, and use the "#ifdef" way, or we
> should go "#if EXPERIMENTAL" instead.

Oh you're right, #ifdef is wrong, it's supposed to be either
"if (EXPERIMENTAL)" or "#if". My bad, I'll fix it.

Re: [PATCH] btrfs-progs: print-tree: print the checksum of header without tailing zeros

[Qu Wenruo]

On 2022/5/10 20:19, David Sterba wrote:
> On Tue, May 10, 2022 at 08:10:22PM +0800, Qu Wenruo wrote:
>> On 2022/5/10 20:02, Qu Wenruo wrote:
>>> On 2022/5/10 19:48, David Sterba wrote:
>>>> On Tue, May 10, 2022 at 02:03:18PM +0800, Qu Wenruo wrote:
>>>>> For the default CRC32 checksum, print-tree now prints tons of
>>>>> unnecessary padding zeros:
>>>>>
>>>>>     btrfs-progs v5.17
>>>>>     chunk tree
>>>>>     leaf 22036480 items 7 free space 15430 generation 6 owner CHUNK_TREE
>>>>>     leaf 22036480 flags 0x1(WRITTEN) backref revision 1
>>>>>     checksum stored
>>>>> 0ac1b9fa00000000000000000000000000000000000000000000000000000000
>>>>>     checksum calced
>>>>> 0ac1b9fa00000000000000000000000000000000000000000000000000000000
>>>>>     fs uuid 3d95b7e3-3ab6-4927-af56-c58aa634342e
>>>>>
>>>>> This is caused by commit 1bb6fb896dfc ("btrfs-progs: btrfstune:
>>>>> experimental, new option to switch csums"), and it looks like most
>>>>> distros just enable EXPERIMENTAL features by default.
>>>>
>>>> Which distros?
>>>
>>> Archlinux.
>>>
>>> Their PKGBUILD can be found here:
>>> https://github.com/archlinux/svntogit-packages/blob/packages/btrfs-progs/trunk/PKGBUILD
>>>
>>>
>>> Which doesn't enable experimental explicitly, but still got the full
>>> csum output.
>>
>> OK, got the reason why the EXPERIMENTAL thing doesn't work as expected.
>>
>> In configure, we set $EXPERIMENTAL=0 by default, then
>> add the line into confdefs.h:
>>
>> #define EXPERIMENTAL 0
>>
>> However in print-tree.c, we use
>>
>> #ifdef EXPERIMENTAL
>>
>> Then it always get enabled, no matter if it's 0 or 1.
>>
>> We should either don't define it at all, and use the "#ifdef" way, or we
>> should go "#if EXPERIMENTAL" instead.
>
> Oh you're right, #ifdef is wrong, it's supposed to be either
> "if (EXPERIMENTAL)" or "#if". My bad, I'll fix it.

Although personally speaking, I'm pretty happy to enable experimental
features for everyone, even just by an accident.

Thanks,
Qu

Re: [PATCH] btrfs-progs: print-tree: print the checksum of header without tailing zeros

[David Sterba]

On Tue, May 10, 2022 at 08:24:33PM +0800, Qu Wenruo wrote:
> >>
> >> In configure, we set $EXPERIMENTAL=0 by default, then
> >> add the line into confdefs.h:
> >>
> >> #define EXPERIMENTAL 0
> >>
> >> However in print-tree.c, we use
> >>
> >> #ifdef EXPERIMENTAL
> >>
> >> Then it always get enabled, no matter if it's 0 or 1.
> >>
> >> We should either don't define it at all, and use the "#ifdef" way, or we
> >> should go "#if EXPERIMENTAL" instead.
> >
> > Oh you're right, #ifdef is wrong, it's supposed to be either
> > "if (EXPERIMENTAL)" or "#if". My bad, I'll fix it.
> 
> Although personally speaking, I'm pretty happy to enable experimental
> features for everyone, even just by an accident.

I understand that for you it's just another feature and you'll get
informed how to use it or what to avoid, or eventually fix or report
bugs you find. But this can't be expected from a common user and the
experimental status means basically anything from serious bugs to
incomplete functionality. Incremental development is the model here and
the build-time option is a shield to avoid anything that might harm
users.

Once a feature is relatively complete and well tested, then it's time to
promote it. At this point the checksum switch is closest to that point.
I don't remember any remaining bugs after you've fixed the enumeration
of extents for the --init-csum-tree, so we can hammer it a bit more and
then drop the status.