btrfs: Avoid BUG_ON()s because of ENOMEM caused by kmalloc() failure

PATCH In review Satoru Takeuchi 2 participants 5 messages Feb 15, 2016 – Feb 18, 2016 Btrfs file system

lore

Share & tools

lore

Satoru Takeuchi

author

2016-02-15 05:39 UTC #0 lore raw

PATCH 1/1 btrfs: Avoid BUG_ON()s because of ENOMEM caused by kmalloc() failure

There are some BUG_ON()'s after kmalloc() as follows.

=====
foo = kmalloc();
BUG_ON(!foo); /* -ENOMEM case */
=====

A Docker + memory cgroup user hit these BUG_ON()s.

https://bugzilla.kernel.org/show_bug.cgi?id=112101

Since it's very hard to handle these ENOMEMs properly, preventing these kmalloc() failures to avoid these BUG_ON()s for now, are a bit better than the current implementation anyway.

Signed-off-by Satoru Takeuchi <[email protected]>

---
 fs/btrfs/extent_io.c  | 6 ++----
 fs/btrfs/inode.c      | 6 ++----
 fs/btrfs/relocation.c | 3 +--
 3 files changed, 5 insertions(+), 10 deletions(-)

fs/btrfs/extent_io.c +2 -4

index 2e7c97a..5f92450 100644
@@ -874,10 +874,8 @@ __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
 
 	bits |= EXTENT_FIRST_DELALLOC;
 again:
