| Message ID | 20191215171237.27482-1-pakki001@umn.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: remove BUG_ON used as assertions | expand |
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?
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.
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
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)
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
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);
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(-)