* [PATCH][v2] btrfs: don't bug_on with enomem in __clear_state_bit
@ 2017-11-10 15:13 Josef Bacik
2017-11-30 17:59 ` David Sterba
0 siblings, 1 reply; 2+ messages in thread
From: Josef Bacik @ 2017-11-10 15:13 UTC (permalink / raw)
To: linux-btrfs, kernel-team; +Cc: Josef Bacik
From: Josef Bacik <jbacik@fb.com>
Since we're allocating under atomic we could every easily enomem, so if
that's the case and we can block then loop around and try to allocate
the prealloc not under a lock.
We also saw this happen during try_to_release_page in production, in
which case it's completely valid to return ENOMEM so we can tell
try_to_release_page that we can't release this page.
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
v1->v2:
- this time with my local changes committed!
fs/btrfs/extent_io.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index dd941885b9c3..fb0c1636c1c4 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -590,8 +590,9 @@ static int __clear_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
struct extent_state *prealloc = NULL;
struct rb_node *node;
u64 last_end;
- int err;
+ int err = 0;
int clear = 0;
+ bool need_prealloc = false;
btrfs_debug_check_extent_io_range(tree, start, end);
@@ -614,6 +615,9 @@ static int __clear_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
* If we end up needing a new extent state we allocate it later.
*/
prealloc = alloc_extent_state(mask);
+ if (!prealloc && need_prealloc)
+ return -ENOMEM;
+ need_prealloc = false;
}
spin_lock(&tree->lock);
@@ -673,7 +677,15 @@ static int __clear_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
if (state->start < start) {
prealloc = alloc_extent_state_atomic(prealloc);
- BUG_ON(!prealloc);
+ if (!prealloc) {
+ if (gfpflags_allow_blocking(mask)) {
+ need_prealloc = true;
+ spin_unlock(&tree->lock);
+ goto again;
+ }
+ err = -ENOMEM;
+ goto out;
+ }
err = split_state(tree, state, prealloc, start);
if (err)
extent_io_tree_panic(tree, err);
@@ -696,7 +708,15 @@ static int __clear_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
*/
if (state->start <= end && state->end > end) {
prealloc = alloc_extent_state_atomic(prealloc);
- BUG_ON(!prealloc);
+ if (!prealloc) {
+ if (gfpflags_allow_blocking(mask)) {
+ need_prealloc = true;
+ spin_unlock(&tree->lock);
+ goto again;
+ }
+ err = -ENOMEM;
+ goto out;
+ }
err = split_state(tree, state, prealloc, end + 1);
if (err)
extent_io_tree_panic(tree, err);
@@ -731,7 +751,7 @@ static int __clear_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
if (prealloc)
free_extent_state(prealloc);
- return 0;
+ return err;
}
--
2.7.5
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH][v2] btrfs: don't bug_on with enomem in __clear_state_bit
2017-11-10 15:13 [PATCH][v2] btrfs: don't bug_on with enomem in __clear_state_bit Josef Bacik
@ 2017-11-30 17:59 ` David Sterba
0 siblings, 0 replies; 2+ messages in thread
From: David Sterba @ 2017-11-30 17:59 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, kernel-team, Josef Bacik
On Fri, Nov 10, 2017 at 10:13:40AM -0500, Josef Bacik wrote:
> From: Josef Bacik <jbacik@fb.com>
>
> Since we're allocating under atomic we could every easily enomem, so if
> that's the case and we can block then loop around and try to allocate
> the prealloc not under a lock.
>
> We also saw this happen during try_to_release_page in production, in
> which case it's completely valid to return ENOMEM so we can tell
> try_to_release_page that we can't release this page.
Have you audited that all direct and indirect callers of
__clear_extent_bit handle the errors? Because they do not. The only case
that seem to understand ENOMEM failures of __clear_extent_bit is
try_release_extent_state.
Almost anything else that calls __clear_extent_bit, clear_extent_bit and
other simple wrappers do not check the value and would happily continue.
> @@ -673,7 +677,15 @@ static int __clear_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
>
> if (state->start < start) {
> prealloc = alloc_extent_state_atomic(prealloc);
> - BUG_ON(!prealloc);
> + if (!prealloc) {
> + if (gfpflags_allow_blocking(mask)) {
> + need_prealloc = true;
> + spin_unlock(&tree->lock);
> + goto again;
> + }
> + err = -ENOMEM;
The retry logic is good, but until ENOMEM is properly handled
everywhere, the safest thing is to move the BUG_ON here.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-11-30 18:01 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-10 15:13 [PATCH][v2] btrfs: don't bug_on with enomem in __clear_state_bit Josef Bacik
2017-11-30 17:59 ` David Sterba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).