[PATCH 0/5] Fix memory leak on failed cache-writes

[PATCH 0/5] Fix memory leak on failed cache-writes

[Johannes Thumshirn]

Fstests' test case generic/475 reliably leaks the btrfs_io_ctl::pages
allocated in __btrfs_write_out_cache().

The first four patches are small things I noticed while hunting down the
problem and are independant of the last patch in this series freeing the pages
when we throw away a dirty block group.

Johannes Thumshirn (5):
  btrfs: use inode from io_ctl in io_ctl_prepare_pages
  btrfs: make the uptodate argument of io_ctl_add_pages() boolean.
  btrfs: use standard debug config option to enable free-space-cache
    debug prints
  btrfs: simplify error handling in __btrfs_write_out_cache()
  btrfs: free allocated pages jon failed cache write-out

 fs/btrfs/disk-io.c          |  6 ++++++
 fs/btrfs/free-space-cache.c | 44 ++++++++++++++++++++++++--------------------
 fs/btrfs/free-space-cache.h |  1 +
 3 files changed, 31 insertions(+), 20 deletions(-)

-- 
2.16.4

[PATCH 1/5] btrfs: use inode from io_ctl in io_ctl_prepare_pages

[Johannes Thumshirn]

io_ctl_prepare_pages() get's a 'struct btrfs_io_ctl' as well as a 'struct
inode', but btrfs_io_ctl::inode points to the same struct inode as this is
assgined in io_ctl_init().

Use the inode form io_ctl to reduce the arguments of
io_ctl_prepare_pages().

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/free-space-cache.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 0598fd3c6e3f..f4ae7629a0e7 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -371,10 +371,10 @@ static void io_ctl_drop_pages(struct btrfs_io_ctl *io_ctl)
 	}
 }
 
-static int io_ctl_prepare_pages(struct btrfs_io_ctl *io_ctl, struct inode *inode,
-				int uptodate)
+static int io_ctl_prepare_pages(struct btrfs_io_ctl *io_ctl, int uptodate)
 {
 	struct page *page;
+	struct inode *inode = io_ctl->inode;
 	gfp_t mask = btrfs_alloc_write_mask(inode->i_mapping);
 	int i;
 
@@ -732,7 +732,7 @@ static int __load_free_space_cache(struct btrfs_root *root, struct inode *inode,
 
 	readahead_cache(inode);
 
-	ret = io_ctl_prepare_pages(&io_ctl, inode, 1);
+	ret = io_ctl_prepare_pages(&io_ctl, 1);
 	if (ret)
 		goto out;
 
@@ -1291,7 +1291,7 @@ static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode,
 	}
 
 	/* Lock all pages first so we can lock the extent safely. */
-	ret = io_ctl_prepare_pages(io_ctl, inode, 0);
+	ret = io_ctl_prepare_pages(io_ctl, 0);
 	if (ret)
 		goto out_unlock;
 
-- 
2.16.4

[PATCH 2/5] btrfs: make the uptodate argument of io_ctl_add_pages() boolean.

[Johannes Thumshirn]

Make the uptodate argument of io_ctl_add_pages() boolean.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/free-space-cache.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index f4ae7629a0e7..98f547e87bb4 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -371,7 +371,7 @@ static void io_ctl_drop_pages(struct btrfs_io_ctl *io_ctl)
 	}
 }
 
