| Message ID | a5ab0e98ae40df23b3bb65235f7bd9296e3b0be4.1705027543.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: tree-checker: dump the tree block when hitting an error | expand |
On Fri, Jan 12, 2024 at 01:16:20PM +1030, Qu Wenruo wrote: > Unlike kernel where tree-checker would provide enough info so later we > can use "btrfs inspect dump-tree" to catch the offending tree block, in > progs we may not even have a btrfs to start "btrfs inspect dump-tree". > E.g during btrfs-convert. > > To make later debug easier, let's call btrfs_print_tree() for every > error we hit inside tree-checker. > > Signed-off-by: Qu Wenruo <wqu@suse.com> Added to devel, thanks. > --- > kernel-shared/tree-checker.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/kernel-shared/tree-checker.c b/kernel-shared/tree-checker.c > index 003156795a43..a98553985402 100644 > --- a/kernel-shared/tree-checker.c > +++ b/kernel-shared/tree-checker.c > @@ -33,6 +33,7 @@ > #include "kernel-shared/accessors.h" > #include "kernel-shared/file-item.h" > #include "kernel-shared/extent_io.h" > +#include "kernel-shared/print-tree.h" > #include "kernel-shared/uapi/btrfs.h" > #include "kernel-shared/uapi/btrfs_tree.h" > #include "common/internal.h" > @@ -95,6 +96,8 @@ static void generic_err(const struct extent_buffer *eb, int slot, > btrfs_header_level(eb) == 0 ? "leaf" : "node", > btrfs_header_owner(eb), btrfs_header_bytenr(eb), slot, &vaf); > va_end(args); > + > + btrfs_print_tree((struct extent_buffer *)eb, 0); Printing the eb should not require writable eb, but there are many functions that would need to be converted to 'const' so the cas is OK for now but cleaning that up would be welcome.
On 2024/1/13 02:06, David Sterba wrote: > On Fri, Jan 12, 2024 at 01:16:20PM +1030, Qu Wenruo wrote: >> Unlike kernel where tree-checker would provide enough info so later we >> can use "btrfs inspect dump-tree" to catch the offending tree block, in >> progs we may not even have a btrfs to start "btrfs inspect dump-tree". >> E.g during btrfs-convert. >> >> To make later debug easier, let's call btrfs_print_tree() for every >> error we hit inside tree-checker. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > Added to devel, thanks. > >> --- >> kernel-shared/tree-checker.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/kernel-shared/tree-checker.c b/kernel-shared/tree-checker.c >> index 003156795a43..a98553985402 100644 >> --- a/kernel-shared/tree-checker.c >> +++ b/kernel-shared/tree-checker.c >> @@ -33,6 +33,7 @@ >> #include "kernel-shared/accessors.h" >> #include "kernel-shared/file-item.h" >> #include "kernel-shared/extent_io.h" >> +#include "kernel-shared/print-tree.h" >> #include "kernel-shared/uapi/btrfs.h" >> #include "kernel-shared/uapi/btrfs_tree.h" >> #include "common/internal.h" >> @@ -95,6 +96,8 @@ static void generic_err(const struct extent_buffer *eb, int slot, >> btrfs_header_level(eb) == 0 ? "leaf" : "node", >> btrfs_header_owner(eb), btrfs_header_bytenr(eb), slot, &vaf); >> va_end(args); >> + >> + btrfs_print_tree((struct extent_buffer *)eb, 0); > > Printing the eb should not require writable eb, but there are many > functions that would need to be converted to 'const' so the cas is OK > for now but cleaning that up would be welcome. I tried but failed. Most of the call sites are fine to be constified, but there is a special trap inside bfs_print_children(), where we call extent_buffer_get(), which can never be called on a const eb pointer. Thus that's why we are still using the ugly forced convert here. Thanks, Qu >
On Sat, Jan 13, 2024 at 07:03:18AM +1030, Qu Wenruo wrote: > >> + btrfs_print_tree((struct extent_buffer *)eb, 0); > > > > Printing the eb should not require writable eb, but there are many > > functions that would need to be converted to 'const' so the cas is OK > > for now but cleaning that up would be welcome. > > I tried but failed. > > Most of the call sites are fine to be constified, but there is a special > trap inside bfs_print_children(), where we call extent_buffer_get(), > which can never be called on a const eb pointer. Oh, I see. We can't remove the ref update but what if we reset the path slot to NULL before releasing it?
On 2024/1/16 01:24, David Sterba wrote: > On Sat, Jan 13, 2024 at 07:03:18AM +1030, Qu Wenruo wrote: >>>> + btrfs_print_tree((struct extent_buffer *)eb, 0); >>> >>> Printing the eb should not require writable eb, but there are many >>> functions that would need to be converted to 'const' so the cas is OK >>> for now but cleaning that up would be welcome. >> >> I tried but failed. >> >> Most of the call sites are fine to be constified, but there is a special >> trap inside bfs_print_children(), where we call extent_buffer_get(), >> which can never be called on a const eb pointer. > > Oh, I see. We can't remove the ref update but what if we reset the path > slot to NULL before releasing it? The other solution would be, only consitfy btrfs_print_node() and btrfs_print_leaf(). For anything that would need to traversal the tree, still allow them to modify the eb. I'll give it a try, and if it passed compile, that would be my next step. Would this be a good idea? Thanks, Qu
On Tue, Jan 16, 2024 at 09:13:23AM +1030, Qu Wenruo wrote: > > > On 2024/1/16 01:24, David Sterba wrote: > > On Sat, Jan 13, 2024 at 07:03:18AM +1030, Qu Wenruo wrote: > >>>> + btrfs_print_tree((struct extent_buffer *)eb, 0); > >>> > >>> Printing the eb should not require writable eb, but there are many > >>> functions that would need to be converted to 'const' so the cas is OK > >>> for now but cleaning that up would be welcome. > >> > >> I tried but failed. > >> > >> Most of the call sites are fine to be constified, but there is a special > >> trap inside bfs_print_children(), where we call extent_buffer_get(), > >> which can never be called on a const eb pointer. > > > > Oh, I see. We can't remove the ref update but what if we reset the path > > slot to NULL before releasing it? > > The other solution would be, only consitfy btrfs_print_node() and > btrfs_print_leaf(). > > For anything that would need to traversal the tree, still allow them to > modify the eb. > > I'll give it a try, and if it passed compile, that would be my next step. > Would this be a good idea? Yeah makes sense, we'll se how much can it can be cleaned up. The const parameters are nice to have as they don't really break anything, but should be used as a matter of good coding practices.
diff --git a/kernel-shared/tree-checker.c b/kernel-shared/tree-checker.c index 003156795a43..a98553985402 100644 --- a/kernel-shared/tree-checker.c +++ b/kernel-shared/tree-checker.c @@ -33,6 +33,7 @@ #include "kernel-shared/accessors.h" #include "kernel-shared/file-item.h" #include "kernel-shared/extent_io.h" +#include "kernel-shared/print-tree.h" #include "kernel-shared/uapi/btrfs.h" #include "kernel-shared/uapi/btrfs_tree.h" #include "common/internal.h" @@ -95,6 +96,8 @@ static void generic_err(const struct extent_buffer *eb, int slot, btrfs_header_level(eb) == 0 ? "leaf" : "node", btrfs_header_owner(eb), btrfs_header_bytenr(eb), slot, &vaf); va_end(args); + + btrfs_print_tree((struct extent_buffer *)eb, 0); } /* @@ -123,6 +126,8 @@ static void file_extent_err(const struct extent_buffer *eb, int slot, btrfs_header_owner(eb), btrfs_header_bytenr(eb), slot, key.objectid, key.offset, &vaf); va_end(args); + + btrfs_print_tree((struct extent_buffer *)eb, 0); } /* @@ -183,6 +188,8 @@ static void dir_item_err(const struct extent_buffer *eb, int slot, btrfs_header_owner(eb), btrfs_header_bytenr(eb), slot, key.objectid, &vaf); va_end(args); + + btrfs_print_tree((struct extent_buffer *)eb, 0); } /* @@ -669,6 +676,8 @@ static void block_group_err(const struct extent_buffer *eb, int slot, btrfs_header_owner(eb), btrfs_header_bytenr(eb), slot, key.objectid, key.offset, &vaf); va_end(args); + + btrfs_print_tree((struct extent_buffer *)eb, 0); } static int check_block_group_item(struct extent_buffer *leaf, @@ -800,6 +809,8 @@ static void chunk_err(const struct extent_buffer *leaf, BTRFS_CHUNK_TREE_OBJECTID, leaf->start, slot, logical, &vaf); va_end(args); + + btrfs_print_tree((struct extent_buffer *)leaf, 0); } /* @@ -1025,6 +1036,8 @@ static void dev_item_err(const struct extent_buffer *eb, int slot, btrfs_header_owner(eb), btrfs_header_bytenr(eb), slot, key.objectid, &vaf); va_end(args); + + btrfs_print_tree((struct extent_buffer *)eb, 0); } static int check_dev_item(struct extent_buffer *leaf, @@ -1279,6 +1292,8 @@ static void extent_err(const struct extent_buffer *eb, int slot, btrfs_header_level(eb) == 0 ? "leaf" : "node", eb->start, slot, bytenr, len, &vaf); va_end(args); + + btrfs_print_tree((struct extent_buffer *)eb, 0); } static int check_extent_item(struct extent_buffer *leaf,
Unlike kernel where tree-checker would provide enough info so later we can use "btrfs inspect dump-tree" to catch the offending tree block, in progs we may not even have a btrfs to start "btrfs inspect dump-tree". E.g during btrfs-convert. To make later debug easier, let's call btrfs_print_tree() for every error we hit inside tree-checker. Signed-off-by: Qu Wenruo <wqu@suse.com> --- kernel-shared/tree-checker.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)