| Message ID | 20171108005426.17903-2-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 8.11.2017 02:54, Qu Wenruo wrote: > [BUG] > If we run btrfs with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y, it will > instantly cause kernel panic like: > > ------ > ... > assertion failed: 0, file: fs/btrfs/disk-io.c, line: 3853 > ... > Call Trace: > btrfs_mark_buffer_dirty+0x187/0x1f0 [btrfs] > setup_items_for_insert+0x385/0x650 [btrfs] > __btrfs_drop_extents+0x129a/0x1870 [btrfs] > ... > ----- > > [Cause] > Btrfs will call btrfs_check_leaf() in btrfs_mark_buffer_dirty() to check > if the leaf is valid with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y. > > However quite some btrfs_mark_buffer_dirty() callers(*) don't really > initialize its item data but only initialize its item pointers, leaving > item data uninitialized. > > This makes tree-checker catch uninitialized data as error, causing > such panic. > > *: These callers include but not limited to > setup_items_for_insert() > btrfs_split_item() > btrfs_expand_item() > > [Fix] > Add a new parameter @check_item_data to btrfs_check_leaf(). > With @check_item_data set to false, item data check will be skipped and > fallback to old btrfs_check_leaf() behavior. > > So we can still get early warning if we screw up item pointers, and > avoid false panic. > > Cc: Filipe Manana <fdmanana@gmail.com> > Reported-by: Lakshmipathi.G <lakshmipathi.g@gmail.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/disk-io.c | 10 ++++++++-- > fs/btrfs/tree-checker.c | 27 ++++++++++++++++++++++----- > fs/btrfs/tree-checker.h | 14 +++++++++++++- > 3 files changed, 43 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index efce9a2fa9be..10a2a579cc7f 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -610,7 +610,7 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio, > * that we don't try and read the other copies of this block, just > * return -EIO. > */ > - if (found_level == 0 && btrfs_check_leaf(root, eb)) { > + if (found_level == 0 && btrfs_check_leaf_full(root, eb)) { > set_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags); > ret = -EIO; > } > @@ -3848,7 +3848,13 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf) > buf->len, > fs_info->dirty_metadata_batch); > #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY > - if (btrfs_header_level(buf) == 0 && btrfs_check_leaf(root, buf)) { > + /* > + * Since btrfs_mark_buffer_dirty() can be called with item pointer set > + * but item data not updated. > + * So here we should only check item pointers, not item data. > + */ > + if (btrfs_header_level(buf) == 0 && > + btrfs_check_leaf_relaxed(root, buf)) { > btrfs_print_leaf(buf); > ASSERT(0); > } > diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c > index 114fc5f0ecc5..ce4ed6ec8f39 100644 > --- a/fs/btrfs/tree-checker.c > +++ b/fs/btrfs/tree-checker.c > @@ -242,7 +242,8 @@ static int check_leaf_item(struct btrfs_root *root, > return ret; > } > > -int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf) > +static int check_leaf(struct btrfs_root *root, struct extent_buffer *leaf, > + bool check_item_data) > { > struct btrfs_fs_info *fs_info = root->fs_info; > /* No valid key type is 0, so all key should be larger than this key */ > @@ -361,10 +362,15 @@ int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf) > return -EUCLEAN; > } > > - /* Check if the item size and content meet other criteria */ > - ret = check_leaf_item(root, leaf, &key, slot); > - if (ret < 0) > - return ret; > + if (check_item_data) { > + /* > + * Check if the item size and content meet other > + * criteria > + */ > + ret = check_leaf_item(root, leaf, &key, slot); > + if (ret < 0) > + return ret; > + } > > prev_key.objectid = key.objectid; > prev_key.type = key.type; > @@ -374,6 +380,17 @@ int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf) > return 0; > } > > +int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf) > +{ > + return check_leaf(root, leaf, true); > +} > + > +int btrfs_check_leaf_relaxed(struct btrfs_root *root, > + struct extent_buffer *leaf) > +{ > + return check_leaf(root, leaf, false); > +} Presumably the compiler will figure it out but such trivial function are usually defined straight into the header file so that the compiler inlines them. Can you check if you have separate objects for btrfs_check_leaf_relaxed with the way you've written the patch or whether all instances have been inlined? If they are not, then move those function definition into tree-checker.h and make them 'static inline' as per the style of the whole kernel > + > int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node) > { > unsigned long nr = btrfs_header_nritems(node); > diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h > index 96c486e95d70..3d53e8d6fda0 100644 > --- a/fs/btrfs/tree-checker.h > +++ b/fs/btrfs/tree-checker.h > @@ -20,7 +20,19 @@ > #include "ctree.h" > #include "extent_io.h" > > -int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf); > +/* > + * Comprehensive leaf checker. > + * Will check not only the item pointers, but also every possible member > + * in item data. > + */ > +int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf); > + > +/* > + * Less strict leaf checker. > + * Will only check item pointers, not reading item data. > + */ > +int btrfs_check_leaf_relaxed(struct btrfs_root *root, > + struct extent_buffer *leaf); > int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node); > > #endif > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017年11月08日 15:55, Nikolay Borisov wrote: > > > On 8.11.2017 02:54, Qu Wenruo wrote: >> [BUG] >> If we run btrfs with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y, it will >> instantly cause kernel panic like: >> >> ------ >> ... >> assertion failed: 0, file: fs/btrfs/disk-io.c, line: 3853 >> ... >> Call Trace: >> btrfs_mark_buffer_dirty+0x187/0x1f0 [btrfs] >> setup_items_for_insert+0x385/0x650 [btrfs] >> __btrfs_drop_extents+0x129a/0x1870 [btrfs] >> ... >> ----- >> >> [Cause] >> Btrfs will call btrfs_check_leaf() in btrfs_mark_buffer_dirty() to check >> if the leaf is valid with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y. >> >> However quite some btrfs_mark_buffer_dirty() callers(*) don't really >> initialize its item data but only initialize its item pointers, leaving >> item data uninitialized. >> >> This makes tree-checker catch uninitialized data as error, causing >> such panic. >> >> *: These callers include but not limited to >> setup_items_for_insert() >> btrfs_split_item() >> btrfs_expand_item() >> >> [Fix] >> Add a new parameter @check_item_data to btrfs_check_leaf(). >> With @check_item_data set to false, item data check will be skipped and >> fallback to old btrfs_check_leaf() behavior. >> >> So we can still get early warning if we screw up item pointers, and >> avoid false panic. >> >> Cc: Filipe Manana <fdmanana@gmail.com> >> Reported-by: Lakshmipathi.G <lakshmipathi.g@gmail.com> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/disk-io.c | 10 ++++++++-- >> fs/btrfs/tree-checker.c | 27 ++++++++++++++++++++++----- >> fs/btrfs/tree-checker.h | 14 +++++++++++++- >> 3 files changed, 43 insertions(+), 8 deletions(-) >> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >> index efce9a2fa9be..10a2a579cc7f 100644 >> --- a/fs/btrfs/disk-io.c >> +++ b/fs/btrfs/disk-io.c >> @@ -610,7 +610,7 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio, >> * that we don't try and read the other copies of this block, just >> * return -EIO. >> */ >> - if (found_level == 0 && btrfs_check_leaf(root, eb)) { >> + if (found_level == 0 && btrfs_check_leaf_full(root, eb)) { >> set_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags); >> ret = -EIO; >> } >> @@ -3848,7 +3848,13 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf) >> buf->len, >> fs_info->dirty_metadata_batch); >> #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY >> - if (btrfs_header_level(buf) == 0 && btrfs_check_leaf(root, buf)) { >> + /* >> + * Since btrfs_mark_buffer_dirty() can be called with item pointer set >> + * but item data not updated. >> + * So here we should only check item pointers, not item data. >> + */ >> + if (btrfs_header_level(buf) == 0 && >> + btrfs_check_leaf_relaxed(root, buf)) { >> btrfs_print_leaf(buf); >> ASSERT(0); >> } >> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c >> index 114fc5f0ecc5..ce4ed6ec8f39 100644 >> --- a/fs/btrfs/tree-checker.c >> +++ b/fs/btrfs/tree-checker.c >> @@ -242,7 +242,8 @@ static int check_leaf_item(struct btrfs_root *root, >> return ret; >> } >> >> -int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf) >> +static int check_leaf(struct btrfs_root *root, struct extent_buffer *leaf, >> + bool check_item_data) >> { >> struct btrfs_fs_info *fs_info = root->fs_info; >> /* No valid key type is 0, so all key should be larger than this key */ >> @@ -361,10 +362,15 @@ int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf) >> return -EUCLEAN; >> } >> >> - /* Check if the item size and content meet other criteria */ >> - ret = check_leaf_item(root, leaf, &key, slot); >> - if (ret < 0) >> - return ret; >> + if (check_item_data) { >> + /* >> + * Check if the item size and content meet other >> + * criteria >> + */ >> + ret = check_leaf_item(root, leaf, &key, slot); >> + if (ret < 0) >> + return ret; >> + } >> >> prev_key.objectid = key.objectid; >> prev_key.type = key.type; >> @@ -374,6 +380,17 @@ int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf) >> return 0; >> } >> >> +int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf) >> +{ >> + return check_leaf(root, leaf, true); >> +} >> + >> +int btrfs_check_leaf_relaxed(struct btrfs_root *root, >> + struct extent_buffer *leaf) >> +{ >> + return check_leaf(root, leaf, false); >> +} > > Presumably the compiler will figure it out but such trivial function are > usually defined straight into the header file so that the compiler > inlines them. In that case, the function check_leaf() must be exported, so that we can inline it in header. But exporting check_leaf() increases the possibility for caller to use it incorrectly, so I prefer no to export any internal used function. And compiler may or may not inline check_leaf() into btrfs_check_leaf_relaxed() function, but it doesn't matter. If optimization can be done by compiler, then let compiler to do it. What we should do is to ensure the abstraction/interface design is good enough, other than doing possible "over-optimization". Thanks, Qu > Can you check if you have separate objects for > btrfs_check_leaf_relaxed with the way you've written the patch or > whether all instances have been inlined? If they are not, then move > those function definition into tree-checker.h and make them 'static > inline' as per the style of the whole kernel > >> + >> int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node) >> { >> unsigned long nr = btrfs_header_nritems(node); >> diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h >> index 96c486e95d70..3d53e8d6fda0 100644 >> --- a/fs/btrfs/tree-checker.h >> +++ b/fs/btrfs/tree-checker.h >> @@ -20,7 +20,19 @@ >> #include "ctree.h" >> #include "extent_io.h" >> >> -int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf); >> +/* >> + * Comprehensive leaf checker. >> + * Will check not only the item pointers, but also every possible member >> + * in item data. >> + */ >> +int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf); >> + >> +/* >> + * Less strict leaf checker. >> + * Will only check item pointers, not reading item data. >> + */ >> +int btrfs_check_leaf_relaxed(struct btrfs_root *root, >> + struct extent_buffer *leaf); >> int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node); >> >> #endif >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Wed, Nov 08, 2017 at 04:03:35PM +0800, Qu Wenruo wrote: > >> +int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf) > >> +{ > >> + return check_leaf(root, leaf, true); > >> +} > >> + > >> +int btrfs_check_leaf_relaxed(struct btrfs_root *root, > >> + struct extent_buffer *leaf) > >> +{ > >> + return check_leaf(root, leaf, false); > >> +} > > > > Presumably the compiler will figure it out but such trivial function are > > usually defined straight into the header file so that the compiler > > inlines them. > > In that case, the function check_leaf() must be exported, so that we can > inline it in header. > > But exporting check_leaf() increases the possibility for caller to use > it incorrectly, so I prefer no to export any internal used function. > > > And compiler may or may not inline check_leaf() into > btrfs_check_leaf_relaxed() function, but it doesn't matter. > > If optimization can be done by compiler, then let compiler to do it. > > What we should do is to ensure the abstraction/interface design is good > enough, other than doing possible "over-optimization". Though my original idea was closer to what Nikolay says, I'm fine with the way you've actually implement it. We don't need the extended check_leaf version that's hidden in the .c file. The function inlining possibilities will be limited, but this is not a performance critical code where the effects of inlining could be observale in practice (I think, no numbers to back that). -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 08, 2017 at 08:54:24AM +0800, Qu Wenruo wrote: > [BUG] > If we run btrfs with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y, it will > instantly cause kernel panic like: > > ------ > ... > assertion failed: 0, file: fs/btrfs/disk-io.c, line: 3853 > ... > Call Trace: > btrfs_mark_buffer_dirty+0x187/0x1f0 [btrfs] > setup_items_for_insert+0x385/0x650 [btrfs] > __btrfs_drop_extents+0x129a/0x1870 [btrfs] > ... > ----- > > [Cause] > Btrfs will call btrfs_check_leaf() in btrfs_mark_buffer_dirty() to check > if the leaf is valid with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y. > > However quite some btrfs_mark_buffer_dirty() callers(*) don't really > initialize its item data but only initialize its item pointers, leaving > item data uninitialized. > > This makes tree-checker catch uninitialized data as error, causing > such panic. > > *: These callers include but not limited to > setup_items_for_insert() > btrfs_split_item() > btrfs_expand_item() > > [Fix] > Add a new parameter @check_item_data to btrfs_check_leaf(). > With @check_item_data set to false, item data check will be skipped and > fallback to old btrfs_check_leaf() behavior. > > So we can still get early warning if we screw up item pointers, and > avoid false panic. > > Cc: Filipe Manana <fdmanana@gmail.com> > Reported-by: Lakshmipathi.G <lakshmipathi.g@gmail.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 08, 2017 at 08:54:24AM +0800, Qu Wenruo wrote: > [BUG] > If we run btrfs with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y, it will > instantly cause kernel panic like: > > ------ > ... > assertion failed: 0, file: fs/btrfs/disk-io.c, line: 3853 > ... > Call Trace: > btrfs_mark_buffer_dirty+0x187/0x1f0 [btrfs] > setup_items_for_insert+0x385/0x650 [btrfs] > __btrfs_drop_extents+0x129a/0x1870 [btrfs] > ... > ----- > > [Cause] > Btrfs will call btrfs_check_leaf() in btrfs_mark_buffer_dirty() to check > if the leaf is valid with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y. > > However quite some btrfs_mark_buffer_dirty() callers(*) don't really > initialize its item data but only initialize its item pointers, leaving > item data uninitialized. > > This makes tree-checker catch uninitialized data as error, causing > such panic. > > *: These callers include but not limited to > setup_items_for_insert() > btrfs_split_item() > btrfs_expand_item() > > [Fix] > Add a new parameter @check_item_data to btrfs_check_leaf(). > With @check_item_data set to false, item data check will be skipped and > fallback to old btrfs_check_leaf() behavior. > > So we can still get early warning if we screw up item pointers, and > avoid false panic. > Reviewed-by: Liu Bo <bo.li.liu@oracle.com> Thanks, -liubo > Cc: Filipe Manana <fdmanana@gmail.com> > Reported-by: Lakshmipathi.G <lakshmipathi.g@gmail.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/disk-io.c | 10 ++++++++-- > fs/btrfs/tree-checker.c | 27 ++++++++++++++++++++++----- > fs/btrfs/tree-checker.h | 14 +++++++++++++- > 3 files changed, 43 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index efce9a2fa9be..10a2a579cc7f 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -610,7 +610,7 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio, > * that we don't try and read the other copies of this block, just > * return -EIO. > */ > - if (found_level == 0 && btrfs_check_leaf(root, eb)) { > + if (found_level == 0 && btrfs_check_leaf_full(root, eb)) { > set_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags); > ret = -EIO; > } > @@ -3848,7 +3848,13 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf) > buf->len, > fs_info->dirty_metadata_batch); > #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY > - if (btrfs_header_level(buf) == 0 && btrfs_check_leaf(root, buf)) { > + /* > + * Since btrfs_mark_buffer_dirty() can be called with item pointer set > + * but item data not updated. > + * So here we should only check item pointers, not item data. > + */ > + if (btrfs_header_level(buf) == 0 && > + btrfs_check_leaf_relaxed(root, buf)) { > btrfs_print_leaf(buf); > ASSERT(0); > } > diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c > index 114fc5f0ecc5..ce4ed6ec8f39 100644 > --- a/fs/btrfs/tree-checker.c > +++ b/fs/btrfs/tree-checker.c > @@ -242,7 +242,8 @@ static int check_leaf_item(struct btrfs_root *root, > return ret; > } > > -int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf) > +static int check_leaf(struct btrfs_root *root, struct extent_buffer *leaf, > + bool check_item_data) > { > struct btrfs_fs_info *fs_info = root->fs_info; > /* No valid key type is 0, so all key should be larger than this key */ > @@ -361,10 +362,15 @@ int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf) > return -EUCLEAN; > } > > - /* Check if the item size and content meet other criteria */ > - ret = check_leaf_item(root, leaf, &key, slot); > - if (ret < 0) > - return ret; > + if (check_item_data) { > + /* > + * Check if the item size and content meet other > + * criteria > + */ > + ret = check_leaf_item(root, leaf, &key, slot); > + if (ret < 0) > + return ret; > + } > > prev_key.objectid = key.objectid; > prev_key.type = key.type; > @@ -374,6 +380,17 @@ int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf) > return 0; > } > > +int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf) > +{ > + return check_leaf(root, leaf, true); > +} > + > +int btrfs_check_leaf_relaxed(struct btrfs_root *root, > + struct extent_buffer *leaf) > +{ > + return check_leaf(root, leaf, false); > +} > + > int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node) > { > unsigned long nr = btrfs_header_nritems(node); > diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h > index 96c486e95d70..3d53e8d6fda0 100644 > --- a/fs/btrfs/tree-checker.h > +++ b/fs/btrfs/tree-checker.h > @@ -20,7 +20,19 @@ > #include "ctree.h" > #include "extent_io.h" > > -int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf); > +/* > + * Comprehensive leaf checker. > + * Will check not only the item pointers, but also every possible member > + * in item data. > + */ > +int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf); > + > +/* > + * Less strict leaf checker. > + * Will only check item pointers, not reading item data. > + */ > +int btrfs_check_leaf_relaxed(struct btrfs_root *root, > + struct extent_buffer *leaf); > int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node); > > #endif > -- > 2.15.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index efce9a2fa9be..10a2a579cc7f 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -610,7 +610,7 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio, * that we don't try and read the other copies of this block, just * return -EIO. */ - if (found_level == 0 && btrfs_check_leaf(root, eb)) { + if (found_level == 0 && btrfs_check_leaf_full(root, eb)) { set_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags); ret = -EIO; } @@ -3848,7 +3848,13 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf) buf->len, fs_info->dirty_metadata_batch); #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY - if (btrfs_header_level(buf) == 0 && btrfs_check_leaf(root, buf)) { + /* + * Since btrfs_mark_buffer_dirty() can be called with item pointer set + * but item data not updated. + * So here we should only check item pointers, not item data. + */ + if (btrfs_header_level(buf) == 0 && + btrfs_check_leaf_relaxed(root, buf)) { btrfs_print_leaf(buf); ASSERT(0); } diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c index 114fc5f0ecc5..ce4ed6ec8f39 100644 --- a/fs/btrfs/tree-checker.c +++ b/fs/btrfs/tree-checker.c @@ -242,7 +242,8 @@ static int check_leaf_item(struct btrfs_root *root, return ret; } -int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf) +static int check_leaf(struct btrfs_root *root, struct extent_buffer *leaf, + bool check_item_data) { struct btrfs_fs_info *fs_info = root->fs_info; /* No valid key type is 0, so all key should be larger than this key */ @@ -361,10 +362,15 @@ int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf) return -EUCLEAN; } - /* Check if the item size and content meet other criteria */ - ret = check_leaf_item(root, leaf, &key, slot); - if (ret < 0) - return ret; + if (check_item_data) { + /* + * Check if the item size and content meet other + * criteria + */ + ret = check_leaf_item(root, leaf, &key, slot); + if (ret < 0) + return ret; + } prev_key.objectid = key.objectid; prev_key.type = key.type; @@ -374,6 +380,17 @@ int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf) return 0; } +int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf) +{ + return check_leaf(root, leaf, true); +} + +int btrfs_check_leaf_relaxed(struct btrfs_root *root, + struct extent_buffer *leaf) +{ + return check_leaf(root, leaf, false); +} + int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node) { unsigned long nr = btrfs_header_nritems(node); diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h index 96c486e95d70..3d53e8d6fda0 100644 --- a/fs/btrfs/tree-checker.h +++ b/fs/btrfs/tree-checker.h @@ -20,7 +20,19 @@ #include "ctree.h" #include "extent_io.h" -int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf); +/* + * Comprehensive leaf checker. + * Will check not only the item pointers, but also every possible member + * in item data. + */ +int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf); + +/* + * Less strict leaf checker. + * Will only check item pointers, not reading item data. + */ +int btrfs_check_leaf_relaxed(struct btrfs_root *root, + struct extent_buffer *leaf); int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node); #endif
[BUG] If we run btrfs with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y, it will instantly cause kernel panic like: ------ ... assertion failed: 0, file: fs/btrfs/disk-io.c, line: 3853 ... Call Trace: btrfs_mark_buffer_dirty+0x187/0x1f0 [btrfs] setup_items_for_insert+0x385/0x650 [btrfs] __btrfs_drop_extents+0x129a/0x1870 [btrfs] ... ----- [Cause] Btrfs will call btrfs_check_leaf() in btrfs_mark_buffer_dirty() to check if the leaf is valid with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y. However quite some btrfs_mark_buffer_dirty() callers(*) don't really initialize its item data but only initialize its item pointers, leaving item data uninitialized. This makes tree-checker catch uninitialized data as error, causing such panic. *: These callers include but not limited to setup_items_for_insert() btrfs_split_item() btrfs_expand_item() [Fix] Add a new parameter @check_item_data to btrfs_check_leaf(). With @check_item_data set to false, item data check will be skipped and fallback to old btrfs_check_leaf() behavior. So we can still get early warning if we screw up item pointers, and avoid false panic. Cc: Filipe Manana <fdmanana@gmail.com> Reported-by: Lakshmipathi.G <lakshmipathi.g@gmail.com> Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/disk-io.c | 10 ++++++++-- fs/btrfs/tree-checker.c | 27 ++++++++++++++++++++++----- fs/btrfs/tree-checker.h | 14 +++++++++++++- 3 files changed, 43 insertions(+), 8 deletions(-)