* [PATCH 0/2] md: memleak fixes @ 2017-11-08 12:44 Zdenek Kabelac 2017-11-08 12:44 ` [PATCH 1/2] md: release allocated bitset sync_set Zdenek Kabelac 2017-11-08 12:44 ` [PATCH 2/2] md: free unused memory after bitmap resize Zdenek Kabelac 0 siblings, 2 replies; 6+ messages in thread From: Zdenek Kabelac @ 2017-11-08 12:44 UTC (permalink / raw) To: linux-raid; +Cc: Zdenek Kabelac Patchset tries to address several kmeleaks visible when lvm2 is using raid target based on md-core. 1st. patch as quite easy to validate 2nd. please do careful review as bitmap.c is hard to follow Zdenek Kabelac (2): md: release allocated bitset sync_set md: free unused memory after bitmap resize drivers/md/bitmap.c | 9 +++++++++ drivers/md/md.c | 8 +++++++- 2 files changed, 16 insertions(+), 1 deletion(-) -- 2.15.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] md: release allocated bitset sync_set 2017-11-08 12:44 [PATCH 0/2] md: memleak fixes Zdenek Kabelac @ 2017-11-08 12:44 ` Zdenek Kabelac 2017-11-09 23:32 ` Shaohua Li 2017-11-08 12:44 ` [PATCH 2/2] md: free unused memory after bitmap resize Zdenek Kabelac 1 sibling, 1 reply; 6+ messages in thread From: Zdenek Kabelac @ 2017-11-08 12:44 UTC (permalink / raw) To: linux-raid; +Cc: Zdenek Kabelac Patch fixes kmemleak on md_stop() path used likely only by dm-raid wrapper. Code of md is using mddev_put() where both bitsets are released however this bitmap freeing is not shared. Also set NULL to bio_set and sync_set pointers just like mddev_put is doing. Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com> --- drivers/md/md.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index 0ff1bbf6c90e..635ad7c38f67 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -5834,8 +5834,14 @@ void md_stop(struct mddev *mddev) * This is called from dm-raid */ __md_stop(mddev); - if (mddev->bio_set) + if (mddev->bio_set) { bioset_free(mddev->bio_set); + mddev->bio_set = NULL; + } + if (mddev->sync_set) { + bioset_free(mddev->sync_set); + mddev->sync_set = NULL; + } } EXPORT_SYMBOL_GPL(md_stop); -- 2.15.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] md: release allocated bitset sync_set 2017-11-08 12:44 ` [PATCH 1/2] md: release allocated bitset sync_set Zdenek Kabelac @ 2017-11-09 23:32 ` Shaohua Li 2017-11-10 10:03 ` Zdenek Kabelac 0 siblings, 1 reply; 6+ messages in thread From: Shaohua Li @ 2017-11-09 23:32 UTC (permalink / raw) To: Zdenek Kabelac; +Cc: linux-raid On Wed, Nov 08, 2017 at 01:44:55PM +0100, Zdenek Kabelac wrote: > Patch fixes kmemleak on md_stop() path used likely only by dm-raid > wrapper. Code of md is using mddev_put() where both bitsets > are released however this bitmap freeing is not shared. what did you mean 'bitmap freeing is not shared' ? The patch doesn't handle bitmap either. > Also set NULL to bio_set and sync_set pointers just like mddev_put is > doing. > > Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com> > --- > drivers/md/md.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/md/md.c b/drivers/md/md.c > index 0ff1bbf6c90e..635ad7c38f67 100644 > --- a/drivers/md/md.c > +++ b/drivers/md/md.c > @@ -5834,8 +5834,14 @@ void md_stop(struct mddev *mddev) > * This is called from dm-raid > */ > __md_stop(mddev); > - if (mddev->bio_set) > + if (mddev->bio_set) { > bioset_free(mddev->bio_set); > + mddev->bio_set = NULL; > + } > + if (mddev->sync_set) { > + bioset_free(mddev->sync_set); > + mddev->sync_set = NULL; > + } > } > > EXPORT_SYMBOL_GPL(md_stop); > -- > 2.15.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-raid" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] md: release allocated bitset sync_set 2017-11-09 23:32 ` Shaohua Li @ 2017-11-10 10:03 ` Zdenek Kabelac 0 siblings, 0 replies; 6+ messages in thread From: Zdenek Kabelac @ 2017-11-10 10:03 UTC (permalink / raw) To: Shaohua Li; +Cc: linux-raid Dne 10.11.2017 v 00:32 Shaohua Li napsal(a): > On Wed, Nov 08, 2017 at 01:44:55PM +0100, Zdenek Kabelac wrote: >> Patch fixes kmemleak on md_stop() path used likely only by dm-raid >> wrapper. Code of md is using mddev_put() where both bitsets >> are released however this bitmap freeing is not shared. > > what did you mean 'bitmap freeing is not shared' ? The patch doesn't handle > bitmap either. > It only means there is some code to teardown md device which is not shared between 'md' and 'dm' wrapper - thus whenever mddev_put() is updated in some way (adding freeing another object) same code should be duplicated into md_stop() which is used only by 'dm' (from my understanding of comment in the source). IMHO this is not ideal - the code for freeing resources should be sharing more code lines... But as of now - patch works and does exactly what is needed... >> Also set NULL to bio_set and sync_set pointers just like mddev_put is >> doing. >> >> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com> >> --- >> drivers/md/md.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/md/md.c b/drivers/md/md.c >> index 0ff1bbf6c90e..635ad7c38f67 100644 >> --- a/drivers/md/md.c >> +++ b/drivers/md/md.c >> @@ -5834,8 +5834,14 @@ void md_stop(struct mddev *mddev) >> * This is called from dm-raid >> */ >> __md_stop(mddev); >> - if (mddev->bio_set) >> + if (mddev->bio_set) { >> bioset_free(mddev->bio_set); >> + mddev->bio_set = NULL; >> + } >> + if (mddev->sync_set) { >> + bioset_free(mddev->sync_set); >> + mddev->sync_set = NULL; >> + } >> } >> >> EXPORT_SYMBOL_GPL(md_stop); >> -- >> 2.15.0 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-raid" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] md: free unused memory after bitmap resize 2017-11-08 12:44 [PATCH 0/2] md: memleak fixes Zdenek Kabelac 2017-11-08 12:44 ` [PATCH 1/2] md: release allocated bitset sync_set Zdenek Kabelac @ 2017-11-08 12:44 ` Zdenek Kabelac 2017-11-09 3:57 ` Guoqing Jiang 1 sibling, 1 reply; 6+ messages in thread From: Zdenek Kabelac @ 2017-11-08 12:44 UTC (permalink / raw) To: linux-raid; +Cc: Zdenek Kabelac When bitmap is resized, the old kalloced chunks just are not released once the resized bitmap starts to use new space. This fixes in particular kmemleak reports like this one: unreferenced object 0xffff8f4311e9c000 (size 4096): comm "lvm", pid 19333, jiffies 4295263268 (age 528.265s) hex dump (first 32 bytes): 02 80 02 80 02 80 02 80 02 80 02 80 02 80 02 80 ................ 02 80 02 80 02 80 02 80 02 80 02 80 02 80 02 80 ................ backtrace: [<ffffffffa69471ca>] kmemleak_alloc+0x4a/0xa0 [<ffffffffa628c10e>] kmem_cache_alloc_trace+0x14e/0x2e0 [<ffffffffa676cfec>] bitmap_checkpage+0x7c/0x110 [<ffffffffa676d0c5>] bitmap_get_counter+0x45/0xd0 [<ffffffffa676d6b3>] bitmap_set_memory_bits+0x43/0xe0 [<ffffffffa676e41c>] bitmap_init_from_disk+0x23c/0x530 [<ffffffffa676f1ae>] bitmap_load+0xbe/0x160 [<ffffffffc04c47d3>] raid_preresume+0x203/0x2f0 [dm_raid] [<ffffffffa677762f>] dm_table_resume_targets+0x4f/0xe0 [<ffffffffa6774b52>] dm_resume+0x122/0x140 [<ffffffffa6779b9f>] dev_suspend+0x18f/0x290 [<ffffffffa677a3a7>] ctl_ioctl+0x287/0x560 [<ffffffffa677a693>] dm_ctl_ioctl+0x13/0x20 [<ffffffffa62d6b46>] do_vfs_ioctl+0xa6/0x750 [<ffffffffa62d7269>] SyS_ioctl+0x79/0x90 [<ffffffffa6956d41>] entry_SYSCALL_64_fastpath+0x1f/0xc2 Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com> --- drivers/md/bitmap.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c index d2121637b4ab..58ee21027709 100644 --- a/drivers/md/bitmap.c +++ b/drivers/md/bitmap.c @@ -2152,6 +2152,7 @@ int bitmap_resize(struct bitmap *bitmap, sector_t blocks, for (k = 0; k < page; k++) { kfree(new_bp[k].map); } + kfree(new_bp); /* restore some fields from old_counts */ bitmap->counts.bp = old_counts.bp; @@ -2202,6 +2203,14 @@ int bitmap_resize(struct bitmap *bitmap, sector_t blocks, block += old_blocks; } + if (bitmap->counts.bp != old_counts.bp) { + unsigned long k; + for (k = 0; k < old_counts.pages; k++) + if (!old_counts.bp[k].hijacked) + kfree(old_counts.bp[k].map); + kfree(old_counts.bp); + } + if (!init) { int i; while (block < (chunks << chunkshift)) { -- 2.15.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] md: free unused memory after bitmap resize 2017-11-08 12:44 ` [PATCH 2/2] md: free unused memory after bitmap resize Zdenek Kabelac @ 2017-11-09 3:57 ` Guoqing Jiang 0 siblings, 0 replies; 6+ messages in thread From: Guoqing Jiang @ 2017-11-09 3:57 UTC (permalink / raw) To: Zdenek Kabelac, linux-raid On 11/08/2017 08:44 PM, Zdenek Kabelac wrote: > When bitmap is resized, the old kalloced chunks just are not released > once the resized bitmap starts to use new space. > > This fixes in particular kmemleak reports like this one: > > unreferenced object 0xffff8f4311e9c000 (size 4096): > comm "lvm", pid 19333, jiffies 4295263268 (age 528.265s) > hex dump (first 32 bytes): > 02 80 02 80 02 80 02 80 02 80 02 80 02 80 02 80 ................ > 02 80 02 80 02 80 02 80 02 80 02 80 02 80 02 80 ................ > backtrace: > [<ffffffffa69471ca>] kmemleak_alloc+0x4a/0xa0 > [<ffffffffa628c10e>] kmem_cache_alloc_trace+0x14e/0x2e0 > [<ffffffffa676cfec>] bitmap_checkpage+0x7c/0x110 > [<ffffffffa676d0c5>] bitmap_get_counter+0x45/0xd0 > [<ffffffffa676d6b3>] bitmap_set_memory_bits+0x43/0xe0 > [<ffffffffa676e41c>] bitmap_init_from_disk+0x23c/0x530 > [<ffffffffa676f1ae>] bitmap_load+0xbe/0x160 > [<ffffffffc04c47d3>] raid_preresume+0x203/0x2f0 [dm_raid] > [<ffffffffa677762f>] dm_table_resume_targets+0x4f/0xe0 > [<ffffffffa6774b52>] dm_resume+0x122/0x140 > [<ffffffffa6779b9f>] dev_suspend+0x18f/0x290 > [<ffffffffa677a3a7>] ctl_ioctl+0x287/0x560 > [<ffffffffa677a693>] dm_ctl_ioctl+0x13/0x20 > [<ffffffffa62d6b46>] do_vfs_ioctl+0xa6/0x750 > [<ffffffffa62d7269>] SyS_ioctl+0x79/0x90 > [<ffffffffa6956d41>] entry_SYSCALL_64_fastpath+0x1f/0xc2 > > Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com> > --- > drivers/md/bitmap.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c > index d2121637b4ab..58ee21027709 100644 > --- a/drivers/md/bitmap.c > +++ b/drivers/md/bitmap.c The bitmap.c is renamed to md-bitmap.c now. > @@ -2152,6 +2152,7 @@ int bitmap_resize(struct bitmap *bitmap, sector_t blocks, > for (k = 0; k < page; k++) { > kfree(new_bp[k].map); > } > + kfree(new_bp); Agreed. > > /* restore some fields from old_counts */ > bitmap->counts.bp = old_counts.bp; > @@ -2202,6 +2203,14 @@ int bitmap_resize(struct bitmap *bitmap, sector_t blocks, > block += old_blocks; > } > > + if (bitmap->counts.bp != old_counts.bp) { > + unsigned long k; Just nit-picking, maybe you can move belows to > + for (k = 0; k < old_counts.pages; k++) > + if (!old_counts.bp[k].hijacked) > + kfree(old_counts.bp[k].map); > + kfree(old_counts.bp); > + } > + > if (!init) { > int i; here and reuse the 'i'. Thanks, Guoqing ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-10 10:03 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-11-08 12:44 [PATCH 0/2] md: memleak fixes Zdenek Kabelac 2017-11-08 12:44 ` [PATCH 1/2] md: release allocated bitset sync_set Zdenek Kabelac 2017-11-09 23:32 ` Shaohua Li 2017-11-10 10:03 ` Zdenek Kabelac 2017-11-08 12:44 ` [PATCH 2/2] md: free unused memory after bitmap resize Zdenek Kabelac 2017-11-09 3:57 ` Guoqing Jiang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.