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

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 <[email protected]>

Signed-off-by Josef Bacik <[email protected]>

---
 fs/btrfs/transaction.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

fs/btrfs/transaction.c +9 -3

index 038fcf6..a7a413f 100644
@@ -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;

Signature · 6 lines

1.8.3.1

--
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

Quoted 8 lines · 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,

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.

Quoted 10 lines · Liu Bo

Quoted 8 lines · 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,

Quoted 4 lines

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

Quoted 16 lines · David Sterba

Quoted 10 lines · Liu Bo

Quoted 8 lines · 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,

Quoted 4 lines

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.

Quoted 7 lines

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,

fs/btrfs/extent-tree.c +1 -1

index 571f402..111380c 100644
@@ -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);

Quoted 10 lines · Liu Bo

Quoted 8 lines · 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,

Quoted 4 lines

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.

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 <[email protected]>

Signed-off-by Josef Bacik <[email protected]>

---
 fs/btrfs/transaction.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

fs/btrfs/transaction.c +9 -3

index 038fcf6..a7a413f 100644
@@ -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;

Signature · 6 lines

1.8.3.1

--
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