btrfs: Move leaf verification to correct timing to avoid false panic for sanity test

PATCH In review Qu Wenruo 4 participants 4 messages Nov 2, 2017 – Nov 3, 2017 Btrfs file system

lore

Share & tools

lore

Qu Wenruo

author

2017-11-02 07:04 UTC #0 lore raw

PATCH 1/1 btrfs: Move leaf verification to correct timing to avoid false panic for sanity test

[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 some btrfs_mark_buffer_dirty() caller, like setup_items_for_insert(), doesn'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.

[Fix]
The correct timing to check leaf validation should be before write IO or after read IO.

Just like ee have already done the tree validation check at btree readpage end io hook, this patch will move the write time tree checker to csum_dirty_buffer().

As csum_dirty_buffer() is called just before submitting btree write bio, as the call path shows:

btree_submit_bio_hook()
|- __btree_submit_bio_start()
|- btree_csum_one_bio()

      |- csum_dirty_buffer()
         |- btrfs_check_leaf()

By this we can ensure the leaf passed in is in consistent status, and can check them without causing tons of false alert.

Reported-by Lakshmipathi.G <[email protected]>

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/disk-io.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

fs/btrfs/disk-io.c +19 -7

index efce9a2fa9be..6c17bce2a05e 100644
@@ -506,6 +506,7 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page)
 	u64 start = page_offset(page);
 	u64 found_start;
 	struct extent_buffer *eb;
+	int ret;
 
 	eb = (struct extent_buffer *)page->private;
 	if (page != eb->pages[0])

@@ -524,7 +525,24 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page)
 	ASSERT(memcmp_extent_buffer(eb, fs_info->fsid,
 			btrfs_header_fsid(), BTRFS_FSID_SIZE) == 0);
 
-	return csum_tree_block(fs_info, eb, 0);
+	ret = csum_tree_block(fs_info, eb, 0);
+	if (ret)
+		return ret;
+
+#ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
+	/*
+	 * Do extra check before we write the tree block into disk.
+	 */
+	if (btrfs_header_level(eb) == 0) {
+	    ret = btrfs_check_leaf(fs_info->tree_root, eb);
+	    if (ret) {
+		btrfs_print_leaf(eb);
+		ASSERT(0);
+		return ret;
+	    }
+	}
+#endif
+	return 0;
 }
 
 static int check_tree_block_fsid(struct btrfs_fs_info *fs_info,

@@ -3847,12 +3865,6 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
 		percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
 					 buf->len,
 					 fs_info->dirty_metadata_batch);
-#ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
-	if (btrfs_header_level(buf) == 0 && btrfs_check_leaf(root, buf)) {
-		btrfs_print_leaf(buf);
-		ASSERT(0);
-	}
-#endif
 }
 
 static void __btrfs_btree_balance_dirty(struct btrfs_fs_info *fs_info,

Signature · 1 line

2.14.3

Nikolay Borisov

Reviewed-by on v1 1/1

2017-11-02 08:46 UTC #1 lore raw

Quoted 40 lines · Qu Wenruo

[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 some btrfs_mark_buffer_dirty() caller, like setup_items_for_insert(), doesn'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.
[Fix]
The correct timing to check leaf validation should be before write IO or after read IO.
Just like ee have already done the tree validation check at btree readpage end io hook, this patch will move the write time tree checker to csum_dirty_buffer().
As csum_dirty_buffer() is called just before submitting btree write bio, as the call path shows:
btree_submit_bio_hook()
|- __btree_submit_bio_start()
|- btree_csum_one_bio()

Quoted 8 lines

      |- csum_dirty_buffer()
         |- btrfs_check_leaf()
By this we can ensure the leaf passed in is in consistent status, and can check them without causing tons of false alert.
Reported-by Lakshmipathi.G <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>

Reviewed-by Nikolay Borisov <[email protected]>

Quoted 56 lines

---
 fs/btrfs/disk-io.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

fs/btrfs/disk-io.c +19 -7

index efce9a2fa9be..6c17bce2a05e 100644
@@ -506,6 +506,7 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page)
 	u64 start = page_offset(page);
 	u64 found_start;
 	struct extent_buffer *eb;
+	int ret;
 
 	eb = (struct extent_buffer *)page->private;
 	if (page != eb->pages[0])

@@ -524,7 +525,24 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page)
 	ASSERT(memcmp_extent_buffer(eb, fs_info->fsid,
 			btrfs_header_fsid(), BTRFS_FSID_SIZE) == 0);
 
