[PATCH] btrfs: remove BUG_ON used as assertions

[PATCH] btrfs: remove BUG_ON used as assertions

[Aditya Pakki]

alloc_extent_state_atomic() allocates extents via GFP_ATOMIC flag
and cannot fail. There are multiple invocations of BUG_ON on the
return value to check for failure. The patch replaces certain
invocations of BUG_ON by returning the error upstream.

Signed-off-by: Aditya Pakki <pakki001@umn.edu>
---
 fs/btrfs/extent_io.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index eb8bd0258360..e72e5a333e71 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -989,7 +989,10 @@ __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 	node = tree_search_for_insert(tree, start, &p, &parent);
 	if (!node) {
 		prealloc = alloc_extent_state_atomic(prealloc);
-		BUG_ON(!prealloc);
+		if (!prealloc) {
+			err = -ENOMEM;
+			goto out;
+		}
 		err = insert_state(tree, prealloc, start, end,
 				   &p, &parent, &bits, changeset);
 		if (err)
@@ -1054,7 +1057,10 @@ __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 		}
 
 		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);
@@ -1091,7 +1097,10 @@ __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 			this_end = last_start - 1;
 
 		prealloc = alloc_extent_state_atomic(prealloc);
-		BUG_ON(!prealloc);
+		if (!prealloc) {
+			err = -ENOMEM;
+			goto out;
+		}
 
 		/*
 		 * Avoid to free 'prealloc' if it can be merged with
@@ -1121,7 +1130,10 @@ __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 		}
 
 		prealloc = alloc_extent_state_atomic(prealloc);
-		BUG_ON(!prealloc);
+		if (!prealloc) {
+			err = -ENOMEM;
+			goto out;
+		}
 		err = split_state(tree, state, prealloc, end + 1);
 		if (err)
 			extent_io_tree_panic(tree, err);
-- 
2.20.1

Re: [PATCH] btrfs: remove BUG_ON used as assertions

[Nikolay Borisov]

On 15.12.19 г. 19:12 ч., Aditya Pakki wrote:
> alloc_extent_state_atomic() allocates extents via GFP_ATOMIC flag
> and cannot fail. There are multiple invocations of BUG_ON on the
> return value to check for failure. The patch replaces certain
> invocations of BUG_ON by returning the error upstream.
> 
> Signed-off-by: Aditya Pakki <pakki001@umn.edu>

Have you actually audited all callers of __set_extent_bit whether they
correctly handle failures?

Re: [PATCH] btrfs: remove BUG_ON used as assertions

[David Sterba]

On Sun, Dec 15, 2019 at 11:12:36AM -0600, Aditya Pakki wrote:
> alloc_extent_state_atomic() allocates extents via GFP_ATOMIC flag
> and cannot fail.

Here you say it cannot fail (without a proof)

> There are multiple invocations of BUG_ON on the
> return value to check for failure. The patch replaces certain
> invocations of BUG_ON by returning the error upstream.
> 
> Signed-off-by: Aditya Pakki <pakki001@umn.edu>
> ---
>  fs/btrfs/extent_io.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index eb8bd0258360..e72e5a333e71 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -989,7 +989,10 @@ __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
>  	node = tree_search_for_insert(tree, start, &p, &parent);
>  	if (!node) {
>  		prealloc = alloc_extent_state_atomic(prealloc);
> -		BUG_ON(!prealloc);
> +		if (!prealloc) {
> +			err = -ENOMEM;
> +			goto out;

And add error handling when it fails, which is in contradiction but
anyway, replacing BUG_ON with error checks requires auditing the whole
call chain up. We've had attempts to do this change already, none of them
was correct so unfortunatelly the BUG_ON needs to stay to avoid errors
being silently ignored.

Re: [PATCH] btrfs: remove BUG_ON used as assertions

[Josef Bacik]

On 12/15/19 12:12 PM, Aditya Pakki wrote:
> alloc_extent_state_atomic() allocates extents via GFP_ATOMIC flag
> and cannot fail. There are multiple invocations of BUG_ON on the
> return value to check for failure. The patch replaces certain
> invocations of BUG_ON by returning the error upstream.
> 
> Signed-off-by: Aditya Pakki <pakki001@umn.edu>

I already tried this a few months ago and gave up.  There are a few things if 
you want to tackle something like this

1) use bpf's error injection thing to make sure you handle every path that can 
error out.  This is the script I wrote to do just that

https://github.com/josefbacik/debug-scripts/blob/master/error-injection-stress.py

2) We actually can't fail here.  We would need to go back and make _all_ callers 
of lock_extent_bits() handle the allocation error.  This is theoretically 
possible, but a giant pain in the ass.  In general we can make allocations here 
and we need to be able to make them.

3) We should probably mark this path with __GFP_NOFAIL because again, this is 
locking and we need locking to succeed.

Thanks,

Josef

Re: [PATCH] btrfs: remove BUG_ON used as assertions

[David Sterba]

On Wed, Dec 18, 2019 at 11:38:18AM -0500, Josef Bacik wrote:
> On 12/15/19 12:12 PM, Aditya Pakki wrote:
> > alloc_extent_state_atomic() allocates extents via GFP_ATOMIC flag
> > and cannot fail. There are multiple invocations of BUG_ON on the
> > return value to check for failure. The patch replaces certain
> > invocations of BUG_ON by returning the error upstream.
> > 
> > Signed-off-by: Aditya Pakki <pakki001@umn.edu>
> 
> I already tried this a few months ago and gave up.  There are a few things if 
> you want to tackle something like this
> 
> 1) use bpf's error injection thing to make sure you handle every path that can 
> error out.  This is the script I wrote to do just that
> 
> https://github.com/josefbacik/debug-scripts/blob/master/error-injection-stress.py
> 
> 2) We actually can't fail here.  We would need to go back and make _all_ callers 
> of lock_extent_bits() handle the allocation error.  This is theoretically 
> possible, but a giant pain in the ass.  In general we can make allocations here 
> and we need to be able to make them.
> 
> 3) We should probably mark this path with __GFP_NOFAIL because again, this is 
> locking and we need locking to succeed.

NOFAIL can introduce loops that could lead to deadlocks, if not used
carefully. __set_extent_bit is not just locking, so if one thread wants
to set bits, allocate, wait, allocator goes to write some memory

eg.

set_extent_bit on some range
  alloc state (NOFAIL)
    allocator wants to flush dome dirty data
                   ------------------------------>
		                               set_extent_bit
					         alloc state (NOFAIL)
						 (wait)

Re: [PATCH] btrfs: remove BUG_ON used as assertions

[Josef Bacik]

On 12/18/19 11:47 AM, David Sterba wrote:
> On Wed, Dec 18, 2019 at 11:38:18AM -0500, Josef Bacik wrote:
>> On 12/15/19 12:12 PM, Aditya Pakki wrote:
>>> alloc_extent_state_atomic() allocates extents via GFP_ATOMIC flag
>>> and cannot fail. There are multiple invocations of BUG_ON on the
>>> return value to check for failure. The patch replaces certain
>>> invocations of BUG_ON by returning the error upstream.
>>>
>>> Signed-off-by: Aditya Pakki <pakki001@umn.edu>
>>
>> I already tried this a few months ago and gave up.  There are a few things if
>> you want to tackle something like this
>>
>> 1) use bpf's error injection thing to make sure you handle every path that can
>> error out.  This is the script I wrote to do just that
>>
>> https://github.com/josefbacik/debug-scripts/blob/master/error-injection-stress.py
>>
>> 2) We actually can't fail here.  We would need to go back and make _all_ callers
>> of lock_extent_bits() handle the allocation error.  This is theoretically
>> possible, but a giant pain in the ass.  In general we can make allocations here
>> and we need to be able to make them.
>>
>> 3) We should probably mark this path with __GFP_NOFAIL because again, this is
>> locking and we need locking to succeed.
> 
> NOFAIL can introduce loops that could lead to deadlocks, if not used
> carefully. __set_extent_bit is not just locking, so if one thread wants
> to set bits, allocate, wait, allocator goes to write some memory
> 
> eg.
> 
> set_extent_bit on some range
>    alloc state (NOFAIL)
>      allocator wants to flush dome dirty data
>                     ------------------------------>
> 		                               set_extent_bit
> 					         alloc state (NOFAIL)
> 						 (wait)
> 

Yes obviously I just want it for EXTENT_LOCKED.  But we could even just use a 
mempool to be really safe, since most places are going to use GFP_KERNEL or 
something else related, we only really need the safety in a few critical areas. 
Thanks,

Josef