Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/5] Fix memory leak on failed cache-writes
@ 2020-02-13 15:57 Johannes Thumshirn
  2020-02-13 15:57 ` [PATCH v2 1/5] btrfs: free allocated pages on failed cache write-out Johannes Thumshirn
                   ` (5 more replies)
  0 siblings, 6 replies; 8+ messages in thread
From: Johannes Thumshirn @ 2020-02-13 15:57 UTC (permalink / raw)
  To: David Sterba; +Cc: linux-btrfs @ vger . kernel . org, Johannes Thumshirn

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

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

Changes to v1:
- Move fix to the first position (David)

Johannes Thumshirn (5):
  btrfs: free allocated pages on failed cache write-out
  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()

 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.24.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/5] btrfs: free allocated pages on failed cache write-out
  2020-02-13 15:57 [PATCH v2 0/5] Fix memory leak on failed cache-writes Johannes Thumshirn
@ 2020-02-13 15:57 ` Johannes Thumshirn
  2020-02-13 15:58 ` [PATCH v2 2/5] btrfs: use inode from io_ctl in io_ctl_prepare_pages Johannes Thumshirn
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Johannes Thumshirn @ 2020-02-13 15:57 UTC (permalink / raw)
  To: David Sterba; +Cc: linux-btrfs @ vger . kernel . org, 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 0598fd3c6e3f..3c7660b04a81 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, struct inode *inode,
 				int uptodate)
 {
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.24.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 2/5] btrfs: use inode from io_ctl in io_ctl_prepare_pages
  2020-02-13 15:57 [PATCH v2 0/5] Fix memory leak on failed cache-writes Johannes Thumshirn
  2020-02-13 15:57 ` [PATCH v2 1/5] btrfs: free allocated pages on failed cache write-out Johannes Thumshirn
@ 2020-02-13 15:58 ` Johannes Thumshirn
  2020-02-13 15:58 ` [PATCH v2 3/5] btrfs: make the uptodate argument of io_ctl_add_pages() boolean Johannes Thumshirn
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Johannes Thumshirn @ 2020-02-13 15:58 UTC (permalink / raw)
  To: David Sterba; +Cc: linux-btrfs @ vger . kernel . org, 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 3c7660b04a81..657a969a93ad 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -377,10 +377,10 @@ void btrfs_drop_dirty_io_ctl(struct btrfs_io_ctl *io_ctl)
 	io_ctl_free(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;
 
@@ -738,7 +738,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;
 
@@ -1297,7 +1297,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.24.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 3/5] btrfs: make the uptodate argument of io_ctl_add_pages() boolean.
  2020-02-13 15:57 [PATCH v2 0/5] Fix memory leak on failed cache-writes Johannes Thumshirn
  2020-02-13 15:57 ` [PATCH v2 1/5] btrfs: free allocated pages on failed cache write-out Johannes Thumshirn
  2020-02-13 15:58 ` [PATCH v2 2/5] btrfs: use inode from io_ctl in io_ctl_prepare_pages Johannes Thumshirn
@ 2020-02-13 15:58 ` Johannes Thumshirn
  2020-02-13 15:58 ` [PATCH v2 4/5] btrfs: use standard debug config option to enable free-space-cache debug prints Johannes Thumshirn
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Johannes Thumshirn @ 2020-02-13 15:58 UTC (permalink / raw)
  To: David Sterba; +Cc: linux-btrfs @ vger . kernel . org, 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 657a969a93ad..8da592eaf6f8 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -377,7 +377,7 @@ void btrfs_drop_dirty_io_ctl(struct btrfs_io_ctl *io_ctl)
 	io_ctl_free(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;
@@ -738,7 +738,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;
 
@@ -1297,7 +1297,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.24.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 4/5] btrfs: use standard debug config option to enable free-space-cache debug prints
  2020-02-13 15:57 [PATCH v2 0/5] Fix memory leak on failed cache-writes Johannes Thumshirn
                   ` (2 preceding siblings ...)
  2020-02-13 15:58 ` [PATCH v2 3/5] btrfs: make the uptodate argument of io_ctl_add_pages() boolean Johannes Thumshirn
@ 2020-02-13 15:58 ` Johannes Thumshirn
  2020-02-13 15:58 ` [PATCH v2 5/5] btrfs: simplify error handling in __btrfs_write_out_cache() Johannes Thumshirn
  2020-02-18 16:50 ` [PATCH v2 0/5] Fix memory leak on failed cache-writes David Sterba
  5 siblings, 0 replies; 8+ messages in thread
From: Johannes Thumshirn @ 2020-02-13 15:58 UTC (permalink / raw)
  To: David Sterba; +Cc: linux-btrfs @ vger . kernel . org, 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 8da592eaf6f8..41e138f2ae12 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -1196,7 +1196,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);