-	return csum_tree_block(fs_info, eb, 0);
+	ret = csum_tree_block(fs_info, eb, 0);
+	if (ret)
+		return ret;
+
+#ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
+	/*
+	 * Do extra check before we write the tree block into disk.
+	 */
+	if (btrfs_header_level(eb) == 0) {
+	    ret = btrfs_check_leaf(fs_info->tree_root, eb);
+	    if (ret) {
+		btrfs_print_leaf(eb);
+		ASSERT(0);
+		return ret;
+	    }
+	}
+#endif
+	return 0;
 }
 
 static int check_tree_block_fsid(struct btrfs_fs_info *fs_info,

@@ -3847,12 +3865,6 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
 		percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
 					 buf->len,
 					 fs_info->dirty_metadata_batch);
-#ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
-	if (btrfs_header_level(buf) == 0 && btrfs_check_leaf(root, buf)) {
-		btrfs_print_leaf(buf);
-		ASSERT(0);
-	}
-#endif
 }
 
 static void __btrfs_btree_balance_dirty(struct btrfs_fs_info *fs_info,

Filipe Manana

2017-11-03 10:59 UTC #2 lore raw

In reply to Qu Wenruo

Quoted 15 lines · Qu Wenruo

[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]
...
------

Quoted 7 lines

[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 some btrfs_mark_buffer_dirty() caller, like setup_items_for_insert(), doesn't really initialize its item data but only initialize its item pointers, leaving item data uninitialized.

So instead of doing this juggling, the best would be to have it not call mark_buffer_dirty(), and leave that responsibility for the caller after it initializes the item data. I give you a very good reason for that below.

Quoted 69 lines

This makes tree-checker catch uninitialized data as error, causing such panic.

[Fix]
The correct timing to check leaf validation should be before write IO or after read IO.

Just like ee have already done the tree validation check at btree readpage end io hook, this patch will move the write time tree checker to csum_dirty_buffer().

As csum_dirty_buffer() is called just before submitting btree write bio, as the call path shows:

btree_submit_bio_hook()
|- __btree_submit_bio_start()
|- btree_csum_one_bio()

      |- csum_dirty_buffer()
         |- btrfs_check_leaf()

By this we can ensure the leaf passed in is in consistent status, and can check them without causing tons of false alert.

Reported-by Lakshmipathi.G <[email protected]>

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/disk-io.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

fs/btrfs/disk-io.c +19 -1

index efce9a2fa9be..6c17bce2a05e 100644
@@ -506,6 +506,7 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page)
        u64 start = page_offset(page);
        u64 found_start;
        struct extent_buffer *eb;
+       int ret;
 
        eb = (struct extent_buffer *)page->private;
        if (page != eb->pages[0])

@@ -524,7 +525,24 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page)
        ASSERT(memcmp_extent_buffer(eb, fs_info->fsid,
                        btrfs_header_fsid(), BTRFS_FSID_SIZE) == 0);
 
-       return csum_tree_block(fs_info, eb, 0);
+       ret = csum_tree_block(fs_info, eb, 0);
+       if (ret)
+               return ret;
+
+#ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
+       /*
+        * Do extra check before we write the tree block into disk.
+        */
+       if (btrfs_header_level(eb) == 0) {
+           ret = btrfs_check_leaf(fs_info->tree_root, eb);
+           if (ret) {
+               btrfs_print_leaf(eb);
+               ASSERT(0);
+               return ret;
+           }
+       }
+#endif
+       return 0;
 }
 
 static int check_tree_block_fsid(struct btrfs_fs_info *fs_info,

fs/btrfs/disk-io.c void btrfs_mark_buffer_dirty(struct extent_buffer *buf) · line 3855

Quoted 9 lines

fs/btrfs/disk-io.c +0 -6

@@ -3847,12 +3865,6 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
                percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
                                         buf->len,
                                         fs_info->dirty_metadata_batch);
-#ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
-       if (btrfs_header_level(buf) == 0 && btrfs_check_leaf(root, buf)) {
-               btrfs_print_leaf(buf);
-               ASSERT(0);
-       }
-#endif

So there's a reason why btrfs_check_leaf() was called here, at mark_buffer_dirty(),
instead of somewhere else like csum_dirty_buffer().

