[PATCH v2 0/3] btrfs: ENOMEM bugfixes

[PATCH v2 0/3] btrfs: ENOMEM bugfixes

[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

 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(-)
-- 
2.3.0

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

[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 <osandov@osandov.com>
---
 fs/btrfs/extent-tree.c | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 571f402..7443d67 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -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 {
-- 
2.3.0

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

[David Sterba]

On Tue, Feb 24, 2015 at 02:47:04AM -0800, Omar Sandoval wrote:
> 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 <osandov@osandov.com>

Reviewed-by: David Sterba <dsterba@suse.cz>

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

[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 <osandov@osandov.com>
---
 fs/btrfs/extent_io.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index c7233ff..7ecfae0 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -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;
 }
-- 
2.3.0

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

[David Sterba]

On Tue, Feb 24, 2015 at 02:47:05AM -0800, Omar Sandoval wrote:
> 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 <osandov@osandov.com>

Reviewed-by: David Sterba <dsterba@suse.cz>

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

[Liu Bo]

On Tue, Feb 24, 2015 at 02:47:05AM -0800, Omar Sandoval wrote:
> 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.

Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

> 
> Signed-off-by: Omar Sandoval <osandov@osandov.com>
> ---
>  fs/btrfs/extent_io.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index c7233ff..7ecfae0 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -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;
>  }
> -- 
> 2.3.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

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

[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 <bo.li.liu@oracle.com>
Signed-off-by: Omar Sandoval <osandov@osandov.com>
---
 fs/btrfs/free-space-cache.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index a719785..03dcda2 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -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);
-- 
2.3.0

Re: [PATCH v2 0/3] btrfs: ENOMEM bugfixes

[Omar Sandoval]

On Tue, Feb 24, 2015 at 02:47:03AM -0800, Omar Sandoval wrote:
> 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(-)
> -- 
> 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!
-- 
Omar

Re: [PATCH v2 0/3] btrfs: ENOMEM bugfixes

[David Sterba]

On Wed, Mar 11, 2015 at 09:40:17PM -0700, Omar Sandoval wrote:
> 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.

Re: [PATCH v2 0/3] btrfs: ENOMEM bugfixes

[Omar Sandoval]

On Fri, Mar 13, 2015 at 12:04:30PM +0100, David Sterba wrote:
> On Wed, Mar 11, 2015 at 09:40:17PM -0700, Omar Sandoval wrote:
> > 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!
-- 
Omar

Re: [PATCH v2 0/3] btrfs: ENOMEM bugfixes

[Omar Sandoval]

On Fri, Mar 13, 2015 at 12:43:42PM -0700, Omar Sandoval wrote:
> On Fri, Mar 13, 2015 at 12:04:30PM +0100, David Sterba wrote:
> > On Wed, Mar 11, 2015 at 09:40:17PM -0700, Omar Sandoval wrote:
> > > 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!

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

Thanks,
-- 
Omar

Re: [PATCH v2 0/3] btrfs: ENOMEM bugfixes

[Omar Sandoval]

On Fri, Mar 27, 2015 at 02:06:49PM -0700, Omar Sandoval wrote:
> On Fri, Mar 13, 2015 at 12:43:42PM -0700, Omar Sandoval wrote:
> > On Fri, Mar 13, 2015 at 12:04:30PM +0100, David Sterba wrote:
> > > On Wed, Mar 11, 2015 at 09:40:17PM -0700, Omar Sandoval wrote:
> > > > 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!
> 
> Chris, would you mind taking these for a spin in your integration branch
> for the next merge window?
> 
> Thanks,
> -- 
> Omar

Ping.

-- 
Omar