| Message ID | 38bf1bf79e8443d570e982edb8a6b71f27cf1ab5.1652162441.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: print-tree: print the checksum of header without tailing zeros | expand |
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
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.
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.
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.
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.
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
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.
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
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(-)