The reason is that once some bad code inserts a key out of order for example (or did any
other bad stuff that check_leaf() catched before you added the tree-checker thing), we
would get a trace that pinpoints exactly where the bad code is. With this change, we will
only know some is bad when writeback of the leaf starts, and before that happens, the leaf might
have been changed dozens of times by many different functions (and this happens very
often, it's far from being a unusual case), in which case the given trace won't tell you which code
misbehaved. This makes it harder to find out bugs, and as it used to be it certainly helped me in
the past several times. IOW, I would prefer what I mentioned earlier or, at very least, do those new
checks that validate data only at writeback start time.

Quoted 10 lines

fs/btrfs/disk-io.c +0 -1
 }
 
 static void __btrfs_btree_balance_dirty(struct btrfs_fs_info *fs_info,
--
2.14.3
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines

Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Qu Wenruo

2017-11-03 11:15 UTC #3 lore raw

Quoted 20 lines · Filipe Manana

Quoted 19 lines · Qu Wenruo
[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.

Quoted 7 lines

Quoted 3 lines
However some btrfs_mark_buffer_dirty() caller, like setup_items_for_insert(), doesn't really initialize its item data but only initialize its item pointers, leaving item data uninitialized.
So instead of doing this juggling, the best would be to have it not call mark_buffer_dirty(), and leave that responsibility for the caller after it initializes the item data. I give you a very good reason for that below.

However setup_items_for_insert() is just one of the possible causes, unless we overhaul all btrfs_mark_buffer_dirty() callers, it will be whac-a-aole.

Quoted 93 lines

Quoted 70 lines
This makes tree-checker catch uninitialized data as error, causing such panic.
[Fix]
The correct timing to check leaf validation should be before write IO or after read IO.
Just like ee have already done the tree validation check at btree readpage end io hook, this patch will move the write time tree checker to csum_dirty_buffer().
As csum_dirty_buffer() is called just before submitting btree write bio, as the call path shows:
btree_submit_bio_hook()
|- __btree_submit_bio_start()
|- btree_csum_one_bio()
      |- csum_dirty_buffer()
         |- btrfs_check_leaf()
By this we can ensure the leaf passed in is in consistent status, and can check them without causing tons of false alert.
Reported-by Lakshmipathi.G <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/disk-io.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)
fs/btrfs/disk-io.c +19 -1
index efce9a2fa9be..6c17bce2a05e 100644
@@ -506,6 +506,7 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page)
        u64 start = page_offset(page);
        u64 found_start;
        struct extent_buffer *eb;
+       int ret;
 
        eb = (struct extent_buffer *)page->private;
        if (page != eb->pages[0])
@@ -524,7 +525,24 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page)
        ASSERT(memcmp_extent_buffer(eb, fs_info->fsid,
                        btrfs_header_fsid(), BTRFS_FSID_SIZE) == 0);
 
-       return csum_tree_block(fs_info, eb, 0);
+       ret = csum_tree_block(fs_info, eb, 0);
+       if (ret)
+               return ret;
+
+#ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
+       /*
+        * Do extra check before we write the tree block into disk.
+        */
+       if (btrfs_header_level(eb) == 0) {
+           ret = btrfs_check_leaf(fs_info->tree_root, eb);
+           if (ret) {
+               btrfs_print_leaf(eb);
+               ASSERT(0);
+               return ret;
+           }
+       }
+#endif
+       return 0;
 }
 
 static int check_tree_block_fsid(struct btrfs_fs_info *fs_info,
fs/btrfs/disk-io.c void btrfs_mark_buffer_dirty(struct extent_buffer *buf) · line 3855
Quoted 9 lines
fs/btrfs/disk-io.c +0 -6
@@ -3847,12 +3865,6 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
                percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
                                         buf->len,
                                         fs_info->dirty_metadata_batch);
-#ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
-       if (btrfs_header_level(buf) == 0 && btrfs_check_leaf(root, buf)) {
-               btrfs_print_leaf(buf);
-               ASSERT(0);
-       }
-#endif
So there's a reason why btrfs_check_leaf() was called here, at mark_buffer_dirty(),
instead of somewhere else like csum_dirty_buffer().
The reason is that once some bad code inserts a key out of order for example (or did any
other bad stuff that check_leaf() catched before you added the tree-checker thing), we
would get a trace that pinpoints exactly where the bad code is. With this change, we will
only know some is bad when writeback of the leaf starts, and before that happens, the leaf might
have been changed dozens of times by many different functions (and

Quoted 8 lines

this happens very
often, it's far from being a unusual case), in which case the given trace won't tell you which code
misbehaved. This makes it harder to find out bugs, and as it used to be it certainly helped me in
the past several times. IOW, I would prefer what I mentioned earlier or, at very least, do those new
checks that validate data only at writeback start time.

OK, I'll skip any item member check in btrfs_mark_buffer_dirty(), to make it still give early warning, and do the full comprehensive check only before writeback.

Thanks,
Qu

Quoted 14 lines

Quoted 11 lines
fs/btrfs/disk-io.c +0 -1
 }
 
 static void __btrfs_btree_balance_dirty(struct btrfs_fs_info *fs_info,
--
2.14.3
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html