-static int io_ctl_prepare_pages(struct btrfs_io_ctl *io_ctl, int uptodate)
+static int io_ctl_prepare_pages(struct btrfs_io_ctl *io_ctl, bool uptodate)
 {
 	struct page *page;
 	struct inode *inode = io_ctl->inode;
@@ -732,7 +732,7 @@ static int __load_free_space_cache(struct btrfs_root *root, struct inode *inode,
 
 	readahead_cache(inode);
 
-	ret = io_ctl_prepare_pages(&io_ctl, 1);
+	ret = io_ctl_prepare_pages(&io_ctl, true);
 	if (ret)
 		goto out;
 
@@ -1291,7 +1291,7 @@ static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode,
 	}
 
 	/* Lock all pages first so we can lock the extent safely. */
-	ret = io_ctl_prepare_pages(io_ctl, 0);
+	ret = io_ctl_prepare_pages(io_ctl, false);
 	if (ret)
 		goto out_unlock;
 
-- 
2.16.4

[PATCH 3/5] btrfs: use standard debug config option to enable free-space-cache debug prints

[Johannes Thumshirn]

free-space-cache.c has it's own set of DEBUG ifdefs which need to be
turned on instead of the global CONFIG_BTRFS_DEBUG to print debug messages
about failed block-group writes.

Switch this over to CONFIG_BTRFS_DEBUG so we always see these messages
when running a debug kernel.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/free-space-cache.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 98f547e87bb4..7d6d6aa7b7d6 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -1190,7 +1190,7 @@ static int __btrfs_wait_cache_io(struct btrfs_root *root,
 		invalidate_inode_pages2(inode->i_mapping);
 		BTRFS_I(inode)->generation = 0;
 		if (block_group) {
-#ifdef DEBUG
+#ifdef CONFIG_BTRFS_DEBUG
 			btrfs_err(root->fs_info,
 				  "failed to write free space cache for block group %llu",
 				  block_group->start);
@@ -1416,7 +1416,7 @@ int btrfs_write_out_cache(struct btrfs_trans_handle *trans,
 	ret = __btrfs_write_out_cache(fs_info->tree_root, inode, ctl,
 				block_group, &block_group->io_ctl, trans);
 	if (ret) {
-#ifdef DEBUG
+#ifdef CONFIG_BTRFS_DEBUG
 		btrfs_err(fs_info,
 			  "failed to write free space cache for block group %llu",
 			  block_group->start);
@@ -4036,7 +4036,7 @@ int btrfs_write_out_ino_cache(struct btrfs_root *root,
 		if (release_metadata)
 			btrfs_delalloc_release_metadata(BTRFS_I(inode),
 					inode->i_size, true);
-#ifdef DEBUG
+#ifdef CONFIG_BTRFS_DEBUG
 		btrfs_err(fs_info,
 			  "failed to write free ino cache for root %llu",
 			  root->root_key.objectid);
-- 
2.16.4

[PATCH 4/5] btrfs: simplify error handling in __btrfs_write_out_cache()

[Johannes Thumshirn]

The error cleanup gotos in __btrfs_write_out_cache() needlessly jump back
making the code less readable then needed.

Flatten out the labels so no back-jump is necessary and the read flow is
uninterrupted.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/free-space-cache.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 7d6d6aa7b7d6..705589daeffa 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -1366,18 +1366,6 @@ static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode,
 
 	return 0;
 
-out:
-	io_ctl->inode = NULL;
-	io_ctl_free(io_ctl);
-	if (ret) {
-		invalidate_inode_pages2(inode->i_mapping);
-		BTRFS_I(inode)->generation = 0;
-	}
-	btrfs_update_inode(trans, root, inode);
-	if (must_iput)
-		iput(inode);
-	return ret;
-
 out_nospc_locked:
 	cleanup_bitmap_list(&bitmap_list);
 	spin_unlock(&ctl->tree_lock);
@@ -1390,7 +1378,17 @@ static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode,
 	if (block_group && (block_group->flags & BTRFS_BLOCK_GROUP_DATA))
 		up_write(&block_group->data_rwsem);
 
-	goto out;
+out:
+	io_ctl->inode = NULL;
+	io_ctl_free(io_ctl);
+	if (ret) {
+		invalidate_inode_pages2(inode->i_mapping);
+		BTRFS_I(inode)->generation = 0;
+	}
+	btrfs_update_inode(trans, root, inode);
+	if (must_iput)
+		iput(inode);
+	return ret;
 }
 
 int btrfs_write_out_cache(struct btrfs_trans_handle *trans,
-- 
2.16.4

[PATCH 5/5] btrfs: free allocated pages jon failed cache write-out

[Johannes Thumshirn]

When we fail to write out a dirty block group, we leak the pages allocated
for a block-group's io_ctl. This can be seen with generic/475 and kmemleak
turned on:

unreferenced object 0xffff8882249c9000 (size 128):
  comm "fsstress", pid 1791, jiffies 4294902054 (age 32.100s)
  hex dump (first 32 bytes):
    80 0e 42 08 00 ea ff ff 00 0d 42 08 00 ea ff ff  ..B.......B.....
    00 eb 0e 08 00 ea ff ff 00 e8 0e 08 00 ea ff ff  ................
  backtrace:
    [<00000000cd20c449>] io_ctl_init+0xa2/0x110 [btrfs]
    [<00000000281944cc>] __btrfs_write_out_cache+0x71/0x410 [btrfs]
    [<000000005d518c07>] btrfs_write_out_cache+0x82/0xd0 [btrfs]
    [<000000002bb2675c>] btrfs_start_dirty_block_groups+0x1f6/0x440 [btrfs]
    [<000000004f955ad0>] btrfs_commit_transaction+0xb7/0x970 [btrfs]
    [<00000000a69c8761>] btrfs_sync_file+0x28f/0x390 [btrfs]
    [<00000000fa939e06>] do_fsync+0x33/0x70
    [<000000002ff0388b>] __x64_sys_fdatasync+0xe/0x20
    [<00000000fdbf32d4>] do_syscall_64+0x43/0x120
    [<00000000b782d265>] entry_SYSCALL_64_after_hwframe+0x44/0xa9

When cleaning up a block group release all allocated pages. As the data in
the pages is already lost, we can at least free the memory occupied by
them.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/disk-io.c          | 6 ++++++
 fs/btrfs/free-space-cache.c | 6 ++++++
 fs/btrfs/free-space-cache.h | 1 +
 3 files changed, 13 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 018681ec159b..b79c194b1126 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4460,6 +4460,12 @@ static void btrfs_cleanup_bg_io(struct btrfs_block_group *cache)
 {
 	struct inode *inode;
 
+	/*
+	 * If we end up here, we want the pages to be already released
+	 * otherwise we'll leak them.
+	 */
+	btrfs_drop_dirty_io_ctl(&cache->io_ctl);
+
 	inode = cache->io_ctl.inode;
 	if (inode) {
 		invalidate_inode_pages2(inode->i_mapping);
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 705589daeffa..c7ba2b393b33 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -371,6 +371,12 @@ static void io_ctl_drop_pages(struct btrfs_io_ctl *io_ctl)
 	}
 }
 
+void btrfs_drop_dirty_io_ctl(struct btrfs_io_ctl *io_ctl)
+{
+	io_ctl_drop_pages(io_ctl);
+	io_ctl_free(io_ctl);
+}
+
 static int io_ctl_prepare_pages(struct btrfs_io_ctl *io_ctl, bool uptodate)
 {
 	struct page *page;
diff --git a/fs/btrfs/free-space-cache.h b/fs/btrfs/free-space-cache.h
index 2e0a8077aa74..cbe25c31041d 100644
--- a/fs/btrfs/free-space-cache.h
+++ b/fs/btrfs/free-space-cache.h
@@ -147,6 +147,7 @@ int btrfs_trim_block_group_extents(struct btrfs_block_group *block_group,
 int btrfs_trim_block_group_bitmaps(struct btrfs_block_group *block_group,
 				   u64 *trimmed, u64 start, u64 end, u64 minlen,
 				   u64 maxlen, bool async);
+void btrfs_drop_dirty_io_ctl(struct btrfs_io_ctl *io_ctl);
 
 /* Support functions for running our sanity tests */
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
-- 
2.16.4

Re: [PATCH 0/5] Fix memory leak on failed cache-writes

[David Sterba]

On Wed, Feb 12, 2020 at 12:10:18AM +0900, Johannes Thumshirn wrote:
> Fstests' test case generic/475 reliably leaks the btrfs_io_ctl::pages
> allocated in __btrfs_write_out_cache().
> 
> The first four patches are small things I noticed while hunting down the
> problem and are independant of the last patch in this series freeing the pages
> when we throw away a dirty block group.

Thanks, just a note about the patch ordering: the fix 5/5 should go
first so it applies cleanly without the unrelated cleanups. As it's a
leak fix it'll go to some rc and then to stable.

Re: [PATCH 0/5] Fix memory leak on failed cache-writes

[Johannes Thumshirn]

On 11/02/2020 19:45, David Sterba wrote:
> On Wed, Feb 12, 2020 at 12:10:18AM +0900, Johannes Thumshirn wrote:
>> Fstests' test case generic/475 reliably leaks the btrfs_io_ctl::pages
>> allocated in __btrfs_write_out_cache().
>>
>> The first four patches are small things I noticed while hunting down the
>> problem and are independant of the last patch in this series freeing the pages
>> when we throw away a dirty block group.
> 
> Thanks, just a note about the patch ordering: the fix 5/5 should go
> first so it applies cleanly without the unrelated cleanups. As it's a
> leak fix it'll go to some rc and then to stable.
> 


OK I can reorder if I need to resend.

Re: [PATCH 0/5] Fix memory leak on failed cache-writes

[Josef Bacik]

On 2/11/20 10:10 AM, Johannes Thumshirn wrote:
> Fstests' test case generic/475 reliably leaks the btrfs_io_ctl::pages
> allocated in __btrfs_write_out_cache().
> 
> The first four patches are small things I noticed while hunting down the
> problem and are independant of the last patch in this series freeing the pages
> when we throw away a dirty block group.
> 
> Johannes Thumshirn (5):
>    btrfs: use inode from io_ctl in io_ctl_prepare_pages
>    btrfs: make the uptodate argument of io_ctl_add_pages() boolean.
>    btrfs: use standard debug config option to enable free-space-cache
>      debug prints
>    btrfs: simplify error handling in __btrfs_write_out_cache()
>    btrfs: free allocated pages jon failed cache write-out
> 
>   fs/btrfs/disk-io.c          |  6 ++++++
>   fs/btrfs/free-space-cache.c | 44 ++++++++++++++++++++++++--------------------
>   fs/btrfs/free-space-cache.h |  1 +
>   3 files changed, 31 insertions(+), 20 deletions(-)
> 

Weird, I swear I reviewed these yesterday, but I don't see any replies from me. 
You can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

to the whole series.  Thanks,

Josef