| Message ID | 20200108120732.30451-1-johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix memory leak in qgroup accounting | expand |
On 2020/1/8 下午8:07, Johannes Thumshirn wrote: > When running xfstests on the current btrfs I get the following splat from > kmemleak: > unreferenced object 0xffff88821b2404e0 (size 32): > comm "kworker/u4:7", pid 26663, jiffies 4295283698 (age 8.776s) > hex dump (first 32 bytes): > 01 00 00 00 00 00 00 00 10 ff fd 26 82 88 ff ff ...........&.... > 10 ff fd 26 82 88 ff ff 20 ff fd 26 82 88 ff ff ...&.... ..&.... > backtrace: > [<00000000f94fd43f>] ulist_alloc+0x25/0x60 [btrfs] > [<00000000fd023d99>] btrfs_find_all_roots_safe+0x41/0x100 [btrfs] > [<000000008f17bd32>] btrfs_find_all_roots+0x52/0x70 [btrfs] > [<00000000b7660afb>] btrfs_qgroup_rescan_worker+0x343/0x680 [btrfs] > [<0000000058e66778>] btrfs_work_helper+0xac/0x1e0 [btrfs] > [<00000000f0188930>] process_one_work+0x1cf/0x350 > [<00000000af5f2f8e>] worker_thread+0x28/0x3c0 > [<00000000b55a1add>] kthread+0x109/0x120 > [<00000000f88cbd17>] ret_from_fork+0x35/0x40 > > This corresponds to: > (gdb) l *(btrfs_find_all_roots_safe+0x41) > 0x8d7e1 is in btrfs_find_all_roots_safe (fs/btrfs/backref.c:1413). > 1408 > 1409 tmp = ulist_alloc(GFP_NOFS); > 1410 if (!tmp) > 1411 return -ENOMEM; > 1412 *roots = ulist_alloc(GFP_NOFS); > 1413 if (!*roots) { > 1414 ulist_free(tmp); > 1415 return -ENOMEM; > 1416 } > 1417 > > Following the lifetime of the allocated 'roots' ulist, it get's freed > again in btrfs_qgroup_account_extent(). > > But this does not happen if the function is called with the > 'BTRFS_FS_QUOTA_ENABLED' flag cleared, then btrfs_qgroup_account_extent() > does a short leave and directly returns. > > Instead of directly returning we should jump to the 'out_free' in order to > free all resources as expected. > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> The patch itself is OK. Reviewed-by: Qu Wenruo <wqu@suse.com> This means the qgroup get disabled when rescan is still running. So I'm a little curious, could we just cancel the running rescan and wait for it before disabling qgroup? Thanks, Qu > --- > fs/btrfs/qgroup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > index b046b04d7cce..7ebcdd201eed 100644 > --- a/fs/btrfs/qgroup.c > +++ b/fs/btrfs/qgroup.c > @@ -2416,7 +2416,7 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr, > int ret = 0; > > if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags)) > - return 0; > + goto out_free; > > if (new_roots) { > if (!maybe_fs_roots(new_roots)) >
On 08/01/2020, 13:16, "Qu Wenruo" <quwenruo.btrfs@gmx.com> wrote:
[...]
The patch itself is OK.
Reviewed-by: Qu Wenruo <wqu@suse.com>
This means the qgroup get disabled when rescan is still running.
So I'm a little curious, could we just cancel the running rescan and
wait for it before disabling qgroup?
Maybe. I'm still not 100% certain what's the actual trigger. I see it most of the time
with btrfs/117, but running btrfs/117 alone doesn't trigger the memleak report.
I also saw the memleak report once with btrfs/116, but only once out of ~20 runs.
The good thing though is, it's 100% reproducible that we get the memleak report when
running the first 120 btrfs fstests .
P.S.: Sorry for the broken quoting in the reply, I still haven't configured my mail client correctly.
Byte,
Johannes
On 2020/1/8 下午8:31, Johannes Thumshirn wrote: > On 08/01/2020, 13:16, "Qu Wenruo" <quwenruo.btrfs@gmx.com> wrote: > [...] > > The patch itself is OK. > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > > This means the qgroup get disabled when rescan is still running. > So I'm a little curious, could we just cancel the running rescan and > wait for it before disabling qgroup? > > > Maybe. I'm still not 100% certain what's the actual trigger. I see it most of the time > with btrfs/117, but running btrfs/117 alone doesn't trigger the memleak report. > I also saw the memleak report once with btrfs/116, but only once out of ~20 runs. > The good thing though is, it's 100% reproducible that we get the memleak report when > running the first 120 btrfs fstests. No problem, since the patch itself is already good enough to solve all the cases. > > P.S.: Sorry for the broken quoting in the reply, I still haven't configured my mail client correctly. Have fun in WDC! Thanks, Qu > > Byte, > Johannes >
On Wed, Jan 08, 2020 at 09:07:32PM +0900, Johannes Thumshirn wrote: > When running xfstests on the current btrfs I get the following splat from > kmemleak: > unreferenced object 0xffff88821b2404e0 (size 32): > comm "kworker/u4:7", pid 26663, jiffies 4295283698 (age 8.776s) > hex dump (first 32 bytes): > 01 00 00 00 00 00 00 00 10 ff fd 26 82 88 ff ff ...........&.... > 10 ff fd 26 82 88 ff ff 20 ff fd 26 82 88 ff ff ...&.... ..&.... > backtrace: > [<00000000f94fd43f>] ulist_alloc+0x25/0x60 [btrfs] > [<00000000fd023d99>] btrfs_find_all_roots_safe+0x41/0x100 [btrfs] > [<000000008f17bd32>] btrfs_find_all_roots+0x52/0x70 [btrfs] > [<00000000b7660afb>] btrfs_qgroup_rescan_worker+0x343/0x680 [btrfs] > [<0000000058e66778>] btrfs_work_helper+0xac/0x1e0 [btrfs] > [<00000000f0188930>] process_one_work+0x1cf/0x350 > [<00000000af5f2f8e>] worker_thread+0x28/0x3c0 > [<00000000b55a1add>] kthread+0x109/0x120 > [<00000000f88cbd17>] ret_from_fork+0x35/0x40 > > This corresponds to: > (gdb) l *(btrfs_find_all_roots_safe+0x41) > 0x8d7e1 is in btrfs_find_all_roots_safe (fs/btrfs/backref.c:1413). > 1408 > 1409 tmp = ulist_alloc(GFP_NOFS); > 1410 if (!tmp) > 1411 return -ENOMEM; > 1412 *roots = ulist_alloc(GFP_NOFS); > 1413 if (!*roots) { > 1414 ulist_free(tmp); > 1415 return -ENOMEM; > 1416 } > 1417 > > Following the lifetime of the allocated 'roots' ulist, it get's freed > again in btrfs_qgroup_account_extent(). > > But this does not happen if the function is called with the > 'BTRFS_FS_QUOTA_ENABLED' flag cleared, then btrfs_qgroup_account_extent() > does a short leave and directly returns. > > Instead of directly returning we should jump to the 'out_free' in order to > free all resources as expected. > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Added to misc-next, with a short comment that we can't do a quick return. Thanks.
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index b046b04d7cce..7ebcdd201eed 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -2416,7 +2416,7 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr, int ret = 0; if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags)) - return 0; + goto out_free; if (new_roots) { if (!maybe_fs_roots(new_roots))
When running xfstests on the current btrfs I get the following splat from kmemleak: unreferenced object 0xffff88821b2404e0 (size 32): comm "kworker/u4:7", pid 26663, jiffies 4295283698 (age 8.776s) hex dump (first 32 bytes): 01 00 00 00 00 00 00 00 10 ff fd 26 82 88 ff ff ...........&.... 10 ff fd 26 82 88 ff ff 20 ff fd 26 82 88 ff ff ...&.... ..&.... backtrace: [<00000000f94fd43f>] ulist_alloc+0x25/0x60 [btrfs] [<00000000fd023d99>] btrfs_find_all_roots_safe+0x41/0x100 [btrfs] [<000000008f17bd32>] btrfs_find_all_roots+0x52/0x70 [btrfs] [<00000000b7660afb>] btrfs_qgroup_rescan_worker+0x343/0x680 [btrfs] [<0000000058e66778>] btrfs_work_helper+0xac/0x1e0 [btrfs] [<00000000f0188930>] process_one_work+0x1cf/0x350 [<00000000af5f2f8e>] worker_thread+0x28/0x3c0 [<00000000b55a1add>] kthread+0x109/0x120 [<00000000f88cbd17>] ret_from_fork+0x35/0x40 This corresponds to: (gdb) l *(btrfs_find_all_roots_safe+0x41) 0x8d7e1 is in btrfs_find_all_roots_safe (fs/btrfs/backref.c:1413). 1408 1409 tmp = ulist_alloc(GFP_NOFS); 1410 if (!tmp) 1411 return -ENOMEM; 1412 *roots = ulist_alloc(GFP_NOFS); 1413 if (!*roots) { 1414 ulist_free(tmp); 1415 return -ENOMEM; 1416 } 1417 Following the lifetime of the allocated 'roots' ulist, it get's freed again in btrfs_qgroup_account_extent(). But this does not happen if the function is called with the 'BTRFS_FS_QUOTA_ENABLED' flag cleared, then btrfs_qgroup_account_extent() does a short leave and directly returns. Instead of directly returning we should jump to the 'out_free' in order to free all resources as expected. Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> --- fs/btrfs/qgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)