[PATCH] btrfs:Change BUG_ON to new error path in __clear_extent_bit

[PATCH] btrfs:Change BUG_ON to new error path in __clear_extent_bit

[Bastien Philbert]

This remove the unnessary BUG_ON if the allocation with
alloc_extent_state_atomic fails due to this function
failure not being unrecoverable. Instead we now change
this BUG_ON into a new error path that jumps to the goto
label, out from freeing previously allocated resources
before returning the error code -ENOMEM to signal callers
that the call to __clear_extent_bit failed due to a memory
allocation failure.

Signed-off-by: Bastien Philbert <bastienphilbert@gmail.com>
---
 fs/btrfs/extent_io.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index d247fc0..4c87b77 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -682,7 +682,10 @@ hit_next:
 
 	if (state->start < start) {
 		prealloc = alloc_extent_state_atomic(prealloc);
-		BUG_ON(!prealloc);
+		if (!prealloc) {
+			err = -ENOMEM;
+			goto out;
+		}
 		err = split_state(tree, state, prealloc, start);
 		if (err)
 			extent_io_tree_panic(tree, err);
-- 
2.5.0

Re: [PATCH] btrfs:Change BUG_ON to new error path in __clear_extent_bit

[Filipe Manana]

On Wed, Apr 6, 2016 at 4:56 PM, Bastien Philbert
<bastienphilbert@gmail.com> wrote:
> This remove the unnessary BUG_ON if the allocation with
> alloc_extent_state_atomic fails due to this function
> failure not being unrecoverable. Instead we now change
> this BUG_ON into a new error path that jumps to the goto
> label, out from freeing previously allocated resources
> before returning the error code -ENOMEM to signal callers
> that the call to __clear_extent_bit failed due to a memory
> allocation failure.
>
> Signed-off-by: Bastien Philbert <bastienphilbert@gmail.com>
> ---
>  fs/btrfs/extent_io.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index d247fc0..4c87b77 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -682,7 +682,10 @@ hit_next:
>
>         if (state->start < start) {
>                 prealloc = alloc_extent_state_atomic(prealloc);
> -               BUG_ON(!prealloc);
> +               if (!prealloc) {
> +                       err = -ENOMEM;
> +                       goto out;
> +               }

Why only in this particular place? We do this in other places in this function.

Second, setting err to -ENOMEM is not enough. Under the out label we
always return 0, so you're not propagating the error to callers.

Now, most importantly, you didn't check if callers handle errors from
this function (__clear_extent_bit()) at all. A failure in this
function is critical.
For example, it can cause a range in an inode's io tree to become
locked forever, blocking any other tasks that want to operate on the
range, and we won't ever know what happened.
So it's far from trivial to handle errors from this function and
that's why the BUG_ON is there.

If you really want to get rid of the BUG_ON() calls you need to make
sure all callers don't ignore the errors and that they deal with them
properly.


>                 err = split_state(tree, state, prealloc, start);
>                 if (err)
>                         extent_io_tree_panic(tree, err);
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH] btrfs:Change BUG_ON to new error path in __clear_extent_bit

[Bastien Philbert]

On 2016-04-06 12:10 PM, Filipe Manana wrote:
> On Wed, Apr 6, 2016 at 4:56 PM, Bastien Philbert
> <bastienphilbert@gmail.com> wrote:
>> This remove the unnessary BUG_ON if the allocation with
>> alloc_extent_state_atomic fails due to this function
>> failure not being unrecoverable. Instead we now change
>> this BUG_ON into a new error path that jumps to the goto
>> label, out from freeing previously allocated resources
>> before returning the error code -ENOMEM to signal callers
>> that the call to __clear_extent_bit failed due to a memory
>> allocation failure.
>>
>> Signed-off-by: Bastien Philbert <bastienphilbert@gmail.com>
>> ---
>>  fs/btrfs/extent_io.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index d247fc0..4c87b77 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -682,7 +682,10 @@ hit_next:
>>
>>         if (state->start < start) {
>>                 prealloc = alloc_extent_state_atomic(prealloc);
>> -               BUG_ON(!prealloc);
>> +               if (!prealloc) {
>> +                       err = -ENOMEM;
>> +                       goto out;
>> +               }
> 
> Why only in this particular place? We do this in other places in this function.
> 
> Second, setting err to -ENOMEM is not enough. Under the out label we
> always return 0, so you're not propagating the error to callers.
> 
> Now, most importantly, you didn't check if callers handle errors from
> this function (__clear_extent_bit()) at all. A failure in this
> function is critical.
> For example, it can cause a range in an inode's io tree to become
> locked forever, blocking any other tasks that want to operate on the
> range, and we won't ever know what happened.
> So it's far from trivial to handle errors from this function and
> that's why the BUG_ON is there.
> 
> If you really want to get rid of the BUG_ON() calls you need to make
> sure all callers don't ignore the errors and that they deal with them
> properly.
Sorry, I feel really stupid about missing those other callers. :(
Bastien
> 
> 
>>                 err = split_state(tree, state, prealloc, start);
>>                 if (err)
>>                         extent_io_tree_panic(tree, err);
>> --
>> 2.5.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
>