| Message ID | 20211120083411.120338-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix the memory leak caused in lzo_compress_pages() | expand |
On Sat, Nov 20, 2021 at 04:34:11PM +0800, Qu Wenruo wrote: > [BUG] > Fstests generic/027 is pretty easy to trigger a slow but steady memory > leak if run with "-o compress=lzo" mount option. > > Normally one single run of generic/027 is enough to eat up at least 4G ram. > > [CAUSE] > In commit d4088803f511 ("btrfs: subpage: make lzo_compress_pages() > compatible") we changed how @page_in is released. > > But that refactor makes @page_in only released after all pages being > compressed. > > This leaves error path not releasing @page_in. And by "error path" > things like incompressible data will also be treated as an error > (-E2BIG). > > Thus it can leave btrfs to leak memory even there is nothing wrong > happened. > > [FIX] > Add check under @out label to release @page_in when needed, so when we > hit any error, the input page is properly released. > > Reported-by: Josef Bacik <josef@toxicpanda.com> > Fixes: d4088803f511 ("btrfs: subpage: make lzo_compress_pages() compatible") > Signed-off-by: Qu Wenruo <wqu@suse.com> Woo now we're making it all the way through the xfstests runs on the vm that has -o compress=lzo. You can add Reviewed-and-tested-by: Josef Bacik <josef@toxicpanda.com> Thanks for digging into this Qu, Josef
On Sat, Nov 20, 2021 at 04:34:11PM +0800, Qu Wenruo wrote: > [BUG] > Fstests generic/027 is pretty easy to trigger a slow but steady memory > leak if run with "-o compress=lzo" mount option. > > Normally one single run of generic/027 is enough to eat up at least 4G ram. > > [CAUSE] > In commit d4088803f511 ("btrfs: subpage: make lzo_compress_pages() > compatible") we changed how @page_in is released. > > But that refactor makes @page_in only released after all pages being > compressed. > > This leaves error path not releasing @page_in. And by "error path" > things like incompressible data will also be treated as an error > (-E2BIG). > > Thus it can leave btrfs to leak memory even there is nothing wrong > happened. > > [FIX] > Add check under @out label to release @page_in when needed, so when we > hit any error, the input page is properly released. > > Reported-by: Josef Bacik <josef@toxicpanda.com> > Fixes: d4088803f511 ("btrfs: subpage: make lzo_compress_pages() compatible") This commit has caused problems I don't want to repeat, it's an example of a 'rewrite in one go' vs 'incremental changes', this is a pattern known to avoid exactly because of the sublte semantics not transferred into the new version. Please keep that in mind for future patches, it's much safer to review 5 patches doing the change in smaller steps. Anyway, thanks for the fix, added to misc-next.
diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c index 2fdfd0904313..12a459073ea1 100644 --- a/fs/btrfs/lzo.c +++ b/fs/btrfs/lzo.c @@ -290,6 +290,8 @@ int lzo_compress_pages(struct list_head *ws, struct address_space *mapping, *total_out = cur_out; *total_in = cur_in - start; out: + if (page_in) + put_page(page_in); *out_pages = DIV_ROUND_UP(cur_out, PAGE_SIZE); return ret; }
[BUG] Fstests generic/027 is pretty easy to trigger a slow but steady memory leak if run with "-o compress=lzo" mount option. Normally one single run of generic/027 is enough to eat up at least 4G ram. [CAUSE] In commit d4088803f511 ("btrfs: subpage: make lzo_compress_pages() compatible") we changed how @page_in is released. But that refactor makes @page_in only released after all pages being compressed. This leaves error path not releasing @page_in. And by "error path" things like incompressible data will also be treated as an error (-E2BIG). Thus it can leave btrfs to leak memory even there is nothing wrong happened. [FIX] Add check under @out label to release @page_in when needed, so when we hit any error, the input page is properly released. Reported-by: Josef Bacik <josef@toxicpanda.com> Fixes: d4088803f511 ("btrfs: subpage: make lzo_compress_pages() compatible") Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/lzo.c | 2 ++ 1 file changed, 2 insertions(+)