Btrfs: allocate extent state and check the result properly
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
--- fs/btrfs/extent_io.c | 29 +++++++++++++++++++++-------- 1 files changed, 21 insertions(+), 8 deletions(-)
fs/btrfs/extent_io.c +21 -8
@@ -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 );
Signature · 1 line
1.7.4
PATCH 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
--- fs/btrfs/extent_io.c | 22 ++++++++++++++-------- 1 files changed, 14 insertions(+), 8 deletions(-)
fs/btrfs/extent_io.c +14 -8
@@ -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;
Signature · 1 line
1.7.4
Quoted 10 lines · Xiao Guangrong
It doesn't allocate
extent_stateand check the result properly:
- inset_extent_bit, it doesn't allocateextent_stateif the path is not allowed wait- in
clear_extent_bit, it doesn't check the result after atomic-ly allocate, we triggerBUG_ON()if it's fail- if allocate fail, we trigger
BUG_ONinstead of returning-ENOMEMsince the return value ofclear_extent_bit()is ignored by many callers
Ping...to see what happened. :-)
Excerpts from Xiao Guangrong's message of 2011-04-18 21:49:52 -0400:
Quoted 5 lines · Xiao Guangrong
Quoted 4 lines
It doesn't allocate
extent_stateand check the result properly:
- inset_extent_bit, it doesn't allocateextent_stateif the path is not allowed wait
Quoted 8 lines
Quoted 6 lines
- in
clear_extent_bit, it doesn't check the result after atomic-ly allocate, we triggerBUG_ON()if it's fail- if allocate fail, we trigger
BUG_ONinstead of returning-ENOMEMsince the return value ofclear_extent_bit()is ignored by many callersPing...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
Quoted 4 lines · Chris Mason
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_ONdeeper into the code.
OK, i have posted the v2 patch to fix it, thanks for your review!