btrfs: Fix no_space on dd and rm loop
cover btrfs: Fix no_space on dd and rm loop
From: Zhao Lei <[email protected]>
I resend this patch set with some changes:
1: Move a cleanup patch for btrfs_check_data_free_space() into 2: Rebased on top of v4.0-rc5
3: Fixed a lock problem reported by:
'Tsutomu Itoh' <[email protected]>
Tested by busy dd and rm loop script in 2000 times. Confirmed having-problem in v4.0-rc5 and no-problem on top of this patchset.
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 | 5 ++++- fs/btrfs/extent-tree.c | 60 ++++++++++++++++++++++++++++++++++++++------------ fs/btrfs/transaction.c | 6 ++++- fs/btrfs/volumes.c | 24 +++++++++++--------- 5 files changed, 69 insertions(+), 27 deletions(-)
Signature · 1 line
1.8.5.1
PATCH 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 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
Re: [PATCH 2/9] btrfs: Fix tail space processing in find_free_dev_extent()
Quoted 10 lines · Zhaolei
From: Zhao Lei <[email protected]>
It is another reason for
NO_SPACEcase.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 causeNO_SPACEin user program.
Quoted 64 lines
Signed-off-by Zhao Lei <[email protected]>--- 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 · 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
PATCH 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 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 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 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 | 5 ++++- fs/btrfs/transaction.c | 6 +++++- 3 files changed, 10 insertions(+), 2 deletions(-)
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 +4 -1
@@ -1778,7 +1778,9 @@ static int cleaner_kthread(void *arg) goto sleep; } +down_read (&root->fs_info->delayed_iput_sem);btrfs_run_delayed_iputs (root); +up_read (&root->fs_info->delayed_iput_sem);btrfs_delete_unused_bgs (root->fs_info); again =btrfs_clean_one_deleted_snapshot (root);mutex_unlock (&root->fs_info->cleaner_mutex);
@@ -2241,11 +2243,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/transaction.c +5 -1
@@ -2076,8 +2076,12 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,kmem_cache_free (btrfs_trans_handle_cachep, trans); -if (current != root->fs_info->transaction_kthread) +if (current != root->fs_info->transaction_kthread) {btrfs_run_delayed_iputs (root); +/* 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); + }return ret;
Signature · 1 line
1.8.5.1
PATCH 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 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
PATCH 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