[PATCH 0/6] btrfs-progs: Variant fixes for fuzz-tests

[PATCH 0/6] btrfs-progs: Variant fixes for fuzz-tests

[Qu Wenruo]

This can be fetched from github:
https://github.com/adam900710/btrfs-progs/tree/fixes_for_fuzz_test

The base HEAD is:
commit d7a1b84756157d544a9ddc399ef48c6132eaafcf (david/devel)
Author: Qu Wenruo <wqu@suse.com>
Date:   Thu Jul 5 15:37:31 2018 +0800

    btrfs-progs: check/original: Don't overwrite return value when we failed to repair


Thanks for the already merged fixes for fuzz/003, the remaining part is
pretty small now, +20/-7.

Mostly of the fixes are for fuzz/003, just a small bunch of BUG_ON()
removal. (Patch 1~3 and 5)

There is also a fix for fuzz/003 dead loop. (Patch 4)

Finally we have a fix for fuzz/007, the bug is a segfault triggered by
accessing poisoned list_head, caused by double list freeing. (Patch 6)

Now fuzz-test should finally work without problem.

Qu Wenruo (6):
  btrfs-progs: Exit gracefully if we hit ENOSPC when allocating tree
    block
  btrfs-progs: Exit gracefully when failed to repair root dir item
  btrfs-progs: Don't report dirty leaked eb using BUG_ON
  btrfs-progs: Fix infinite loop when failed to repair bad key order
  btrfs-progs: Exit gracefull when we failed to alloc dev extent
  btrfs-progs: rescue-super: Don't double free fs_devices

 check/main.c    | 10 +++++++++-
 extent-tree.c   |  5 ++++-
 extent_io.c     |  6 +++++-
 super-recover.c |  3 ---
 volumes.c       |  3 ++-
 5 files changed, 20 insertions(+), 7 deletions(-)

-- 
2.18.0

[PATCH 1/6] btrfs-progs: Exit gracefully if we hit ENOSPC when allocating tree block

[Qu Wenruo]

When running test fuzz/003, we could hit the following BUG_ON:
------
====== RUN MAYFAIL /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-155621-bad-block-group-offset.raw.restored
Unable to find block group for 0
Unable to find block group for 0
Unable to find block group for 0
extent-tree.c:2657: alloc_tree_block: BUG_ON `ret` triggered, value -28
failed (ignored, ret=134): /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-155621-bad-block-group-offset.raw.restored
mayfail: returned code 134 (SIGABRT), not ignored
test failed for case 003-multi-check-unmounted
------

