btrfs: Fix no_space on dd and rm loop
v2 cover btrfs: Fix no_space on dd and rm loop
From: Zhao Lei <[email protected]>
This is v2 of resend-fix-no-space.
Most of them are send in single patch, I resend them in patchset to make it easy to access.
Notice that "Btrfs: fix find_free_dev_extent() malfunction in case device tree has hole" from Forrest Liu in:
https://patchwork.kernel.org/patch/5800231/
is also need to fix all known no_space bug.
Changelog v1->v2:
1: Rebased on top of v4.0-rc7
2: Fixed a lock problem reported by:
'Tsutomu Itoh' <[email protected]>
3: Add Reviewed-by: Liu Bo <[email protected]>
to [PATCH 2/9] btrfs:
Tested by busy dd and rm loop script in 2000 times. I'll add xfstests for this case later.
This is available at fix_no_space branch on my tree:
git://github.com/zhaoleidd/btrfs.git
It is also included in integration-for-chris branch in above tree.
Thanks
Zhaolei
Zhao Lei (9):
btrfs: fix condition of commit transaction
btrfs: Fix tail space processing in find_free_dev_extent()
btrfs: Adjust commit-transaction condition to avoid NO_SPACE more
btrfs: Set relative data on clear btrfs_block_group_cache->pinned
btrfs: add WARN_ON() to check is space_info op current
btrfs: Fix NO_SPACE bug caused by delayed-iput
btrfs: Support busy loop of write and delete
btrfs: wait for delayed iputs on no space
btrfs: cleanup unused alloc_chunk varible
fs/btrfs/ctree.h | 1 + fs/btrfs/disk-io.c | 3 ++- fs/btrfs/extent-tree.c | 66 +++++++++++++++++++++++++++++++++++++++----------- fs/btrfs/inode.c | 4 +++ fs/btrfs/volumes.c | 24 +++++++++--------- 5 files changed, 72 insertions(+), 26 deletions(-)
Signature · 1 line
1.8.5.1
PATCH v2 1/9 btrfs: fix condition of commit transaction
From: Zhao Lei <[email protected]>
Old code bypass commit transaction when we don't have enough pinned space, but another case is there exist freed bgs in current transction, it have possibility to make alloc_chunk success.
This patch modify the condition to:
if (have_free_bg || have_pinned_space) commit_transaction()
Confirmed above action by printk before and after patch.
--- fs/btrfs/extent-tree.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
fs/btrfs/extent-tree.c +13 -7
@@ -3641,7 +3641,7 @@ int btrfs_check_data_free_space(struct inode *inode, u64 bytes)struct btrfs_root *root =BTRFS_I (inode)->root;struct btrfs_fs_info *fs_info = root->fs_info;u64 used; -int ret =0 , committed =0 , alloc_chunk =1 ; +int ret =0 , committed =0 , have_pinned_space =1 , alloc_chunk =1 ;/* make sure bytes are sectorsize aligned */ bytes =ALIGN (bytes, root->sectorsize);
@@ -3709,11 +3709,12 @@ alloc:/* * If we don't have enough pinned space to deal with this -* allocation don't bother committing the transaction. +* allocation, and no removed chunk in current transaction, +* don't bother committing the transaction. */ if (percpu_counter_compare (&data_sinfo->total_bytes_pinned, bytes) <0 ) - committed =1 ; + have_pinned_space =0 ;spin_unlock (&data_sinfo->lock);/* commit the current transaction and try again */
@@ -3725,10 +3726,15 @@ commit_trans: trans =btrfs_join_transaction (root);if (IS_ERR (trans))return PTR_ERR (trans); - ret =btrfs_commit_transaction (trans, root); -if (ret) -return ret; - goto again; +if (have_pinned_space || + trans->transaction->have_free_bgs) { + ret =btrfs_commit_transaction (trans, root); +if (ret) +return ret; + goto again; + }else { +btrfs_end_transaction (trans, root); + } } trace_btrfs_space_reservation(root->fs_info,
Signature · 1 line
1.8.5.1
PATCH v2 2/9 btrfs: Fix tail space processing in find_free_dev_extent()
In reply to ZhaoleiFrom: Zhao Lei <[email protected]>
It is another reason for NO_SPACE case.
When we found enough free space in loop and saved them to max_hole_start/size before, and tail space contains pending extent, origional innocent max_hole_start/size are reset in retry.
As a result, find_free_dev_extent() returns less space than it can, and cause NO_SPACE in user program.
--- fs/btrfs/volumes.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)
fs/btrfs/volumes.c +13 -11
@@ -1136,11 +1136,11 @@ int find_free_dev_extent(struct btrfs_trans_handle *trans, path =btrfs_alloc_path ();if (!path)return -ENOMEM ; -again: + max_hole_start = search_start; max_hole_size =0 ; - hole_size =0 ; +again:if (search_start >= search_end || device->is_tgtdev_for_dev_replace) { ret = -ENOSPC ; goto out;
@@ -1233,21 +1233,23 @@ next:* allocated dev extents, and when shrinking the device, * search_end may be smaller than search_start. */ -if (search_end > search_start) +if (search_end > search_start) { hole_size = search_end - search_start; -if (hole_size > max_hole_size) { - max_hole_start = search_start; - max_hole_size = hole_size; - } +if (contains_pending_extent (trans, device, &search_start, + hole_size)) { +btrfs_release_path (path); + goto again; + } -if (contains_pending_extent (trans, device, &search_start, hole_size)) { -btrfs_release_path (path); - goto again; +if (hole_size > max_hole_size) { + max_hole_start = search_start; + max_hole_size = hole_size; + } }/* See above. */ -if (hole_size < num_bytes) +if (max_hole_size < num_bytes) ret = -ENOSPC ;else ret =0 ;
Signature · 1 line
1.8.5.1
PATCH v2 3/9 btrfs: Adjust commit-transaction condition to avoid NO_SPACE more
In reply to ZhaoleiFrom: Zhao Lei <[email protected]>
If we have any chance to make a successful write, we should not give up.
This patch adjust commit-transaction condition from:
pinned >= wanted
to
left + pinned >= wanted
--- fs/btrfs/extent-tree.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
fs/btrfs/extent-tree.c +2 -1
@@ -3713,7 +3713,8 @@ alloc:* don't bother committing the transaction. */ if (percpu_counter_compare (&data_sinfo->total_bytes_pinned, - bytes) <0 ) + used + bytes - + data_sinfo->total_bytes) <0 ) have_pinned_space =0 ;spin_unlock (&data_sinfo->lock);
Signature · 1 line
1.8.5.1
PATCH v2 4/9 btrfs: Set relative data on clear btrfs_block_group_cache->pinned
In reply to ZhaoleiFrom: Zhao Lei <[email protected]>
Bug1:
space_info->bytes_readonly was set to very large(negative) value in btrfs_remove_block_group().
Reason:
Current code set block_group_cache->pinned = 0 in btrfs_delete_unused_bgs(), but above space was not counted to space_info->bytes_readonly.
Then in btrfs_remove_block_group():
block_group->space_info->bytes_readonly -= block_group->key.offset;
We can see following value in trace:
btrfs_remove_block_group: pid=2677 comm=btrfs-cleaner WARNING: bytes_readonly=12582912, key.offset=134217728Bug2:
space_info->total_bytes_pinned grow to value larger than fs size. In a 1.2G fs, we can get following trace log:
at first:
ZL_DEBUG: add_pinned_bytes: pid=2710 comm=sync change total_bytes_pinned flags=1 869793792 + 95944704 = 965738496
after some op:
ZL_DEBUG: add_pinned_bytes: pid=2770 comm=sync change total_bytes_pinned flags=1 1780178944 + 95944704 = 1876123648
after some op:
ZL_DEBUG: add_pinned_bytes: pid=3193 comm=sync change total_bytes_pinned flags=1 2924568576 + 95551488 = 3020120064...
Reason:
Similar to bug1, we also need to adjust space_info->total_bytes_pinned in above code block.
--- fs/btrfs/extent-tree.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
fs/btrfs/extent-tree.c +10 -0
@@ -9654,8 +9654,18 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)mutex_unlock (&fs_info->unused_bg_unpin_mutex);/* Reset pinned so btrfs_put_block_group doesn't complain */ +spin_lock (&space_info->lock); +spin_lock (&block_group->lock); + + space_info->bytes_pinned -= block_group->pinned; + space_info->bytes_readonly += block_group->pinned; +percpu_counter_add (&space_info->total_bytes_pinned, + -block_group->pinned); block_group->pinned =0 ; +spin_unlock (&block_group->lock); +spin_unlock (&space_info->lock); +/* * Btrfs_remove_chunk will abort the transaction if things go * horribly wrong.
Signature · 1 line
1.8.5.1
PATCH v2 5/9 btrfs: add WARN_ON() to check is space_info op current
In reply to ZhaoleiFrom: Zhao Lei <[email protected]>
space_info's value calculation is some complex and easy to cause bug, add WARN_ON() to help debug.
Changelog v1->v2:
Put WARN_ON()s under the ENOSPC_DEBUG mount option. Suggested by: David Sterba <[email protected]>
--- fs/btrfs/extent-tree.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
fs/btrfs/extent-tree.c +10 -0
@@ -9464,9 +9464,19 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,spin_lock (&block_group->space_info->lock);list_del_init (&block_group->ro_list); + +if (btrfs_test_opt (root,ENOSPC_DEBUG )) { +WARN_ON (block_group->space_info->total_bytes + < block_group->key.offset); +WARN_ON (block_group->space_info->bytes_readonly + < block_group->key.offset); +WARN_ON (block_group->space_info->disk_total + < block_group->key.offset * factor); + } block_group->space_info->total_bytes -= block_group->key.offset; block_group->space_info->bytes_readonly -= block_group->key.offset; block_group->space_info->disk_total -= block_group->key.offset * factor; +spin_unlock (&block_group->space_info->lock);memcpy (&key, &block_group->key,sizeof (key));
Signature · 1 line
1.8.5.1
PATCH v2 6/9 btrfs: Fix NO_SPACE bug caused by delayed-iput
In reply to ZhaoleiFrom: Zhao Lei <[email protected]>
Steps to reproduce:
while true; do
dd if=/dev/zero of=/btrfs_dir/file count=[fs_size * 75%]
rm /btrfs_dir/file
syncdone
And we'll see dd failed because btrfs return NO_SPACE.
Reason:
Normally, btrfs_commit_transaction() call btrfs_run_delayed_iputs() in end to free fs space for next write, but sometimes it hadn't done work on time, because btrfs-cleaner thread get delayed-iputs from list before, but do iput() after next write.
This is log:
[ 2569.050776] comm=btrfs-cleaner func=btrfs_evict_inode() begin
[ 2569.084280] comm=sync func=btrfs_commit_transaction() call btrfs_run_delayed_iputs() [ 2569.085418] comm=sync func=btrfs_commit_transaction() done btrfs_run_delayed_iputs() [ 2569.087554] comm=sync func=btrfs_commit_transaction() end
[ 2569.191081] comm=dd begin [ 2569.790112] comm=dd func=__btrfs_buffered_write() ret=-28
[ 2569.847479] comm=btrfs-cleaner func=add_pinned_bytes() 0 + 32677888 = 32677888 [ 2569.849530] comm=btrfs-cleaner func=add_pinned_bytes() 32677888 + 23834624 = 56512512 ... [ 2569.903893] comm=btrfs-cleaner func=add_pinned_bytes() 943976448 + 21762048 = 965738496 [ 2569.908270] comm=btrfs-cleaner func=btrfs_evict_inode() end
Fix:
Make btrfs_commit_transaction() wait current running btrfs-cleaner's delayed-iputs() done in end.
Test:
Use script similar to above(more complex),
before patch:
7 failed in 100 * 20 loop.
after patch:
0 failed in 100 * 20 loop.--- fs/btrfs/ctree.h | 1 + fs/btrfs/disk-io.c | 3 ++- fs/btrfs/extent-tree.c | 6 ++++++ fs/btrfs/inode.c | 4 ++++ 4 files changed, 13 insertions(+), 1 deletion(-)
fs/btrfs/ctree.h +1 -0
@@ -1513,6 +1513,7 @@ struct btrfs_fs_info {spinlock_t delayed_iput_lock;struct list_head delayed_iputs; +struct rw_semaphore delayed_iput_sem;/* this protects tree_mod_seq_list */ spinlock_t tree_mod_seq_lock;
fs/btrfs/disk-io.c +2 -1
@@ -2241,11 +2241,12 @@ int open_ctree(struct super_block *sb,spin_lock_init (&fs_info->qgroup_op_lock);spin_lock_init (&fs_info->buffer_lock);spin_lock_init (&fs_info->unused_bgs_lock); -mutex_init (&fs_info->unused_bg_unpin_mutex);rwlock_init (&fs_info->tree_mod_log_lock); +mutex_init (&fs_info->unused_bg_unpin_mutex);mutex_init (&fs_info->reloc_mutex);mutex_init (&fs_info->delalloc_root_mutex);seqlock_init (&fs_info->profiles_lock); +init_rwsem (&fs_info->delayed_iput_sem);init_completion (&fs_info->kobj_unregister);INIT_LIST_HEAD (&fs_info->dirty_cowonly_roots);
fs/btrfs/extent-tree.c +6 -0
@@ -3732,6 +3732,12 @@ commit_trans: ret =btrfs_commit_transaction (trans, root);if (ret)return ret; +/* +* make sure that all running delayed iput are +* done +*/ +down_write (&root->fs_info->delayed_iput_sem); +up_write (&root->fs_info->delayed_iput_sem); goto again; }else {btrfs_end_transaction (trans, root);
fs/btrfs/inode.c +4 -0
@@ -3110,6 +3110,8 @@ void btrfs_run_delayed_iputs(struct btrfs_root *root)if (empty)return ; +down_read (&fs_info->delayed_iput_sem); +spin_lock (&fs_info->delayed_iput_lock);list_splice_init (&fs_info->delayed_iputs, &list);spin_unlock (&fs_info->delayed_iput_lock);
@@ -3120,6 +3122,8 @@ void btrfs_run_delayed_iputs(struct btrfs_root *root)iput (delayed->inode);kfree (delayed); } + +up_read (&root->fs_info->delayed_iput_sem); }/*
Signature · 1 line
1.8.5.1
Re: [PATCH 6/9] btrfs: Fix NO_SPACE bug caused by delayed-iput
Hi,
Quoted 113 lines · Zhaolei
From: Zhao Lei <[email protected]>
Steps to reproduce:
while true; dodd if=/dev/zero of=/btrfs_dir/file count=[fs_size * 75%] rm /btrfs_dir/file syncdone
And we'll see dd failed because btrfs return
NO_SPACE.Reason:
Normally,btrfs_commit_transaction()callbtrfs_run_delayed_iputs()in end to free fs space for next write, but sometimes it hadn't done work on time, because btrfs-cleaner thread get delayed-iputs from list before, but doiput()after next write.This is log:
[ 2569.050776] comm=btrfs-cleaner func=btrfs_evict_inode() begin[ 2569.084280] comm=sync func=btrfs_commit_transaction() call btrfs_run_delayed_iputs() [ 2569.085418] comm=sync func=btrfs_commit_transaction() done btrfs_run_delayed_iputs() [ 2569.087554] comm=sync func=btrfs_commit_transaction() end[ 2569.191081] comm=dd begin [ 2569.790112] comm=dd func=__btrfs_buffered_write() ret=-28[ 2569.847479] comm=btrfs-cleaner func=add_pinned_bytes() 0 + 32677888 = 32677888 [ 2569.849530] comm=btrfs-cleaner func=add_pinned_bytes() 32677888 + 23834624 = 56512512 ... [ 2569.903893] comm=btrfs-cleaner func=add_pinned_bytes() 943976448 + 21762048 = 965738496 [ 2569.908270] comm=btrfs-cleaner func=btrfs_evict_inode() endFix:
Makebtrfs_commit_transaction()wait current running btrfs-cleaner's delayed-iputs() done in end.Test:
Use script similar to above(more complex),
before patch:7 failed in 100 * 20 loop. after patch: 0 failed in 100 * 20 loop.Signed-off-by Zhao Lei <[email protected]>--- fs/btrfs/ctree.h | 1 + fs/btrfs/disk-io.c | 3 ++- fs/btrfs/extent-tree.c | 6 ++++++ fs/btrfs/inode.c | 4 ++++ 4 files changed, 13 insertions(+), 1 deletion(-)fs/btrfs/ctree.h +1
-0@@ -1513,6 +1513,7 @@ struct btrfs_fs_info {spinlock_t delayed_iput_lock;struct list_head delayed_iputs; +struct rw_semaphore delayed_iput_sem;/* this protects tree_mod_seq_list */ spinlock_t tree_mod_seq_lock;fs/btrfs/disk-io.c +2
-1@@ -2241,11 +2241,12 @@ int open_ctree(struct super_block *sb,spin_lock_init (&fs_info->qgroup_op_lock);spin_lock_init (&fs_info->buffer_lock);spin_lock_init (&fs_info->unused_bgs_lock); -mutex_init (&fs_info->unused_bg_unpin_mutex);rwlock_init (&fs_info->tree_mod_log_lock); +mutex_init (&fs_info->unused_bg_unpin_mutex);mutex_init (&fs_info->reloc_mutex);mutex_init (&fs_info->delalloc_root_mutex);seqlock_init (&fs_info->profiles_lock); +init_rwsem (&fs_info->delayed_iput_sem);init_completion (&fs_info->kobj_unregister);INIT_LIST_HEAD (&fs_info->dirty_cowonly_roots);fs/btrfs/extent-tree.c +6
-0@@ -3732,6 +3732,12 @@ commit_trans: ret =btrfs_commit_transaction (trans, root);if (ret)return ret; +/* +* make sure that all running delayed iput are +* done +*/ +down_write (&root->fs_info->delayed_iput_sem); +up_write (&root->fs_info->delayed_iput_sem); goto again; }else {btrfs_end_transaction (trans, root);fs/btrfs/inode.c +2
-0@@ -3110,6 +3110,8 @@ void btrfs_run_delayed_iputs(struct btrfs_root *root)if (empty)return ; +down_read (&fs_info->delayed_iput_sem); +spin_lock (&fs_info->delayed_iput_lock);list_splice_init (&fs_info->delayed_iputs, &list);spin_unlock (&fs_info->delayed_iput_lock);
fs/btrfs/inode.c void btrfs_run_delayed_iputs(struct btrfs_root *root) · line 3126
Quoted 5 lines
fs/btrfs/inode.c +2
-0@@ -3120,6 +3122,8 @@ void btrfs_run_delayed_iputs(struct btrfs_root *root)iput (delayed->inode);kfree (delayed); } + +up_read (&root->fs_info->delayed_iput_sem);
By introducing this "delayed_iput_sem", fstests's generic/241 cries about a possible deadlock, can we use a waitqueue to avoid this?
[ 2061.345955] =============================================
[ 2061.346027] [ INFO: possible recursive locking detected ]
[ 2061.346027] 4.1.0+ #268 Tainted: G W
[ 2061.346027] ---------------------------------------------
[ 2061.346027] btrfs-cleaner/3045 is trying to acquire lock:
[ 2061.346027] (&fs_info->delayed_iput_sem){++++..}, at:
[<ffffffff814063ab>] btrfs_run_delayed_iputs+0x6b/0x100
[ 2061.346027] but task is already holding lock:
[ 2061.346027] (&fs_info->delayed_iput_sem){++++..}, at: [<ffffffff814063ab>] btrfs_run_delayed_iputs+0x6b/0x100
[ 2061.346027] other info that might help us debug this:
[ 2061.346027] Possible unsafe locking scenario:[ 2061.346027] CPU0 [ 2061.346027] ---- [ 2061.346027] lock(&fs_info->delayed_iput_sem); [ 2061.346027] lock(&fs_info->delayed_iput_sem); [ 2061.346027] *** DEADLOCK ***
[ 2061.346027] May be due to missing lock nesting notation
[ 2061.346027] 2 locks held by btrfs-cleaner/3045:
[ 2061.346027] #0: (&fs_info->cleaner_mutex){+.+.+.}, at: [<ffffffff813f6cc4>] cleaner_kthread+0x64/0x1a0
[ 2061.346027] #1: (&fs_info->delayed_iput_sem){++++..}, at: [<ffffffff814063ab>] btrfs_run_delayed_iputs+0x6b/0x100
[ 2061.346027] stack backtrace:
[ 2061.346027] CPU: 0 PID: 3045 Comm: btrfs-cleaner Tainted: G W 4.1.0+ #268
[ 2061.346027] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140709_153950- 04/01/2014
[ 2061.346027] ffff88013760a700 ffff8800b4aff888 ffffffff818bb68e 0000000000000007
[ 2061.346027] ffffffff828fa0a0 ffff8800b4aff968 ffffffff810db18d ffff8800b4aff978
[ 2061.346027] ffffffff00000000 ffff88013760a750 ffffffff00000001 ffff88013760b470
[ 2061.346027] Call Trace:
[ 2061.346027] [<ffffffff818bb68e>] dump_stack+0x4c/0x65
[ 2061.346027] [<ffffffff810db18d>] __lock_acquire+0x214d/0x22f0
[ 2061.346027] [<ffffffff810d5e3d>] ? trace_hardirqs_off+0xd/0x10
[ 2061.346027] [<ffffffff810d649d>] ? __lock_is_held+0x4d/0x70
[ 2061.346027] [<ffffffff810dc050>] lock_acquire+0xd0/0x280
[ 2061.346027] [<ffffffff814063ab>] ? btrfs_run_delayed_iputs+0x6b/0x100
[ 2061.346027] [<ffffffff818c275c>] down_read+0x4c/0x70
[ 2061.346027] [<ffffffff814063ab>] ? btrfs_run_delayed_iputs+0x6b/0x100
[ 2061.346027] [<ffffffff818c48cb>] ? _raw_spin_unlock+0x2b/0x40
[ 2061.346027] [<ffffffff814063ab>] btrfs_run_delayed_iputs+0x6b/0x100
[ 2061.346027] [<ffffffff8140042f>] ? btrfs_commit_transaction+0xa1f/0xc50
[ 2061.346027] [<ffffffff81400454>] btrfs_commit_transaction+0xa44/0xc50
[ 2061.346027] [<ffffffff810cdbd0>] ? wait_woken+0xa0/0xa0
[ 2061.346027] [<ffffffff813e5247>] flush_space+0x97/0x4b0
[ 2061.346027] [<ffffffff818c48cb>] ? _raw_spin_unlock+0x2b/0x40
[ 2061.346027] [<ffffffff813e5a3b>] ? reserve_metadata_bytes+0x20b/0x6d0
[ 2061.346027] [<ffffffff813e5a52>] reserve_metadata_bytes+0x222/0x6d0
[ 2061.346027] [<ffffffff813e6245>] ? btrfs_block_rsv_refill+0x65/0x90
[ 2061.346027] [<ffffffff813e6257>] btrfs_block_rsv_refill+0x77/0x90
[ 2061.346027] [<ffffffff8140c723>] btrfs_evict_inode+0x473/0x700
[ 2061.346027] [<ffffffff8123dcbb>] evict+0xab/0x170
[ 2061.346027] [<ffffffff8123e6fd>] iput+0x1cd/0x350
[ 2061.346027] [<ffffffff81406409>] btrfs_run_delayed_iputs+0xc9/0x100
[ 2061.346027] [<ffffffff813f6cc4>] ? cleaner_kthread+0x64/0x1a0
[ 2061.346027] [<ffffffff813f6dba>] cleaner_kthread+0x15a/0x1a0
[ 2061.346027] [<ffffffff813f6c60>] ? btree_invalidatepage+0x90/0x90
[ 2061.346027] [<ffffffff810a3d5f>] kthread+0xef/0x110
[ 2061.346027] [<ffffffff810a3c70>] ? kthread_create_on_node+0x210/0x210
[ 2061.346027] [<ffffffff818c550f>] ret_from_fork+0x3f/0x70
[ 2061.346027] [<ffffffff810a3c70>] ? kthread_create_on_node+0x210/0x210Thanks,
-liubo
Quoted 10 lines
fs/btrfs/inode.c +0
-0}/* Signature · 6 lines
1.8.5.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
RE: [PATCH 6/9] btrfs: Fix NO_SPACE bug caused by delayed-iput
Hi, Liubo
Quoted 199 lines
-----Original Message-----
From: Liu Bo [mailto:[email protected]]
Sent: Tuesday, July 07, 2015 4:14 PM
To: Zhaolei
Cc: [email protected]
Subject: Re: [PATCH 6/9] btrfs: FixNO_SPACEbug caused by delayed-iputHi,
Quoted 17 lines · Zhaolei
From: Zhao Lei <[email protected]>
Steps to reproduce:
while true; dodd if=/dev/zero of=/btrfs_dir/file count=[fs_size * 75%] rm /btrfs_dir/file syncdone
And we'll see dd failed because btrfs return
NO_SPACE.Reason:
Normally,btrfs_commit_transaction()callbtrfs_run_delayed_iputs()in end to free fs space for next write, but sometimes it hadn't done work on time, because btrfs-cleaner thread get delayed-iputs from list before, but doiput()after next write.Quoted 4 lines
This is log:
[ 2569.050776] comm=btrfs-cleaner func=btrfs_evict_inode() begin[ 2569.084280] comm=sync func=btrfs_commit_transaction() call
btrfs_run_delayed_iputs()Quoted 1 line
[ 2569.085418] comm=sync func=btrfs_commit_transaction() done
btrfs_run_delayed_iputs()Quoted 6 lines
[ 2569.087554] comm=sync func=btrfs_commit_transaction() end[ 2569.191081] comm=dd begin [ 2569.790112] comm=dd func=__btrfs_buffered_write() ret=-28[ 2569.847479] comm=btrfs-cleaner func=add_pinned_bytes() 0 +32677888 = 32677888
Quoted 1 line
[ 2569.849530] comm=btrfs-cleaner func=add_pinned_bytes() 32677888 +23834624 = 56512512
Quoted 2 lines
...
[ 2569.903893] comm=btrfs-cleaner func=add_pinned_bytes() 943976448+ 21762048 = 965738496
Quoted 58 lines
[ 2569.908270] comm=btrfs-cleaner func=btrfs_evict_inode() endFix:
Makebtrfs_commit_transaction()wait current running btrfs-cleaner's delayed-iputs() done in end.Test:
Use script similar to above(more complex),
before patch:7 failed in 100 * 20 loop. after patch: 0 failed in 100 * 20 loop.Signed-off-by Zhao Lei <[email protected]>--- fs/btrfs/ctree.h | 1 + fs/btrfs/disk-io.c | 3 ++- fs/btrfs/extent-tree.c | 6 ++++++ fs/btrfs/inode.c | 4 ++++ 4 files changed, 13 insertions(+), 1 deletion(-)fs/btrfs/ctree.h index renamed from fs/btrfs/ctree.h +0
-0f9c89ca..54d4d78 100644
fs/btrfs/ctree.h +1
-0@@ -1513,6 +1513,7 @@ struct btrfs_fs_info {spinlock_t delayed_iput_lock;struct list_head delayed_iputs; +struct rw_semaphore delayed_iput_sem;/* this protects tree_mod_seq_list */ spinlock_t tree_mod_seq_lock;fs/btrfs/disk-io.c index renamed from fs/btrfs/disk-io.c +0
-0639f266..6867471 100644
fs/btrfs/disk-io.c +2
-1@@ -2241,11 +2241,12 @@ int open_ctree(struct super_block *sb,spin_lock_init (&fs_info->qgroup_op_lock);spin_lock_init (&fs_info->buffer_lock);spin_lock_init (&fs_info->unused_bgs_lock); -mutex_init (&fs_info->unused_bg_unpin_mutex);rwlock_init (&fs_info->tree_mod_log_lock); +mutex_init (&fs_info->unused_bg_unpin_mutex);mutex_init (&fs_info->reloc_mutex);mutex_init (&fs_info->delalloc_root_mutex);seqlock_init (&fs_info->profiles_lock); +init_rwsem (&fs_info->delayed_iput_sem);init_completion (&fs_info->kobj_unregister);INIT_LIST_HEAD (&fs_info->dirty_cowonly_roots);fs/btrfs/extent-tree.c index renamed from fs/btrfs/extent-tree.c +0
-0203ac63..6fd7dca 100644
fs/btrfs/extent-tree.c +0
-0@@ -3732,6 +3732,12 @@ commit_trans: ret =btrfs_commit_transaction (trans, root);if (ret)fs/btrfs/extent-tree.c commit_trans: · line 3743
Quoted 10 lines
fs/btrfs/extent-tree.c +6
-0return ret; +/* +* make sure that all running delayed iput are +* done +*/ +down_write (&root->fs_info->delayed_iput_sem); +up_write (&root->fs_info->delayed_iput_sem); goto again; }else { btrfs_end_transaction(trans, root); diff --git
a/fs/btrfs/inode.cfs/btrfs/inode.c void btrfs_run_delayed_iputs(struct btrfs_root
Quoted 4 lines
b/fs/btrfs/inode.cindex d2e732d..34d10be 100644fs/btrfs/inode.c +0
-0@@ -3110,6 +3110,8 @@ void btrfs_run_delayed_iputs(struct btrfs_root*root)
fs/btrfs/inode.c void btrfs_run_delayed_iputs(struct btrfs_root
Quoted 9 lines
fs/btrfs/inode.c +2
-0if (empty)return ; +down_read (&fs_info->delayed_iput_sem); +spin_lock (&fs_info->delayed_iput_lock);list_splice_init (&fs_info->delayed_iputs, &list);spin_unlock (&fs_info->delayed_iput_lock);@@ -3120,6 +3122,8 @@ void btrfs_run_delayed_iputs(struct btrfs_root*root)
fs/btrfs/inode.c void btrfs_run_delayed_iputs(struct btrfs_root *root) · line 3126
Quoted 5 lines
fs/btrfs/inode.c +2
-0iput (delayed->inode);kfree (delayed); } + +up_read (&root->fs_info->delayed_iput_sem);By introducing this "
delayed_iput_sem", fstests's generic/241 cries about a possible deadlock, can we use a waitqueue to avoid this?[ 2061.345955] ============================================= [ 2061.346027] [ INFO: possible recursive locking detected ] [ 2061.346027] 4.1.0+ #268 Tainted: G W [ 2061.346027] --------------------------------------------- [ 2061.346027] btrfs-cleaner/3045 is trying to acquire lock: [ 2061.346027] (&fs_info->delayed_iput_sem){++++..}, at: [<ffffffff814063ab>] btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] but task is already holding lock: [ 2061.346027] (&fs_info->delayed_iput_sem){++++..}, at: [<ffffffff814063ab>] btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] other info that might help us debug this: [ 2061.346027] Possible unsafe locking scenario:[ 2061.346027] CPU0 [ 2061.346027] ---- [ 2061.346027] lock(&fs_info->delayed_iput_sem); [ 2061.346027] lock(&fs_info->delayed_iput_sem); [ 2061.346027] *** DEADLOCK ***[ 2061.346027] May be due to missing lock nesting notation[ 2061.346027] 2 locks held by btrfs-cleaner/3045: [ 2061.346027] #0: (&fs_info->cleaner_mutex){+.+.+.}, at: [<ffffffff813f6cc4>] cleaner_kthread+0x64/0x1a0 [ 2061.346027] #1: (&fs_info->delayed_iput_sem){++++..}, at: [<ffffffff814063ab>] btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] stack backtrace: [ 2061.346027] CPU: 0 PID: 3045 Comm: btrfs-cleaner Tainted: G W 4.1.0+ #268 [ 2061.346027] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140709_153950- 04/01/2014 [ 2061.346027] ffff88013760a700 ffff8800b4aff888 ffffffff818bb68e 0000000000000007 [ 2061.346027] ffffffff828fa0a0 ffff8800b4aff968 ffffffff810db18d ffff8800b4aff978 [ 2061.346027] ffffffff00000000 ffff88013760a750 ffffffff00000001 ffff88013760b470 [ 2061.346027] Call Trace: [ 2061.346027] [<ffffffff818bb68e>] dump_stack+0x4c/0x65 [ 2061.346027] [<ffffffff810db18d>] __lock_acquire+0x214d/0x22f0 [ 2061.346027] [<ffffffff810d5e3d>] ? trace_hardirqs_off+0xd/0x10 [ 2061.346027] [<ffffffff810d649d>] ? __lock_is_held+0x4d/0x70 [ 2061.346027] [<ffffffff810dc050>] lock_acquire+0xd0/0x280 [ 2061.346027] [<ffffffff814063ab>] ? btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] [<ffffffff818c275c>] down_read+0x4c/0x70 [ 2061.346027] [<ffffffff814063ab>] ? btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] [<ffffffff818c48cb>] ? _raw_spin_unlock+0x2b/0x40 [ 2061.346027] [<ffffffff814063ab>] btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] [<ffffffff8140042f>] ? btrfs_commit_transaction+0xa1f/0xc50 [ 2061.346027] [<ffffffff81400454>] btrfs_commit_transaction+0xa44/0xc50 [ 2061.346027] [<ffffffff810cdbd0>] ? wait_woken+0xa0/0xa0 [ 2061.346027] [<ffffffff813e5247>] flush_space+0x97/0x4b0 [ 2061.346027] [<ffffffff818c48cb>] ? _raw_spin_unlock+0x2b/0x40 [ 2061.346027] [<ffffffff813e5a3b>] ? reserve_metadata_bytes+0x20b/0x6d0 [ 2061.346027] [<ffffffff813e5a52>] reserve_metadata_bytes+0x222/0x6d0 [ 2061.346027] [<ffffffff813e6245>] ? btrfs_block_rsv_refill+0x65/0x90 [ 2061.346027] [<ffffffff813e6257>] btrfs_block_rsv_refill+0x77/0x90 [ 2061.346027] [<ffffffff8140c723>] btrfs_evict_inode+0x473/0x700 [ 2061.346027] [<ffffffff8123dcbb>] evict+0xab/0x170 [ 2061.346027] [<ffffffff8123e6fd>] iput+0x1cd/0x350 [ 2061.346027] [<ffffffff81406409>]
Quoted 9 lines
btrfs_run_delayed_iputs+0xc9/0x100 [ 2061.346027] [<ffffffff813f6cc4>] ? cleaner_kthread+0x64/0x1a0 [ 2061.346027] [<ffffffff813f6dba>] cleaner_kthread+0x15a/0x1a0 [ 2061.346027] [<ffffffff813f6c60>] ? btree_invalidatepage+0x90/0x90 [ 2061.346027] [<ffffffff810a3d5f>] kthread+0xef/0x110 [ 2061.346027] [<ffffffff810a3c70>] ? kthread_create_on_node+0x210/0x210 [ 2061.346027] [<ffffffff818c550f>] ret_from_fork+0x3f/0x70 [ 2061.346027] [<ffffffff810a3c70>] ? kthread_create_on_node+0x210/0x210
Thanks for report above problem.
Seems it is caused by recursion of btrfs_run_delayed_iputs:
cleaner_kthread
-> btrfs_run_delayed_iputs() *1
-> get delayed_iput_sem lock *2
-> iput()
-> ...
-> btrfs_commit_transaction()
-> btrfs_run_delayed_iputs() *1
-> get delayed_iput_sem lock (dead lock) *2
*1: recursion of btrfs_run_delayed_iputs()
*2: dead lock of delayed_iput_sem
I'll reproduce this problem in my env, and comfirm fix of above problem.
Thanks
Zhaolei
Quoted 14 lines
Thanks,
-liubo
Quoted 10 lines
fs/btrfs/inode.c +0
-1}/* -- 1.8.5.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
Re: [PATCH 6/9] btrfs: Fix NO_SPACE bug caused by delayed-iput
Quoted 222 lines · Zhao Lei
Hi, Liubo
Quoted 199 lines
-----Original Message-----
From: Liu Bo [mailto:[email protected]]
Sent: Tuesday, July 07, 2015 4:14 PM
To: Zhaolei
Cc: [email protected]
Subject: Re: [PATCH 6/9] btrfs: FixNO_SPACEbug caused by delayed-iputHi,
Quoted 17 lines · Zhaolei
From: Zhao Lei <[email protected]>
Steps to reproduce:
while true; dodd if=/dev/zero of=/btrfs_dir/file count=[fs_size * 75%] rm /btrfs_dir/file syncdone
And we'll see dd failed because btrfs return
NO_SPACE.Reason:
Normally,btrfs_commit_transaction()callbtrfs_run_delayed_iputs()in end to free fs space for next write, but sometimes it hadn't done work on time, because btrfs-cleaner thread get delayed-iputs from list before, but doiput()after next write.Quoted 4 lines
This is log:
[ 2569.050776] comm=btrfs-cleaner func=btrfs_evict_inode() begin[ 2569.084280] comm=sync func=btrfs_commit_transaction() call
btrfs_run_delayed_iputs()Quoted 1 line
[ 2569.085418] comm=sync func=btrfs_commit_transaction() done
btrfs_run_delayed_iputs()Quoted 6 lines
[ 2569.087554] comm=sync func=btrfs_commit_transaction() end[ 2569.191081] comm=dd begin [ 2569.790112] comm=dd func=__btrfs_buffered_write() ret=-28[ 2569.847479] comm=btrfs-cleaner func=add_pinned_bytes() 0 +32677888 = 32677888
Quoted 1 line
[ 2569.849530] comm=btrfs-cleaner func=add_pinned_bytes() 32677888 +23834624 = 56512512
Quoted 2 lines
...
[ 2569.903893] comm=btrfs-cleaner func=add_pinned_bytes() 943976448+ 21762048 = 965738496
Quoted 58 lines
[ 2569.908270] comm=btrfs-cleaner func=btrfs_evict_inode() endFix:
Makebtrfs_commit_transaction()wait current running btrfs-cleaner's delayed-iputs() done in end.Test:
Use script similar to above(more complex),
before patch:7 failed in 100 * 20 loop. after patch: 0 failed in 100 * 20 loop.Signed-off-by Zhao Lei <[email protected]>--- fs/btrfs/ctree.h | 1 + fs/btrfs/disk-io.c | 3 ++- fs/btrfs/extent-tree.c | 6 ++++++ fs/btrfs/inode.c | 4 ++++ 4 files changed, 13 insertions(+), 1 deletion(-)fs/btrfs/ctree.h index renamed from fs/btrfs/ctree.h +0
-0f9c89ca..54d4d78 100644
fs/btrfs/ctree.h +1
-0@@ -1513,6 +1513,7 @@ struct btrfs_fs_info {spinlock_t delayed_iput_lock;struct list_head delayed_iputs; +struct rw_semaphore delayed_iput_sem;/* this protects tree_mod_seq_list */ spinlock_t tree_mod_seq_lock;fs/btrfs/disk-io.c index renamed from fs/btrfs/disk-io.c +0
-0639f266..6867471 100644
fs/btrfs/disk-io.c +2
-1@@ -2241,11 +2241,12 @@ int open_ctree(struct super_block *sb,spin_lock_init (&fs_info->qgroup_op_lock);spin_lock_init (&fs_info->buffer_lock);spin_lock_init (&fs_info->unused_bgs_lock); -mutex_init (&fs_info->unused_bg_unpin_mutex);rwlock_init (&fs_info->tree_mod_log_lock); +mutex_init (&fs_info->unused_bg_unpin_mutex);mutex_init (&fs_info->reloc_mutex);mutex_init (&fs_info->delalloc_root_mutex);seqlock_init (&fs_info->profiles_lock); +init_rwsem (&fs_info->delayed_iput_sem);init_completion (&fs_info->kobj_unregister);INIT_LIST_HEAD (&fs_info->dirty_cowonly_roots);fs/btrfs/extent-tree.c index renamed from fs/btrfs/extent-tree.c +0
-0203ac63..6fd7dca 100644
fs/btrfs/extent-tree.c +0
-0@@ -3732,6 +3732,12 @@ commit_trans: ret =btrfs_commit_transaction (trans, root);if (ret)fs/btrfs/extent-tree.c commit_trans: · line 3743
Quoted 10 lines
fs/btrfs/extent-tree.c +6
-0return ret; +/* +* make sure that all running delayed iput are +* done +*/ +down_write (&root->fs_info->delayed_iput_sem); +up_write (&root->fs_info->delayed_iput_sem); goto again; }else { btrfs_end_transaction(trans, root); diff --git
a/fs/btrfs/inode.cfs/btrfs/inode.c void btrfs_run_delayed_iputs(struct btrfs_root
Quoted 4 lines
b/fs/btrfs/inode.cindex d2e732d..34d10be 100644fs/btrfs/inode.c +0
-0@@ -3110,6 +3110,8 @@ void btrfs_run_delayed_iputs(struct btrfs_root*root)
fs/btrfs/inode.c void btrfs_run_delayed_iputs(struct btrfs_root
Quoted 9 lines
fs/btrfs/inode.c +2
-0if (empty)return ; +down_read (&fs_info->delayed_iput_sem); +spin_lock (&fs_info->delayed_iput_lock);list_splice_init (&fs_info->delayed_iputs, &list);spin_unlock (&fs_info->delayed_iput_lock);@@ -3120,6 +3122,8 @@ void btrfs_run_delayed_iputs(struct btrfs_root*root)
fs/btrfs/inode.c void btrfs_run_delayed_iputs(struct btrfs_root *root) · line 3126
Quoted 5 lines
fs/btrfs/inode.c +2
-0iput (delayed->inode);kfree (delayed); } + +up_read (&root->fs_info->delayed_iput_sem);By introducing this "
delayed_iput_sem", fstests's generic/241 cries about a possible deadlock, can we use a waitqueue to avoid this?[ 2061.345955] ============================================= [ 2061.346027] [ INFO: possible recursive locking detected ] [ 2061.346027] 4.1.0+ #268 Tainted: G W [ 2061.346027] --------------------------------------------- [ 2061.346027] btrfs-cleaner/3045 is trying to acquire lock: [ 2061.346027] (&fs_info->delayed_iput_sem){++++..}, at: [<ffffffff814063ab>] btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] but task is already holding lock: [ 2061.346027] (&fs_info->delayed_iput_sem){++++..}, at: [<ffffffff814063ab>] btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] other info that might help us debug this: [ 2061.346027] Possible unsafe locking scenario:[ 2061.346027] CPU0 [ 2061.346027] ---- [ 2061.346027] lock(&fs_info->delayed_iput_sem); [ 2061.346027] lock(&fs_info->delayed_iput_sem); [ 2061.346027] *** DEADLOCK ***[ 2061.346027] May be due to missing lock nesting notation[ 2061.346027] 2 locks held by btrfs-cleaner/3045: [ 2061.346027] #0: (&fs_info->cleaner_mutex){+.+.+.}, at: [<ffffffff813f6cc4>] cleaner_kthread+0x64/0x1a0 [ 2061.346027] #1: (&fs_info->delayed_iput_sem){++++..}, at: [<ffffffff814063ab>] btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] stack backtrace: [ 2061.346027] CPU: 0 PID: 3045 Comm: btrfs-cleaner Tainted: G W 4.1.0+ #268 [ 2061.346027] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140709_153950- 04/01/2014 [ 2061.346027] ffff88013760a700 ffff8800b4aff888 ffffffff818bb68e 0000000000000007 [ 2061.346027] ffffffff828fa0a0 ffff8800b4aff968 ffffffff810db18d ffff8800b4aff978 [ 2061.346027] ffffffff00000000 ffff88013760a750 ffffffff00000001 ffff88013760b470 [ 2061.346027] Call Trace: [ 2061.346027] [<ffffffff818bb68e>] dump_stack+0x4c/0x65 [ 2061.346027] [<ffffffff810db18d>] __lock_acquire+0x214d/0x22f0 [ 2061.346027] [<ffffffff810d5e3d>] ? trace_hardirqs_off+0xd/0x10 [ 2061.346027] [<ffffffff810d649d>] ? __lock_is_held+0x4d/0x70 [ 2061.346027] [<ffffffff810dc050>] lock_acquire+0xd0/0x280 [ 2061.346027] [<ffffffff814063ab>] ? btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] [<ffffffff818c275c>] down_read+0x4c/0x70 [ 2061.346027] [<ffffffff814063ab>] ? btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] [<ffffffff818c48cb>] ? _raw_spin_unlock+0x2b/0x40 [ 2061.346027] [<ffffffff814063ab>] btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] [<ffffffff8140042f>] ? btrfs_commit_transaction+0xa1f/0xc50 [ 2061.346027] [<ffffffff81400454>] btrfs_commit_transaction+0xa44/0xc50 [ 2061.346027] [<ffffffff810cdbd0>] ? wait_woken+0xa0/0xa0 [ 2061.346027] [<ffffffff813e5247>] flush_space+0x97/0x4b0 [ 2061.346027] [<ffffffff818c48cb>] ? _raw_spin_unlock+0x2b/0x40 [ 2061.346027] [<ffffffff813e5a3b>] ? reserve_metadata_bytes+0x20b/0x6d0 [ 2061.346027] [<ffffffff813e5a52>] reserve_metadata_bytes+0x222/0x6d0 [ 2061.346027] [<ffffffff813e6245>] ? btrfs_block_rsv_refill+0x65/0x90 [ 2061.346027] [<ffffffff813e6257>] btrfs_block_rsv_refill+0x77/0x90 [ 2061.346027] [<ffffffff8140c723>] btrfs_evict_inode+0x473/0x700 [ 2061.346027] [<ffffffff8123dcbb>] evict+0xab/0x170 [ 2061.346027] [<ffffffff8123e6fd>] iput+0x1cd/0x350 [ 2061.346027] [<ffffffff81406409>]Quoted 9 lines
btrfs_run_delayed_iputs+0xc9/0x100 [ 2061.346027] [<ffffffff813f6cc4>] ? cleaner_kthread+0x64/0x1a0 [ 2061.346027] [<ffffffff813f6dba>] cleaner_kthread+0x15a/0x1a0 [ 2061.346027] [<ffffffff813f6c60>] ? btree_invalidatepage+0x90/0x90 [ 2061.346027] [<ffffffff810a3d5f>] kthread+0xef/0x110 [ 2061.346027] [<ffffffff810a3c70>] ? kthread_create_on_node+0x210/0x210 [ 2061.346027] [<ffffffff818c550f>] ret_from_fork+0x3f/0x70 [ 2061.346027] [<ffffffff810a3c70>] ? kthread_create_on_node+0x210/0x210Thanks for report above problem.
Seems it is caused by recursion ofbtrfs_run_delayed_iputs:
cleaner_kthread
->btrfs_run_delayed_iputs()*1
-> getdelayed_iput_semlock *2
->iput()
-> ...
->btrfs_commit_transaction()
->btrfs_run_delayed_iputs()*1
-> getdelayed_iput_semlock (dead lock) *2
Quoted 2 lines
*1: recursion of
btrfs_run_delayed_iputs()
*2: dead lock ofdelayed_iput_sem
Yep, that's what stack trace shows, but I guess it hardly runs to deadlock, since we've checked if the list is empty before acquiring the semephore.
Quoted 3 lines
I'll reproduce this problem in my env, and comfirm fix of above problem.
It didn't occur every time, but quite often on my box.
Thanks,
-liubo
Quoted 20 lines
Thanks
ZhaoleiQuoted 14 lines
Thanks,
-liubo
Quoted 10 lines
fs/btrfs/inode.c +0
-1}/* -- 1.8.5.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
RE: [PATCH 6/9] btrfs: Fix NO_SPACE bug caused by delayed-iput
Hi, Liubo
Quoted 248 lines
-----Original Message-----
From: Liu Bo [mailto:[email protected]]
Sent: Tuesday, July 07, 2015 5:27 PM
To: Zhao Lei
Cc: [email protected]
Subject: Re: [PATCH 6/9] btrfs: FixNO_SPACEbug caused by delayed-iputQuoted 177 lines · Zhao Lei
Hi, Liubo
Quoted 175 lines
-----Original Message-----
From: Liu Bo [mailto:[email protected]]
Sent: Tuesday, July 07, 2015 4:14 PM
To: Zhaolei
Cc: [email protected]
Subject: Re: [PATCH 6/9] btrfs: FixNO_SPACEbug caused by delayed-iputHi,
Quoted 17 lines · Zhaolei
From: Zhao Lei <[email protected]>
Steps to reproduce:
while true; dodd if=/dev/zero of=/btrfs_dir/file count=[fs_size * 75%] rm /btrfs_dir/file syncdone
And we'll see dd failed because btrfs return
NO_SPACE.Reason:
Normally,btrfs_commit_transaction()callbtrfs_run_delayed_iputs()in end to free fs space for next write, but sometimes it hadn't done work on time, because btrfs-cleaner thread get delayed-iputs from list before, but doiput()after next write.Quoted 4 lines
This is log:
[ 2569.050776] comm=btrfs-cleaner func=btrfs_evict_inode() begin[ 2569.084280] comm=sync func=btrfs_commit_transaction() call
btrfs_run_delayed_iputs()Quoted 1 line
[ 2569.085418] comm=sync func=btrfs_commit_transaction() done
btrfs_run_delayed_iputs()Quoted 6 lines
[ 2569.087554] comm=sync func=btrfs_commit_transaction() end[ 2569.191081] comm=dd begin [ 2569.790112] comm=dd func=__btrfs_buffered_write() ret=-28[ 2569.847479] comm=btrfs-cleaner func=add_pinned_bytes() 0 +32677888 = 32677888
Quoted 2 lines
[ 2569.849530] comm=btrfs-cleaner func=add_pinned_bytes()32677888 +
23834624 = 56512512
Quoted 3 lines
...
[ 2569.903893] comm=btrfs-cleaner func=add_pinned_bytes()943976448
+ 21762048 = 965738496
Quoted 58 lines
[ 2569.908270] comm=btrfs-cleaner func=btrfs_evict_inode() endFix:
Makebtrfs_commit_transaction()wait current running btrfs-cleaner's delayed-iputs() done in end.Test:
Use script similar to above(more complex),
before patch:7 failed in 100 * 20 loop. after patch: 0 failed in 100 * 20 loop.Signed-off-by Zhao Lei <[email protected]>--- fs/btrfs/ctree.h | 1 + fs/btrfs/disk-io.c | 3 ++- fs/btrfs/extent-tree.c | 6 ++++++ fs/btrfs/inode.c | 4 ++++ 4 files changed, 13 insertions(+), 1 deletion(-)fs/btrfs/ctree.h index renamed from fs/btrfs/ctree.h +0
-0f9c89ca..54d4d78 100644
fs/btrfs/ctree.h +1
-0@@ -1513,6 +1513,7 @@ struct btrfs_fs_info {spinlock_t delayed_iput_lock;struct list_head delayed_iputs; +struct rw_semaphore delayed_iput_sem;/* this protects tree_mod_seq_list */ spinlock_t tree_mod_seq_lock;fs/btrfs/disk-io.c index renamed from fs/btrfs/disk-io.c +0
-0639f266..6867471 100644
fs/btrfs/disk-io.c +2
-1@@ -2241,11 +2241,12 @@ int open_ctree(struct super_block *sb,spin_lock_init (&fs_info->qgroup_op_lock);spin_lock_init (&fs_info->buffer_lock);spin_lock_init (&fs_info->unused_bgs_lock); -mutex_init (&fs_info->unused_bg_unpin_mutex);rwlock_init (&fs_info->tree_mod_log_lock); +mutex_init (&fs_info->unused_bg_unpin_mutex);mutex_init (&fs_info->reloc_mutex);mutex_init (&fs_info->delalloc_root_mutex);seqlock_init (&fs_info->profiles_lock); +init_rwsem (&fs_info->delayed_iput_sem);init_completion (&fs_info->kobj_unregister);INIT_LIST_HEAD (&fs_info->dirty_cowonly_roots);fs/btrfs/extent-tree.c index renamed from fs/btrfs/extent-tree.c +0
-0203ac63..6fd7dca 100644
fs/btrfs/extent-tree.c +0
-0@@ -3732,6 +3732,12 @@ commit_trans: ret =btrfs_commit_transaction (trans, root);if (ret)fs/btrfs/extent-tree.c commit_trans: · line 3743
Quoted 10 lines
fs/btrfs/extent-tree.c +6
-0return ret; +/* +* make sure that all running delayed iput are +* done +*/ +down_write (&root->fs_info->delayed_iput_sem); +up_write (&root->fs_info->delayed_iput_sem); goto again; }else { btrfs_end_transaction(trans, root); diff --git
a/fs/btrfs/inode.cQuoted 5 lines
b/fs/btrfs/inode.cindex d2e732d..34d10be 100644fs/btrfs/inode.c +0
-0@@ -3110,6 +3110,8 @@ void btrfs_run_delayed_iputs(struct
btrfs_root*root)
Quoted 10 lines
fs/btrfs/inode.c +2
-0if (empty)return ; +down_read (&fs_info->delayed_iput_sem); +spin_lock (&fs_info->delayed_iput_lock);list_splice_init (&fs_info->delayed_iputs, &list);spin_unlock (&fs_info->delayed_iput_lock);@@ -3120,6 +3122,8 @@ void btrfs_run_delayed_iputs(struct
btrfs_root*root)
fs/btrfs/inode.c void btrfs_run_delayed_iputs(struct btrfs_root *root) · line 3126
Quoted 5 lines
fs/btrfs/inode.c +2
-0iput (delayed->inode);kfree (delayed); } + +up_read (&root->fs_info->delayed_iput_sem);By introducing this "
delayed_iput_sem", fstests's generic/241 cries about a possible deadlock, can we use a waitqueue to avoid this?[ 2061.345955] ============================================= [ 2061.346027] [ INFO: possible recursive locking detected ] [ 2061.346027] 4.1.0+ #268 Tainted: G W [ 2061.346027] --------------------------------------------- [ 2061.346027] btrfs-cleaner/3045 is trying to acquire lock: [ 2061.346027] (&fs_info->delayed_iput_sem){++++..}, at: [<ffffffff814063ab>] btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] but task is already holding lock: [ 2061.346027] (&fs_info->delayed_iput_sem){++++..}, at: [<ffffffff814063ab>] btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] other info that might help us debug this: [ 2061.346027] Possible unsafe locking scenario:[ 2061.346027] CPU0 [ 2061.346027] ---- [ 2061.346027] lock(&fs_info->delayed_iput_sem); [ 2061.346027] lock(&fs_info->delayed_iput_sem); [ 2061.346027] *** DEADLOCK ***[ 2061.346027] May be due to missing lock nesting notation[ 2061.346027] 2 locks held by btrfs-cleaner/3045: [ 2061.346027] #0: (&fs_info->cleaner_mutex){+.+.+.}, at: [<ffffffff813f6cc4>] cleaner_kthread+0x64/0x1a0 [ 2061.346027] #1: (&fs_info->delayed_iput_sem){++++..}, at: [<ffffffff814063ab>] btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] stack backtrace: [ 2061.346027] CPU: 0 PID: 3045 Comm: btrfs-cleaner Tainted: GW
Quoted 62 lines
Quoted 47 lines
4.1.0+ #268
[ 2061.346027] Hardware name: QEMU Standard PC (i440FX + PIIX,1996), BIOS
1.7.5-20140709_153950- 04/01/2014 [ 2061.346027]ffff88013760a700ffff8800b4aff888ffffffff818bb68e0000000000000007 [ 2061.346027]ffffffff828fa0a0ffff8800b4aff968ffffffff810db18dffff8800b4aff978[ 2061.346027] ffffffff00000000 ffff88013760a750 ffffffff00000001 ffff88013760b470 [ 2061.346027] Call Trace: [ 2061.346027] [<ffffffff818bb68e>] dump_stack+0x4c/0x65 [2061.346027] [<
ffffffff810db18d>]__lock_acquire+0x214d/0x22f0[ 2061.346027] [<ffffffff810d5e3d>] ?trace_hardirqs_off+0xd/0x10[ 2061.346027] [<ffffffff810d649d>] ?__lock_is_held+0x4d/0x70[ 2061.346027] [<ffffffff810dc050>]lock_acquire+0xd0/0x280[ 2061.346027] [<ffffffff814063ab>] ?btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] [<ffffffff818c275c>] down_read+0x4c/0x70 [ 2061.346027] [<ffffffff814063ab>] ? btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] [<ffffffff818c48cb>] ? _raw_spin_unlock+0x2b/0x40 [ 2061.346027] [<ffffffff814063ab>] btrfs_run_delayed_iputs+0x6b/0x100 [ 2061.346027] [<ffffffff8140042f>] ? btrfs_commit_transaction+0xa1f/0xc50 [ 2061.346027] [<ffffffff81400454>] btrfs_commit_transaction+0xa44/0xc50 [ 2061.346027] [<ffffffff810cdbd0>] ? wait_woken+0xa0/0xa0 [ 2061.346027] [<ffffffff813e5247>] flush_space+0x97/0x4b0 [ 2061.346027] [<ffffffff818c48cb>] ? _raw_spin_unlock+0x2b/0x40 [ 2061.346027] [<ffffffff813e5a3b>] ? reserve_metadata_bytes+0x20b/0x6d0 [ 2061.346027] [<ffffffff813e5a52>] reserve_metadata_bytes+0x222/0x6d0 [ 2061.346027] [<ffffffff813e6245>] ? btrfs_block_rsv_refill+0x65/0x90 [ 2061.346027] [<ffffffff813e6257>] btrfs_block_rsv_refill+0x77/0x90 [ 2061.346027] [<ffffffff8140c723>] btrfs_evict_inode+0x473/0x700 [ 2061.346027] [<ffffffff8123dcbb>] evict+0xab/0x170 [ 2061.346027] [<ffffffff8123e6fd>] iput+0x1cd/0x350 [ 2061.346027] [<ffffffff81406409>] btrfs_run_delayed_iputs+0xc9/0x100 [ 2061.346027] [<ffffffff813f6cc4>] ? cleaner_kthread+0x64/0x1a0 [ 2061.346027] [<ffffffff813f6dba>] cleaner_kthread+0x15a/0x1a0 [ 2061.346027] [<ffffffff813f6c60>] ? btree_invalidatepage+0x90/0x90 [ 2061.346027] [<ffffffff810a3d5f>] kthread+0xef/0x110 [ 2061.346027] [<ffffffff810a3c70>] ? kthread_create_on_node+0x210/0x210 [ 2061.346027] [<ffffffff818c550f>] ret_from_fork+0x3f/0x70 [Quoted 3 lines
2061.346027] [<
ffffffff810a3c70>] ?kthread_create_on_node+0x210/0x210Thanks for report above problem.
Seems it is caused by recursion ofbtrfs_run_delayed_iputs:
cleaner_kthread
->btrfs_run_delayed_iputs()*1
-> getdelayed_iput_semlock *2
->iput()
-> ...
->btrfs_commit_transaction()
->btrfs_run_delayed_iputs()*1
-> getdelayed_iput_semlock (dead lock) *2
Quoted 6 lines
Quoted 2 lines
*1: recursion of
btrfs_run_delayed_iputs()
*2: dead lock ofdelayed_iput_semYep, that's what stack trace shows, but I guess it hardly runs to deadlock, since we've checked if the list is empty before acquiring the semephore.
Or some reason, which cause " fs_info->delayed_iputs not empty" in second call. (only guess)
Quoted 6 lines
Quoted 3 lines
I'll reproduce this problem in my env, and comfirm fix of above problem.
It didn't occur every time, but quite often on my box.
Got it.
So, 1000 times test.
Thanks
Zhaolei
Quoted 23 lines
Thanks,
-liubo
Quoted 20 lines
Thanks
ZhaoleiQuoted 14 lines
Thanks,
-liubo
Quoted 10 lines
}
/*
--
1.8.5.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
PATCH v2 7/9 btrfs: Support busy loop of write and delete
In reply to ZhaoleiFrom: Zhao Lei <[email protected]>
Reproduce:
while true; do
dd if=/dev/zero of=/mnt/btrfs/file count=[75% fs_size] rm /mnt/btrfs/file
done
Then we can see above loop failed on NO_SPACE.
It it long-term problem since very beginning, because delayed-iput after rm are not run.
We already have commit_transaction() in alloc_space code, but it is not triggered in above case.
This patch trigger commit_transaction() to run delayed-iput and reflash pinned-space to to make write success.
It is based on previous fix of delayed-iput in commit_transaction(), need to be applied on top of:
btrfs: Fix NO_SPACE bug caused by delayed-iput
--- fs/btrfs/extent-tree.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)
fs/btrfs/extent-tree.c +13 -11
@@ -3641,13 +3641,13 @@ int btrfs_check_data_free_space(struct inode *inode, u64 bytes)struct btrfs_root *root =BTRFS_I (inode)->root;struct btrfs_fs_info *fs_info = root->fs_info;u64 used; -int ret =0 , committed =0 , have_pinned_space =1 , alloc_chunk =1 ; +int ret =0 , need_commit =2 , have_pinned_space, alloc_chunk =1 ;/* make sure bytes are sectorsize aligned */ bytes =ALIGN (bytes, root->sectorsize);if (btrfs_is_free_space_inode (inode)) { - committed =1 ; + need_commit =0 ;ASSERT (current->journal_info); }
@@ -3697,8 +3697,10 @@ alloc:if (ret <0 ) {if (ret != -ENOSPC )return ret; -else +else { + have_pinned_space =1 ; goto commit_trans; + } }if (!data_sinfo)
@@ -3712,23 +3714,23 @@ alloc:* allocation, and no removed chunk in current transaction, * don't bother committing the transaction. */ -if (percpu_counter_compare (&data_sinfo->total_bytes_pinned, - used + bytes - - data_sinfo->total_bytes) <0 ) - have_pinned_space =0 ; + have_pinned_space =percpu_counter_compare ( + &data_sinfo->total_bytes_pinned, + used + bytes - data_sinfo->total_bytes);spin_unlock (&data_sinfo->lock);/* commit the current transaction and try again */ commit_trans: -if (!committed && +if (need_commit && !atomic_read (&root->fs_info->open_ioctl_trans)) { - committed =1 ; + need_commit--; trans =btrfs_join_transaction (root);if (IS_ERR (trans))return PTR_ERR (trans); -if (have_pinned_space || - trans->transaction->have_free_bgs) { +if (have_pinned_space >=0 || + trans->transaction->have_free_bgs || + need_commit >0 ) { ret =btrfs_commit_transaction (trans, root);if (ret)return ret;
Signature · 1 line
1.8.5.1
PATCH v2 8/9 btrfs: wait for delayed iputs on no space
In reply to ZhaoleiFrom: Zhao Lei <[email protected]>
btrfs will report no_space when we run following write and delete file loop:
# FILE_SIZE_M=[ 75% of fs space ]
# DEV=[ some dev ]
# MNT=[ some dir ]
#
# mkfs.btrfs -f "$DEV" # mount -o nodatacow "$DEV" "$MNT" # for ((i = 0; i < 100; i++)); do dd if=/dev/zero of="$MNT"/file0 bs=1M count="$FILE_SIZE_M"; rm -f "$MNT"/file0; done #
Reason:
iput() and evict() is run after write pages to block device, if write pages work is not finished before next write, the "rm"ed space is not freed, and caused above bug.
Fix:
We can add "-o flushoncommit" mount option to avoid above bug, but it have performance problem. Actually, we can to wait for on-the-fly writes only when no-space happened, it is which this patch do.
--- fs/btrfs/extent-tree.c | 3 +++ 1 file changed, 3 insertions(+)
fs/btrfs/extent-tree.c +3 -0
@@ -3725,6 +3725,9 @@ commit_trans: !atomic_read(&root->fs_info->open_ioctl_trans)) { need_commit--; +if (need_commit >0 ) +btrfs_wait_ordered_roots (fs_info,-1 ); + trans =btrfs_join_transaction (root);if (IS_ERR (trans))return PTR_ERR (trans);
Signature · 1 line
1.8.5.1
Re: [PATCH 8/9] btrfs: wait for delayed iputs on no space
Quoted 18 lines · Zhaolei
From: Zhao Lei <[email protected]>
btrfs will report
no_spacewhen we run following write and delete file loop:
# FILE_SIZE_M=[ 75% of fs space ]
# DEV=[ some dev ]
# MNT=[ some dir ]
## mkfs.btrfs -f "$DEV" # mount -o nodatacow "$DEV" "$MNT" # for ((i = 0; i < 100; i++)); do dd if=/dev/zero of="$MNT"/file0 bs=1M count="$FILE_SIZE_M"; rm -f "$MNT"/file0; done #Reason:
iput()andevict()is run after write pages to block device, if write pages work is not finished before next write, the "rm"ed space is not freed, and caused above bug.
Quoted 4 lines
Fix:
We can add "-o flushoncommit" mount option to avoid above bug, but it have performance problem. Actually, we can to wait for on-the-fly writes only when no-space happened, it is which this patch do.
Can you please change this so we only do this flush if the first commit doesn't free up enough space? I think this is going to have a performance impact as the FS fills up.
-chris
RE: [PATCH 8/9] btrfs: wait for delayed iputs on no space
Hi, Chris
Quoted 25 lines
-----Original Message-----
From: Chris Mason [mailto:[email protected]]
Sent: Monday, April 13, 2015 10:55 PM
To: Zhaolei; [email protected]
Subject: Re: [PATCH 8/9] btrfs: wait for delayed iputs on no spaceQuoted 18 lines · Zhaolei
From: Zhao Lei <[email protected]>
btrfs will report
no_spacewhen we run following write and delete file loop:
# FILE_SIZE_M=[ 75% of fs space ]
# DEV=[ some dev ]
# MNT=[ some dir ]
## mkfs.btrfs -f "$DEV" # mount -o nodatacow "$DEV" "$MNT" # for ((i = 0; i < 100; i++)); do dd if=/dev/zero of="$MNT"/file0 bs=1M count="$FILE_SIZE_M"; rm -f "$MNT"/file0; done #Reason:
iput()andevict()is run after write pages to block device, if
write pages work is not finished before next write, the "rm"ed space is not freed, and caused above bug.
Quoted 9 lines
Quoted 4 lines
Fix:
We can add "-o flushoncommit" mount option to avoid above bug, but
it have performance problem. Actually, we can to wait for on-the-fly writes only when no-space happened, it is which this patch do.Can you please change this so we only do this flush if the first commit doesn't free up enough space? I think this is going to have a performance impact as the FS fills up.
btrfs_wait_ordered_roots() can only ensure that all bio are finished, and relative iputs are added into delayed_iputs in end_io. And we need 2 commit to make free space accessable:
One for run delayed_iputs(), and another for unpin.
It is why I put above line to first commit, to ensure we have enough commit operation to make free space accessable.
It is only called then the disk is almost full, and have no performance impact in most case(disk not full).
Another way is to call btrfs_wait_ordered_roots() after first commit() try, but give it addition commit().
Thanks
Zhaolei
PATCH v2 9/9 btrfs: cleanup unused alloc_chunk varible
In reply to ZhaoleiFrom: Zhao Lei <[email protected]>
Remove int alloc_chunk in btrfs_check_data_free_space() for not necessary.
--- fs/btrfs/extent-tree.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
fs/btrfs/extent-tree.c +2 -2
@@ -3641,7 +3641,7 @@ int btrfs_check_data_free_space(struct inode *inode, u64 bytes)struct btrfs_root *root =BTRFS_I (inode)->root;struct btrfs_fs_info *fs_info = root->fs_info;u64 used; -int ret =0 , need_commit =2 , have_pinned_space, alloc_chunk =1 ; +int ret =0 , need_commit =2 , have_pinned_space;/* make sure bytes are sectorsize aligned */ bytes =ALIGN (bytes, root->sectorsize);
@@ -3669,7 +3669,7 @@ again:* if we don't have enough free bytes in this space then we need * to alloc a new chunk. */ -if (!data_sinfo->full && alloc_chunk) { +if (!data_sinfo->full) {u64 alloc_target; data_sinfo->force_alloc =CHUNK_ALLOC_FORCE ;
Signature · 1 line
1.8.5.1