@@ -1422,7 +1422,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);
@@ -4042,7 +4042,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.24.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 5/5] btrfs: simplify error handling in __btrfs_write_out_cache()
  2020-02-13 15:57 [PATCH v2 0/5] Fix memory leak on failed cache-writes Johannes Thumshirn
                   ` (3 preceding siblings ...)
  2020-02-13 15:58 ` [PATCH v2 4/5] btrfs: use standard debug config option to enable free-space-cache debug prints Johannes Thumshirn
@ 2020-02-13 15:58 ` Johannes Thumshirn
  2020-02-18 16:50 ` [PATCH v2 0/5] Fix memory leak on failed cache-writes David Sterba
  5 siblings, 0 replies; 8+ messages in thread
From: Johannes Thumshirn @ 2020-02-13 15:58 UTC (permalink / raw)
  To: David Sterba; +Cc: linux-btrfs @ vger . kernel . org, 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 41e138f2ae12..c7ba2b393b33 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -1372,18 +1372,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);
@@ -1396,7 +1384,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.24.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/5] Fix memory leak on failed cache-writes
  2020-02-13 15:57 [PATCH v2 0/5] Fix memory leak on failed cache-writes Johannes Thumshirn
                   ` (4 preceding siblings ...)
  2020-02-13 15:58 ` [PATCH v2 5/5] btrfs: simplify error handling in __btrfs_write_out_cache() Johannes Thumshirn
@ 2020-02-18 16:50 ` David Sterba
  2020-02-18 16:54   ` Johannes Thumshirn
  5 siblings, 1 reply; 8+ messages in thread
From: David Sterba @ 2020-02-18 16:50 UTC (permalink / raw)
  To: Johannes Thumshirn; +Cc: David Sterba, linux-btrfs @ vger . kernel . org

On Fri, Feb 14, 2020 at 12:57:58AM +0900, Johannes Thumshirn wrote:
> Fstests' test case generic/475 reliably leaks the btrfs_io_ctl::pages
> allocated in __btrfs_write_out_cache().
> 
> The first patch in this series is freeing the pages when we throw away a dirty
> block group. The other patches are small things I noticed while hunting down the
> problem and are independant of fix.
> 
> Changes to v1:
> - Move fix to the first position (David)
> 
> Johannes Thumshirn (5):
>   btrfs: free allocated pages on failed cache write-out
>   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()

I've seen some weird test failures and this patchset was in the tested
branch (either for-next or standalone). I need to retest misc-next again
to have a clean baseline so I can see the diff.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/5] Fix memory leak on failed cache-writes
  2020-02-18 16:50 ` [PATCH v2 0/5] Fix memory leak on failed cache-writes David Sterba
@ 2020-02-18 16:54   ` Johannes Thumshirn
  0 siblings, 0 replies; 8+ messages in thread
From: Johannes Thumshirn @ 2020-02-18 16:54 UTC (permalink / raw)
  To: dsterba; +Cc: linux-btrfs @ vger . kernel . org

On 18/02/2020 17:50, David Sterba wrote:
> On Fri, Feb 14, 2020 at 12:57:58AM +0900, Johannes Thumshirn wrote:
>> Fstests' test case generic/475 reliably leaks the btrfs_io_ctl::pages
>> allocated in __btrfs_write_out_cache().
>>
>> The first patch in this series is freeing the pages when we throw away a dirty
>> block group. The other patches are small things I noticed while hunting down the
>> problem and are independant of fix.
>>
>> Changes to v1:
>> - Move fix to the first position (David)
>>
>> Johannes Thumshirn (5):
>>    btrfs: free allocated pages on failed cache write-out
>>    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()
> 
> I've seen some weird test failures and this patchset was in the tested
> branch (either for-next or standalone). I need to retest misc-next again
> to have a clean baseline so I can see the diff.
> 

Interesting, can you forward me the failures?


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-13 15:57 [PATCH v2 0/5] Fix memory leak on failed cache-writes Johannes Thumshirn
2020-02-13 15:57 ` [PATCH v2 1/5] btrfs: free allocated pages on failed cache write-out Johannes Thumshirn
2020-02-13 15:58 ` [PATCH v2 2/5] btrfs: use inode from io_ctl in io_ctl_prepare_pages Johannes Thumshirn
2020-02-13 15:58 ` [PATCH v2 3/5] btrfs: make the uptodate argument of io_ctl_add_pages() boolean Johannes Thumshirn
2020-02-13 15:58 ` [PATCH v2 4/5] btrfs: use standard debug config option to enable free-space-cache debug prints Johannes Thumshirn
2020-02-13 15:58 ` [PATCH v2 5/5] btrfs: simplify error handling in __btrfs_write_out_cache() Johannes Thumshirn
2020-02-18 16:50 ` [PATCH v2 0/5] Fix memory leak on failed cache-writes David Sterba
2020-02-18 16:54   ` Johannes Thumshirn

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org
	public-inbox-index linux-btrfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git