-	if (!prealloc && gfpflags_allow_blocking(mask)) {
-		prealloc = alloc_extent_state(mask);
-		BUG_ON(!prealloc);
-	}
+	if (!prealloc && gfpflags_allow_blocking(mask))
+		prealloc = alloc_extent_state(mask|__GFP_NOFAIL);
 
 	spin_lock(&tree->lock);
 	if (cached_state && *cached_state) {

fs/btrfs/inode.c +2 -4

index 85afe66..d20e5c5 100644
@@ -357,8 +357,7 @@ static noinline int add_async_extent(struct async_cow *cow,
 {
 	struct async_extent *async_extent;
 
-	async_extent = kmalloc(sizeof(*async_extent), GFP_NOFS);
-	BUG_ON(!async_extent); /* -ENOMEM */
+	async_extent = kmalloc(sizeof(*async_extent), GFP_NOFS|__GFP_NOFAIL);
 	async_extent->start = start;
 	async_extent->ram_size = ram_size;
 	async_extent->compressed_size = compressed_size;

@@ -1143,8 +1142,7 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
 	clear_extent_bit(&BTRFS_I(inode)->io_tree, start, end, EXTENT_LOCKED,
 			 1, 0, NULL, GFP_NOFS);
 	while (start < end) {
-		async_cow = kmalloc(sizeof(*async_cow), GFP_NOFS);
-		BUG_ON(!async_cow); /* -ENOMEM */
+		async_cow = kmalloc(sizeof(*async_cow), GFP_NOFS|__GFP_NOFAIL);
 		async_cow->inode = igrab(inode);
 		async_cow->root = root;
 		async_cow->locked_page = locked_page;

fs/btrfs/relocation.c +1 -2

index ef6d8fc..6b9f718 100644
@@ -1373,8 +1373,7 @@ static struct btrfs_root *create_reloc_root(struct btrfs_trans_handle *trans,
 	u64 last_snap = 0;
 	int ret;
 
-	root_item = kmalloc(sizeof(*root_item), GFP_NOFS);
-	BUG_ON(!root_item);
+	root_item = kmalloc(sizeof(*root_item), GFP_NOFS|__GFP_NOFAIL);
 
 	root_key.objectid = BTRFS_TREE_RELOC_OBJECTID;
 	root_key.type = BTRFS_ROOT_ITEM_KEY;

Signature · 1 line

2.5.0

David Sterba

2016-02-15 17:53 UTC #1 lore raw

Quoted 7 lines · Satoru Takeuchi

There are some BUG_ON()'s after kmalloc() as follows.
=====
foo = kmalloc();
BUG_ON(!foo); /* -ENOMEM case */
=====

Quoted 8 lines

A Docker + memory cgroup user hit these BUG_ON()s.
https://bugzilla.kernel.org/show_bug.cgi?id=112101
Since it's very hard to handle these ENOMEMs properly, preventing these kmalloc() failures to avoid these BUG_ON()s for now, are a bit better than the current implementation anyway.

Beware that the NOFAIL semantics is can cause deadlocks if it's on the critical writeback path or and can be reentered from itself through the reclaim. Unless you're sure that this is not the case, please do not add them just because it would seemingly fix the allocation failures.

In the docker example, the memory is limited by cgroups so the NOFAIL mode can exhaust all reserves and just loop endlessly waiting for the OOM killer to get some memory or just waiting without any chance to progress.

Satoru Takeuchi

author

2016-02-17 06:06 UTC #2 lore raw

Quoted 17 lines · David Sterba

Quoted 7 lines · Satoru Takeuchi
There are some BUG_ON()'s after kmalloc() as follows.
=====
foo = kmalloc();
BUG_ON(!foo); /* -ENOMEM case */
=====
Quoted 8 lines
A Docker + memory cgroup user hit these BUG_ON()s.
https://bugzilla.kernel.org/show_bug.cgi?id=112101
Since it's very hard to handle these ENOMEMs properly, preventing these kmalloc() failures to avoid these BUG_ON()s for now, are a bit better than the current implementation anyway.

Quoted 4 lines

Beware that the NOFAIL semantics is can cause deadlocks if it's on the critical writeback path or and can be reentered from itself through the reclaim. Unless you're sure that this is not the case, please do not add them just because it would seemingly fix the allocation failures.

About the all cases I changed, kmalloc()s can block since gfp_flags_allow_blocking() are true. Then no locks are acquired here and deadlocks don't happen.

Am I missing something?

Quoted 5 lines

In the docker example, the memory is limited by cgroups so the NOFAIL mode can exhaust all reserves and just loop endlessly waiting for the OOM killer to get some memory or just waiting without any chance to progress.

I consider triggering OOM killer and killing processes in a cgroup are better than killing whole system.

About the possibility of endless loop, there are many such problems in the whole kernel. Of course it can be said to Btrfs.

==========================================

$ grep -rnH __GFP_NOFAIL fs/btrfs/
fs/btrfs/extent-tree.c:5970: GFP_NOFS | __GFP_NOFAIL);
fs/btrfs/extent-tree.c:6043: bytenr + num_bytes - 1, GFP_NOFS | __GFP_NOFAIL);
fs/btrfs/extent_io.c:4643: eb = kmem_cache_zalloc(extent_buffer_cache, GFP_NOFS|__GFP_NOFAIL);
fs/btrfs/extent_io.c:4909: p = find_or_create_page(mapping, index, GFP_NOFS|__GFP_NOFAIL);
==========================================

I understand fixing these problems cooperate with memory cgroup guys is the best in the long run. However, I consider bypassing this problem for now is better than the current implementation.

Thanks,
Satoru

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Sterba

2016-02-17 15:11 UTC #3 lore raw

Quoted 23 lines · Satoru Takeuchi

Quoted 17 lines · David Sterba
Quoted 7 lines · Satoru Takeuchi
There are some BUG_ON()'s after kmalloc() as follows.
=====
foo = kmalloc();
BUG_ON(!foo); /* -ENOMEM case */
=====
Quoted 8 lines
A Docker + memory cgroup user hit these BUG_ON()s.
https://bugzilla.kernel.org/show_bug.cgi?id=112101
Since it's very hard to handle these ENOMEMs properly, preventing these kmalloc() failures to avoid these BUG_ON()s for now, are a bit better than the current implementation anyway.
Quoted 4 lines
Beware that the NOFAIL semantics is can cause deadlocks if it's on the critical writeback path or and can be reentered from itself through the reclaim. Unless you're sure that this is not the case, please do not add them just because it would seemingly fix the allocation failures.

Quoted 5 lines

About the all cases I changed, kmalloc()s can block since gfp_flags_allow_blocking() are true. Then no locks are acquired here and deadlocks don't happen.
Am I missing something?

Waiting as in GFP_WAIT is not the same as GFP_NOFAIL that can wait indefinetelly. The locking of NOFAIL is implied. The kmalloc callsite will block until there's memory available. If another thread of btrfs waits for this code to progress, it will block as well.

Quoted 7 lines

Quoted 4 lines
In the docker example, the memory is limited by cgroups so the NOFAIL mode can exhaust all reserves and just loop endlessly waiting for the OOM killer to get some memory or just waiting without any chance to progress.
I consider triggering OOM killer and killing processes in a cgroup are better than killing whole system.

The action of OOM killer is not a problem. The cgroup memory limit can be low or all the memory is unreclaimable. At this point btrfs code will block.

Quoted 3 lines

About the possibility of endless loop, there are many such problems in the whole kernel. Of course it can be said to Btrfs.

Many? Examples? In this context we're talking about endless loops caused by non-failing allocations.

Quoted 7 lines

==========================================

$ grep -rnH __GFP_NOFAIL fs/btrfs/
fs/btrfs/extent-tree.c:5970: GFP_NOFS | __GFP_NOFAIL);
fs/btrfs/extent-tree.c:6043: bytenr + num_bytes - 1, GFP_NOFS | __GFP_NOFAIL);
fs/btrfs/extent_io.c:4643: eb = kmem_cache_zalloc(extent_buffer_cache, GFP_NOFS|__GFP_NOFAIL);
fs/btrfs/extent_io.c:4909: p = find_or_create_page(mapping, index, GFP_NOFS|__GFP_NOFAIL);
==========================================

I'm aware of the existing NOFAIL allocations. There are two at extent_buffer allocation time, added recently and provoked by the intended changes to memory allocator that would fail the GFP_NOFS allocations.

The other two are from year 2010, set_extent_bit, IMHO added so the error handling does not get complicated and expressing "we don't want to fail here". But there are many other calls to set_extent_bit that could fail. This is inconsistent and should be unified. In a way that we're sure that we're not introducing potential hangs.

Quoted 2 lines

I understand fixing these problems cooperate with
memory cgroup guys is the best in the long run.

It's not about cgroups, btrfs needs to ideally handle all memory allocation failures in a way that uses some sort of fallbacks but still can lead to a transaction abort if there's simply no memory left.

Quoted 2 lines

However, I consider bypassing this problem for now is better than the current implementation.

It's more like replacing one problem with another. With every new NOFAIL, one has to think about the runtime interactions with the others. I'd rather see this fixed with a particular call path in mind or class, eg. the extent_map bit settings, than throwing NOFAIL at places that somebody accidentally hit.

As a temporary fix we can add __GFP_HIGH to the interesting sites so we can get access to the emergency reserves, and this is on my list of things to do after the NOFS -> KERNEL updates are done.

Satoru Takeuchi

author

2016-02-18 01:59 UTC #4 lore raw

Quoted 90 lines · David Sterba

Quoted 23 lines · Satoru Takeuchi
Quoted 17 lines · David Sterba
Quoted 7 lines · Satoru Takeuchi
There are some BUG_ON()'s after kmalloc() as follows.
=====
foo = kmalloc();
BUG_ON(!foo); /* -ENOMEM case */
=====
Quoted 8 lines
A Docker + memory cgroup user hit these BUG_ON()s.
https://bugzilla.kernel.org/show_bug.cgi?id=112101
Since it's very hard to handle these ENOMEMs properly, preventing these kmalloc() failures to avoid these BUG_ON()s for now, are a bit better than the current implementation anyway.
Quoted 4 lines
Beware that the NOFAIL semantics is can cause deadlocks if it's on the critical writeback path or and can be reentered from itself through the reclaim. Unless you're sure that this is not the case, please do not add them just because it would seemingly fix the allocation failures.
Quoted 5 lines
About the all cases I changed, kmalloc()s can block since gfp_flags_allow_blocking() are true. Then no locks are acquired here and deadlocks don't happen.
Am I missing something?
Waiting as in GFP_WAIT is not the same as GFP_NOFAIL that can wait indefinetelly. The locking of NOFAIL is implied. The kmalloc callsite will block until there's memory available. If another thread of btrfs waits for this code to progress, it will block as well.
Quoted 7 lines
Quoted 4 lines
In the docker example, the memory is limited by cgroups so the NOFAIL mode can exhaust all reserves and just loop endlessly waiting for the OOM killer to get some memory or just waiting without any chance to progress.
I consider triggering OOM killer and killing processes in a cgroup are better than killing whole system.
The action of OOM killer is not a problem. The cgroup memory limit can be low or all the memory is unreclaimable. At this point btrfs code will block.
Quoted 3 lines
About the possibility of endless loop, there are many such problems in the whole kernel. Of course it can be said to Btrfs.
Many? Examples? In this context we're talking about endless loops caused by non-failing allocations.
Quoted 7 lines
==========================================
$ grep -rnH __GFP_NOFAIL fs/btrfs/
fs/btrfs/extent-tree.c:5970: GFP_NOFS | __GFP_NOFAIL);
fs/btrfs/extent-tree.c:6043: bytenr + num_bytes - 1, GFP_NOFS | __GFP_NOFAIL);
fs/btrfs/extent_io.c:4643: eb = kmem_cache_zalloc(extent_buffer_cache, GFP_NOFS|__GFP_NOFAIL);
fs/btrfs/extent_io.c:4909: p = find_or_create_page(mapping, index, GFP_NOFS|__GFP_NOFAIL);
==========================================
I'm aware of the existing NOFAIL allocations. There are two at extent_buffer allocation time, added recently and provoked by the intended changes to memory allocator that would fail the GFP_NOFS allocations.
The other two are from year 2010, set_extent_bit, IMHO added so the error handling does not get complicated and expressing "we don't want to fail here". But there are many other calls to set_extent_bit that could fail. This is inconsistent and should be unified. In a way that we're sure that we're not introducing potential hangs.
Quoted 2 lines
I understand fixing these problems cooperate with
memory cgroup guys is the best in the long run.
It's not about cgroups, btrfs needs to ideally handle all memory allocation failures in a way that uses some sort of fallbacks but still can lead to a transaction abort if there's simply no memory left.
Quoted 2 lines
However, I consider bypassing this problem for now is better than the current implementation.
It's more like replacing one problem with another. With every new NOFAIL, one has to think about the runtime interactions with the others. I'd rather see this fixed with a particular call path in mind or class, eg. the extent_map bit settings, than throwing NOFAIL at places that somebody accidentally hit.

Quoted 3 lines

As a temporary fix we can add __GFP_HIGH to the interesting sites so we can get access to the emergency reserves, and this is on my list of things to do after the NOFS -> KERNEL updates are done.

OK, got it. I'll reconsider how to fix there problem.

Thanks,
Satoru

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html