Xiao Guangrong
2011-Apr-12 08:14 UTC
[PATCH 1/2] Btrfs: allocate extent state and check the result properly
It doesn''t allocate extent_state and check the result properly:
- in set_extent_bit, it doesn''t allocate extent_state if the path is
not
allowed wait
- in clear_extent_bit, it doesn''t check the result after atomic-ly
allocate,
we trigger BUG_ON() if it''s fail
- if allocate fail, we trigger BUG_ON instead of returning -ENOMEM since
the return value of clear_extent_bit() is ignored by many callers
Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
fs/btrfs/extent_io.c | 29 +++++++++++++++++++++--------
1 files changed, 21 insertions(+), 8 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 77c65a0..62d5bca 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -439,6 +439,16 @@ static int clear_state_bit(struct extent_io_tree *tree,
return ret;
}
+static struct extent_state *
+alloc_extent_state_atomic(struct extent_state *prealloc)
+{
+ if (!prealloc)
+ prealloc = alloc_extent_state(GFP_ATOMIC);
+
+ BUG_ON(!prealloc);
+ return prealloc;
+}
+
/*
* clear some bits on a range in the tree. This may require splitting
* or inserting elements in the tree, so the gfp mask is used to
@@ -476,8 +486,7 @@ int clear_extent_bit(struct extent_io_tree *tree, u64 start,
u64 end,
again:
if (!prealloc && (mask & __GFP_WAIT)) {
prealloc = alloc_extent_state(mask);
- if (!prealloc)
- return -ENOMEM;
+ BUG_ON(!prealloc);
}
spin_lock(&tree->lock);
@@ -529,8 +538,7 @@ hit_next:
*/
if (state->start < start) {
- if (!prealloc)
- prealloc = alloc_extent_state(GFP_ATOMIC);
+ prealloc = alloc_extent_state_atomic(prealloc);
err = split_state(tree, state, prealloc, start);
BUG_ON(err == -EEXIST);
prealloc = NULL;
@@ -551,8 +559,7 @@ hit_next:
* on the first half
*/
if (state->start <= end && state->end > end) {
- if (!prealloc)
- prealloc = alloc_extent_state(GFP_ATOMIC);
+ prealloc = alloc_extent_state_atomic(prealloc);
err = split_state(tree, state, prealloc, end + 1);
BUG_ON(err == -EEXIST);
if (wake)
@@ -716,8 +723,7 @@ int set_extent_bit(struct extent_io_tree *tree, u64 start,
u64 end,
again:
if (!prealloc && (mask & __GFP_WAIT)) {
prealloc = alloc_extent_state(mask);
- if (!prealloc)
- return -ENOMEM;
+ BUG_ON(!prealloc);
}
spin_lock(&tree->lock);
@@ -734,6 +740,7 @@ again:
*/
node = tree_search(tree, start);
if (!node) {
+ prealloc = alloc_extent_state_atomic(prealloc);
err = insert_state(tree, prealloc, start, end, &bits);
prealloc = NULL;
BUG_ON(err == -EEXIST);
@@ -802,6 +809,8 @@ hit_next:
err = -EEXIST;
goto out;
}
+
+ prealloc = alloc_extent_state_atomic(prealloc);
err = split_state(tree, state, prealloc, start);
BUG_ON(err == -EEXIST);
prealloc = NULL;
@@ -832,6 +841,8 @@ hit_next:
this_end = end;
else
this_end = last_start - 1;
+
+ prealloc = alloc_extent_state_atomic(prealloc);
err = insert_state(tree, prealloc, start, this_end,
&bits);
BUG_ON(err == -EEXIST);
@@ -856,6 +867,8 @@ hit_next:
err = -EEXIST;
goto out;
}
+
+ prealloc = alloc_extent_state_atomic(prealloc);
err = split_state(tree, state, prealloc, end + 1);
BUG_ON(err == -EEXIST);
--
1.7.4
merge_state can free the current state if it can be merged with the next node,
but in set_extent_bit(), after merge_state, we still use the current extent to
get the next node and cache it into cached_state
Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
fs/btrfs/extent_io.c | 22 ++++++++++++++--------
1 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 62d5bca..40cb450 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -769,20 +769,18 @@ hit_next:
if (err)
goto out;
+ next_node = rb_next(node);
cache_state(state, cached_state);
merge_state(tree, state);
if (last_end == (u64)-1)
goto out;
start = last_end + 1;
- if (start < end && prealloc && !need_resched()) {
- next_node = rb_next(node);
- if (next_node) {
- state = rb_entry(next_node, struct extent_state,
- rb_node);
- if (state->start == start)
- goto hit_next;
- }
+ if (next_node && start < end && prealloc &&
!need_resched()) {
+ state = rb_entry(next_node, struct extent_state,
+ rb_node);
+ if (state->start == start)
+ goto hit_next;
}
goto search_again;
}
@@ -843,14 +841,22 @@ hit_next:
this_end = last_start - 1;
prealloc = alloc_extent_state_atomic(prealloc);
+
+ /*
+ * Avoid to free ''prealloc'' if it can be merged with
+ * the later extent.
+ */
+ atomic_inc(&prealloc->refs);
err = insert_state(tree, prealloc, start, this_end,
&bits);
BUG_ON(err == -EEXIST);
if (err) {
+ free_extent_state(prealloc);
prealloc = NULL;
goto out;
}
cache_state(prealloc, cached_state);
+ free_extent_state(prealloc);
prealloc = NULL;
start = this_end + 1;
goto search_again;
--
1.7.4
Xiao Guangrong
2011-Apr-19 01:49 UTC
Re: [PATCH 1/2] Btrfs: allocate extent state and check the result properly
On 04/12/2011 04:14 PM, Xiao Guangrong wrote:> It doesn''t allocate extent_state and check the result properly: > - in set_extent_bit, it doesn''t allocate extent_state if the path is not > allowed wait > > - in clear_extent_bit, it doesn''t check the result after atomic-ly allocate, > we trigger BUG_ON() if it''s fail > > - if allocate fail, we trigger BUG_ON instead of returning -ENOMEM since > the return value of clear_extent_bit() is ignored by many callers >Ping...to see what happened. :-)
Chris Mason
2011-Apr-19 11:55 UTC
Re: [PATCH 1/2] Btrfs: allocate extent state and check the result properly
Excerpts from Xiao Guangrong''s message of 2011-04-18 21:49:52 -0400:> On 04/12/2011 04:14 PM, Xiao Guangrong wrote: > > It doesn''t allocate extent_state and check the result properly: > > - in set_extent_bit, it doesn''t allocate extent_state if the path is not > > allowed wait > > > > - in clear_extent_bit, it doesn''t check the result after atomic-ly allocate, > > we trigger BUG_ON() if it''s fail > > > > - if allocate fail, we trigger BUG_ON instead of returning -ENOMEM since > > the return value of clear_extent_bit() is ignored by many callers > > > > Ping...to see what happened. :-)Sorry, could you please change this to check the results of the atomic allocatoin in set/clear_extent_bit? I''d rather not add a new BUG_ON deeper into the code. -chris
Xiao Guangrong
2011-Apr-20 06:49 UTC
Re: [PATCH 1/2] Btrfs: allocate extent state and check the result properly
On 04/19/2011 07:55 PM, Chris Mason wrote:> Sorry, could you please change this to check the results of the atomic > allocatoin in set/clear_extent_bit? I''d rather not add a new BUG_ON > deeper into the code. >OK, i have posted the v2 patch to fix it, thanks for your review!