btrfs: Delay block group auto removal to avoid interfering qgroups
Subsystems (2)
cover btrfs: Delay block group auto removal to avoid interfering qgroups
The patchset can be fetched from github:
https://github.com/adam900710/linux/tree/delayed_bg_removal
It's based on v4.17-rc5 branch.
This bug is reported from SUSE openQA, although it's pretty hard to hit in real world (even real world VM), it's believed block group auto removal (anyway, there isn't much thing can remove a chunk mapping in btrfs) could interfere with qgroup's search on commit root.
Full details can be found in the 3rd patch.
The patchset uses 2 submitted cleanup/refactor patches as basis, and the 3rd patch will ensure unused block group will only be deleted after current transaction is done.
Qu Wenruo (3):
btrfs: trace: Add trace points for unused block groups
btrfs: Use btrfs_mark_bg_unused() to replace open code
btrfs: Delayed empty block group auto removal to next transaction
fs/btrfs/ctree.h | 2 ++ fs/btrfs/extent-tree.c | 62 ++++++++++++++++++++++++++---------- fs/btrfs/scrub.c | 8 +---- fs/btrfs/transaction.c | 7 ++++ fs/btrfs/transaction.h | 10 ++++++ include/trace/events/btrfs.h | 42 ++++++++++++++++++++++++ 6 files changed, 107 insertions(+), 24 deletions(-)
Signature · 1 line
2.17.0
PATCH 1/3 btrfs: trace: Add trace points for unused block groups
This patch will add the following trace events:
1) btrfs_remove_block_group
For btrfs_remove_block_group() function. Triggered when a block group is really removed.
2) btrfs_add_unused_block_group
Triggered which block group is added to unused_bgs list.
3) btrfs_skip_unused_block_group
Triggered which unused block group is not deleted.
These trace events is pretty handy to debug case related to block group auto remove.
--- fs/btrfs/extent-tree.c | 4 ++++ fs/btrfs/scrub.c | 1 + include/trace/events/btrfs.h | 42 ++++++++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+)
fs/btrfs/extent-tree.c +4 -0
@@ -6354,6 +6354,7 @@ static int update_block_group(struct btrfs_trans_handle *trans,spin_lock (&info->unused_bgs_lock);if (list_empty (&cache->bg_list)) {btrfs_get_block_group (cache); +trace_btrfs_add_unused_block_group (cache);list_add_tail (&cache->bg_list, &info->unused_bgs); }
@@ -10204,6 +10205,7 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)/* Should always be true but just in case. */ if (list_empty (&cache->bg_list)) {btrfs_get_block_group (cache); +trace_btrfs_add_unused_block_group (cache);list_add_tail (&cache->bg_list, &info->unused_bgs); }
@@ -10391,6 +10393,7 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,BUG_ON (!block_group);BUG_ON (!block_group->ro); +trace_btrfs_remove_block_group (block_group);/* * Free the reserved super bytes from this block group before * remove it.
@@ -10755,6 +10758,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)* the ro check in case balance is currently acting on * this block group. */ +trace_btrfs_skip_unused_block_group (block_group);spin_unlock (&block_group->lock);up_write (&space_info->groups_sem); goto next;
fs/btrfs/scrub.c +1 -0
@@ -3984,6 +3984,7 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,spin_lock (&fs_info->unused_bgs_lock);if (list_empty (&cache->bg_list)) {btrfs_get_block_group (cache); +trace_btrfs_add_unused_block_group (cache);list_add_tail (&cache->bg_list, &fs_info->unused_bgs); }
include/trace/events/btrfs.h +42 -0
48 lines not on this page · raw message
Signature · 1 line
2.17.0
PATCH 2/3 btrfs: Use btrfs_mark_bg_unused() to replace open code
In reply to Qu WenruoIntroduce a small helper, btrfs_mark_bg_unused(), to accquire needed locks and add a block group to unused_bgs list.
No functional modification, and only 3 callers are involved.
--- fs/btrfs/ctree.h | 1 + fs/btrfs/extent-tree.c | 36 +++++++++++++++++------------------- fs/btrfs/scrub.c | 9 +-------- 3 files changed, 19 insertions(+), 27 deletions(-)
fs/btrfs/ctree.h +1 -0
@@ -2829,6 +2829,7 @@ void check_system_chunk(struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info, const u64 type);u64 add_new_free_space (struct btrfs_block_group_cache *block_group,struct btrfs_fs_info *info,u64 start,u64 end); +void btrfs_mark_bg_unused (struct btrfs_block_group_cache *bg);/* ctree.c */ int btrfs_bin_search(struct extent_buffer *eb, const struct btrfs_key *key,
fs/btrfs/extent-tree.c +17 -19
@@ -6350,16 +6350,8 @@ static int update_block_group(struct btrfs_trans_handle *trans,* dirty list to avoid races between cleaner kthread and space * cache writeout. */ -if (!alloc && old_val ==0 ) { -spin_lock (&info->unused_bgs_lock); -if (list_empty (&cache->bg_list)) { -btrfs_get_block_group (cache); -trace_btrfs_add_unused_block_group (cache); -list_add_tail (&cache->bg_list, - &info->unused_bgs); - } -spin_unlock (&info->unused_bgs_lock); - } +if (!alloc && old_val ==0 ) +btrfs_mark_bg_unused (cache);btrfs_put_block_group (cache); total -= num_bytes;
@@ -10201,15 +10193,8 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)if (btrfs_chunk_readonly (info, cache->key.objectid)) {inc_block_group_ro (cache,1 ); }else if (btrfs_block_group_used (&cache->item) ==0 ) { -spin_lock (&info->unused_bgs_lock); -/* Should always be true but just in case. */ -if (list_empty (&cache->bg_list)) { -btrfs_get_block_group (cache); -trace_btrfs_add_unused_block_group (cache); -list_add_tail (&cache->bg_list, - &info->unused_bgs); - } -spin_unlock (&info->unused_bgs_lock); +ASSERT (list_empty (&cache->bg_list)); +btrfs_mark_bg_unused (cache); } }
@@ -11133,3 +11118,16 @@ void btrfs_wait_for_snapshot_creation(struct btrfs_root *root) !atomic_read(&root->will_be_snapshotted)); } } + +void btrfs_mark_bg_unused (struct btrfs_block_group_cache *bg) +{ +struct btrfs_fs_info *fs_info = bg->fs_info; + +spin_lock (&fs_info->unused_bgs_lock); +if (list_empty (&bg->bg_list)) { +btrfs_get_block_group (bg); +trace_btrfs_add_unused_block_group (bg); +list_add_tail (&bg->bg_list, &fs_info->unused_bgs); + } +spin_unlock (&fs_info->unused_bgs_lock); +}
fs/btrfs/scrub.c +1 -8
@@ -3981,14 +3981,7 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,if (!cache->removed && !cache->ro && cache->reserved ==0 &&btrfs_block_group_used (&cache->item) ==0 ) {spin_unlock (&cache->lock); -spin_lock (&fs_info->unused_bgs_lock); -if (list_empty (&cache->bg_list)) { -btrfs_get_block_group (cache); -trace_btrfs_add_unused_block_group (cache); -list_add_tail (&cache->bg_list, - &fs_info->unused_bgs); - } -spin_unlock (&fs_info->unused_bgs_lock); +btrfs_mark_bg_unused (cache); }else {spin_unlock (&cache->lock); }
Signature · 1 line
2.17.0
Re: [PATCH 2/3] btrfs: Use btrfs_mark_bg_unused() to replace open code
Quoted 6 lines · Qu Wenruo
Introduce a small helper,
btrfs_mark_bg_unused(), to accquire needed locks and add a block group tounused_bgslist.No functional modification, and only 3 callers are involved.
Signed-off-by Qu Wenruo <[email protected]>
Quoted 97 lines
--- fs/btrfs/ctree.h | 1 + fs/btrfs/extent-tree.c | 36 +++++++++++++++++------------------- fs/btrfs/scrub.c | 9 +-------- 3 files changed, 19 insertions(+), 27 deletions(-)fs/btrfs/ctree.h +1
-0@@ -2829,6 +2829,7 @@ void check_system_chunk(struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info, const u64 type);u64 add_new_free_space (struct btrfs_block_group_cache *block_group,struct btrfs_fs_info *info,u64 start,u64 end); +void btrfs_mark_bg_unused (struct btrfs_block_group_cache *bg);/* ctree.c */ int btrfs_bin_search(struct extent_buffer *eb, const struct btrfs_key *key,fs/btrfs/extent-tree.c +17
-19@@ -6350,16 +6350,8 @@ static int update_block_group(struct btrfs_trans_handle *trans,* dirty list to avoid races between cleaner kthread and space * cache writeout. */ -if (!alloc && old_val ==0 ) { -spin_lock (&info->unused_bgs_lock); -if (list_empty (&cache->bg_list)) { -btrfs_get_block_group (cache); -trace_btrfs_add_unused_block_group (cache); -list_add_tail (&cache->bg_list, - &info->unused_bgs); - } -spin_unlock (&info->unused_bgs_lock); - } +if (!alloc && old_val ==0 ) +btrfs_mark_bg_unused (cache);btrfs_put_block_group (cache); total -= num_bytes;@@ -10201,15 +10193,8 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)if (btrfs_chunk_readonly (info, cache->key.objectid)) {inc_block_group_ro (cache,1 ); }else if (btrfs_block_group_used (&cache->item) ==0 ) { -spin_lock (&info->unused_bgs_lock); -/* Should always be true but just in case. */ -if (list_empty (&cache->bg_list)) { -btrfs_get_block_group (cache); -trace_btrfs_add_unused_block_group (cache); -list_add_tail (&cache->bg_list, - &info->unused_bgs); - } -spin_unlock (&info->unused_bgs_lock); +ASSERT (list_empty (&cache->bg_list)); +btrfs_mark_bg_unused (cache); } }@@ -11133,3 +11118,16 @@ void btrfs_wait_for_snapshot_creation(struct btrfs_root *root) !atomic_read(&root->will_be_snapshotted)); } } + +void btrfs_mark_bg_unused (struct btrfs_block_group_cache *bg) +{ +struct btrfs_fs_info *fs_info = bg->fs_info; + +spin_lock (&fs_info->unused_bgs_lock); +if (list_empty (&bg->bg_list)) { +btrfs_get_block_group (bg); +trace_btrfs_add_unused_block_group (bg); +list_add_tail (&bg->bg_list, &fs_info->unused_bgs); + } +spin_unlock (&fs_info->unused_bgs_lock); +}fs/btrfs/scrub.c +1
-8@@ -3981,14 +3981,7 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,if (!cache->removed && !cache->ro && cache->reserved ==0 &&btrfs_block_group_used (&cache->item) ==0 ) {spin_unlock (&cache->lock); -spin_lock (&fs_info->unused_bgs_lock); -if (list_empty (&cache->bg_list)) { -btrfs_get_block_group (cache); -trace_btrfs_add_unused_block_group (cache); -list_add_tail (&cache->bg_list, - &fs_info->unused_bgs); - } -spin_unlock (&fs_info->unused_bgs_lock); +btrfs_mark_bg_unused (cache); }else {spin_unlock (&cache->lock); }
PATCH 3/3 btrfs: Delayed empty block group auto removal to next transaction
In reply to Qu WenruoUnder certain KVM load and LTP tests, we are possible to hit the following calltrace if quota is enabled:
------
BTRFS critical (device vda2): unable to find logical 8820195328 length 4096 BTRFS critical (device vda2): unable to find logical 8820195328 length 4096
------------[ cut here ]------------ WARNING: CPU: 0 PID: 49 at ../block/blk-core.c:172 blk_status_to_errno+0x1a/0x30 CPU: 0 PID: 49 Comm: kworker/u2:1 Not tainted 4.12.14-15-default #1 SLE15 (unreleased) Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014 Workqueue: btrfs-endio-write btrfs_endio_write_helper [btrfs] task: ffff9f827b340bc0 task.stack: ffffb4f8c0304000 RIP: 0010:blk_status_to_errno+0x1a/0x30 Call Trace: submit_extent_page+0x191/0x270 [btrfs] ? btrfs_create_repair_bio+0x130/0x130 [btrfs] __do_readpage+0x2d2/0x810 [btrfs] ? btrfs_create_repair_bio+0x130/0x130 [btrfs] ? run_one_async_done+0xc0/0xc0 [btrfs] __extent_read_full_page+0xe7/0x100 [btrfs] ? run_one_async_done+0xc0/0xc0 [btrfs] read_extent_buffer_pages+0x1ab/0x2d0 [btrfs] ? run_one_async_done+0xc0/0xc0 [btrfs] btree_read_extent_buffer_pages+0x94/0xf0 [btrfs] read_tree_block+0x31/0x60 [btrfs] read_block_for_search.isra.35+0xf0/0x2e0 [btrfs] btrfs_search_slot+0x46b/0xa00 [btrfs] ? kmem_cache_alloc+0x1a8/0x510 ? btrfs_get_token_32+0x5b/0x120 [btrfs] find_parent_nodes+0x11d/0xeb0 [btrfs] ? leaf_space_used+0xb8/0xd0 [btrfs] ? btrfs_leaf_free_space+0x49/0x90 [btrfs] ? btrfs_find_all_roots_safe+0x93/0x100 [btrfs] btrfs_find_all_roots_safe+0x93/0x100 [btrfs] btrfs_find_all_roots+0x45/0x60 [btrfs] btrfs_qgroup_trace_extent_post+0x20/0x40 [btrfs] btrfs_add_delayed_data_ref+0x1a3/0x1d0 [btrfs] btrfs_alloc_reserved_file_extent+0x38/0x40 [btrfs] insert_reserved_file_extent.constprop.71+0x289/0x2e0 [btrfs] btrfs_finish_ordered_io+0x2f4/0x7f0 [btrfs] ? pick_next_task_fair+0x2cd/0x530 ? __switch_to+0x92/0x4b0 btrfs_worker_helper+0x81/0x300 [btrfs] process_one_work+0x1da/0x3f0 worker_thread+0x2b/0x3f0 ? process_one_work+0x3f0/0x3f0 kthread+0x11a/0x130 ? kthread_create_on_node+0x40/0x40 ret_from_fork+0x35/0x40 Code: 00 00 5b c3 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 40 80 ff 0c 40 0f b6 c7 77 0b 48 c1 e0 04 8b 80 00 bf c8 bd c3 <0f> 0b b8 fb ff ff ff c3 0f 1f 40 00 66 2e 0f 1f 84 00 00 00 00 ---[ end trace f079fb809e7a862b ]---
BTRFS critical (device vda2): unable to find logical 8820195328 length 16384
BTRFS: error (device vda2) in btrfs_finish_ordered_io:3023: errno=-5 IO failure BTRFS info (device vda2): forced readonly
BTRFS error (device vda2): pending csums is 2887680
------
Although we haven't hit the same bug in real world, it shows that since qgroup is heavily relying on commit root, if block group auto removal happens and removed the empty block group, qgroup could failed to find its old referencers.
This patch will add a new member for btrfs_transaction, pending_unused_bgs.
Now unused bgs will go trans->transaction->pending_unused_bgs, then fs_info->unused_bgs, and finally get removed by btrfs_deleted_unused_bgs().
--- fs/btrfs/ctree.h | 3 ++- fs/btrfs/extent-tree.c | 34 ++++++++++++++++++++++++++++++---- fs/btrfs/scrub.c | 2 +- fs/btrfs/transaction.c | 7 +++++++ fs/btrfs/transaction.h | 10 ++++++++++ 5 files changed, 50 insertions(+), 6 deletions(-)
fs/btrfs/ctree.h +2 -1
@@ -2829,7 +2829,8 @@ void check_system_chunk(struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info, const u64 type);u64 add_new_free_space (struct btrfs_block_group_cache *block_group,struct btrfs_fs_info *info,u64 start,u64 end); -void btrfs_mark_bg_unused (struct btrfs_block_group_cache *bg); +void btrfs_mark_bg_unused (struct btrfs_block_group_cache *bg, +struct btrfs_trans_handle *trans);/* ctree.c */ int btrfs_bin_search(struct extent_buffer *eb, const struct btrfs_key *key,
fs/btrfs/extent-tree.c +30 -4
@@ -6351,7 +6351,7 @@ static int update_block_group(struct btrfs_trans_handle *trans,* cache writeout. */ if (!alloc && old_val ==0 ) -btrfs_mark_bg_unused (cache); +btrfs_mark_bg_unused (cache, trans);btrfs_put_block_group (cache); total -= num_bytes;
@@ -10194,7 +10194,7 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)inc_block_group_ro (cache,1 ); }else if (btrfs_block_group_used (&cache->item) ==0 ) {ASSERT (list_empty (&cache->bg_list)); -btrfs_mark_bg_unused (cache); +btrfs_mark_bg_unused (cache,NULL ); } }
@@ -11119,15 +11119,41 @@ void btrfs_wait_for_snapshot_creation(struct btrfs_root *root) } } -void btrfs_mark_bg_unused (struct btrfs_block_group_cache *bg) +/* +* Get a block group cache and mark it as unused (if not already +* unused/deleted) +* +* NOTE: +* Since qgroup could search commit root at any time, we can't just +* delete the block group and its chunk mapping in current trans. +* So we record bgs to be deleted in trans->transaction and then +* queue them into fs_info->unused_bgs. +* +* @bg: The block group cache +* @trans: The trans handler +*/ +void btrfs_mark_bg_unused (struct btrfs_block_group_cache *bg, +struct btrfs_trans_handle *trans) {struct btrfs_fs_info *fs_info = bg->fs_info;spin_lock (&fs_info->unused_bgs_lock);if (list_empty (&bg->bg_list)) { +/* +* Get one reference, will be freed at btrfs_delete_unused_bgs() +*/ btrfs_get_block_group (bg);trace_btrfs_add_unused_block_group (bg); -list_add_tail (&bg->bg_list, &fs_info->unused_bgs); + +/* +* If we don't have a trans, it means we are at mount time. +* It's completely fine to mark it ready to be deleted. +*/ +if (!trans) +list_add_tail (&bg->bg_list, &fs_info->unused_bgs); +else +list_add_tail (&bg->bg_list, + &trans->transaction->pending_unused_bgs); }spin_unlock (&fs_info->unused_bgs_lock); }
fs/btrfs/scrub.c +1 -1
@@ -3981,7 +3981,7 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,if (!cache->removed && !cache->ro && cache->reserved ==0 &&btrfs_block_group_used (&cache->item) ==0 ) {spin_unlock (&cache->lock); -btrfs_mark_bg_unused (cache); +btrfs_mark_bg_unused (cache,NULL ); }else {spin_unlock (&cache->lock); }
fs/btrfs/transaction.c +7 -0
@@ -264,6 +264,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,INIT_LIST_HEAD (&cur_trans->pending_snapshots);INIT_LIST_HEAD (&cur_trans->pending_chunks); +INIT_LIST_HEAD (&cur_trans->pending_unused_bgs);INIT_LIST_HEAD (&cur_trans->switch_commits);INIT_LIST_HEAD (&cur_trans->dirty_bgs);INIT_LIST_HEAD (&cur_trans->io_bgs);
@@ -2269,6 +2270,12 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)wake_up (&cur_trans->commit_wait);clear_bit (BTRFS_FS_NEED_ASYNC_COMMIT , &fs_info->flags); +/* transaction is done, queue pending unused bgs to cleaner */ +spin_lock (&fs_info->unused_bgs_lock); +list_splice_tail_init (&cur_trans->pending_unused_bgs, + &fs_info->unused_bgs); +spin_unlock (&fs_info->unused_bgs_lock); +spin_lock (&fs_info->trans_lock);list_del_init (&cur_trans->list);spin_unlock (&fs_info->trans_lock);
fs/btrfs/transaction.h +10 -0
@@ -57,6 +57,16 @@ struct btrfs_transaction {struct list_head switch_commits;struct list_head dirty_bgs; +/* +* Unused block groups waiting to be deleted after current transaction. +* Protected by fs_info->unused_bgs_lock. +* +* Since qgroup could search commit root, we can't delete unused block +* group in the same trans when it get emptied, but delay it to next +* transaction (or next mount if power loss happens) +*/ +struct list_head pending_unused_bgs; +/* * There is no explicit lock which protects io_bgs, rather its * consistency is implied by the fact that all the sites which modify
Signature · 1 line
2.17.0
Re: [PATCH 3/3] btrfs: Delayed empty block group auto removal to next transaction
Quoted 61 lines · Qu Wenruo
Under certain KVM load and LTP tests, we are possible to hit the following calltrace if quota is enabled:
------
BTRFS critical (device vda2): unable to find logical 8820195328 length 4096 BTRFS critical (device vda2): unable to find logical 8820195328 length 4096------------[ cut here ]------------ WARNING: CPU: 0 PID: 49 at ../block/blk-core.c:172 blk_status_to_errno+0x1a/0x30 CPU: 0 PID: 49 Comm: kworker/u2:1 Not tainted 4.12.14-15-default #1 SLE15 (unreleased) Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014 Workqueue: btrfs-endio-write btrfs_endio_write_helper [btrfs] task: ffff9f827b340bc0 task.stack: ffffb4f8c0304000 RIP: 0010:blk_status_to_errno+0x1a/0x30 Call Trace: submit_extent_page+0x191/0x270 [btrfs] ? btrfs_create_repair_bio+0x130/0x130 [btrfs] __do_readpage+0x2d2/0x810 [btrfs] ? btrfs_create_repair_bio+0x130/0x130 [btrfs] ? run_one_async_done+0xc0/0xc0 [btrfs] __extent_read_full_page+0xe7/0x100 [btrfs] ? run_one_async_done+0xc0/0xc0 [btrfs] read_extent_buffer_pages+0x1ab/0x2d0 [btrfs] ? run_one_async_done+0xc0/0xc0 [btrfs] btree_read_extent_buffer_pages+0x94/0xf0 [btrfs] read_tree_block+0x31/0x60 [btrfs] read_block_for_search.isra.35+0xf0/0x2e0 [btrfs] btrfs_search_slot+0x46b/0xa00 [btrfs] ? kmem_cache_alloc+0x1a8/0x510 ? btrfs_get_token_32+0x5b/0x120 [btrfs] find_parent_nodes+0x11d/0xeb0 [btrfs] ? leaf_space_used+0xb8/0xd0 [btrfs] ? btrfs_leaf_free_space+0x49/0x90 [btrfs] ? btrfs_find_all_roots_safe+0x93/0x100 [btrfs] btrfs_find_all_roots_safe+0x93/0x100 [btrfs] btrfs_find_all_roots+0x45/0x60 [btrfs] btrfs_qgroup_trace_extent_post+0x20/0x40 [btrfs] btrfs_add_delayed_data_ref+0x1a3/0x1d0 [btrfs] btrfs_alloc_reserved_file_extent+0x38/0x40 [btrfs] insert_reserved_file_extent.constprop.71+0x289/0x2e0 [btrfs] btrfs_finish_ordered_io+0x2f4/0x7f0 [btrfs] ? pick_next_task_fair+0x2cd/0x530 ? __switch_to+0x92/0x4b0 btrfs_worker_helper+0x81/0x300 [btrfs] process_one_work+0x1da/0x3f0 worker_thread+0x2b/0x3f0 ? process_one_work+0x3f0/0x3f0 kthread+0x11a/0x130 ? kthread_create_on_node+0x40/0x40 ret_from_fork+0x35/0x40 Code: 00 00 5b c3 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 40 80 ff 0c 40 0f b6 c7 77 0b 48 c1 e0 04 8b 80 00 bf c8 bd c3 <0f> 0b b8 fb ff ff ff c3 0f 1f 40 00 66 2e 0f 1f 84 00 00 00 00 ---[ end trace f079fb809e7a862b ]---BTRFS critical (device vda2): unable to find logical 8820195328 length 16384
BTRFS: error (device vda2) in btrfs_finish_ordered_io:3023: errno=-5 IO failure BTRFS info (device vda2): forced readonly
BTRFS error (device vda2): pending csums is 2887680
------Although we haven't hit the same bug in real world, it shows that since qgroup is heavily relying on commit root, if block group auto removal happens and removed the empty block group, qgroup could failed to find its old referencers.
Quoted 7 lines
This patch will add a new member for
btrfs_transaction,pending_unused_bgs.
Now unused bgs will gotrans->transaction->pending_unused_bgs, thenfs_info->unused_bgs, and finally get removed bybtrfs_deleted_unused_bgs().Signed-off-by Qu Wenruo <[email protected]>
Overall looks good and simple, couple of minor nits which are not really critical.
Quoted 36 lines
--- fs/btrfs/ctree.h | 3 ++- fs/btrfs/extent-tree.c | 34 ++++++++++++++++++++++++++++++---- fs/btrfs/scrub.c | 2 +- fs/btrfs/transaction.c | 7 +++++++ fs/btrfs/transaction.h | 10 ++++++++++ 5 files changed, 50 insertions(+), 6 deletions(-)fs/btrfs/ctree.h +2
-1@@ -2829,7 +2829,8 @@ void check_system_chunk(struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info, const u64 type);u64 add_new_free_space (struct btrfs_block_group_cache *block_group,struct btrfs_fs_info *info,u64 start,u64 end); -void btrfs_mark_bg_unused (struct btrfs_block_group_cache *bg); +void btrfs_mark_bg_unused (struct btrfs_block_group_cache *bg, +struct btrfs_trans_handle *trans);/* ctree.c */ int btrfs_bin_search(struct extent_buffer *eb, const struct btrfs_key *key,fs/btrfs/extent-tree.c +1
-1@@ -6351,7 +6351,7 @@ static int update_block_group(struct btrfs_trans_handle *trans,* cache writeout. */ if (!alloc && old_val ==0 ) -btrfs_mark_bg_unused (cache); +btrfs_mark_bg_unused (cache, trans);btrfs_put_block_group (cache); total -= num_bytes;
fs/btrfs/extent-tree.c int btrfs_read_block_groups(struct btrfs_fs_info *info) · line 10197
Quoted 5 lines
fs/btrfs/extent-tree.c +1
-1@@ -10194,7 +10194,7 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)inc_block_group_ro (cache,1 ); }else if (btrfs_block_group_used (&cache->item) ==0 ) {ASSERT (list_empty (&cache->bg_list)); -btrfs_mark_bg_unused (cache); +btrfs_mark_bg_unused (cache,NULL );
nit: btrfs_mark_bg_unused(cache, false); looks better than passing NULL, see below for full sugestion.
Quoted 17 lines
fs/btrfs/extent-tree.c +9
-1} }@@ -11119,15 +11119,41 @@ void btrfs_wait_for_snapshot_creation(struct btrfs_root *root) } } -void btrfs_mark_bg_unused (struct btrfs_block_group_cache *bg) +/* +* Get a block group cache and mark it as unused (if not already +* unused/deleted) +* +* NOTE: +* Since qgroup could search commit root at any time, we can't just +* delete the block group and its chunk mapping in current trans. +* So we record bgs to be deleted in trans->transaction and then +* queue them into fs_info->unused_bgs.
fs/btrfs/extent-tree.c void btrfs_wait_for_snapshot_creation(struct btrfs_root *root) · line 11140
Quoted 10 lines
fs/btrfs/extent-tree.c +6
-0+* +* @bg: The block group cache +* @trans: The trans handler +*/ +void btrfs_mark_bg_unused (struct btrfs_block_group_cache *bg, +struct btrfs_trans_handle *trans) {struct btrfs_fs_info *fs_info = bg->fs_info;spin_lock (&fs_info->unused_bgs_lock);
With this patch fs_info->unused_bgs_lock actually become sort-of global, since it protects the fs_info->unused_bgs, but it also protects the per-transaction pending_unused_bgs. I guess there shouldn't be much contention for that lock since we don't really delete that many unused bgs.
fs/btrfs/extent-tree.c void btrfs_wait_for_snapshot_creation(struct btrfs_root *root) · line 11144
Quoted 4 lines
fs/btrfs/extent-tree.c +3
-0if (list_empty (&bg->bg_list)) { +/* +* Get one reference, will be freed at btrfs_delete_unused_bgs() +*/
nit: Why not make the coment a single line /* text */
fs/btrfs/extent-tree.c void btrfs_wait_for_snapshot_creation(struct btrfs_root *root) · line 11152
Quoted 9 lines
fs/btrfs/extent-tree.c +6
-1btrfs_get_block_group (bg);trace_btrfs_add_unused_block_group (bg); -list_add_tail (&bg->bg_list, &fs_info->unused_bgs); + +/* +* If we don't have a trans, it means we are at mount time. +* It's completely fine to mark it ready to be deleted. +*/ +if (!trans)
Since you don't really use the transaction struct, perhaps make this parameter a boolean and name it something like fast_delete or direct_delete or quick_delete or some such? Then you can put this comment as the description of the parameter rather than in the code.
Quoted 50 lines
fs/btrfs/extent-tree.c +4
-0+list_add_tail (&bg->bg_list, &fs_info->unused_bgs); + else + list_add_tail(&bg->bg_list, + &trans->transaction->pending_unused_bgs); }spin_unlock (&fs_info->unused_bgs_lock); }fs/btrfs/scrub.c +1
-1@@ -3981,7 +3981,7 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,if (!cache->removed && !cache->ro && cache->reserved ==0 &&btrfs_block_group_used (&cache->item) ==0 ) {spin_unlock (&cache->lock); -btrfs_mark_bg_unused (cache); +btrfs_mark_bg_unused (cache,NULL ); }else {spin_unlock (&cache->lock); }fs/btrfs/transaction.c +7
-0@@ -264,6 +264,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,INIT_LIST_HEAD (&cur_trans->pending_snapshots);INIT_LIST_HEAD (&cur_trans->pending_chunks); +INIT_LIST_HEAD (&cur_trans->pending_unused_bgs);INIT_LIST_HEAD (&cur_trans->switch_commits);INIT_LIST_HEAD (&cur_trans->dirty_bgs);INIT_LIST_HEAD (&cur_trans->io_bgs);@@ -2269,6 +2270,12 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)wake_up (&cur_trans->commit_wait);clear_bit (BTRFS_FS_NEED_ASYNC_COMMIT , &fs_info->flags); +/* transaction is done, queue pending unused bgs to cleaner */ +spin_lock (&fs_info->unused_bgs_lock); +list_splice_tail_init (&cur_trans->pending_unused_bgs, + &fs_info->unused_bgs); +spin_unlock (&fs_info->unused_bgs_lock); +spin_lock (&fs_info->trans_lock);list_del_init (&cur_trans->list);spin_unlock (&fs_info->trans_lock);fs/btrfs/transaction.h +0
-0
fs/btrfs/transaction.h struct btrfs_transaction { · line 62
Quoted 6 lines
fs/btrfs/transaction.h +3
-0@@ -57,6 +57,16 @@ struct btrfs_transaction {struct list_head switch_commits;struct list_head dirty_bgs; +/* +* Unused block groups waiting to be deleted after current transaction. +* Protected by fs_info->unused_bgs_lock.
I think the right way to document the lock protecting the list would be : btrfs_fs_info::unuused_bgs_lock, since you want to refer to the type and not a de-factor naming standard (which might as well change in the future).
Quoted 11 lines
fs/btrfs/transaction.h +7
-0+* +* Since qgroup could search commit root, we can't delete unused block +* group in the same trans when it get emptied, but delay it to next +* transaction (or next mount if power loss happens) +*/ +struct list_head pending_unused_bgs; +/* * There is no explicit lock which protects io_bgs, rather its * consistency is implied by the fact that all the sites which modify
Re: [PATCH 3/3] btrfs: Delayed empty block group auto removal to next transaction
Quoted 145 lines · Nikolay Borisov
Quoted 61 lines · Qu Wenruo
Under certain KVM load and LTP tests, we are possible to hit the following calltrace if quota is enabled:
------
BTRFS critical (device vda2): unable to find logical 8820195328 length 4096 BTRFS critical (device vda2): unable to find logical 8820195328 length 4096------------[ cut here ]------------ WARNING: CPU: 0 PID: 49 at ../block/blk-core.c:172 blk_status_to_errno+0x1a/0x30 CPU: 0 PID: 49 Comm: kworker/u2:1 Not tainted 4.12.14-15-default #1 SLE15 (unreleased) Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014 Workqueue: btrfs-endio-write btrfs_endio_write_helper [btrfs] task: ffff9f827b340bc0 task.stack: ffffb4f8c0304000 RIP: 0010:blk_status_to_errno+0x1a/0x30 Call Trace: submit_extent_page+0x191/0x270 [btrfs] ? btrfs_create_repair_bio+0x130/0x130 [btrfs] __do_readpage+0x2d2/0x810 [btrfs] ? btrfs_create_repair_bio+0x130/0x130 [btrfs] ? run_one_async_done+0xc0/0xc0 [btrfs] __extent_read_full_page+0xe7/0x100 [btrfs] ? run_one_async_done+0xc0/0xc0 [btrfs] read_extent_buffer_pages+0x1ab/0x2d0 [btrfs] ? run_one_async_done+0xc0/0xc0 [btrfs] btree_read_extent_buffer_pages+0x94/0xf0 [btrfs] read_tree_block+0x31/0x60 [btrfs] read_block_for_search.isra.35+0xf0/0x2e0 [btrfs] btrfs_search_slot+0x46b/0xa00 [btrfs] ? kmem_cache_alloc+0x1a8/0x510 ? btrfs_get_token_32+0x5b/0x120 [btrfs] find_parent_nodes+0x11d/0xeb0 [btrfs] ? leaf_space_used+0xb8/0xd0 [btrfs] ? btrfs_leaf_free_space+0x49/0x90 [btrfs] ? btrfs_find_all_roots_safe+0x93/0x100 [btrfs] btrfs_find_all_roots_safe+0x93/0x100 [btrfs] btrfs_find_all_roots+0x45/0x60 [btrfs] btrfs_qgroup_trace_extent_post+0x20/0x40 [btrfs] btrfs_add_delayed_data_ref+0x1a3/0x1d0 [btrfs] btrfs_alloc_reserved_file_extent+0x38/0x40 [btrfs] insert_reserved_file_extent.constprop.71+0x289/0x2e0 [btrfs] btrfs_finish_ordered_io+0x2f4/0x7f0 [btrfs] ? pick_next_task_fair+0x2cd/0x530 ? __switch_to+0x92/0x4b0 btrfs_worker_helper+0x81/0x300 [btrfs] process_one_work+0x1da/0x3f0 worker_thread+0x2b/0x3f0 ? process_one_work+0x3f0/0x3f0 kthread+0x11a/0x130 ? kthread_create_on_node+0x40/0x40 ret_from_fork+0x35/0x40 Code: 00 00 5b c3 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 40 80 ff 0c 40 0f b6 c7 77 0b 48 c1 e0 04 8b 80 00 bf c8 bd c3 <0f> 0b b8 fb ff ff ff c3 0f 1f 40 00 66 2e 0f 1f 84 00 00 00 00 ---[ end trace f079fb809e7a862b ]---BTRFS critical (device vda2): unable to find logical 8820195328 length 16384
BTRFS: error (device vda2) in btrfs_finish_ordered_io:3023: errno=-5 IO failure BTRFS info (device vda2): forced readonly
BTRFS error (device vda2): pending csums is 2887680
------Although we haven't hit the same bug in real world, it shows that since qgroup is heavily relying on commit root, if block group auto removal happens and removed the empty block group, qgroup could failed to find its old referencers.
Quoted 7 lines
This patch will add a new member for
btrfs_transaction,pending_unused_bgs.
Now unused bgs will gotrans->transaction->pending_unused_bgs, thenfs_info->unused_bgs, and finally get removed bybtrfs_deleted_unused_bgs().Signed-off-by Qu Wenruo <[email protected]>Overall looks good and simple, couple of minor nits which are not really critical.
Quoted 36 lines
--- fs/btrfs/ctree.h | 3 ++- fs/btrfs/extent-tree.c | 34 ++++++++++++++++++++++++++++++---- fs/btrfs/scrub.c | 2 +- fs/btrfs/transaction.c | 7 +++++++ fs/btrfs/transaction.h | 10 ++++++++++ 5 files changed, 50 insertions(+), 6 deletions(-)fs/btrfs/ctree.h +2
-1@@ -2829,7 +2829,8 @@ void check_system_chunk(struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info, const u64 type);u64 add_new_free_space (struct btrfs_block_group_cache *block_group,struct btrfs_fs_info *info,u64 start,u64 end); -void btrfs_mark_bg_unused (struct btrfs_block_group_cache *bg); +void btrfs_mark_bg_unused (struct btrfs_block_group_cache *bg, +struct btrfs_trans_handle *trans);/* ctree.c */ int btrfs_bin_search(struct extent_buffer *eb, const struct btrfs_key *key,fs/btrfs/extent-tree.c +1
-1@@ -6351,7 +6351,7 @@ static int update_block_group(struct btrfs_trans_handle *trans,* cache writeout. */ if (!alloc && old_val ==0 ) -btrfs_mark_bg_unused (cache); +btrfs_mark_bg_unused (cache, trans);btrfs_put_block_group (cache); total -= num_bytes;fs/btrfs/extent-tree.c int btrfs_read_block_groups(struct btrfs_fs_info *info) · line 10197
Quoted 5 lines
fs/btrfs/extent-tree.c +1
-1@@ -10194,7 +10194,7 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)inc_block_group_ro (cache,1 ); }else if (btrfs_block_group_used (&cache->item) ==0 ) {ASSERT (list_empty (&cache->bg_list)); -btrfs_mark_bg_unused (cache); +btrfs_mark_bg_unused (cache,NULL );nit:
btrfs_mark_bg_unused(cache, false); looks better than passing NULL, see below for full sugestion.Quoted 26 lines
fs/btrfs/extent-tree.c +15
-1} }@@ -11119,15 +11119,41 @@ void btrfs_wait_for_snapshot_creation(struct btrfs_root *root) } } -void btrfs_mark_bg_unused (struct btrfs_block_group_cache *bg) +/* +* Get a block group cache and mark it as unused (if not already +* unused/deleted) +* +* NOTE: +* Since qgroup could search commit root at any time, we can't just +* delete the block group and its chunk mapping in current trans. +* So we record bgs to be deleted in trans->transaction and then +* queue them into fs_info->unused_bgs. +* +* @bg: The block group cache +* @trans: The trans handler +*/ +void btrfs_mark_bg_unused (struct btrfs_block_group_cache *bg, +struct btrfs_trans_handle *trans) {struct btrfs_fs_info *fs_info = bg->fs_info;
Quoted 5 lines
fs/btrfs/extent-tree.c void btrfs_wait_for_snapshot_creation(struct btrfs_root *root) · line 11140
Quoted 1 line
fs/btrfs/extent-tree.c +0
-0spin_lock (&fs_info->unused_bgs_lock);With this patch
fs_info->unused_bgs_lockactually become sort-of global, since it protects thefs_info->unused_bgs, but it also protects the per-transactionpending_unused_bgs.
Yep, that's true.
Quoted 2 lines
I guess there shouldn't be much
contention for that lock since we don't really delete that many unused bgs.
Right, that's one of the reasons I'm reusing the lock. Another problem is introducing new lock will increase the possibility about ABBA deadlock.
Since it's not really performance critical, I prefer using the global lock other than introducing a new one.
Quoted 6 lines
fs/btrfs/extent-tree.c void btrfs_wait_for_snapshot_creation(struct btrfs_root *root) · line 11144
Quoted 5 lines
fs/btrfs/extent-tree.c +3
-0if (list_empty (&bg->bg_list)) { +/* +* Get one reference, will be freed at btrfs_delete_unused_bgs() +*/ nit: Why not make the coment a single line /* text */
IIRC it's over 80 chars if single lined.
The last ')' is just the 80th character.
Quoted 14 lines
fs/btrfs/extent-tree.c void btrfs_wait_for_snapshot_creation(struct btrfs_root *root) · line 11152
Quoted 10 lines
fs/btrfs/extent-tree.c +6
-1btrfs_get_block_group (bg);trace_btrfs_add_unused_block_group (bg); -list_add_tail (&bg->bg_list, &fs_info->unused_bgs); + +/* +* If we don't have a trans, it means we are at mount time. +* It's completely fine to mark it ready to be deleted. +*/ +if (!trans)Since you don't really use the transaction struct, perhaps make this parameter a boolean and name it something like
fast_deleteordirect_deleteorquick_deleteor some such? Then you can put this comment as the description of the parameter rather than in the code.
I also considered this, however the problem here is we need extra function calls to get current transaction and need to keep an eye on the other call sites like in btrfs_read_block_groups().
Where if we're trying to start a transaction at improper time, it could cause extra problem.
Thus here using transaction handler provides a good balance for caller and the complexity of the function.
For caller, they don't really care about too much thing, if you have a trans handler, pass it.
If not, pass NULL. Pretty instinctive.
In fact, it's even better than a bool, in which case caller needs to determine which bool should be passed.
And the function has no need to try to start/get a transaction.
Thanks,
Qu
Quoted 77 lines
Quoted 50 lines
fs/btrfs/extent-tree.c +4
-0+list_add_tail (&bg->bg_list, &fs_info->unused_bgs); + else + list_add_tail(&bg->bg_list, + &trans->transaction->pending_unused_bgs); }spin_unlock (&fs_info->unused_bgs_lock); }fs/btrfs/scrub.c +1
-1@@ -3981,7 +3981,7 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,if (!cache->removed && !cache->ro && cache->reserved ==0 &&btrfs_block_group_used (&cache->item) ==0 ) {spin_unlock (&cache->lock); -btrfs_mark_bg_unused (cache); +btrfs_mark_bg_unused (cache,NULL ); }else {spin_unlock (&cache->lock); }fs/btrfs/transaction.c +7
-0@@ -264,6 +264,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,INIT_LIST_HEAD (&cur_trans->pending_snapshots);INIT_LIST_HEAD (&cur_trans->pending_chunks); +INIT_LIST_HEAD (&cur_trans->pending_unused_bgs);INIT_LIST_HEAD (&cur_trans->switch_commits);INIT_LIST_HEAD (&cur_trans->dirty_bgs);INIT_LIST_HEAD (&cur_trans->io_bgs);@@ -2269,6 +2270,12 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)wake_up (&cur_trans->commit_wait);clear_bit (BTRFS_FS_NEED_ASYNC_COMMIT , &fs_info->flags); +/* transaction is done, queue pending unused bgs to cleaner */ +spin_lock (&fs_info->unused_bgs_lock); +list_splice_tail_init (&cur_trans->pending_unused_bgs, + &fs_info->unused_bgs); +spin_unlock (&fs_info->unused_bgs_lock); +spin_lock (&fs_info->trans_lock);list_del_init (&cur_trans->list);spin_unlock (&fs_info->trans_lock);fs/btrfs/transaction.h +0
-0fs/btrfs/transaction.h struct btrfs_transaction { · line 62
Quoted 6 lines
fs/btrfs/transaction.h +3
-0@@ -57,6 +57,16 @@ struct btrfs_transaction {struct list_head switch_commits;struct list_head dirty_bgs; +/* +* Unused block groups waiting to be deleted after current transaction. +* Protected by fs_info->unused_bgs_lock. I think the right way to document the lock protecting the list would be :
btrfs_fs_info::unuused_bgs_lock, since you want to refer to the type and not a de-factor naming standard (which might as well change in the future).fs/btrfs/transaction.h struct btrfs_transaction { · line 72
Quoted 11 lines
fs/btrfs/transaction.h +7
-0+* +* Since qgroup could search commit root, we can't delete unused block +* group in the same trans when it get emptied, but delay it to next +* transaction (or next mount if power loss happens) +*/ +struct list_head pending_unused_bgs; +/* * There is no explicit lock which protects io_bgs, rather its * consistency is implied by the fact that all the sites which modify --
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 4 lines · Qu Wenruo
The patchset can be fetched from github:
https://github.com/adam900710/linux/tree/delayed_bg_removalIt's based on v4.17-rc5 branch.
Please refresh the patches on top of today's misc-next. The patch 2/3 has been there already and now is deleted as it conflicts with 1/3, so I'm going to apply them in the order as they are in this pathset.
Patch 3/3 is nontrivial and I'd like to have a closer look at the bg tracking and locking, 1 and 2 can go to misc-next now.