* [PATCH 0/5] Fix memory leak on failed cache-writes
@ 2020-02-11 15:10 Johannes Thumshirn
2020-02-11 15:10 ` [PATCH 1/5] btrfs: use inode from io_ctl in io_ctl_prepare_pages Johannes Thumshirn
` (6 more replies)
0 siblings, 7 replies; 9+ messages in thread
From: Johannes Thumshirn @ 2020-02-11 15:10 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 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/5] btrfs: use inode from io_ctl in io_ctl_prepare_pages
2020-02-11 15:10 [PATCH 0/5] Fix memory leak on failed cache-writes Johannes Thumshirn
@ 2020-02-11 15:10 ` Johannes Thumshirn
2020-02-11 15:10 ` [PATCH 2/5] btrfs: make the uptodate argument of io_ctl_add_pages() boolean Johannes Thumshirn
` (5 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Johannes Thumshirn @ 2020-02-11 15:10 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 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/5] btrfs: make the uptodate argument of io_ctl_add_pages() boolean.
2020-02-11 15:10 [PATCH 0/5] Fix memory leak on failed cache-writes Johannes Thumshirn
2020-02-11 15:10 ` [PATCH 1/5] btrfs: use inode from io_ctl in io_ctl_prepare_pages Johannes Thumshirn
@ 2020-02-11 15:10 ` Johannes Thumshirn
2020-02-11 15:10 ` [PATCH 3/5] btrfs: use standard debug config option to enable free-space-cache debug prints Johannes Thumshirn
` (4 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Johannes Thumshirn @ 2020-02-11 15:10 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 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/5] btrfs: use standard debug config option to enable free-space-cache debug prints
2020-02-11 15:10 [PATCH 0/5] Fix memory leak on failed cache-writes Johannes Thumshirn
2020-02-11 15:10 ` [PATCH 1/5] btrfs: use inode from io_ctl in io_ctl_prepare_pages Johannes Thumshirn
2020-02-11 15:10 ` [PATCH 2/5] btrfs: make the uptodate argument of io_ctl_add_pages() boolean Johannes Thumshirn
@ 2020-02-11 15:10 ` Johannes Thumshirn
2020-02-11 15:10 ` [PATCH 4/5] btrfs: simplify error handling in __btrfs_write_out_cache() Johannes Thumshirn
` (3 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Johannes Thumshirn @ 2020-02-11 15:10 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 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/5] btrfs: simplify error handling in __btrfs_write_out_cache()
2020-02-11 15:10 [PATCH 0/5] Fix memory leak on failed cache-writes Johannes Thumshirn
` (2 preceding siblings ...)
2020-02-11 15:10 ` [PATCH 3/5] btrfs: use standard debug config option to enable free-space-cache debug prints Johannes Thumshirn
@ 2020-02-11 15:10 ` Johannes Thumshirn
2020-02-11 15:10 ` [PATCH 5/5] btrfs: free allocated pages jon failed cache write-out Johannes Thumshirn
` (2 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Johannes Thumshirn @ 2020-02-11 15:10 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 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 5/5] btrfs: free allocated pages jon failed cache write-out
2020-02-11 15:10 [PATCH 0/5] Fix memory leak on failed cache-writes Johannes Thumshirn
` (3 preceding siblings ...)
2020-02-11 15:10 ` [PATCH 4/5] btrfs: simplify error handling in __btrfs_write_out_cache() Johannes Thumshirn
@ 2020-02-11 15:10 ` Johannes Thumshirn
2020-02-11 17:59 ` [PATCH 0/5] Fix memory leak on failed cache-writes David Sterba
2020-02-13 20:10 ` Josef Bacik
6 siblings, 0 replies; 9+ messages in thread
From: Johannes Thumshirn @ 2020-02-11 15:10 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 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 0/5] Fix memory leak on failed cache-writes
2020-02-11 15:10 [PATCH 0/5] Fix memory leak on failed cache-writes Johannes Thumshirn
` (4 preceding siblings ...)
2020-02-11 15:10 ` [PATCH 5/5] btrfs: free allocated pages jon failed cache write-out Johannes Thumshirn
@ 2020-02-11 17:59 ` David Sterba
2020-02-11 18:47 ` Johannes Thumshirn
2020-02-13 20:10 ` Josef Bacik
6 siblings, 1 reply; 9+ messages in thread
From: David Sterba @ 2020-02-11 17:59 UTC (permalink / raw)
To: Johannes Thumshirn; +Cc: David Sterba, linux-btrfs @ vger . kernel . org
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/5] Fix memory leak on failed cache-writes
2020-02-11 17:59 ` [PATCH 0/5] Fix memory leak on failed cache-writes David Sterba
@ 2020-02-11 18:47 ` Johannes Thumshirn
0 siblings, 0 replies; 9+ messages in thread
From: Johannes Thumshirn @ 2020-02-11 18:47 UTC (permalink / raw)
To: dsterba; +Cc: linux-btrfs @ vger . kernel . org
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/5] Fix memory leak on failed cache-writes
2020-02-11 15:10 [PATCH 0/5] Fix memory leak on failed cache-writes Johannes Thumshirn
` (5 preceding siblings ...)
2020-02-11 17:59 ` [PATCH 0/5] Fix memory leak on failed cache-writes David Sterba
@ 2020-02-13 20:10 ` Josef Bacik
6 siblings, 0 replies; 9+ messages in thread
From: Josef Bacik @ 2020-02-13 20:10 UTC (permalink / raw)
To: Johannes Thumshirn, David Sterba; +Cc: linux-btrfs @ vger . kernel . org
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-02-13 20:10 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-11 15:10 [PATCH 0/5] Fix memory leak on failed cache-writes Johannes Thumshirn
2020-02-11 15:10 ` [PATCH 1/5] btrfs: use inode from io_ctl in io_ctl_prepare_pages Johannes Thumshirn
2020-02-11 15:10 ` [PATCH 2/5] btrfs: make the uptodate argument of io_ctl_add_pages() boolean Johannes Thumshirn
2020-02-11 15:10 ` [PATCH 3/5] btrfs: use standard debug config option to enable free-space-cache debug prints Johannes Thumshirn
2020-02-11 15:10 ` [PATCH 4/5] btrfs: simplify error handling in __btrfs_write_out_cache() Johannes Thumshirn
2020-02-11 15:10 ` [PATCH 5/5] btrfs: free allocated pages jon failed cache write-out Johannes Thumshirn
2020-02-11 17:59 ` [PATCH 0/5] Fix memory leak on failed cache-writes David Sterba
2020-02-11 18:47 ` Johannes Thumshirn
2020-02-13 20:10 ` Josef Bacik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).