Just remove that BUG_ON() and allow us to exit gracefully.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 extent-tree.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/extent-tree.c b/extent-tree.c
index b9d51b388c9a..a1f711ece7a8 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -2654,7 +2654,10 @@ static int alloc_tree_block(struct btrfs_trans_handle *trans,
 
 	ret = btrfs_reserve_extent(trans, root, num_bytes, empty_size,
 				   hint_byte, search_end, ins, 0);
-	BUG_ON(ret);
+	if (ret < 0) {
+		btrfs_free_delayed_extent_op(extent_op);
+		return ret;
+	}
 
 	if (key)
 		memcpy(&extent_op->key, key, sizeof(extent_op->key));
-- 
2.18.0

Re: [PATCH 1/6] btrfs-progs: Exit gracefully if we hit ENOSPC when allocating tree block

[Nikolay Borisov]

On  3.08.2018 08:50, Qu Wenruo wrote:
> When running test fuzz/003, we could hit the following BUG_ON:
> ------
> ====== RUN MAYFAIL /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-155621-bad-block-group-offset.raw.restored
> Unable to find block group for 0
> Unable to find block group for 0
> Unable to find block group for 0
> extent-tree.c:2657: alloc_tree_block: BUG_ON `ret` triggered, value -28
> failed (ignored, ret=134): /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-155621-bad-block-group-offset.raw.restored
> mayfail: returned code 134 (SIGABRT), not ignored
> test failed for case 003-multi-check-unmounted
> ------
> 
> Just remove that BUG_ON() and allow us to exit gracefully.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

However there are a lot more BUG_ONs in btrfs_reserve_extent :(
> ---
>  extent-tree.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/extent-tree.c b/extent-tree.c
> index b9d51b388c9a..a1f711ece7a8 100644
> --- a/extent-tree.c
> +++ b/extent-tree.c
> @@ -2654,7 +2654,10 @@ static int alloc_tree_block(struct btrfs_trans_handle *trans,
>  
>  	ret = btrfs_reserve_extent(trans, root, num_bytes, empty_size,
>  				   hint_byte, search_end, ins, 0);
> -	BUG_ON(ret);
> +	if (ret < 0) {
> +		btrfs_free_delayed_extent_op(extent_op);
> +		return ret;
> +	}
>  
>  	if (key)
>  		memcpy(&extent_op->key, key, sizeof(extent_op->key));
> 

[PATCH 2/6] btrfs-progs: Exit gracefully when failed to repair root dir item

[Qu Wenruo]

Another BUG_ON() during fuzz/003:
------
====== RUN MAYFAIL /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-161821.raw.restored
[1/7] checking root items
Fixed 0 roots.
[2/7] checking extents
parent transid verify failed on 4198400 wanted 14 found 1114126
parent transid verify failed on 4198400 wanted 14 found 1114126
Ignoring transid failure
owner ref check failed [4198400 4096]
repair deleting extent record: key [4198400,169,0]
adding new tree backref on start 4198400 len 4096 parent 0 root 5
Repaired extent references for 4198400
ref mismatch on [4222976 4096] extent item 1, found 0
backref 4222976 root 7 not referenced back 0x55e9cc694780
incorrect global backref count on 4222976 found 1 wanted 0
backpointer mismatch on [4222976 4096]
owner ref check failed [4222976 4096]
repair deleting extent record: key [4222976,169,0]
Repaired extent references for 4222976
[3/7] checking free space cache
[4/7] checking fs roots
parent transid verify failed on 4198400 wanted 14 found 1114126
Ignoring transid failure
Wrong generation of child node/leaf, wanted: 1114126, have: 14
root 5 missing its root dir, recreating
parent transid verify failed on 4198400 wanted 14 found 1114126
Ignoring transid failure
ERROR: child eb corrupted: parent bytenr=4222976 item=0 parent level=1 child level=2
check/main.c:2738: check_inode_recs: BUG_ON `ret` triggered, value -5
failed (ignored, ret=134): /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-161821.raw.restored
mayfail: returned code 134 (SIGABRT), not ignored
test failed for case 003-multi-check-unmounted
------

Just abort current transaction and exit gracefully in this case.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/check/main.c b/check/main.c
index b597e2a607e5..e6756e0f852b 100644
--- a/check/main.c
+++ b/check/main.c
@@ -2735,7 +2735,10 @@ static int check_inode_recs(struct btrfs_root *root,
 				(unsigned long long)root->objectid);
 
 			ret = btrfs_make_root_dir(trans, root, root_dirid);
-			BUG_ON(ret);
+			if (ret < 0) {
+				btrfs_abort_transaction(trans, ret);
+				return ret;
+			}
 
 			btrfs_commit_transaction(trans, root);
 			return -EAGAIN;
-- 
2.18.0

Re: [PATCH 2/6] btrfs-progs: Exit gracefully when failed to repair root dir item

[Nikolay Borisov]

On  3.08.2018 08:50, Qu Wenruo wrote:
> Another BUG_ON() during fuzz/003:
> ------
> ====== RUN MAYFAIL /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-161821.raw.restored
> [1/7] checking root items
> Fixed 0 roots.
> [2/7] checking extents
> parent transid verify failed on 4198400 wanted 14 found 1114126
> parent transid verify failed on 4198400 wanted 14 found 1114126
> Ignoring transid failure
> owner ref check failed [4198400 4096]
> repair deleting extent record: key [4198400,169,0]
> adding new tree backref on start 4198400 len 4096 parent 0 root 5
> Repaired extent references for 4198400
> ref mismatch on [4222976 4096] extent item 1, found 0
> backref 4222976 root 7 not referenced back 0x55e9cc694780
> incorrect global backref count on 4222976 found 1 wanted 0
> backpointer mismatch on [4222976 4096]
> owner ref check failed [4222976 4096]
> repair deleting extent record: key [4222976,169,0]
> Repaired extent references for 4222976
> [3/7] checking free space cache
> [4/7] checking fs roots
> parent transid verify failed on 4198400 wanted 14 found 1114126
> Ignoring transid failure
> Wrong generation of child node/leaf, wanted: 1114126, have: 14
> root 5 missing its root dir, recreating
> parent transid verify failed on 4198400 wanted 14 found 1114126
> Ignoring transid failure
> ERROR: child eb corrupted: parent bytenr=4222976 item=0 parent level=1 child level=2
> check/main.c:2738: check_inode_recs: BUG_ON `ret` triggered, value -5
> failed (ignored, ret=134): /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-161821.raw.restored
> mayfail: returned code 134 (SIGABRT), not ignored
> test failed for case 003-multi-check-unmounted
> ------
> 
> Just abort current transaction and exit gracefully in this case.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

LGTM:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  check/main.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/check/main.c b/check/main.c
> index b597e2a607e5..e6756e0f852b 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -2735,7 +2735,10 @@ static int check_inode_recs(struct btrfs_root *root,
>  				(unsigned long long)root->objectid);
>  
>  			ret = btrfs_make_root_dir(trans, root, root_dirid);
> -			BUG_ON(ret);
> +			if (ret < 0) {
> +				btrfs_abort_transaction(trans, ret);
> +				return ret;
> +			}
>  
>  			btrfs_commit_transaction(trans, root);
>  			return -EAGAIN;
> 

[PATCH 3/6] btrfs-progs: Don't report dirty leaked eb using BUG_ON

[Qu Wenruo]

Another BUG_ON() during fuzz/003:
------
====== RUN MAYFAIL /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-161821.raw.restored
[1/7] checking root items
Fixed 0 roots.
[2/7] checking extents
parent transid verify failed on 4198400 wanted 14 found 1114126
parent transid verify failed on 4198400 wanted 14 found 1114126
Ignoring transid failure
owner ref check failed [4198400 4096]
repair deleting extent record: key [4198400,169,0]
adding new tree backref on start 4198400 len 4096 parent 0 root 5
Repaired extent references for 4198400
ref mismatch on [4222976 4096] extent item 1, found 0
backref 4222976 root 7 not referenced back 0x5617f8ecf780
incorrect global backref count on 4222976 found 1 wanted 0
backpointer mismatch on [4222976 4096]
owner ref check failed [4222976 4096]
repair deleting extent record: key [4222976,169,0]
Repaired extent references for 4222976
[3/7] checking free space cache
[4/7] checking fs roots
parent transid verify failed on 4198400 wanted 14 found 1114126
Ignoring transid failure
Wrong generation of child node/leaf, wanted: 1114126, have: 14
root 5 missing its root dir, recreating
parent transid verify failed on 4198400 wanted 14 found 1114126
Ignoring transid failure
ERROR: child eb corrupted: parent bytenr=4222976 item=0 parent level=1 child level=2
ERROR: errors found in fs roots
extent buffer leak: start 4222976 len 4096
extent_io.c:611: free_extent_buffer_internal: BUG_ON `eb->flags & EXTENT_DIRTY` triggered, value 1
failed (ignored, ret=134): /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-161821.raw.restored
mayfail: returned code 134 (SIGABRT), not ignored
test failed for case 003-multi-check-unmounted
------

Since we're shifting to using btrfs_abort_transaction() in btrfs-progs,
it will be more and more common to see dirty leaked eb.
Instead of BUG_ON(), we only needs to report it as warning.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 extent_io.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/extent_io.c b/extent_io.c
index 198492699438..b8510b0ae94e 100644
--- a/extent_io.c
+++ b/extent_io.c
@@ -608,7 +608,11 @@ static void free_extent_buffer_internal(struct extent_buffer *eb, bool free_now)
 	eb->refs--;
 	BUG_ON(eb->refs < 0);
 	if (eb->refs == 0) {
-		BUG_ON(eb->flags & EXTENT_DIRTY);
+		if (eb->flags & EXTENT_DIRTY) {
+			warning(
+			"dirty eb leak (aborted trans): start %llu len %u",
+				eb->start, eb->len);
+		}
 		list_del_init(&eb->recow);
 		if (eb->flags & EXTENT_BUFFER_DUMMY || free_now)
 			free_extent_buffer_final(eb);
-- 
2.18.0

Re: [PATCH 3/6] btrfs-progs: Don't report dirty leaked eb using BUG_ON

[Nikolay Borisov]

On  3.08.2018 08:50, Qu Wenruo wrote:
> Another BUG_ON() during fuzz/003:
> ------
> ====== RUN MAYFAIL /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-161821.raw.restored
> [1/7] checking root items
> Fixed 0 roots.
> [2/7] checking extents
> parent transid verify failed on 4198400 wanted 14 found 1114126
> parent transid verify failed on 4198400 wanted 14 found 1114126
> Ignoring transid failure
> owner ref check failed [4198400 4096]
> repair deleting extent record: key [4198400,169,0]
> adding new tree backref on start 4198400 len 4096 parent 0 root 5
> Repaired extent references for 4198400
> ref mismatch on [4222976 4096] extent item 1, found 0
> backref 4222976 root 7 not referenced back 0x5617f8ecf780
> incorrect global backref count on 4222976 found 1 wanted 0
> backpointer mismatch on [4222976 4096]
> owner ref check failed [4222976 4096]
> repair deleting extent record: key [4222976,169,0]
> Repaired extent references for 4222976
> [3/7] checking free space cache
> [4/7] checking fs roots
> parent transid verify failed on 4198400 wanted 14 found 1114126
> Ignoring transid failure
> Wrong generation of child node/leaf, wanted: 1114126, have: 14
> root 5 missing its root dir, recreating
> parent transid verify failed on 4198400 wanted 14 found 1114126
> Ignoring transid failure
> ERROR: child eb corrupted: parent bytenr=4222976 item=0 parent level=1 child level=2
> ERROR: errors found in fs roots
> extent buffer leak: start 4222976 len 4096
> extent_io.c:611: free_extent_buffer_internal: BUG_ON `eb->flags & EXTENT_DIRTY` triggered, value 1
> failed (ignored, ret=134): /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-161821.raw.restored
> mayfail: returned code 134 (SIGABRT), not ignored
> test failed for case 003-multi-check-unmounted
> ------
> 
> Since we're shifting to using btrfs_abort_transaction() in btrfs-progs,
> it will be more and more common to see dirty leaked eb.
> Instead of BUG_ON(), we only needs to report it as warning.


So how are such leaked extents supposed to be cleaned? So when
transaction_aborted is set we just return some errors from various
functions but I don't see how modified extents in the transaction are freed?
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  extent_io.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/extent_io.c b/extent_io.c
> index 198492699438..b8510b0ae94e 100644
> --- a/extent_io.c
> +++ b/extent_io.c
> @@ -608,7 +608,11 @@ static void free_extent_buffer_internal(struct extent_buffer *eb, bool free_now)
>  	eb->refs--;
>  	BUG_ON(eb->refs < 0);
>  	if (eb->refs == 0) {
> -		BUG_ON(eb->flags & EXTENT_DIRTY);
> +		if (eb->flags & EXTENT_DIRTY) {
> +			warning(
> +			"dirty eb leak (aborted trans): start %llu len %u",
> +				eb->start, eb->len);
> +		}
>  		list_del_init(&eb->recow);
>  		if (eb->flags & EXTENT_BUFFER_DUMMY || free_now)
>  			free_extent_buffer_final(eb);
> 

Re: [PATCH 3/6] btrfs-progs: Don't report dirty leaked eb using BUG_ON

[Qu Wenruo]

On 2018/8/29 下午10:52, Nikolay Borisov wrote:
> 
> 
> On  3.08.2018 08:50, Qu Wenruo wrote:
>> Another BUG_ON() during fuzz/003:
>> ------
>> ====== RUN MAYFAIL /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-161821.raw.restored
>> [1/7] checking root items
>> Fixed 0 roots.
>> [2/7] checking extents
>> parent transid verify failed on 4198400 wanted 14 found 1114126
>> parent transid verify failed on 4198400 wanted 14 found 1114126
>> Ignoring transid failure
>> owner ref check failed [4198400 4096]
>> repair deleting extent record: key [4198400,169,0]
>> adding new tree backref on start 4198400 len 4096 parent 0 root 5
>> Repaired extent references for 4198400
>> ref mismatch on [4222976 4096] extent item 1, found 0
>> backref 4222976 root 7 not referenced back 0x5617f8ecf780
>> incorrect global backref count on 4222976 found 1 wanted 0
>> backpointer mismatch on [4222976 4096]
>> owner ref check failed [4222976 4096]
>> repair deleting extent record: key [4222976,169,0]
>> Repaired extent references for 4222976
>> [3/7] checking free space cache
>> [4/7] checking fs roots
>> parent transid verify failed on 4198400 wanted 14 found 1114126
>> Ignoring transid failure
>> Wrong generation of child node/leaf, wanted: 1114126, have: 14
>> root 5 missing its root dir, recreating
>> parent transid verify failed on 4198400 wanted 14 found 1114126
>> Ignoring transid failure
>> ERROR: child eb corrupted: parent bytenr=4222976 item=0 parent level=1 child level=2
>> ERROR: errors found in fs roots
>> extent buffer leak: start 4222976 len 4096
>> extent_io.c:611: free_extent_buffer_internal: BUG_ON `eb->flags & EXTENT_DIRTY` triggered, value 1
>> failed (ignored, ret=134): /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-161821.raw.restored
>> mayfail: returned code 134 (SIGABRT), not ignored
>> test failed for case 003-multi-check-unmounted
>> ------
>>
>> Since we're shifting to using btrfs_abort_transaction() in btrfs-progs,
>> it will be more and more common to see dirty leaked eb.
>> Instead of BUG_ON(), we only needs to report it as warning.
> 
> 
> So how are such leaked extents supposed to be cleaned? So when
> transaction_aborted is set we just return some errors from various
> functions but I don't see how modified extents in the transaction are freed?

They're freed by extent_io_tree_cleanup(), called by the following call
trace:
close_ctree_fs_info()
|- btrfs_cleanup_all_caches()
   |- extent_io_tree_cleanup(&fs_info->extent_cache)
      |- free_extent_buffer_nocache()

And inside extent_io_tree_cleanup(), it's also where we do leaked extent
buffer detection.

Thanks,
Qu

>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  extent_io.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/extent_io.c b/extent_io.c
>> index 198492699438..b8510b0ae94e 100644
>> --- a/extent_io.c
>> +++ b/extent_io.c
>> @@ -608,7 +608,11 @@ static void free_extent_buffer_internal(struct extent_buffer *eb, bool free_now)
>>  	eb->refs--;
>>  	BUG_ON(eb->refs < 0);
>>  	if (eb->refs == 0) {
>> -		BUG_ON(eb->flags & EXTENT_DIRTY);
>> +		if (eb->flags & EXTENT_DIRTY) {
>> +			warning(
>> +			"dirty eb leak (aborted trans): start %llu len %u",
>> +				eb->start, eb->len);
>> +		}
>>  		list_del_init(&eb->recow);
>>  		if (eb->flags & EXTENT_BUFFER_DUMMY || free_now)
>>  			free_extent_buffer_final(eb);
>>

[PATCH 4/6] btrfs-progs: Fix infinite loop when failed to repair bad key order

[Qu Wenruo]

[BUG]
An infinite loop can be triggered during fuzz/003:
------
====== RUN MAYFAIL /home/adam/btrfs/btrfs-progs/btrfs check --repair /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-199833-reloc-recovery-crash.raw.restored
[1/7] checking root items
Fixed 0 roots.
[2/7] checking extents
ctree.c:1650: leaf_space_used: Warning: assertion `data_len < 0` failed, value 1
bad key ordering 18 19
ctree.c:1650: leaf_space_used: Warning: assertion `data_len < 0` failed, value 1
bad key ordering 18 19
ctree.c:1650: leaf_space_used: Warning: assertion `data_len < 0` failed, value 1
bad key ordering 18 19
...
------

[CAUSE]
In try_to_fix_bad_block() it's possible that btrfs_find_all_roots()
finds no root referring to that tree block, thus we can't do any repair.

However in that case, we still return 0 since the last caller assigning
@ret is btrfs_find_all_roots(), and the ulist while loop doesn't get run
at all.

And since try_to_fix_bad_block() returns 0, check_block() in
check/main.c will return -EAGAIN to re-check the tree block.

This leads to the infinite loop.

[FIX]
Change the default return value from 0 to -EIO in
try_to_fix_bad_block(), so if there is no tree referring to the bad tree
block, it won't cause infinite loop any more.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/check/main.c b/check/main.c
index e6756e0f852b..571edc7ef928 100644
--- a/check/main.c
+++ b/check/main.c
@@ -4051,6 +4051,11 @@ static int try_to_fix_bad_block(struct btrfs_root *root,
 
 	btrfs_init_path(&path);
 	ULIST_ITER_INIT(&iter);
+	/*
+	 * If we found no roots referrening to this tree block, there is no
+	 * chance to fix. So our default ret is -EIO.
+	 */
+	ret = -EIO;
 	while ((node = ulist_next(roots, &iter))) {
 		root_key.objectid = node->val;
 		root_key.type = BTRFS_ROOT_ITEM_KEY;
-- 
2.18.0

[PATCH 5/6] btrfs-progs: Exit gracefull when we failed to alloc dev extent

[Qu Wenruo]

Another BUG_ON() during fuzz/003:
------
====== RUN MAYFAIL /home/adam/btrfs/btrfs-progs/btrfs check --repair /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-199833-reloc-recovery-crash.raw.restored
[1/7] checking root items
Fixed 0 roots.
[2/7] checking extents
ctree.c:1650: leaf_space_used: Warning: assertion `data_len < 0` failed, value 1
bad key ordering 18 19
bad block 29409280
ERROR: errors found in extent allocation tree or chunk allocation
WARNING: minor unaligned/mismatch device size detected
WARNING: recommended to use 'btrfs rescue fix-device-size' to fix it
[3/7] checking free space cache
[4/7] checking fs roots
ctree.c:1650: leaf_space_used: Warning: assertion `data_len < 0` failed, value 1
bad key ordering 18 19
root 18446744073709551608 missing its root dir, recreating
Unable to find block group for 0
Unable to find block group for 0
Unable to find block group for 0
volumes.c:564: btrfs_alloc_dev_extent: BUG_ON `ret` triggered, value -28
failed (ignored, ret=134): /home/adam/btrfs/btrfs-progs/btrfs check --repair /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-199833-reloc-recovery-crash.raw.restored
mayfail: returned code 134 (SIGABRT), not ignored
test failed for case 003-multi-check-unmounted
------

However the culprit function btrfs_alloc_dev_extent() has proper error
handler tag err:, just use that tag would solve the problem easily.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 volumes.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/volumes.c b/volumes.c
index d81b348eb14d..f7a413b71d52 100644
--- a/volumes.c
+++ b/volumes.c
@@ -561,7 +561,8 @@ static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
 	key.type = BTRFS_DEV_EXTENT_KEY;
 	ret = btrfs_insert_empty_item(trans, root, path, &key,
 				      sizeof(*extent));
-	BUG_ON(ret);
+	if (ret < 0)
+		goto err;
 
 	leaf = path->nodes[0];
 	extent = btrfs_item_ptr(leaf, path->slots[0],
-- 
2.18.0

Re: [PATCH 5/6] btrfs-progs: Exit gracefull when we failed to alloc dev extent

[Nikolay Borisov]

On  3.08.2018 08:50, Qu Wenruo wrote:
> Another BUG_ON() during fuzz/003:
> ------
> ====== RUN MAYFAIL /home/adam/btrfs/btrfs-progs/btrfs check --repair /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-199833-reloc-recovery-crash.raw.restored
> [1/7] checking root items
> Fixed 0 roots.
> [2/7] checking extents
> ctree.c:1650: leaf_space_used: Warning: assertion `data_len < 0` failed, value 1
> bad key ordering 18 19
> bad block 29409280
> ERROR: errors found in extent allocation tree or chunk allocation
> WARNING: minor unaligned/mismatch device size detected
> WARNING: recommended to use 'btrfs rescue fix-device-size' to fix it
> [3/7] checking free space cache
> [4/7] checking fs roots
> ctree.c:1650: leaf_space_used: Warning: assertion `data_len < 0` failed, value 1
> bad key ordering 18 19
> root 18446744073709551608 missing its root dir, recreating
> Unable to find block group for 0
> Unable to find block group for 0
> Unable to find block group for 0
> volumes.c:564: btrfs_alloc_dev_extent: BUG_ON `ret` triggered, value -28
> failed (ignored, ret=134): /home/adam/btrfs/btrfs-progs/btrfs check --repair /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-199833-reloc-recovery-crash.raw.restored
> mayfail: returned code 134 (SIGABRT), not ignored
> test failed for case 003-multi-check-unmounted
> ------
> 
> However the culprit function btrfs_alloc_dev_extent() has proper error
> handler tag err:, just use that tag would solve the problem easily.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  volumes.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/volumes.c b/volumes.c
> index d81b348eb14d..f7a413b71d52 100644
> --- a/volumes.c
> +++ b/volumes.c
> @@ -561,7 +561,8 @@ static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
>  	key.type = BTRFS_DEV_EXTENT_KEY;
>  	ret = btrfs_insert_empty_item(trans, root, path, &key,
>  				      sizeof(*extent));
> -	BUG_ON(ret);
> +	if (ret < 0)
> +		goto err;
>  
>  	leaf = path->nodes[0];
>  	extent = btrfs_item_ptr(leaf, path->slots[0],
> 

[PATCH 6/6] btrfs-progs: rescue-super: Don't double free fs_devices

[Qu Wenruo]

[BUG]
During fuzz/007 we hit the following error:
------
====== RUN MAYFAIL /home/adam/btrfs/btrfs-progs/btrfs rescue super-recover -y -v /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-200409.raw.restored.scratch
ERROR: tree_root block unaligned: 33554431
ERROR: superblock checksum matches but it has invalid members
ERROR: tree_root block unaligned: 33554431
ERROR: superblock checksum matches but it has invalid members
ERROR: tree_root block unaligned: 33554431
ERROR: superblock checksum matches but it has invalid members
ERROR: failed to add chunk map start=12582912 len=8454144: -17 (File exists)
Couldn't read chunk tree
failed (ignored, ret=139): /home/adam/btrfs/btrfs-progs/btrfs rescue super-recover -y -v /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-200409.raw.restored.scratch
mayfail: returned code 139 (SEGFAULT), not ignored
test failed for case 007-simple-super-recover
------

[CAUSE]
In __open_ctree_fd(), if we have valid @open_ctree_flags and
btrfs_scan_fs_devices() successes without problem, no matter what
happens we will call btrfs_close_devices(), thus free all related
devices.

In super-recover, before we call open_ctree(), we have called
btrfs_scan_fs_devices() already, so btrfs_scan_fs_devices() should not
fail in open_ctree(), fs_devices will always be freed in open_ctree() or
close_ctree().

[FIX]
So in super-recover.c, we should not call btrfs_close_devices(), or we
will find fs_devices->list get poisoned, and trigger segfault when
exiting.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 super-recover.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/super-recover.c b/super-recover.c
index 880fd7712546..86b3df9867dc 100644
--- a/super-recover.c
+++ b/super-recover.c
@@ -292,9 +292,6 @@ int btrfs_recover_superblocks(const char *dname,
 no_recover:
 	recover_err_str(ret);
 	free_recover_superblock(&recover);
-	/* check if we have freed fs_devices in close_ctree() */
-	if (!root)
-		btrfs_close_devices(recover.fs_devices);
 	return ret;
 }
 
-- 
2.18.0

Re: [PATCH 6/6] btrfs-progs: rescue-super: Don't double free fs_devices

[Nikolay Borisov]

On  3.08.2018 08:50, Qu Wenruo wrote:
> [BUG]
> During fuzz/007 we hit the following error:
> ------
> ====== RUN MAYFAIL /home/adam/btrfs/btrfs-progs/btrfs rescue super-recover -y -v /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-200409.raw.restored.scratch
> ERROR: tree_root block unaligned: 33554431
> ERROR: superblock checksum matches but it has invalid members
> ERROR: tree_root block unaligned: 33554431
> ERROR: superblock checksum matches but it has invalid members
> ERROR: tree_root block unaligned: 33554431
> ERROR: superblock checksum matches but it has invalid members
> ERROR: failed to add chunk map start=12582912 len=8454144: -17 (File exists)
> Couldn't read chunk tree
> failed (ignored, ret=139): /home/adam/btrfs/btrfs-progs/btrfs rescue super-recover -y -v /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-200409.raw.restored.scratch
> mayfail: returned code 139 (SEGFAULT), not ignored
> test failed for case 007-simple-super-recover
> ------
> 
> [CAUSE]
> In __open_ctree_fd(), if we have valid @open_ctree_flags and
> btrfs_scan_fs_devices() successes without problem, no matter what
> happens we will call btrfs_close_devices(), thus free all related
> devices.

Why do you think it's _always_ going to be called? Looking into that
function it seems this can happen if
btrfs_setup_chunk_tree_and_device_map fails.
> 
> In super-recover, before we call open_ctree(), we have called
> btrfs_scan_fs_devices() already, so btrfs_scan_fs_devices() should not
> fail in open_ctree(), fs_devices will always be freed in open_ctree() or
> close_ctree().

Isn't the actual issue just that we call close_ctree. So the actual life
time of fs_devices is :

1. Create in btrfs_scan_fs_devices called from btrfs_recover_superblocks
2. All other references to those fs_devices will just return the same
reference.
3. Calling close_ctree frees fs_devices.

> 
> [FIX]
> So in super-recover.c, we should not call btrfs_close_devices(), or we
> will find fs_devices->list get poisoned, and trigger segfault when
> exiting.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  super-recover.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/super-recover.c b/super-recover.c
> index 880fd7712546..86b3df9867dc 100644
> --- a/super-recover.c
> +++ b/super-recover.c
> @@ -292,9 +292,6 @@ int btrfs_recover_superblocks(const char *dname,
>  no_recover:
>  	recover_err_str(ret);
>  	free_recover_superblock(&recover);
> -	/* check if we have freed fs_devices in close_ctree() */
> -	if (!root)
> -		btrfs_close_devices(recover.fs_devices);
>  	return ret;
>  }
>  
> 

Re: [PATCH 6/6] btrfs-progs: rescue-super: Don't double free fs_devices

[Qu Wenruo]

On 2018/8/29 下午11:38, Nikolay Borisov wrote:
> 
> 
> On  3.08.2018 08:50, Qu Wenruo wrote:
>> [BUG]
>> During fuzz/007 we hit the following error:
>> ------
>> ====== RUN MAYFAIL /home/adam/btrfs/btrfs-progs/btrfs rescue super-recover -y -v /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-200409.raw.restored.scratch
>> ERROR: tree_root block unaligned: 33554431
>> ERROR: superblock checksum matches but it has invalid members
>> ERROR: tree_root block unaligned: 33554431
>> ERROR: superblock checksum matches but it has invalid members
>> ERROR: tree_root block unaligned: 33554431
>> ERROR: superblock checksum matches but it has invalid members
>> ERROR: failed to add chunk map start=12582912 len=8454144: -17 (File exists)
>> Couldn't read chunk tree
>> failed (ignored, ret=139): /home/adam/btrfs/btrfs-progs/btrfs rescue super-recover -y -v /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-200409.raw.restored.scratch
>> mayfail: returned code 139 (SEGFAULT), not ignored
>> test failed for case 007-simple-super-recover
>> ------
>>
>> [CAUSE]
>> In __open_ctree_fd(), if we have valid @open_ctree_flags and
>> btrfs_scan_fs_devices() successes without problem, no matter what
>> happens we will call btrfs_close_devices(), thus free all related
>> devices.
> 
> Why do you think it's _always_ going to be called? Looking into that
> function it seems this can happen if
> btrfs_setup_chunk_tree_and_device_map fails.

No need to reach btrfS_setup_chunk_tree_and_device_map().

As long as we could reach btrfs_open_devices(), no matter whether if
succeeded or not, we will call btrfs_close_devices() to cleanup the
@fs_devices.

If btrfs_open_devices() fails, we goto fail label in
btrfs_open_devices() which calls btrfs_close_devices().

Or we succeeded in btrfs_open_devices(), then next error label is
out_devices in __open_ctree_fd(), and will call btrfs_close_devices() too.

And since in super recovery we have already called
btrfs_scan_fs_devices() so in __open_ctree_fd() it shouldn't fail.

So either we will hit btrfs_close_devices(), no matter whatever happens.

>>
>> In super-recover, before we call open_ctree(), we have called
>> btrfs_scan_fs_devices() already, so btrfs_scan_fs_devices() should not
>> fail in open_ctree(), fs_devices will always be freed in open_ctree() or
>> close_ctree().
> 
> Isn't the actual issue just that we call close_ctree. So the actual life
> time of fs_devices is :

No, no need to call close_ctree().
Just as described above, even failed __open_ctree_fd() could call
btrfs_close_devices() and free @fs_devices.

Thanks,
Qu

> 
> 1. Create in btrfs_scan_fs_devices called from btrfs_recover_superblocks
> 2. All other references to those fs_devices will just return the same
> reference.
> 3. Calling close_ctree frees fs_devices.
> 
>>
>> [FIX]
>> So in super-recover.c, we should not call btrfs_close_devices(), or we
>> will find fs_devices->list get poisoned, and trigger segfault when
>> exiting.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  super-recover.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/super-recover.c b/super-recover.c
>> index 880fd7712546..86b3df9867dc 100644
>> --- a/super-recover.c
>> +++ b/super-recover.c
>> @@ -292,9 +292,6 @@ int btrfs_recover_superblocks(const char *dname,
>>  no_recover:
>>  	recover_err_str(ret);
>>  	free_recover_superblock(&recover);
>> -	/* check if we have freed fs_devices in close_ctree() */
>> -	if (!root)
>> -		btrfs_close_devices(recover.fs_devices);
>>  	return ret;
>>  }
>>  
>>

Re: [PATCH 0/6] btrfs-progs: Variant fixes for fuzz-tests

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1696 bytes --]

Gentle ping.

These fixes are pretty small, I'd like to see them merged before I need
to rebase them again and again.

Thanks,
Qu

On 2018/8/3 下午1:50, Qu Wenruo wrote:
> This can be fetched from github:
> https://github.com/adam900710/btrfs-progs/tree/fixes_for_fuzz_test
> 
> The base HEAD is:
> commit d7a1b84756157d544a9ddc399ef48c6132eaafcf (david/devel)
> Author: Qu Wenruo <wqu@suse.com>
> Date:   Thu Jul 5 15:37:31 2018 +0800
> 
>     btrfs-progs: check/original: Don't overwrite return value when we failed to repair
> 
> 
> Thanks for the already merged fixes for fuzz/003, the remaining part is
> pretty small now, +20/-7.
> 
> Mostly of the fixes are for fuzz/003, just a small bunch of BUG_ON()
> removal. (Patch 1~3 and 5)
> 
> There is also a fix for fuzz/003 dead loop. (Patch 4)
> 
> Finally we have a fix for fuzz/007, the bug is a segfault triggered by
> accessing poisoned list_head, caused by double list freeing. (Patch 6)
> 
> Now fuzz-test should finally work without problem.
> 
> Qu Wenruo (6):
>   btrfs-progs: Exit gracefully if we hit ENOSPC when allocating tree
>     block
>   btrfs-progs: Exit gracefully when failed to repair root dir item
>   btrfs-progs: Don't report dirty leaked eb using BUG_ON
>   btrfs-progs: Fix infinite loop when failed to repair bad key order
>   btrfs-progs: Exit gracefull when we failed to alloc dev extent
>   btrfs-progs: rescue-super: Don't double free fs_devices
> 
>  check/main.c    | 10 +++++++++-
>  extent-tree.c   |  5 ++++-
>  extent_io.c     |  6 +++++-
>  super-recover.c |  3 ---
>  volumes.c       |  3 ++-
>  5 files changed, 20 insertions(+), 7 deletions(-)
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/6] btrfs-progs: Variant fixes for fuzz-tests

[David Sterba]

On Wed, Aug 29, 2018 at 01:27:39PM +0800, Qu Wenruo wrote:
> Gentle ping.
> 
> These fixes are pretty small, I'd like to see them merged before I need
> to rebase them again and again.

I've merged them now, thanks. I had to edit all changelogs and remove
the ----- lines, shortened the paths in the test logs and fixed typos.
Please have a look at the committed patches and compare to what you
sent.

Re: [PATCH 0/6] btrfs-progs: Variant fixes for fuzz-tests

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 674 bytes --]



On 2018/9/12 上午12:38, David Sterba wrote:
> On Wed, Aug 29, 2018 at 01:27:39PM +0800, Qu Wenruo wrote:
>> Gentle ping.
>>
>> These fixes are pretty small, I'd like to see them merged before I need
>> to rebase them again and again.
> 
> I've merged them now, thanks. I had to edit all changelogs and remove
> the ----- lines, shortened the paths in the test logs and fixed typos.

Thanks for the editing.

I'll keep the log format as default for all later patches.

BTW, is there any difference between 2 spaces indent and 1 space indent
for log?

Thanks,
Qu

> Please have a look at the committed patches and compare to what you
> sent.
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/6] btrfs-progs: Variant fixes for fuzz-tests

[David Sterba]

On Wed, Sep 12, 2018 at 07:59:34AM +0800, Qu Wenruo wrote:
> 
> 
> On 2018/9/12 上午12:38, David Sterba wrote:
> > On Wed, Aug 29, 2018 at 01:27:39PM +0800, Qu Wenruo wrote:
> >> Gentle ping.
> >>
> >> These fixes are pretty small, I'd like to see them merged before I need
> >> to rebase them again and again.
> > 
> > I've merged them now, thanks. I had to edit all changelogs and remove
> > the ----- lines, shortened the paths in the test logs and fixed typos.
> 
> Thanks for the editing.
> 
> I'll keep the log format as default for all later patches.
> 
> BTW, is there any difference between 2 spaces indent and 1 space indent
> for log?

No, it's for some visual separation so on first glance is obvious what's
the changelog text and what are logs or commands to reproduce. One space
is ok too so the log is not drifting away.