Skip to contentSkip to conversation

btrfs: ENOMEM bugfixes

PATCH In review Omar Sandoval 3 participants 12 messages Feb 24, 2015 – Apr 13, 2015 Btrfs file system

lore
Share & tools
lore

Omar Sandoval

author
#0 lore raw

v2 cover btrfs: ENOMEM bugfixes

Hi, everyone,

This patch series fixes a few bugs that occur under low memory conditions. These were exposed by a change in behavior of GFP_NOFS allocations in 3.19-rc7, by commit 9879de7373fc ("mm: page_alloc: embed OOM killing naturally into allocation slowpath"). While the mm people sort that out, we can fix these issues, which are bugs no matter what the outcome there is.

Here's a quick script which reproduces these bugs. With the patches applied, the filesystem will drop into read-only mode instead of blowing up.

----
#!/bin/sh

cgcreate -g memory:enomem
MEM=$((64 * 1024 * 1024))
echo $MEM > /sys/fs/cgroup/memory/enomem/memory.limit_in_bytes

cgexec -g memory:enomem ~/xfstests/ltp/fsstress -p128 -n999999999 -d /mnt/test & trap "killall fsstress; exit 0" SIGINT SIGTERM

while true; do
cgexec -g memory:enomem python -c '
l = []
while True:

    l.append(0)'

done
----

Version 2 rebases on top of 4.0-rc1, has a simpler fix for the alloc_extent_buffer race, expands the commit messages to mention changed comments, and adds Liu Bo's Reviewed-by.

Thanks!

Omar Sandoval (3):
btrfs: handle ENOMEM in btrfs_alloc_tree_block
btrfs: fix race on ENOMEM in alloc_extent_buffer
btrfs: check io_ctl_prepare_pages return in __btrfs_write_out_cache

 fs/btrfs/extent-tree.c      | 41 ++++++++++++++++++++++++++++-------------
 fs/btrfs/extent_io.c        |  3 ++-
 fs/btrfs/free-space-cache.c | 10 ++++++----
 3 files changed, 36 insertions(+), 18 deletions(-)
Signature · 1 line
2.3.0
author
#1 lore raw

PATCH v2 1/3 btrfs: handle ENOMEM in btrfs_alloc_tree_block

This is one of the first places to give out when memory is tight. Handle it properly rather than with a BUG_ON.

Also fix the comment about the return value, which is an ERR_PTR, not NULL, on error.

Signed-off-by Omar Sandoval <[email protected]>
---
 fs/btrfs/extent-tree.c | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)
