* [PATCH 1/6] Btrfs: don't put removals from push_node_left into tree mod log twice
2012-10-23 13:55 [PATCH 0/6] Btrfs: tree mod log fixes for 3.7-rc3 Jan Schmidt
@ 2012-10-23 13:55 ` Jan Schmidt
2012-10-23 15:23 ` Liu Bo
2012-10-23 13:55 ` [PATCH 2/6] Btrfs: fix a tree mod logging issue for root replacement operations Jan Schmidt
` (4 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Jan Schmidt @ 2012-10-23 13:55 UTC (permalink / raw)
To: chris.mason; +Cc: linux-btrfs, bo.li.liu
Independant of the check (push_items < src_items) tree_mod_log_eb_copy did
log the removal of the old data entries from the source buffer. Therefore,
we must not call tree_mod_log_eb_move if the check evaluates to true, as
that would log the removal twice, finally resulting in (rewinded) buffers
with wrong values for header_nritems.
Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
---
fs/btrfs/ctree.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index b334362..44a7e25 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1225,6 +1225,8 @@ tree_mod_log_rewind(struct btrfs_fs_info *fs_info, struct extent_buffer *eb,
free_extent_buffer(eb);
__tree_mod_log_rewind(eb_rewin, time_seq, tm);
+ WARN_ON(btrfs_header_nritems(eb_rewin) >
+ BTRFS_NODEPTRS_PER_BLOCK(fs_info->fs_root));
return eb_rewin;
}
@@ -1280,6 +1282,7 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
else
WARN_ON(btrfs_header_level(eb) != 0);
extent_buffer_get(eb);
+ WARN_ON(btrfs_header_nritems(eb) > BTRFS_NODEPTRS_PER_BLOCK(root));
return eb;
}
@@ -2970,8 +2973,10 @@ static int push_node_left(struct btrfs_trans_handle *trans,
push_items * sizeof(struct btrfs_key_ptr));
if (push_items < src_nritems) {
- tree_mod_log_eb_move(root->fs_info, src, 0, push_items,
- src_nritems - push_items);
+ /*
+ * don't call tree_mod_log_eb_move here, key removal was already
+ * fully logged by tree_mod_log_eb_copy above.
+ */
memmove_extent_buffer(src, btrfs_node_key_ptr_offset(0),
btrfs_node_key_ptr_offset(push_items),
(src_nritems - push_items) *
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 1/6] Btrfs: don't put removals from push_node_left into tree mod log twice
2012-10-23 13:55 ` [PATCH 1/6] Btrfs: don't put removals from push_node_left into tree mod log twice Jan Schmidt
@ 2012-10-23 15:23 ` Liu Bo
0 siblings, 0 replies; 17+ messages in thread
From: Liu Bo @ 2012-10-23 15:23 UTC (permalink / raw)
To: Jan Schmidt; +Cc: chris.mason, linux-btrfs
On 10/23/2012 09:55 PM, Jan Schmidt wrote:
> Independant of the check (push_items < src_items) tree_mod_log_eb_copy did
> log the removal of the old data entries from the source buffer. Therefore,
> we must not call tree_mod_log_eb_move if the check evaluates to true, as
> that would log the removal twice, finally resulting in (rewinded) buffers
> with wrong values for header_nritems.
>
Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
Tested-by: Liu Bo <bo.li.liu@oracle.com>
thanks,
liubo
> Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
> ---
> fs/btrfs/ctree.c | 9 +++++++--
> 1 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index b334362..44a7e25 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -1225,6 +1225,8 @@ tree_mod_log_rewind(struct btrfs_fs_info *fs_info, struct extent_buffer *eb,
> free_extent_buffer(eb);
>
> __tree_mod_log_rewind(eb_rewin, time_seq, tm);
> + WARN_ON(btrfs_header_nritems(eb_rewin) >
> + BTRFS_NODEPTRS_PER_BLOCK(fs_info->fs_root));
>
> return eb_rewin;
> }
> @@ -1280,6 +1282,7 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
> else
> WARN_ON(btrfs_header_level(eb) != 0);
> extent_buffer_get(eb);
> + WARN_ON(btrfs_header_nritems(eb) > BTRFS_NODEPTRS_PER_BLOCK(root));
>
> return eb;
> }
> @@ -2970,8 +2973,10 @@ static int push_node_left(struct btrfs_trans_handle *trans,
> push_items * sizeof(struct btrfs_key_ptr));
>
> if (push_items < src_nritems) {
> - tree_mod_log_eb_move(root->fs_info, src, 0, push_items,
> - src_nritems - push_items);
> + /*
> + * don't call tree_mod_log_eb_move here, key removal was already
> + * fully logged by tree_mod_log_eb_copy above.
> + */
> memmove_extent_buffer(src, btrfs_node_key_ptr_offset(0),
> btrfs_node_key_ptr_offset(push_items),
> (src_nritems - push_items) *
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/6] Btrfs: fix a tree mod logging issue for root replacement operations
2012-10-23 13:55 [PATCH 0/6] Btrfs: tree mod log fixes for 3.7-rc3 Jan Schmidt
2012-10-23 13:55 ` [PATCH 1/6] Btrfs: don't put removals from push_node_left into tree mod log twice Jan Schmidt
@ 2012-10-23 13:55 ` Jan Schmidt
2012-10-23 15:28 ` Liu Bo
2012-10-23 13:55 ` [PATCH 3/6] Btrfs: tree mod log's old roots could still be part of the tree Jan Schmidt
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Jan Schmidt @ 2012-10-23 13:55 UTC (permalink / raw)
To: chris.mason; +Cc: linux-btrfs, bo.li.liu
Avoid the implicit free by tree_mod_log_set_root_pointer, which is wrong in
two places. Where needed, we call tree_mod_log_free_eb explicitly now.
Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
---
fs/btrfs/ctree.c | 10 ++--------
1 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 44a7e25..e6b75cc 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -647,8 +647,6 @@ tree_mod_log_insert_root(struct btrfs_fs_info *fs_info,
if (tree_mod_dont_log(fs_info, NULL))
return 0;
- __tree_mod_log_free_eb(fs_info, old_root);
-
ret = tree_mod_alloc(fs_info, flags, &tm);
if (ret < 0)
goto out;
@@ -926,12 +924,7 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans,
ret = btrfs_dec_ref(trans, root, buf, 1, 1);
BUG_ON(ret); /* -ENOMEM */
}
- /*
- * don't log freeing in case we're freeing the root node, this
- * is done by tree_mod_log_set_root_pointer later
- */
- if (buf != root->node && btrfs_header_level(buf) != 0)
- tree_mod_log_free_eb(root->fs_info, buf);
+ tree_mod_log_free_eb(root->fs_info, buf);
clean_tree_block(trans, root, buf);
*last_ref = 1;
}
@@ -1728,6 +1721,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
goto enospc;
}
+ tree_mod_log_free_eb(root->fs_info, root->node);
tree_mod_log_set_root_pointer(root, child);
rcu_assign_pointer(root->node, child);
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 2/6] Btrfs: fix a tree mod logging issue for root replacement operations
2012-10-23 13:55 ` [PATCH 2/6] Btrfs: fix a tree mod logging issue for root replacement operations Jan Schmidt
@ 2012-10-23 15:28 ` Liu Bo
2012-10-23 15:51 ` Jan Schmidt
0 siblings, 1 reply; 17+ messages in thread
From: Liu Bo @ 2012-10-23 15:28 UTC (permalink / raw)
To: Jan Schmidt; +Cc: chris.mason, linux-btrfs
On 10/23/2012 09:55 PM, Jan Schmidt wrote:
> Avoid the implicit free by tree_mod_log_set_root_pointer, which is wrong in
> two places. Where needed, we call tree_mod_log_free_eb explicitly now.
>
> Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
> ---
> fs/btrfs/ctree.c | 10 ++--------
> 1 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 44a7e25..e6b75cc 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -647,8 +647,6 @@ tree_mod_log_insert_root(struct btrfs_fs_info *fs_info,
> if (tree_mod_dont_log(fs_info, NULL))
> return 0;
>
> - __tree_mod_log_free_eb(fs_info, old_root);
> -
> ret = tree_mod_alloc(fs_info, flags, &tm);
> if (ret < 0)
> goto out;
> @@ -926,12 +924,7 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans,
> ret = btrfs_dec_ref(trans, root, buf, 1, 1);
> BUG_ON(ret); /* -ENOMEM */
> }
> - /*
> - * don't log freeing in case we're freeing the root node, this
> - * is done by tree_mod_log_set_root_pointer later
> - */
> - if (buf != root->node && btrfs_header_level(buf) != 0)
> - tree_mod_log_free_eb(root->fs_info, buf);
> + tree_mod_log_free_eb(root->fs_info, buf);
Since we've owned a check for if eb's level is 0 in tree_mod_dont_log(),
we can also get rid of these level checks in other places, can't we?
> clean_tree_block(trans, root, buf);
> *last_ref = 1;
> }
> @@ -1728,6 +1721,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
> goto enospc;
> }
>
> + tree_mod_log_free_eb(root->fs_info, root->node);
> tree_mod_log_set_root_pointer(root, child);
> rcu_assign_pointer(root->node, child);
>
>
Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
Tested-by: Liu Bo <bo.li.liu@oracle.com>
thanks,
liubo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/6] Btrfs: fix a tree mod logging issue for root replacement operations
2012-10-23 15:28 ` Liu Bo
@ 2012-10-23 15:51 ` Jan Schmidt
0 siblings, 0 replies; 17+ messages in thread
From: Jan Schmidt @ 2012-10-23 15:51 UTC (permalink / raw)
To: Liu Bo; +Cc: chris.mason, linux-btrfs
On Tue, October 23, 2012 at 17:28 (+0200), Liu Bo wrote:
> On 10/23/2012 09:55 PM, Jan Schmidt wrote:
>> Avoid the implicit free by tree_mod_log_set_root_pointer, which is wrong in
>> two places. Where needed, we call tree_mod_log_free_eb explicitly now.
>>
>> Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
>> ---
>> fs/btrfs/ctree.c | 10 ++--------
>> 1 files changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>> index 44a7e25..e6b75cc 100644
>> --- a/fs/btrfs/ctree.c
>> +++ b/fs/btrfs/ctree.c
>> @@ -647,8 +647,6 @@ tree_mod_log_insert_root(struct btrfs_fs_info *fs_info,
>> if (tree_mod_dont_log(fs_info, NULL))
>> return 0;
>>
>> - __tree_mod_log_free_eb(fs_info, old_root);
>> -
>> ret = tree_mod_alloc(fs_info, flags, &tm);
>> if (ret < 0)
>> goto out;
>> @@ -926,12 +924,7 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans,
>> ret = btrfs_dec_ref(trans, root, buf, 1, 1);
>> BUG_ON(ret); /* -ENOMEM */
>> }
>> - /*
>> - * don't log freeing in case we're freeing the root node, this
>> - * is done by tree_mod_log_set_root_pointer later
>> - */
>> - if (buf != root->node && btrfs_header_level(buf) != 0)
>> - tree_mod_log_free_eb(root->fs_info, buf);
>> + tree_mod_log_free_eb(root->fs_info, buf);
>
> Since we've owned a check for if eb's level is 0 in tree_mod_dont_log(),
> we can also get rid of these level checks in other places, can't we?
We can, yes. There may be hot paths where its worth an extra check to save the
overhead. That would require looking closer at the individual sites. Here it
definitely wasn't worth any extra check, so I dropped it en passant.
Thanks for reviewing and testing!
-Jan
>> clean_tree_block(trans, root, buf);
>> *last_ref = 1;
>> }
>> @@ -1728,6 +1721,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
>> goto enospc;
>> }
>>
>> + tree_mod_log_free_eb(root->fs_info, root->node);
>> tree_mod_log_set_root_pointer(root, child);
>> rcu_assign_pointer(root->node, child);
>>
>>
>
> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
> Tested-by: Liu Bo <bo.li.liu@oracle.com>
>
> thanks,
> liubo
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/6] Btrfs: tree mod log's old roots could still be part of the tree
2012-10-23 13:55 [PATCH 0/6] Btrfs: tree mod log fixes for 3.7-rc3 Jan Schmidt
2012-10-23 13:55 ` [PATCH 1/6] Btrfs: don't put removals from push_node_left into tree mod log twice Jan Schmidt
2012-10-23 13:55 ` [PATCH 2/6] Btrfs: fix a tree mod logging issue for root replacement operations Jan Schmidt
@ 2012-10-23 13:55 ` Jan Schmidt
2012-10-23 15:30 ` Liu Bo
2012-10-24 9:55 ` Liu Bo
2012-10-23 13:55 ` [PATCH 4/6] Btrfs: determine level of old roots Jan Schmidt
` (2 subsequent siblings)
5 siblings, 2 replies; 17+ messages in thread
From: Jan Schmidt @ 2012-10-23 13:55 UTC (permalink / raw)
To: chris.mason; +Cc: linux-btrfs, bo.li.liu
Tree mod log treated old root buffers as always empty buffers when starting
the rewind operations. However, the old root may still be part of the
current tree at a lower level, with still some valid entries.
Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
---
fs/btrfs/ctree.c | 16 ++++++++++++++--
1 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index e6b75cc..1b60c56 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1239,6 +1239,7 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
struct tree_mod_root *old_root = NULL;
u64 old_generation = 0;
u64 logical;
+ u32 blocksize;
eb = btrfs_read_lock_root_node(root);
tm = __tree_mod_log_oldest_root(root->fs_info, root, time_seq);
@@ -1254,10 +1255,21 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
}
tm = tree_mod_log_search(root->fs_info, logical, time_seq);
- if (old_root)
+ if (old_root && tm && tm->op != MOD_LOG_KEY_REMOVE_WHILE_FREEING) {
+ blocksize = btrfs_level_size(root, old_root->level);
+ eb = read_tree_block(root, logical, blocksize, 0);
+ if (!eb) {
+ pr_warn("btrfs: failed to read tree block %llu from get_old_root\n",
+ logical);
+ WARN_ON(1);
+ } else {
+ eb = btrfs_clone_extent_buffer(eb);
+ }
+ } else if (old_root) {
eb = alloc_dummy_extent_buffer(logical, root->nodesize);
- else
+ } else {
eb = btrfs_clone_extent_buffer(root->node);
+ }
btrfs_tree_read_unlock(root->node);
free_extent_buffer(root->node);
if (!eb)
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 3/6] Btrfs: tree mod log's old roots could still be part of the tree
2012-10-23 13:55 ` [PATCH 3/6] Btrfs: tree mod log's old roots could still be part of the tree Jan Schmidt
@ 2012-10-23 15:30 ` Liu Bo
2012-10-24 9:55 ` Liu Bo
1 sibling, 0 replies; 17+ messages in thread
From: Liu Bo @ 2012-10-23 15:30 UTC (permalink / raw)
To: Jan Schmidt; +Cc: chris.mason, linux-btrfs
On 10/23/2012 09:55 PM, Jan Schmidt wrote:
> Tree mod log treated old root buffers as always empty buffers when starting
> the rewind operations. However, the old root may still be part of the
> current tree at a lower level, with still some valid entries.
>
Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
Tested-by: Liu Bo <bo.li.liu@oracle.com>
thanks,
liubo
> Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
> ---
> fs/btrfs/ctree.c | 16 ++++++++++++++--
> 1 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index e6b75cc..1b60c56 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -1239,6 +1239,7 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
> struct tree_mod_root *old_root = NULL;
> u64 old_generation = 0;
> u64 logical;
> + u32 blocksize;
>
> eb = btrfs_read_lock_root_node(root);
> tm = __tree_mod_log_oldest_root(root->fs_info, root, time_seq);
> @@ -1254,10 +1255,21 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
> }
>
> tm = tree_mod_log_search(root->fs_info, logical, time_seq);
> - if (old_root)
> + if (old_root && tm && tm->op != MOD_LOG_KEY_REMOVE_WHILE_FREEING) {
> + blocksize = btrfs_level_size(root, old_root->level);
> + eb = read_tree_block(root, logical, blocksize, 0);
> + if (!eb) {
> + pr_warn("btrfs: failed to read tree block %llu from get_old_root\n",
> + logical);
> + WARN_ON(1);
> + } else {
> + eb = btrfs_clone_extent_buffer(eb);
> + }
> + } else if (old_root) {
> eb = alloc_dummy_extent_buffer(logical, root->nodesize);
> - else
> + } else {
> eb = btrfs_clone_extent_buffer(root->node);
> + }
> btrfs_tree_read_unlock(root->node);
> free_extent_buffer(root->node);
> if (!eb)
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/6] Btrfs: tree mod log's old roots could still be part of the tree
2012-10-23 13:55 ` [PATCH 3/6] Btrfs: tree mod log's old roots could still be part of the tree Jan Schmidt
2012-10-23 15:30 ` Liu Bo
@ 2012-10-24 9:55 ` Liu Bo
2012-10-24 10:45 ` [PATCH] Btrfs: drop locks before calling read_tree_block from get_old_root Jan Schmidt
1 sibling, 1 reply; 17+ messages in thread
From: Liu Bo @ 2012-10-24 9:55 UTC (permalink / raw)
To: Jan Schmidt; +Cc: chris.mason, linux-btrfs
On 10/23/2012 09:55 PM, Jan Schmidt wrote:
> Tree mod log treated old root buffers as always empty buffers when starting
> the rewind operations. However, the old root may still be part of the
> current tree at a lower level, with still some valid entries.
>
> Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
> ---
> fs/btrfs/ctree.c | 16 ++++++++++++++--
> 1 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index e6b75cc..1b60c56 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -1239,6 +1239,7 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
> struct tree_mod_root *old_root = NULL;
> u64 old_generation = 0;
> u64 logical;
> + u32 blocksize;
>
> eb = btrfs_read_lock_root_node(root);
> tm = __tree_mod_log_oldest_root(root->fs_info, root, time_seq);
> @@ -1254,10 +1255,21 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
> }
>
> tm = tree_mod_log_search(root->fs_info, logical, time_seq);
> - if (old_root)
> + if (old_root && tm && tm->op != MOD_LOG_KEY_REMOVE_WHILE_FREEING) {
> + blocksize = btrfs_level_size(root, old_root->level);
> + eb = read_tree_block(root, logical, blocksize, 0);
I found that read_tree_block() may use GFP_NOFS if we need to create an eb,
and this may block read_lock().
It's not always blocked but it did block, so we'd better give an atomic version of this.
thanks,
liubo
> + if (!eb) {
> + pr_warn("btrfs: failed to read tree block %llu from get_old_root\n",
> + logical);
> + WARN_ON(1);
> + } else {
> + eb = btrfs_clone_extent_buffer(eb);
> + }
> + } else if (old_root) {
> eb = alloc_dummy_extent_buffer(logical, root->nodesize);
> - else
> + } else {
> eb = btrfs_clone_extent_buffer(root->node);
> + }
> btrfs_tree_read_unlock(root->node);
> free_extent_buffer(root->node);
> if (!eb)
>
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH] Btrfs: drop locks before calling read_tree_block from get_old_root
2012-10-24 9:55 ` Liu Bo
@ 2012-10-24 10:45 ` Jan Schmidt
2012-10-24 14:18 ` Liu Bo
0 siblings, 1 reply; 17+ messages in thread
From: Jan Schmidt @ 2012-10-24 10:45 UTC (permalink / raw)
To: chris.mason, bo.li.liu; +Cc: linux-btrfs
get_old_root needs its lock on the root node only to clone that buffer.
However, when we call read_tree_block, we know already that we're not
going to clone the root node. Thus we can safely unlock before, to avoid
issues where read_tree_block makes potentially sleeping allocations.
Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
---
Thanks for pointing that out, liubo!
I added this additional patch on top of my patch staple at:
git://git.jan-o-sch.net/btrfs-unstable for-chris
I also made a second branch, because this last commit actually fixes
that's introduced by one of the patches in the patch set. So I folded
it into the respective commit:
git://git.jan-o-sch.net/btrfs-unstable for-chris-fixed
I made sure that the resulting code is the same.
Chris: please pull either one for rc3.
-Jan
---
fs/btrfs/ctree.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index c608b3c..eba44b0 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1261,6 +1261,8 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
tm = tree_mod_log_search(root->fs_info, logical, time_seq);
if (old_root && tm && tm->op != MOD_LOG_KEY_REMOVE_WHILE_FREEING) {
+ btrfs_tree_read_unlock(root->node);
+ free_extent_buffer(root->node);
blocksize = btrfs_level_size(root, old_root->level);
eb = read_tree_block(root, logical, blocksize, 0);
if (!eb) {
@@ -1271,12 +1273,15 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
eb = btrfs_clone_extent_buffer(eb);
}
} else if (old_root) {
+ btrfs_tree_read_unlock(root->node);
+ free_extent_buffer(root->node);
eb = alloc_dummy_extent_buffer(logical, root->nodesize);
} else {
eb = btrfs_clone_extent_buffer(root->node);
+ btrfs_tree_read_unlock(root->node);
+ free_extent_buffer(root->node);
}
- btrfs_tree_read_unlock(root->node);
- free_extent_buffer(root->node);
+
if (!eb)
return NULL;
extent_buffer_get(eb);
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH] Btrfs: drop locks before calling read_tree_block from get_old_root
2012-10-24 10:45 ` [PATCH] Btrfs: drop locks before calling read_tree_block from get_old_root Jan Schmidt
@ 2012-10-24 14:18 ` Liu Bo
0 siblings, 0 replies; 17+ messages in thread
From: Liu Bo @ 2012-10-24 14:18 UTC (permalink / raw)
To: Jan Schmidt; +Cc: chris.mason, linux-btrfs
On 10/24/2012 06:45 PM, Jan Schmidt wrote:
> get_old_root needs its lock on the root node only to clone that buffer.
> However, when we call read_tree_block, we know already that we're not
> going to clone the root node. Thus we can safely unlock before, to avoid
> issues where read_tree_block makes potentially sleeping allocations.
>
I tested this for 20 times, it's no more blocked.
So you can add
Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
Tested-by: Liu Bo <bo.li.liu@oracle.com>
thanks,
liubo
> Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
> ---
> Thanks for pointing that out, liubo!
>
> I added this additional patch on top of my patch staple at:
>
> git://git.jan-o-sch.net/btrfs-unstable for-chris
>
> I also made a second branch, because this last commit actually fixes
> that's introduced by one of the patches in the patch set. So I folded
> it into the respective commit:
>
> git://git.jan-o-sch.net/btrfs-unstable for-chris-fixed
>
> I made sure that the resulting code is the same.
>
> Chris: please pull either one for rc3.
>
> -Jan
>
> ---
> fs/btrfs/ctree.c | 9 +++++++--
> 1 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index c608b3c..eba44b0 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -1261,6 +1261,8 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
>
> tm = tree_mod_log_search(root->fs_info, logical, time_seq);
> if (old_root && tm && tm->op != MOD_LOG_KEY_REMOVE_WHILE_FREEING) {
> + btrfs_tree_read_unlock(root->node);
> + free_extent_buffer(root->node);
> blocksize = btrfs_level_size(root, old_root->level);
> eb = read_tree_block(root, logical, blocksize, 0);
> if (!eb) {
> @@ -1271,12 +1273,15 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
> eb = btrfs_clone_extent_buffer(eb);
> }
> } else if (old_root) {
> + btrfs_tree_read_unlock(root->node);
> + free_extent_buffer(root->node);
> eb = alloc_dummy_extent_buffer(logical, root->nodesize);
> } else {
> eb = btrfs_clone_extent_buffer(root->node);
> + btrfs_tree_read_unlock(root->node);
> + free_extent_buffer(root->node);
> }
> - btrfs_tree_read_unlock(root->node);
> - free_extent_buffer(root->node);
> +
> if (!eb)
> return NULL;
> extent_buffer_get(eb);
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/6] Btrfs: determine level of old roots
2012-10-23 13:55 [PATCH 0/6] Btrfs: tree mod log fixes for 3.7-rc3 Jan Schmidt
` (2 preceding siblings ...)
2012-10-23 13:55 ` [PATCH 3/6] Btrfs: tree mod log's old roots could still be part of the tree Jan Schmidt
@ 2012-10-23 13:55 ` Jan Schmidt
2012-10-23 15:31 ` Liu Bo
2012-10-23 13:55 ` [PATCH 5/6] Btrfs: fix extent buffer reference for tree mod log roots Jan Schmidt
2012-10-23 13:55 ` [PATCH 6/6] Btrfs: comment for loop in tree_mod_log_insert_move Jan Schmidt
5 siblings, 1 reply; 17+ messages in thread
From: Jan Schmidt @ 2012-10-23 13:55 UTC (permalink / raw)
To: chris.mason; +Cc: linux-btrfs, bo.li.liu
In btrfs_find_all_roots' termination condition, we compare the level of the
old buffer we got from btrfs_search_old_slot to the level of the current
root node. We'd better compare it to the level of the rewinded root node.
Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
---
fs/btrfs/backref.c | 4 +---
fs/btrfs/ctree.c | 17 +++++++++++++++++
fs/btrfs/ctree.h | 1 +
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index f318793..65608fb 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -283,9 +283,7 @@ static int __resolve_indirect_ref(struct btrfs_fs_info *fs_info,
goto out;
}
- rcu_read_lock();
- root_level = btrfs_header_level(root->node);
- rcu_read_unlock();
+ root_level = btrfs_old_root_level(root, time_seq);
if (root_level + 1 == level)
goto out;
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 1b60c56..7093ff5 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1292,6 +1292,23 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
return eb;
}
+int btrfs_old_root_level(struct btrfs_root *root, u64 time_seq)
+{
+ struct tree_mod_elem *tm;
+ int level;
+
+ tm = __tree_mod_log_oldest_root(root->fs_info, root, time_seq);
+ if (tm && tm->op == MOD_LOG_ROOT_REPLACE) {
+ level = tm->old_root.level;
+ } else {
+ rcu_read_lock();
+ level = btrfs_header_level(root->node);
+ rcu_read_unlock();
+ }
+
+ return level;
+}
+
static inline int should_cow_block(struct btrfs_trans_handle *trans,
struct btrfs_root *root,
struct extent_buffer *buf)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 1630be8..34c5a44 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3120,6 +3120,7 @@ static inline u64 btrfs_inc_tree_mod_seq(struct btrfs_fs_info *fs_info)
{
return atomic_inc_return(&fs_info->tree_mod_seq);
}
+int btrfs_old_root_level(struct btrfs_root *root, u64 time_seq);
/* root-item.c */
int btrfs_find_root_ref(struct btrfs_root *tree_root,
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 4/6] Btrfs: determine level of old roots
2012-10-23 13:55 ` [PATCH 4/6] Btrfs: determine level of old roots Jan Schmidt
@ 2012-10-23 15:31 ` Liu Bo
0 siblings, 0 replies; 17+ messages in thread
From: Liu Bo @ 2012-10-23 15:31 UTC (permalink / raw)
To: Jan Schmidt; +Cc: chris.mason, linux-btrfs
On 10/23/2012 09:55 PM, Jan Schmidt wrote:
> In btrfs_find_all_roots' termination condition, we compare the level of the
> old buffer we got from btrfs_search_old_slot to the level of the current
> root node. We'd better compare it to the level of the rewinded root node.
>
Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
Tested-by: Liu Bo <bo.li.liu@oracle.com>
thanks,
liubo
> Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
> ---
> fs/btrfs/backref.c | 4 +---
> fs/btrfs/ctree.c | 17 +++++++++++++++++
> fs/btrfs/ctree.h | 1 +
> 3 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index f318793..65608fb 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -283,9 +283,7 @@ static int __resolve_indirect_ref(struct btrfs_fs_info *fs_info,
> goto out;
> }
>
> - rcu_read_lock();
> - root_level = btrfs_header_level(root->node);
> - rcu_read_unlock();
> + root_level = btrfs_old_root_level(root, time_seq);
>
> if (root_level + 1 == level)
> goto out;
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 1b60c56..7093ff5 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -1292,6 +1292,23 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
> return eb;
> }
>
> +int btrfs_old_root_level(struct btrfs_root *root, u64 time_seq)
> +{
> + struct tree_mod_elem *tm;
> + int level;
> +
> + tm = __tree_mod_log_oldest_root(root->fs_info, root, time_seq);
> + if (tm && tm->op == MOD_LOG_ROOT_REPLACE) {
> + level = tm->old_root.level;
> + } else {
> + rcu_read_lock();
> + level = btrfs_header_level(root->node);
> + rcu_read_unlock();
> + }
> +
> + return level;
> +}
> +
> static inline int should_cow_block(struct btrfs_trans_handle *trans,
> struct btrfs_root *root,
> struct extent_buffer *buf)
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 1630be8..34c5a44 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3120,6 +3120,7 @@ static inline u64 btrfs_inc_tree_mod_seq(struct btrfs_fs_info *fs_info)
> {
> return atomic_inc_return(&fs_info->tree_mod_seq);
> }
> +int btrfs_old_root_level(struct btrfs_root *root, u64 time_seq);
>
> /* root-item.c */
> int btrfs_find_root_ref(struct btrfs_root *tree_root,
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/6] Btrfs: fix extent buffer reference for tree mod log roots
2012-10-23 13:55 [PATCH 0/6] Btrfs: tree mod log fixes for 3.7-rc3 Jan Schmidt
` (3 preceding siblings ...)
2012-10-23 13:55 ` [PATCH 4/6] Btrfs: determine level of old roots Jan Schmidt
@ 2012-10-23 13:55 ` Jan Schmidt
2012-10-23 15:32 ` Liu Bo
2012-10-23 13:55 ` [PATCH 6/6] Btrfs: comment for loop in tree_mod_log_insert_move Jan Schmidt
5 siblings, 1 reply; 17+ messages in thread
From: Jan Schmidt @ 2012-10-23 13:55 UTC (permalink / raw)
To: chris.mason; +Cc: linux-btrfs, bo.li.liu
In get_old_root we grab a lock on the extent buffer before we obtain a
reference on that buffer. That order is changed now.
Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
---
fs/btrfs/ctree.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 7093ff5..bed06e8 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1274,6 +1274,7 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
free_extent_buffer(root->node);
if (!eb)
return NULL;
+ extent_buffer_get(eb);
btrfs_tree_read_lock(eb);
if (old_root) {
btrfs_set_header_bytenr(eb, eb->start);
@@ -1286,7 +1287,6 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
__tree_mod_log_rewind(eb, time_seq, tm);
else
WARN_ON(btrfs_header_level(eb) != 0);
- extent_buffer_get(eb);
WARN_ON(btrfs_header_nritems(eb) > BTRFS_NODEPTRS_PER_BLOCK(root));
return eb;
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 5/6] Btrfs: fix extent buffer reference for tree mod log roots
2012-10-23 13:55 ` [PATCH 5/6] Btrfs: fix extent buffer reference for tree mod log roots Jan Schmidt
@ 2012-10-23 15:32 ` Liu Bo
0 siblings, 0 replies; 17+ messages in thread
From: Liu Bo @ 2012-10-23 15:32 UTC (permalink / raw)
To: Jan Schmidt; +Cc: chris.mason, linux-btrfs
On 10/23/2012 09:55 PM, Jan Schmidt wrote:
> In get_old_root we grab a lock on the extent buffer before we obtain a
> reference on that buffer. That order is changed now.
>
Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
thanks,
liubo
> Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
> ---
> fs/btrfs/ctree.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 7093ff5..bed06e8 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -1274,6 +1274,7 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
> free_extent_buffer(root->node);
> if (!eb)
> return NULL;
> + extent_buffer_get(eb);
> btrfs_tree_read_lock(eb);
> if (old_root) {
> btrfs_set_header_bytenr(eb, eb->start);
> @@ -1286,7 +1287,6 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
> __tree_mod_log_rewind(eb, time_seq, tm);
> else
> WARN_ON(btrfs_header_level(eb) != 0);
> - extent_buffer_get(eb);
> WARN_ON(btrfs_header_nritems(eb) > BTRFS_NODEPTRS_PER_BLOCK(root));
>
> return eb;
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 6/6] Btrfs: comment for loop in tree_mod_log_insert_move
2012-10-23 13:55 [PATCH 0/6] Btrfs: tree mod log fixes for 3.7-rc3 Jan Schmidt
` (4 preceding siblings ...)
2012-10-23 13:55 ` [PATCH 5/6] Btrfs: fix extent buffer reference for tree mod log roots Jan Schmidt
@ 2012-10-23 13:55 ` Jan Schmidt
2012-10-23 15:32 ` Liu Bo
5 siblings, 1 reply; 17+ messages in thread
From: Jan Schmidt @ 2012-10-23 13:55 UTC (permalink / raw)
To: chris.mason; +Cc: linux-btrfs, bo.li.liu
Emphasis the way tree_mod_log_insert_move avoids adding
MOD_LOG_KEY_REMOVE_WHILE_MOVING operations, depending on the direction of
the move operation.
Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
---
fs/btrfs/ctree.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index bed06e8..c608b3c 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -596,6 +596,11 @@ tree_mod_log_insert_move(struct btrfs_fs_info *fs_info,
if (tree_mod_dont_log(fs_info, eb))
return 0;
+ /*
+ * When we override something during the move, we log these removals.
+ * This can only happen when we move towards the beginning of the
+ * buffer, i.e. dst_slot < src_slot.
+ */
for (i = 0; i + dst_slot < src_slot && i < nr_items; i++) {
ret = tree_mod_log_insert_key_locked(fs_info, eb, i + dst_slot,
MOD_LOG_KEY_REMOVE_WHILE_MOVING);
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 6/6] Btrfs: comment for loop in tree_mod_log_insert_move
2012-10-23 13:55 ` [PATCH 6/6] Btrfs: comment for loop in tree_mod_log_insert_move Jan Schmidt
@ 2012-10-23 15:32 ` Liu Bo
0 siblings, 0 replies; 17+ messages in thread
From: Liu Bo @ 2012-10-23 15:32 UTC (permalink / raw)
To: Jan Schmidt; +Cc: chris.mason, linux-btrfs
On 10/23/2012 09:55 PM, Jan Schmidt wrote:
> Emphasis the way tree_mod_log_insert_move avoids adding
> MOD_LOG_KEY_REMOVE_WHILE_MOVING operations, depending on the direction of
> the move operation.
>
Good work.
Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
thanks,
liubo
> Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
> ---
> fs/btrfs/ctree.c | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index bed06e8..c608b3c 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -596,6 +596,11 @@ tree_mod_log_insert_move(struct btrfs_fs_info *fs_info,
> if (tree_mod_dont_log(fs_info, eb))
> return 0;
>
> + /*
> + * When we override something during the move, we log these removals.
> + * This can only happen when we move towards the beginning of the
> + * buffer, i.e. dst_slot < src_slot.
> + */
> for (i = 0; i + dst_slot < src_slot && i < nr_items; i++) {
> ret = tree_mod_log_insert_key_locked(fs_info, eb, i + dst_slot,
> MOD_LOG_KEY_REMOVE_WHILE_MOVING);
>
^ permalink raw reply [flat|nested] 17+ messages in thread