[PATCH] Btrfs: fix ASSERT(list_empty(&cur_trans->dirty_bgs_list)

[PATCH] Btrfs: fix ASSERT(list_empty(&cur_trans->dirty_bgs_list)

[Josef Bacik]

Dave could hit this assert consistently running btrfs/078.  This is because
when we update the block groups we could truncate the free space, which would
try to delete the csums for that range and dirty the csum root.  For this to
happen we have to have already written out the csum root so it's kind of hard to
hit this case.  This patch fixes this by changing the logic to only write the
dirty block groups if the dirty_cowonly_roots list is empty.  This will get us
the same effect as before since we add the extent root last, and will cover the
case that we dirty some other root again but not the extent root.  Thanks,

Reported-by: David Sterba <dsterba@suse.cz>
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/transaction.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 038fcf6..a7a413f 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1023,16 +1023,22 @@ static int update_cowonly_root(struct btrfs_trans_handle *trans,
 	u64 old_root_bytenr;
 	u64 old_root_used;
 	struct btrfs_root *tree_root = root->fs_info->tree_root;
-	bool extent_root = (root->objectid == BTRFS_EXTENT_TREE_OBJECTID);
+	struct btrfs_fs_info *fs_info = root->fs_info;
 
 	old_root_used = btrfs_root_used(&root->root_item);
 	btrfs_write_dirty_block_groups(trans, root);
 
 	while (1) {
 		old_root_bytenr = btrfs_root_bytenr(&root->root_item);
+
+		/*
+		 * We can only break out if our root matches the root item and
+		 * we either have more roots to process or we have no more roots
+		 * to process and there are no empty bgs.
+		 */
 		if (old_root_bytenr == root->node->start &&
 		    old_root_used == btrfs_root_used(&root->root_item) &&
-		    (!extent_root ||
+		    (!list_empty(&fs_info->dirty_cowonly_roots) ||
 		     list_empty(&trans->transaction->dirty_bgs)))
 			break;
 
@@ -1044,7 +1050,7 @@ static int update_cowonly_root(struct btrfs_trans_handle *trans,
 			return ret;
 
 		old_root_used = btrfs_root_used(&root->root_item);
-		if (extent_root) {
+		if (list_empty(&fs_info->dirty_cowonly_roots)) {
 			ret = btrfs_write_dirty_block_groups(trans, root);
 			if (ret)
 				return ret;
-- 
1.8.3.1

Re: [PATCH] Btrfs: fix ASSERT(list_empty(&cur_trans->dirty_bgs_list)

[Liu Bo]

On Mon, Mar 02, 2015 at 12:46:55PM -0500, Josef Bacik wrote:
> Dave could hit this assert consistently running btrfs/078.  This is because
> when we update the block groups we could truncate the free space, which would
> try to delete the csums for that range and dirty the csum root.  For this to
> happen we have to have already written out the csum root so it's kind of hard to
> hit this case.  This patch fixes this by changing the logic to only write the
> dirty block groups if the dirty_cowonly_roots list is empty.  This will get us
> the same effect as before since we add the extent root last, and will cover the
> case that we dirty some other root again but not the extent root.  Thanks,

Free space inode is NODATASUM, so its searching csum tree in
__btrfs_free_extent() is really unnecessary, by skipping that, csum tree
won't be inserted into dirty_cowonly_roots list again, so it also passed btrfs/078,
at least on my box.

Thanks,

-liubo

> 
> Reported-by: David Sterba <dsterba@suse.cz>
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  fs/btrfs/transaction.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 038fcf6..a7a413f 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1023,16 +1023,22 @@ static int update_cowonly_root(struct btrfs_trans_handle *trans,
>  	u64 old_root_bytenr;
>  	u64 old_root_used;
>  	struct btrfs_root *tree_root = root->fs_info->tree_root;
> -	bool extent_root = (root->objectid == BTRFS_EXTENT_TREE_OBJECTID);
> +	struct btrfs_fs_info *fs_info = root->fs_info;
>  
>  	old_root_used = btrfs_root_used(&root->root_item);
>  	btrfs_write_dirty_block_groups(trans, root);
>  
>  	while (1) {
>  		old_root_bytenr = btrfs_root_bytenr(&root->root_item);
> +
> +		/*
> +		 * We can only break out if our root matches the root item and
> +		 * we either have more roots to process or we have no more roots
> +		 * to process and there are no empty bgs.
> +		 */
>  		if (old_root_bytenr == root->node->start &&
>  		    old_root_used == btrfs_root_used(&root->root_item) &&
> -		    (!extent_root ||
> +		    (!list_empty(&fs_info->dirty_cowonly_roots) ||
>  		     list_empty(&trans->transaction->dirty_bgs)))
>  			break;
>  
> @@ -1044,7 +1050,7 @@ static int update_cowonly_root(struct btrfs_trans_handle *trans,
>  			return ret;
>  
>  		old_root_used = btrfs_root_used(&root->root_item);
> -		if (extent_root) {
> +		if (list_empty(&fs_info->dirty_cowonly_roots)) {
>  			ret = btrfs_write_dirty_block_groups(trans, root);
>  			if (ret)
>  				return ret;
> -- 
> 1.8.3.1
> 
> --
> 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

Re: [PATCH] Btrfs: fix ASSERT(list_empty(&cur_trans->dirty_bgs_list)

[David Sterba]

On Tue, Mar 03, 2015 at 07:02:58PM +0800, Liu Bo wrote:
> On Mon, Mar 02, 2015 at 12:46:55PM -0500, Josef Bacik wrote:
> > Dave could hit this assert consistently running btrfs/078.  This is because
> > when we update the block groups we could truncate the free space, which would
> > try to delete the csums for that range and dirty the csum root.  For this to
> > happen we have to have already written out the csum root so it's kind of hard to
> > hit this case.  This patch fixes this by changing the logic to only write the
> > dirty block groups if the dirty_cowonly_roots list is empty.  This will get us
> > the same effect as before since we add the extent root last, and will cover the
> > case that we dirty some other root again but not the extent root.  Thanks,
> 
> Free space inode is NODATASUM, so its searching csum tree in
> __btrfs_free_extent() is really unnecessary, by skipping that, csum tree
> won't be inserted into dirty_cowonly_roots list again, so it also passed btrfs/078,
> at least on my box.

I can hit the bug [1] even with Josef's patch, I'm can test your fix if you
want.

MKFS_OPTIONS  -- -O skinny-metadata,extref -m single -d single --mixed /dev/sdc1
MOUNT_OPTIONS -- -o enospc_debug,space_cache,noatime /dev/sdc1 /mnt/a2

on top of 3.19-rc5 with Chris' for-linus.


[1]

[  773.601170] BTRFS: assertion failed: list_empty(&cur_trans->dirty_bgs), file: fs/btrfs/transaction.c, line: 2007
[  773.601170] ------------[ cut here ]------------
[  773.601172] kernel BUG at fs/btrfs/ctree.h:4012!
[  773.601177] invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC
[  773.601183] Modules linked in: rpcsec_gss_krb5 loop btrfs
[  773.601187] CPU: 1 PID: 28350 Comm: fsstress Tainted: G        W      3.19.0-rc5-default+ #233
[  773.601189] Hardware name: Intel Corporation Santa Rosa platform/Matanzas, BIOS TSRSCRB1.86C.0047.B00.0610170821 10/17/06
[  773.601191] task: ffff88004e220000 ti: ffff880076f30000 task.ti: ffff880076f30000
[  773.601220] RIP: 0010:[<ffffffffa003824e>]  [<ffffffffa003824e>] assfail.clone.0+0x1e/0x20 [btrfs]
[  773.601224] RSP: 0018:ffff880076f33e58  EFLAGS: 00010292
[  773.601226] RAX: 0000000000000064 RBX: ffff8800789026e0 RCX: 0000000000000001
[  773.601227] RDX: 0000000000000001 RSI: 0000000000000001 RDI: 0000000000000001
[  773.601229] RBP: ffff880076f33e58 R08: 0000000000000001 R09: 0000000000000000
[  773.601230] R10: 0000000000000001 R11: 0000000000000001 R12: ffff880075c3d000
[  773.601232] R13: ffff880078927518 R14: ffff880078927380 R15: ffff88006bab63a8
[  773.601234] FS:  00007fb829fd4700(0000) GS:ffff88007d400000(0000) knlGS:0000000000000000
[  773.601236] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  773.601237] CR2: 00007f0fa86f3990 CR3: 0000000079102000 CR4: 00000000000007e0
[  773.601239] Stack:
[  773.601244]  ffff880076f33ed8 ffffffffa0039d9b ffff88007892738c 00ff880075c3d000
[  773.601249]  0000000000000000 ffff88004e220000 ffff8800781dd730 ffff880075c3d000
[  773.601254]  ffff880075c3d000 ffff880075f7e868 0000000000000001 ffff8800781dc000
[  773.601256] Call Trace:
[  773.601274]  [<ffffffffa0039d9b>] btrfs_commit_transaction+0xb1b/0xb60 [btrfs]
[  773.601289]  [<ffffffffa0004679>] btrfs_sync_fs+0x79/0x120 [btrfs]
[  773.601295]  [<ffffffff811d3610>] ? SyS_tee+0x360/0x360
[  773.601298]  [<ffffffff811d3630>] sync_fs_one_sb+0x20/0x30
[  773.601303]  [<ffffffff811a5231>] iterate_supers+0xf1/0x100
[  773.601307]  [<ffffffff811d3965>] sys_sync+0x55/0x90
[  773.601313]  [<ffffffff81a9fc12>] system_call_fastpath+0x12/0x17

Re: [PATCH] Btrfs: fix ASSERT(list_empty(&cur_trans->dirty_bgs_list)

[Liu Bo]

On Tue, Mar 03, 2015 at 05:35:33PM +0100, David Sterba wrote:
> On Tue, Mar 03, 2015 at 07:02:58PM +0800, Liu Bo wrote:
> > On Mon, Mar 02, 2015 at 12:46:55PM -0500, Josef Bacik wrote:
> > > Dave could hit this assert consistently running btrfs/078.  This is because
> > > when we update the block groups we could truncate the free space, which would
> > > try to delete the csums for that range and dirty the csum root.  For this to
> > > happen we have to have already written out the csum root so it's kind of hard to
> > > hit this case.  This patch fixes this by changing the logic to only write the
> > > dirty block groups if the dirty_cowonly_roots list is empty.  This will get us
> > > the same effect as before since we add the extent root last, and will cover the
> > > case that we dirty some other root again but not the extent root.  Thanks,
> > 
> > Free space inode is NODATASUM, so its searching csum tree in
> > __btrfs_free_extent() is really unnecessary, by skipping that, csum tree
> > won't be inserted into dirty_cowonly_roots list again, so it also passed btrfs/078,
> > at least on my box.
> 
> I can hit the bug [1] even with Josef's patch, I'm can test your fix if you
> want.
> 
> MKFS_OPTIONS  -- -O skinny-metadata,extref -m single -d single --mixed /dev/sdc1
> MOUNT_OPTIONS -- -o enospc_debug,space_cache,noatime /dev/sdc1 /mnt/a2
> 
> on top of 3.19-rc5 with Chris' for-linus.

I tested this fix on top of 4.0.0-rc1 which has the same commits with
Chris's for-linus, I looply ran btrfs/078 for 10 times(5 times with the
default mkfs option while 5 with your mkfs option), works good so far.

So here it is,

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 571f402..111380c 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -6033,7 +6033,7 @@ static int __btrfs_free_extent(struct
btrfs_trans_handle *trans,
 		}
 		btrfs_release_path(path);
 
-		if (is_data) {
+		if (is_data && root_objectid != BTRFS_ROOT_TREE_OBJECTID) {
 			ret = btrfs_del_csums(trans, root, bytenr, num_bytes);
 			if (ret) {
 				btrfs_abort_transaction(trans, extent_root, ret);


Thanks,

-liubo
> 
> 
> [1]
> 
> [  773.601170] BTRFS: assertion failed: list_empty(&cur_trans->dirty_bgs), file: fs/btrfs/transaction.c, line: 2007
> [  773.601170] ------------[ cut here ]------------
> [  773.601172] kernel BUG at fs/btrfs/ctree.h:4012!
> [  773.601177] invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC
> [  773.601183] Modules linked in: rpcsec_gss_krb5 loop btrfs
> [  773.601187] CPU: 1 PID: 28350 Comm: fsstress Tainted: G        W      3.19.0-rc5-default+ #233
> [  773.601189] Hardware name: Intel Corporation Santa Rosa platform/Matanzas, BIOS TSRSCRB1.86C.0047.B00.0610170821 10/17/06
> [  773.601191] task: ffff88004e220000 ti: ffff880076f30000 task.ti: ffff880076f30000
> [  773.601220] RIP: 0010:[<ffffffffa003824e>]  [<ffffffffa003824e>] assfail.clone.0+0x1e/0x20 [btrfs]
> [  773.601224] RSP: 0018:ffff880076f33e58  EFLAGS: 00010292
> [  773.601226] RAX: 0000000000000064 RBX: ffff8800789026e0 RCX: 0000000000000001
> [  773.601227] RDX: 0000000000000001 RSI: 0000000000000001 RDI: 0000000000000001
> [  773.601229] RBP: ffff880076f33e58 R08: 0000000000000001 R09: 0000000000000000
> [  773.601230] R10: 0000000000000001 R11: 0000000000000001 R12: ffff880075c3d000
> [  773.601232] R13: ffff880078927518 R14: ffff880078927380 R15: ffff88006bab63a8
> [  773.601234] FS:  00007fb829fd4700(0000) GS:ffff88007d400000(0000) knlGS:0000000000000000
> [  773.601236] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [  773.601237] CR2: 00007f0fa86f3990 CR3: 0000000079102000 CR4: 00000000000007e0
> [  773.601239] Stack:
> [  773.601244]  ffff880076f33ed8 ffffffffa0039d9b ffff88007892738c 00ff880075c3d000
> [  773.601249]  0000000000000000 ffff88004e220000 ffff8800781dd730 ffff880075c3d000
> [  773.601254]  ffff880075c3d000 ffff880075f7e868 0000000000000001 ffff8800781dc000
> [  773.601256] Call Trace:
> [  773.601274]  [<ffffffffa0039d9b>] btrfs_commit_transaction+0xb1b/0xb60 [btrfs]
> [  773.601289]  [<ffffffffa0004679>] btrfs_sync_fs+0x79/0x120 [btrfs]
> [  773.601295]  [<ffffffff811d3610>] ? SyS_tee+0x360/0x360
> [  773.601298]  [<ffffffff811d3630>] sync_fs_one_sb+0x20/0x30
> [  773.601303]  [<ffffffff811a5231>] iterate_supers+0xf1/0x100
> [  773.601307]  [<ffffffff811d3965>] sys_sync+0x55/0x90
> [  773.601313]  [<ffffffff81a9fc12>] system_call_fastpath+0x12/0x17

Re: [PATCH] Btrfs: fix ASSERT(list_empty(&cur_trans->dirty_bgs_list)

[Josef Bacik]

On 03/03/2015 10:12 PM, Liu Bo wrote:
> On Tue, Mar 03, 2015 at 05:35:33PM +0100, David Sterba wrote:
>> On Tue, Mar 03, 2015 at 07:02:58PM +0800, Liu Bo wrote:
>>> On Mon, Mar 02, 2015 at 12:46:55PM -0500, Josef Bacik wrote:
>>>> Dave could hit this assert consistently running btrfs/078.  This is because
>>>> when we update the block groups we could truncate the free space, which would
>>>> try to delete the csums for that range and dirty the csum root.  For this to
>>>> happen we have to have already written out the csum root so it's kind of hard to
>>>> hit this case.  This patch fixes this by changing the logic to only write the
>>>> dirty block groups if the dirty_cowonly_roots list is empty.  This will get us
>>>> the same effect as before since we add the extent root last, and will cover the
>>>> case that we dirty some other root again but not the extent root.  Thanks,
>>>
>>> Free space inode is NODATASUM, so its searching csum tree in
>>> __btrfs_free_extent() is really unnecessary, by skipping that, csum tree
>>> won't be inserted into dirty_cowonly_roots list again, so it also passed btrfs/078,
>>> at least on my box.
>>
>> I can hit the bug [1] even with Josef's patch, I'm can test your fix if you
>> want.
>>
>> MKFS_OPTIONS  -- -O skinny-metadata,extref -m single -d single --mixed /dev/sdc1
>> MOUNT_OPTIONS -- -o enospc_debug,space_cache,noatime /dev/sdc1 /mnt/a2
>>
>> on top of 3.19-rc5 with Chris' for-linus.
>
> I tested this fix on top of 4.0.0-rc1 which has the same commits with
> Chris's for-linus, I looply ran btrfs/078 for 10 times(5 times with the
> default mkfs option while 5 with your mkfs option), works good so far.
>
> So here it is,
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 571f402..111380c 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -6033,7 +6033,7 @@ static int __btrfs_free_extent(struct
> btrfs_trans_handle *trans,
>   		}
>   		btrfs_release_path(path);
>
> -		if (is_data) {
> +		if (is_data && root_objectid != BTRFS_ROOT_TREE_OBJECTID) {
>   			ret = btrfs_del_csums(trans, root, bytenr, num_bytes);
>   			if (ret) {
>   				btrfs_abort_transaction(trans, extent_root, ret);
>
>

I have this locally but it was causing problems with xfstests (or 
there's unrelated problems that I just haven't noticed before), so I 
didn't send it out yet.  My patch _should_ have fixed the problem, so if 
it didn't I want to figure out why before we go fixing other things. 
Thanks,

Josef

Re: [PATCH] Btrfs: fix ASSERT(list_empty(&cur_trans->dirty_bgs_list)

[Josef Bacik]

[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

On 03/03/2015 11:35 AM, David Sterba wrote:
> On Tue, Mar 03, 2015 at 07:02:58PM +0800, Liu Bo wrote:
>> On Mon, Mar 02, 2015 at 12:46:55PM -0500, Josef Bacik wrote:
>>> Dave could hit this assert consistently running btrfs/078.  This is because
>>> when we update the block groups we could truncate the free space, which would
>>> try to delete the csums for that range and dirty the csum root.  For this to
>>> happen we have to have already written out the csum root so it's kind of hard to
>>> hit this case.  This patch fixes this by changing the logic to only write the
>>> dirty block groups if the dirty_cowonly_roots list is empty.  This will get us
>>> the same effect as before since we add the extent root last, and will cover the
>>> case that we dirty some other root again but not the extent root.  Thanks,
>>
>> Free space inode is NODATASUM, so its searching csum tree in
>> __btrfs_free_extent() is really unnecessary, by skipping that, csum tree
>> won't be inserted into dirty_cowonly_roots list again, so it also passed btrfs/078,
>> at least on my box.
>
> I can hit the bug [1] even with Josef's patch, I'm can test your fix if you
> want.
>
> MKFS_OPTIONS  -- -O skinny-metadata,extref -m single -d single --mixed /dev/sdc1
> MOUNT_OPTIONS -- -o enospc_debug,space_cache,noatime /dev/sdc1 /mnt/a2
>
> on top of 3.19-rc5 with Chris' for-linus.
>

Can you try this on top of this patch and send me the logs?  Thanks,

Josef


[-- Attachment #2: out.txt --]
[-- Type: text/plain, Size: 2787 bytes --]

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 9936421..a54f9b5 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -219,6 +219,7 @@ static void add_root_to_dirty_list(struct btrfs_root *root)
 
 	spin_lock(&root->fs_info->trans_lock);
 	if (!test_and_set_bit(BTRFS_ROOT_DIRTY, &root->state)) {
+		printk(KERN_ERR "dirtying root %llu\n", root->objectid);
 		/* Want the extent tree to be the last on the list */
 		if (root->objectid == BTRFS_EXTENT_TREE_OBJECTID)
 			list_move_tail(&root->dirty_list,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index b7f0502..2488a35 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5369,6 +5369,8 @@ static int update_block_group(struct btrfs_trans_handle *trans,
 
 		spin_lock(&trans->transaction->dirty_bgs_lock);
 		if (list_empty(&cache->dirty_list)) {
+			printk(KERN_ERR "dirtying bg %llu\n",
+			       cache->key.objectid);
 			list_add_tail(&cache->dirty_list,
 				      &trans->transaction->dirty_bgs);
 			btrfs_get_block_group(cache);
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index e1a96af..ec50d17 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1027,6 +1027,10 @@ static int update_cowonly_root(struct btrfs_trans_handle *trans,
 
 	old_root_used = btrfs_root_used(&root->root_item);
 
+	printk(KERN_ERR "writing root %llu, dirty cowonly roots empty %d, "
+	       "dirty bgs empty %d\n", root->objectid,
+	       list_empty(&fs_info->dirty_cowonly_roots),
+	       list_empty(&trans->transaction->dirty_bgs));
 	while (1) {
 		old_root_bytenr = btrfs_root_bytenr(&root->root_item);
 
@@ -1050,6 +1054,7 @@ static int update_cowonly_root(struct btrfs_trans_handle *trans,
 
 		old_root_used = btrfs_root_used(&root->root_item);
 		if (list_empty(&fs_info->dirty_cowonly_roots)) {
+			printk(KERN_ERR "writing dirty block groups\n");
 			ret = btrfs_write_dirty_block_groups(trans, root);
 			if (ret)
 				return ret;
@@ -1123,6 +1128,7 @@ static noinline int commit_cowonly_roots(struct btrfs_trans_handle *trans,
 			return ret;
 	}
 
+	printk(KERN_ERR "done writy dirty cowonly roots\n");
 	list_add_tail(&fs_info->extent_root->dirty_list,
 		      &trans->transaction->switch_commits);
 	btrfs_after_dev_replace_commit(fs_info);
@@ -2000,6 +2006,14 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	switch_commit_roots(cur_trans, root->fs_info);
 
 	assert_qgroups_uptodate(trans);
+	if (!list_empty(&cur_trans->dirty_bgs)) {
+		struct btrfs_block_group_cache *cache;
+
+		cache = list_first_entry(&cur_trans->dirty_bgs,
+					 struct btrfs_block_group_cache,
+					 bg_list);
+		printk(KERN_ERR "bg %llu still dirty\n", cache->key.objectid);
+	}
 	ASSERT(list_empty(&cur_trans->dirty_bgs));
 	update_super_roots(root);
 

Re: [PATCH] Btrfs: fix ASSERT(list_empty(&cur_trans->dirty_bgs_list)

[Liu Bo]

On Mon, Mar 02, 2015 at 12:46:55PM -0500, Josef Bacik wrote:
> Dave could hit this assert consistently running btrfs/078.  This is because
> when we update the block groups we could truncate the free space, which would
> try to delete the csums for that range and dirty the csum root.  For this to
> happen we have to have already written out the csum root so it's kind of hard to
> hit this case.  This patch fixes this by changing the logic to only write the
> dirty block groups if the dirty_cowonly_roots list is empty.  This will get us
> the same effect as before since we add the extent root last, and will cover the
> case that we dirty some other root again but not the extent root.  Thanks,

Btw, in the title, s/dirty_bgs_list/dirty_bgs/  ;-)

Thanks,

-liubo

> 
> Reported-by: David Sterba <dsterba@suse.cz>
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  fs/btrfs/transaction.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 038fcf6..a7a413f 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1023,16 +1023,22 @@ static int update_cowonly_root(struct btrfs_trans_handle *trans,
>  	u64 old_root_bytenr;
>  	u64 old_root_used;
>  	struct btrfs_root *tree_root = root->fs_info->tree_root;
> -	bool extent_root = (root->objectid == BTRFS_EXTENT_TREE_OBJECTID);
> +	struct btrfs_fs_info *fs_info = root->fs_info;
>  
>  	old_root_used = btrfs_root_used(&root->root_item);
>  	btrfs_write_dirty_block_groups(trans, root);
>  
>  	while (1) {
>  		old_root_bytenr = btrfs_root_bytenr(&root->root_item);
> +
> +		/*
> +		 * We can only break out if our root matches the root item and
> +		 * we either have more roots to process or we have no more roots
> +		 * to process and there are no empty bgs.
> +		 */
>  		if (old_root_bytenr == root->node->start &&
>  		    old_root_used == btrfs_root_used(&root->root_item) &&
> -		    (!extent_root ||
> +		    (!list_empty(&fs_info->dirty_cowonly_roots) ||
>  		     list_empty(&trans->transaction->dirty_bgs)))
>  			break;
>  
> @@ -1044,7 +1050,7 @@ static int update_cowonly_root(struct btrfs_trans_handle *trans,
>  			return ret;
>  
>  		old_root_used = btrfs_root_used(&root->root_item);
> -		if (extent_root) {
> +		if (list_empty(&fs_info->dirty_cowonly_roots)) {
>  			ret = btrfs_write_dirty_block_groups(trans, root);
>  			if (ret)
>  				return ret;
> -- 
> 1.8.3.1
> 
> --
> 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