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 <fdman...@gmail.com>
>> Reported-by: Lakshmipathi.G <lakshmipath...@gmail.com>
>> Signed-off-by: Qu Wenruo <w...@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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
signature.asc
Description: OpenPGP digital signature