fs/btrfs/extent-tree.c +28 -13
index 571f402..7443d67 100644
@@ -7262,7 +7262,7 @@ static void unuse_block_rsv(struct btrfs_fs_info *fs_info,
  * returns the key for the extent through ins, and a tree buffer for
  * the first block of the extent through buf.
  *
- * returns the tree buffer or NULL.
+ * returns the tree buffer or an ERR_PTR on error.
  */
 struct extent_buffer *btrfs_alloc_tree_block(struct btrfs_trans_handle *trans,
 					struct btrfs_root *root,

@@ -7273,6 +7273,7 @@ struct extent_buffer *btrfs_alloc_tree_block(struct btrfs_trans_handle *trans,
 	struct btrfs_key ins;
 	struct btrfs_block_rsv *block_rsv;
 	struct extent_buffer *buf;
+	struct btrfs_delayed_extent_op *extent_op;
 	u64 flags = 0;
 	int ret;
 	u32 blocksize = root->nodesize;

@@ -7293,13 +7294,14 @@ struct extent_buffer *btrfs_alloc_tree_block(struct btrfs_trans_handle *trans,
 
 	ret = btrfs_reserve_extent(root, blocksize, blocksize,
 				   empty_size, hint, &ins, 0, 0);
-	if (ret) {
-		unuse_block_rsv(root->fs_info, block_rsv, blocksize);
-		return ERR_PTR(ret);
-	}
+	if (ret)
+		goto out_unuse;
 
 	buf = btrfs_init_new_buffer(trans, root, ins.objectid, level);
-	BUG_ON(IS_ERR(buf)); /* -ENOMEM */
+	if (IS_ERR(buf)) {
+		ret = PTR_ERR(buf);
+		goto out_free_reserved;
+	}
 
 	if (root_objectid == BTRFS_TREE_RELOC_OBJECTID) {
 		if (parent == 0)

@@ -7309,9 +7311,11 @@ struct extent_buffer *btrfs_alloc_tree_block(struct btrfs_trans_handle *trans,
 		BUG_ON(parent > 0);
 
 	if (root_objectid != BTRFS_TREE_LOG_OBJECTID) {
-		struct btrfs_delayed_extent_op *extent_op;
 		extent_op = btrfs_alloc_delayed_extent_op();
-		BUG_ON(!extent_op); /* -ENOMEM */
+		if (!extent_op) {
+			ret = -ENOMEM;
+			goto out_free_buf;
+		}
 		if (key)
 			memcpy(&extent_op->key, key, sizeof(extent_op->key));
 		else

@@ -7326,13 +7330,24 @@ struct extent_buffer *btrfs_alloc_tree_block(struct btrfs_trans_handle *trans,
 		extent_op->level = level;
 
 		ret = btrfs_add_delayed_tree_ref(root->fs_info, trans,
-					ins.objectid,
-					ins.offset, parent, root_objectid,
-					level, BTRFS_ADD_DELAYED_EXTENT,
-					extent_op, 0);
-		BUG_ON(ret); /* -ENOMEM */
+						 ins.objectid, ins.offset,
+						 parent, root_objectid, level,
+						 BTRFS_ADD_DELAYED_EXTENT,
+						 extent_op, 0);
+		if (ret)
+			goto out_free_delayed;
 	}
 	return buf;
+
+out_free_delayed:
+	btrfs_free_delayed_extent_op(extent_op);
+out_free_buf:
+	free_extent_buffer(buf);
+out_free_reserved:
+	btrfs_free_reserved_extent(root, ins.objectid, ins.offset, 0);
+out_unuse:
+	unuse_block_rsv(root->fs_info, block_rsv, blocksize);
+	return ERR_PTR(ret);
 }
 
 struct walk_control {
Signature · 1 line
2.3.0
Reviewed-by on v2 1/3
#2 lore raw

Re: [PATCH v2 1/3] btrfs: handle ENOMEM in btrfs_alloc_tree_block

Quoted 7 lines · Omar Sandoval

This is one of the first places to give out when memory is tight. Handle it properly rather than with a BUG_ON.

Also fix the comment about the return value, which is an ERR_PTR, not NULL, on error.

Signed-off-by Omar Sandoval <[email protected]>
Reviewed-by David Sterba <[email protected]>
author
#3 lore raw

PATCH v2 2/3 btrfs: fix race on ENOMEM in alloc_extent_buffer

In reply to Omar Sandoval

Consider the following interleaving of overlapping calls to alloc_extent_buffer:

Call 1:

- Successfully allocates a few pages with find_or_create_page
- find_or_create_page fails, goto free_eb
- Unlocks the allocated pages

Call 2:
- Calls find_or_create_page and gets a page in call 1's extent_buffer
- Finds that the page is already associated with an extent_buffer
- Grabs a reference to the half-written extent_buffer and calls mark_extent_buffer_accessed on it

mark_extent_buffer_accessed will then try to call mark_page_accessed on a null page and panic.

The fix is to decrement the reference count on the half-written extent_buffer before unlocking the pages so call 2 won't use it. We should also set exists = NULL in the case that we don't use exists to avoid accidentally returning a freed extent_buffer in an error case.

Signed-off-by Omar Sandoval <[email protected]>
---
 fs/btrfs/extent_io.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
fs/btrfs/extent_io.c +2 -1
index c7233ff..7ecfae0 100644
@@ -4867,6 +4867,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 				mark_extent_buffer_accessed(exists, p);
 				goto free_eb;
 			}
+			exists = NULL;
 
 			/*
 			 * Do this so attach doesn't complain and we need to

@@ -4930,12 +4931,12 @@ again:
 	return eb;
 
 free_eb:
+	WARN_ON(!atomic_dec_and_test(&eb->refs));
 	for (i = 0; i < num_pages; i++) {
 		if (eb->pages[i])
 			unlock_page(eb->pages[i]);
 	}
 
-	WARN_ON(!atomic_dec_and_test(&eb->refs));
 	btrfs_release_extent_buffer(eb);
 	return exists;
 }
Signature · 1 line
2.3.0
Reviewed-by on v2 2/3
#4 lore raw

Re: [PATCH v2 2/3] btrfs: fix race on ENOMEM in alloc_extent_buffer

Quoted 18 lines · Omar Sandoval

Consider the following interleaving of overlapping calls to alloc_extent_buffer:

Call 1:

- Successfully allocates a few pages with find_or_create_page
- find_or_create_page fails, goto free_eb
- Unlocks the allocated pages

Call 2:
- Calls find_or_create_page and gets a page in call 1's extent_buffer
- Finds that the page is already associated with an extent_buffer
- Grabs a reference to the half-written extent_buffer and calls mark_extent_buffer_accessed on it

mark_extent_buffer_accessed will then try to call mark_page_accessed on a null page and panic.

Quoted 6 lines

The fix is to decrement the reference count on the half-written extent_buffer before unlocking the pages so call 2 won't use it. We should also set exists = NULL in the case that we don't use exists to avoid accidentally returning a freed extent_buffer in an error case.

Signed-off-by Omar Sandoval <[email protected]>
Reviewed-by David Sterba <[email protected]>
Reviewed-by on v2 2/3
#5 lore raw

Re: [PATCH v2 2/3] btrfs: fix race on ENOMEM in alloc_extent_buffer

In reply to Omar Sandoval
Quoted 15 lines · Omar Sandoval

Consider the following interleaving of overlapping calls to alloc_extent_buffer:

Call 1:

- Successfully allocates a few pages with find_or_create_page
- find_or_create_page fails, goto free_eb
- Unlocks the allocated pages

Call 2:
- Calls find_or_create_page and gets a page in call 1's extent_buffer
- Finds that the page is already associated with an extent_buffer
- Grabs a reference to the half-written extent_buffer and calls mark_extent_buffer_accessed on it

Quoted 7 lines

mark_extent_buffer_accessed will then try to call mark_page_accessed on a null page and panic.

The fix is to decrement the reference count on the half-written extent_buffer before unlocking the pages so call 2 won't use it. We should also set exists = NULL in the case that we don't use exists to avoid accidentally returning a freed extent_buffer in an error case.

Reviewed-by Liu Bo <[email protected]>
Quoted 39 lines
Signed-off-by Omar Sandoval <[email protected]>
---
 fs/btrfs/extent_io.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
fs/btrfs/extent_io.c +2 -1
index c7233ff..7ecfae0 100644
@@ -4867,6 +4867,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 				mark_extent_buffer_accessed(exists, p);
 				goto free_eb;
 			}
+			exists = NULL;
 
 			/*
 			 * Do this so attach doesn't complain and we need to

@@ -4930,12 +4931,12 @@ again:
 	return eb;
 
 free_eb:
+	WARN_ON(!atomic_dec_and_test(&eb->refs));
 	for (i = 0; i < num_pages; i++) {
 		if (eb->pages[i])
 			unlock_page(eb->pages[i]);
 	}
 
-	WARN_ON(!atomic_dec_and_test(&eb->refs));
 	btrfs_release_extent_buffer(eb);
 	return exists;
 }
Signature · 6 lines
2.3.0

--
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
author
#6 lore raw

PATCH v2 3/3 btrfs: check io_ctl_prepare_pages return in __btrfs_write_out_cache

In reply to Omar Sandoval

If io_ctl_prepare_pages fails, the pages in io_ctl.pages are not valid. When we try to access them later, things will blow up in various ways.

Also fix the comment about the return value, which is an errno on error, not -1, and update the cases where it was not.

Reviewed-by Liu Bo <[email protected]>
Signed-off-by Omar Sandoval <[email protected]>
---
 fs/btrfs/free-space-cache.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
fs/btrfs/free-space-cache.c +6 -4
index a719785..03dcda2 100644
@@ -1112,7 +1112,7 @@ cleanup_write_cache_enospc(struct inode *inode,
  *
  * This function writes out a free space cache struct to disk for quick recovery
  * on mount.  This will return 0 if it was successfull in writing the cache out,
- * and -1 if it was not.
+ * or an errno if it was not.
  */
 static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode,
 				   struct btrfs_free_space_ctl *ctl,

@@ -1128,11 +1128,11 @@ static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode,
 	int ret;
 
 	if (!i_size_read(inode))
-		return -1;
+		return -EIO;
 
 	ret = io_ctl_init(&io_ctl, inode, root, 1);
 	if (ret)
-		return -1;
+		return ret;
 
 	if (block_group && (block_group->flags & BTRFS_BLOCK_GROUP_DATA)) {
 		down_write(&block_group->data_rwsem);

@@ -1149,7 +1149,9 @@ static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode,
 	}
 
 	/* Lock all pages first so we can lock the extent safely. */
-	io_ctl_prepare_pages(&io_ctl, inode, 0);
+	ret = io_ctl_prepare_pages(&io_ctl, inode, 0);
+	if (ret)
+		goto out;
 
 	lock_extent_bits(&BTRFS_I(inode)->io_tree, 0, i_size_read(inode) - 1,
 			 0, &cached_state);
Signature · 1 line
2.3.0
author
#7 lore raw
In reply to Omar Sandoval
Quoted 40 lines · Omar Sandoval

Hi, everyone,

This patch series fixes a few bugs that occur under low memory conditions. These were exposed by a change in behavior of GFP_NOFS allocations in 3.19-rc7, by commit 9879de7373fc ("mm: page_alloc: embed OOM killing naturally into allocation slowpath"). While the mm people sort that out, we can fix these issues, which are bugs no matter what the outcome there is.

Here's a quick script which reproduces these bugs. With the patches applied, the filesystem will drop into read-only mode instead of blowing up.

----
#!/bin/sh

cgcreate -g memory:enomem
MEM=$((64 * 1024 * 1024))
echo $MEM > /sys/fs/cgroup/memory/enomem/memory.limit_in_bytes

cgexec -g memory:enomem ~/xfstests/ltp/fsstress -p128 -n999999999 -d /mnt/test & trap "killall fsstress; exit 0" SIGINT SIGTERM

while true; do
cgexec -g memory:enomem python -c '
l = []
while True:

    l.append(0)'

done
----

Version 2 rebases on top of 4.0-rc1, has a simpler fix for the alloc_extent_buffer race, expands the commit messages to mention changed comments, and adds Liu Bo's Reviewed-by.

Thanks!

Omar Sandoval (3):
btrfs: handle ENOMEM in btrfs_alloc_tree_block
btrfs: fix race on ENOMEM in alloc_extent_buffer
btrfs: check io_ctl_prepare_pages return in __btrfs_write_out_cache

Quoted 7 lines
 fs/btrfs/extent-tree.c      | 41 ++++++++++++++++++++++++++++-------------
 fs/btrfs/extent_io.c        |  3 ++-
 fs/btrfs/free-space-cache.c | 10 ++++++----
 3 files changed, 36 insertions(+), 18 deletions(-)
Signature · 1 line
2.3.0

Ping. For anyone following along, it looks like commit cc87317726f8 ("mm: page_alloc: revert inadvertent !__GFP_FS retry behavior change") reverted the commit that exposed these bugs. Josef said he was okay with taking these, will they make it to an upcoming -rc soon?

Thanks!

Signature · 1 line
Omar
#8 lore raw
Quoted 4 lines · Omar Sandoval

Ping. For anyone following along, it looks like commit cc87317726f8 ("mm: page_alloc: revert inadvertent !__GFP_FS retry behavior change") reverted the commit that exposed these bugs. Josef said he was okay with taking these, will they make it to an upcoming -rc soon?

Upcoming yes, but based on my experience with pushing patches that are not really regressions in late rc's it's unlikely for 4.1.

author
#9 lore raw
Quoted 8 lines · David Sterba
Quoted 4 lines · Omar Sandoval

Ping. For anyone following along, it looks like commit cc87317726f8 ("mm: page_alloc: revert inadvertent !__GFP_FS retry behavior change") reverted the commit that exposed these bugs. Josef said he was okay with taking these, will they make it to an upcoming -rc soon?

Upcoming yes, but based on my experience with pushing patches that are not really regressions in late rc's it's unlikely for 4.1.

Ok, seeing as these bugs are going to be really hard to trigger now that the old GFP_FS behavior has been restored, I'm fine with waiting for the next merge window.

Thank you!

Signature · 1 line
Omar
author
#10 lore raw
Quoted 7 lines · Omar Sandoval
Quoted 6 lines · David Sterba
Quoted 4 lines · Omar Sandoval

Ping. For anyone following along, it looks like commit cc87317726f8 ("mm: page_alloc: revert inadvertent !__GFP_FS retry behavior change") reverted the commit that exposed these bugs. Josef said he was okay with taking these, will they make it to an upcoming -rc soon?

Quoted 8 lines
Quoted 2 lines

Upcoming yes, but based on my experience with pushing patches that are not really regressions in late rc's it's unlikely for 4.1.

Ok, seeing as these bugs are going to be really hard to trigger now that the old GFP_FS behavior has been restored, I'm fine with waiting for the next merge window.

Thank you!

Chris, would you mind taking these for a spin in your integration branch for the next merge window?

Thanks,

Signature · 1 line
Omar
author
#11 lore raw
Quoted 15 lines · Omar Sandoval
Quoted 14 lines · Omar Sandoval
Quoted 8 lines · David Sterba
Quoted 4 lines · Omar Sandoval

Ping. For anyone following along, it looks like commit cc87317726f8 ("mm: page_alloc: revert inadvertent !__GFP_FS retry behavior change") reverted the commit that exposed these bugs. Josef said he was okay with taking these, will they make it to an upcoming -rc soon?

Upcoming yes, but based on my experience with pushing patches that are not really regressions in late rc's it's unlikely for 4.1.

Ok, seeing as these bugs are going to be really hard to trigger now that the old GFP_FS behavior has been restored, I'm fine with waiting for the next merge window.

Quoted 8 lines
Quoted 1 line

Thank you!

Chris, would you mind taking these for a spin in your integration branch for the next merge window?

Thanks,

Signature · 1 line
Omar

Ping.

Signature · 1 line
Omar