2011-04-20 06:44:13

by Xiao Guangrong

Subject: [PATCH v2 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 <[email protected]>
---
fs/btrfs/extent_io.c | 34 ++++++++++++++++++++++++++--------
1 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 5ae0bff..f36e284 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -439,6 +439,15 @@ 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);
+
+ 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 +485,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 +537,8 @@ hit_next:
*/

if (state->start < start) {
- if (!prealloc)
- prealloc = alloc_extent_state(GFP_ATOMIC);
+ prealloc = alloc_extent_state_atomic(prealloc);
+ BUG_ON(!prealloc);
err = split_state(tree, state, prealloc, start);
BUG_ON(err == -EEXIST);
prealloc = NULL;
@@ -551,8 +559,8 @@ 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);
+ BUG_ON(!prealloc);
err = split_state(tree, state, prealloc, end + 1);
BUG_ON(err == -EEXIST);
if (wake)
@@ -725,8 +733,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);
@@ -743,6 +750,8 @@ again:
*/
node = tree_search(tree, start);
if (!node) {
+ prealloc = alloc_extent_state_atomic(prealloc);
+ BUG_ON(!prealloc);
err = insert_state(tree, prealloc, start, end, &bits);
prealloc = NULL;
BUG_ON(err == -EEXIST);
@@ -811,6 +820,9 @@ hit_next:
err = -EEXIST;
goto out;
}
+
+ prealloc = alloc_extent_state_atomic(prealloc);
+ BUG_ON(!prealloc);
err = split_state(tree, state, prealloc, start);
BUG_ON(err == -EEXIST);
prealloc = NULL;
@@ -841,6 +853,9 @@ hit_next:
this_end = end;
else
this_end = last_start - 1;
+
+ prealloc = alloc_extent_state_atomic(prealloc);
+ BUG_ON(!prealloc);
err = insert_state(tree, prealloc, start, this_end,
&bits);
BUG_ON(err == -EEXIST);
@@ -865,6 +880,9 @@ hit_next:
err = -EEXIST;
goto out;
}
+
+ prealloc = alloc_extent_state_atomic(prealloc);
+ BUG_ON(!prealloc);
err = split_state(tree, state, prealloc, end + 1);
BUG_ON(err == -EEXIST);

--
1.7.4.2

2011-04-20 06:45:06

by Xiao Guangrong

Subject: [PATCH v2 2/2] Btrfs: fix unsafe usage of merge_state

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 <[email protected]>
---
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 f36e284..129601e 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -780,20 +780,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;
}
@@ -856,14 +854,22 @@ hit_next:

prealloc = alloc_extent_state_atomic(prealloc);
BUG_ON(!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.2