@@ -9466,10 +9466,6 @@ void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans,
memcpy(&key, &block_group->key, sizeof(key));
spin_unlock(&block_group->lock);
- ret = btrfs_insert_item(trans, extent_root, &key, &item,
- sizeof(item));
- if (ret)
- btrfs_abort_transaction(trans, extent_root, ret);
ret = btrfs_finish_chunk_alloc(trans, extent_root,
key.objectid, key.offset);
if (ret)
@@ -9490,8 +9486,6 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans,
extent_root = root->fs_info->extent_root;
- btrfs_set_log_full_commit(root->fs_info, trans);
-
cache = btrfs_create_block_group_cache(root, chunk_offset, size);
if (!cache)
return -ENOMEM;
@@ -9519,6 +9513,27 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans,
free_excluded_extents(root, cache);
+ ret = btrfs_insert_item(trans, extent_root, &cache->key, &cache->item,
+ sizeof(cache->item));
+ if (ret) {
+ btrfs_remove_free_space_cache(cache);
+ btrfs_put_block_group(cache);
+ btrfs_abort_transaction(trans, root, ret);
+ return ret;
+ }
+
+ /*
+ * We must make sure our block group item is in the extent tree before
+ * we force fsyncs to commit the current transaction and before the
+ * block group is made visible to other tasks that are about to allocate
+ * space (and therefore mark our block group dirty). This is because if
+ * we didn't do this, other tasks could start the writeout of the block
+ * group caches (btrfs_start_dirty_block_groups()) and not find our
+ * block group's item in the extent tree, resulting in an abortion of
+ * the current transaction and turning the fs into readonly mode.
+ */
+ btrfs_set_log_full_commit(root->fs_info, trans);
+
ret = btrfs_add_block_group_cache(root->fs_info, cache);
if (ret) {
btrfs_remove_free_space_cache(cache);
So creating a block group has 2 distinct phases: Phase 1 - creates the btrfs_block_group_cache item and adds it to the rbtree fs_info->block_group_cache_tree and to the corresponding list space_info->block_groups[]; Phase 2 - adds the block group item to the extent tree and corresponding items to the chunk tree. The first phase adds the block_group_cache_item to a list of pending block groups in the transaction handle, and phase 2 happens when btrfs_end_transaction() is called against the transaction handle. It happens that once phase 1 completes, other concurrent tasks that use their own transaction handle, but points to the same running transaction (struct btrfs_trans_handle->transaction), can use this block group for space allocations and therefore mark it dirty. Dirty block groups are tracked in a list belonging to the currently running transaction (struct btrfs_transaction) and not in the transaction handle (btrfs_trans_handle). This is a problem because once a task calls btrfs_commit_transaction(), it calls btrfs_start_dirty_block_groups() which will see all dirty block groups and attempt to start their writeout, including those that are still attached to the transaction handle of some concurrent task that hasn't called btrfs_end_transaction() yet - which means those block groups haven't gone through phase 2 yet and therefore when write_one_cache_group() is called, it won't find the block group items in the extent tree and abort the current transaction with -ENOENT, turning the fs into readonly mode and require a remount. Fix this by adding the block group items to the extent tree in phase 1, before the new block groups can be used for space allocations by concurrent tasks. This is a simple solution and is not more expensive than doing it at phase 2 - alternatively if write_one_cache_group() couldn't find a block group's item in the extent tree, we could not abort the transaction and re-add the block group to the dirty list - but this is slightly more complex and implies writing the block again in the commit critical section, not to mention hiding other potential bugs related to missing block group items in the extent tree. This issue happened twice, once while running fstests btrfs/067 and once for btrfs/078, which produced the following trace: [ 3278.703014] WARNING: CPU: 7 PID: 18499 at fs/btrfs/super.c:260 __btrfs_abort_transaction+0x52/0x114 [btrfs]() [ 3278.707329] BTRFS: Transaction aborted (error -2) (...) [ 3278.731555] Call Trace: [ 3278.732396] [<ffffffff8142fa46>] dump_stack+0x4f/0x7b [ 3278.733860] [<ffffffff8108b6a2>] ? console_unlock+0x361/0x3ad [ 3278.735312] [<ffffffff81045ea5>] warn_slowpath_common+0xa1/0xbb [ 3278.736874] [<ffffffffa03ada6d>] ? __btrfs_abort_transaction+0x52/0x114 [btrfs] [ 3278.738302] [<ffffffff81045f05>] warn_slowpath_fmt+0x46/0x48 [ 3278.739520] [<ffffffffa03ada6d>] __btrfs_abort_transaction+0x52/0x114 [btrfs] [ 3278.741222] [<ffffffffa03b9e56>] write_one_cache_group+0xae/0xbf [btrfs] [ 3278.742797] [<ffffffffa03c487b>] btrfs_start_dirty_block_groups+0x170/0x2b2 [btrfs] [ 3278.744492] [<ffffffffa03d309c>] btrfs_commit_transaction+0x130/0x9c9 [btrfs] [ 3278.746084] [<ffffffff8107d33d>] ? trace_hardirqs_on+0xd/0xf [ 3278.747249] [<ffffffffa03e5660>] btrfs_sync_file+0x313/0x387 [btrfs] [ 3278.748744] [<ffffffff8117acad>] vfs_fsync_range+0x95/0xa4 [ 3278.749958] [<ffffffff81435b54>] ? ret_from_sys_call+0x1d/0x58 [ 3278.751218] [<ffffffff8117acd8>] vfs_fsync+0x1c/0x1e [ 3278.754197] [<ffffffff8117ae54>] do_fsync+0x34/0x4e [ 3278.755192] [<ffffffff8117b07c>] SyS_fsync+0x10/0x14 [ 3278.756236] [<ffffffff81435b32>] system_call_fastpath+0x12/0x17 [ 3278.757366] ---[ end trace 9a4d4df4969709aa ]--- Fixes: 1bbc621ef284 ("Btrfs: allow block group cache writeout outside critical section in commit") Signed-off-by: Filipe Manana <fdmanana@suse.com> --- fs/btrfs/extent-tree.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)