* [PATCH 1/2] dm thin: fix memory leak of singleton bio-prison cell
@ 2012-04-12 22:34 Mike Snitzer
2012-04-12 22:34 ` [PATCH 2/2] dm thin: use slab_pool for caches Mike Snitzer
2012-04-23 7:24 ` [PATCH 1/2] dm thin: fix memory leak of singleton bio-prison cell Joe Thornber
0 siblings, 2 replies; 6+ messages in thread
From: Mike Snitzer @ 2012-04-12 22:34 UTC (permalink / raw)
To: dm-devel; +Cc: ejt
Add missing mempool_free() to __cell_release_singleton().
This is a pretty significant leak that will accumulate to over 2GB of
leaked memory just from running the full thinp-test-suite. Leak was
known to exist but the kmalloc_pool which was used for prison cell
allocation caused all leaked memory to be attributed to the "size-128"
cache. kmemleak proved useful in identifying the source of this
illusive leak.
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
drivers/md/dm-thin.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
Index: linux-2.6/drivers/md/dm-thin.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-thin.c
+++ linux-2.6/drivers/md/dm-thin.c
@@ -279,8 +279,10 @@ static void __cell_release(struct cell *
hlist_del(&cell->list);
- bio_list_add(inmates, cell->holder);
- bio_list_merge(inmates, &cell->bios);
+ if (inmates) {
+ bio_list_add(inmates, cell->holder);
+ bio_list_merge(inmates, &cell->bios);
+ }
mempool_free(cell, prison->cell_pool);
}
@@ -303,9 +305,10 @@ static void cell_release(struct cell *ce
*/
static void __cell_release_singleton(struct cell *cell, struct bio *bio)
{
- hlist_del(&cell->list);
BUG_ON(cell->holder != bio);
BUG_ON(!bio_list_empty(&cell->bios));
+
+ __cell_release(cell, NULL);
}
static void cell_release_singleton(struct cell *cell, struct bio *bio)
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 2/2] dm thin: use slab_pool for caches 2012-04-12 22:34 [PATCH 1/2] dm thin: fix memory leak of singleton bio-prison cell Mike Snitzer @ 2012-04-12 22:34 ` Mike Snitzer 2012-04-12 22:39 ` [PATCH 2/2 v2] dm thin: use slab mempools with local caches Mike Snitzer 2012-04-23 7:24 ` [PATCH 1/2] dm thin: fix memory leak of singleton bio-prison cell Joe Thornber 1 sibling, 1 reply; 6+ messages in thread From: Mike Snitzer @ 2012-04-12 22:34 UTC (permalink / raw) To: dm-devel; +Cc: ejt Use dedicated caches prefixed with a "dm_" name rather than rely on generic slab caches. This will aid in debugging thinp memory leaks should they occur. Signed-off-by: Mike Snitzer <snitzer@redhat.com> --- drivers/md/dm-thin.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 5 deletions(-) Index: linux-2.6/drivers/md/dm-thin.c =================================================================== --- linux-2.6.orig/drivers/md/dm-thin.c +++ linux-2.6/drivers/md/dm-thin.c @@ -141,6 +141,8 @@ static uint32_t calc_nr_buckets(unsigned return n; } +struct kmem_cache *_cell_cache; + /* * @nr_cells should be the number of cells you want in use _concurrently_. * Don't confuse it with the number of distinct keys. @@ -157,8 +159,7 @@ static struct bio_prison *prison_create( return NULL; spin_lock_init(&prison->lock); - prison->cell_pool = mempool_create_kmalloc_pool(nr_cells, - sizeof(struct cell)); + prison->cell_pool = mempool_create_slab_pool(nr_cells, _cell_cache); if (!prison->cell_pool) { kfree(prison); return NULL; @@ -1649,6 +1650,9 @@ static void pool_features_init(struct po pf->discard_passdown = 1; } +struct kmem_cache *_new_mapping_cache; +struct kmem_cache *_endio_hook_cache; + static void __pool_destroy(struct pool *pool) { __pool_table_remove(pool); @@ -1738,7 +1742,7 @@ static struct pool *pool_create(struct m pool->next_mapping = NULL; pool->mapping_pool = - mempool_create_kmalloc_pool(MAPPING_POOL_SIZE, sizeof(struct new_mapping)); + mempool_create_slab_pool(MAPPING_POOL_SIZE, _new_mapping_cache); if (!pool->mapping_pool) { *error = "Error creating pool's mapping mempool"; err_p = ERR_PTR(-ENOMEM); @@ -1746,7 +1750,7 @@ static struct pool *pool_create(struct m } pool->endio_hook_pool = - mempool_create_kmalloc_pool(ENDIO_HOOK_POOL_SIZE, sizeof(struct endio_hook)); + mempool_create_slab_pool(ENDIO_HOOK_POOL_SIZE, _endio_hook_cache); if (!pool->endio_hook_pool) { *error = "Error creating pool's endio_hook mempool"; err_p = ERR_PTR(-ENOMEM); @@ -2748,7 +2752,42 @@ static int __init dm_thin_init(void) r = dm_register_target(&pool_target); if (r) - dm_unregister_target(&thin_target); + goto bad_pool_target; + + _cell_cache = kmem_cache_create("dm_bio_prison_cell", + sizeof(struct cell), + __alignof__(struct cell), 0, NULL); + if (!_cell_cache) { + r = -ENOMEM; + goto bad_cell_cache; + } + + _new_mapping_cache = kmem_cache_create("dm_thin_new_mapping", + sizeof(struct new_mapping), + __alignof__(struct new_mapping), 0, NULL); + if (!_new_mapping_cache) { + r = -ENOMEM; + goto bad_new_mapping_cache; + } + + _endio_hook_cache = kmem_cache_create("dm_thin_endio_hook", + sizeof(struct endio_hook), + __alignof__(struct endio_hook), 0, NULL); + if (!_endio_hook_cache) { + r = -ENOMEM; + goto bad_endio_hook_cache; + } + + return 0; + +bad_endio_hook_cache: + kmem_cache_destroy(_new_mapping_cache); +bad_new_mapping_cache: + kmem_cache_destroy(_cell_cache); +bad_cell_cache: + dm_unregister_target(&pool_target); +bad_pool_target: + dm_unregister_target(&thin_target); return r; } @@ -2757,6 +2796,9 @@ static void dm_thin_exit(void) { dm_unregister_target(&thin_target); dm_unregister_target(&pool_target); + kmem_cache_destroy(_cell_cache); + kmem_cache_destroy(_new_mapping_cache); + kmem_cache_destroy(_endio_hook_cache); } module_init(dm_thin_init); ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2 v2] dm thin: use slab mempools with local caches 2012-04-12 22:34 ` [PATCH 2/2] dm thin: use slab_pool for caches Mike Snitzer @ 2012-04-12 22:39 ` Mike Snitzer 0 siblings, 0 replies; 6+ messages in thread From: Mike Snitzer @ 2012-04-12 22:39 UTC (permalink / raw) To: dm-devel; +Cc: ejt Use dedicated caches prefixed with a "dm_" name rather than rely on kmalloc mempools backed by generic slab caches. This will aid in debugging thinp memory leaks should they occur. Signed-off-by: Mike Snitzer <snitzer@redhat.com> --- drivers/md/dm-thin.c | 52 +++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 47 insertions(+), 5 deletions(-) v2: tweaked subject and header some, no code changes. diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c index 301db0f..57d40b1 100644 --- a/drivers/md/dm-thin.c +++ b/drivers/md/dm-thin.c @@ -141,6 +141,8 @@ static uint32_t calc_nr_buckets(unsigned nr_cells) return n; } +struct kmem_cache *_cell_cache; + /* * @nr_cells should be the number of cells you want in use _concurrently_. * Don't confuse it with the number of distinct keys. @@ -157,8 +159,7 @@ static struct bio_prison *prison_create(unsigned nr_cells) return NULL; spin_lock_init(&prison->lock); - prison->cell_pool = mempool_create_kmalloc_pool(nr_cells, - sizeof(struct cell)); + prison->cell_pool = mempool_create_slab_pool(nr_cells, _cell_cache); if (!prison->cell_pool) { kfree(prison); return NULL; @@ -1649,6 +1650,9 @@ static void pool_features_init(struct pool_features *pf) pf->discard_passdown = 1; } +struct kmem_cache *_new_mapping_cache; +struct kmem_cache *_endio_hook_cache; + static void __pool_destroy(struct pool *pool) { __pool_table_remove(pool); @@ -1738,7 +1742,7 @@ static struct pool *pool_create(struct mapped_device *pool_md, pool->next_mapping = NULL; pool->mapping_pool = - mempool_create_kmalloc_pool(MAPPING_POOL_SIZE, sizeof(struct new_mapping)); + mempool_create_slab_pool(MAPPING_POOL_SIZE, _new_mapping_cache); if (!pool->mapping_pool) { *error = "Error creating pool's mapping mempool"; err_p = ERR_PTR(-ENOMEM); @@ -1746,7 +1750,7 @@ static struct pool *pool_create(struct mapped_device *pool_md, } pool->endio_hook_pool = - mempool_create_kmalloc_pool(ENDIO_HOOK_POOL_SIZE, sizeof(struct endio_hook)); + mempool_create_slab_pool(ENDIO_HOOK_POOL_SIZE, _endio_hook_cache); if (!pool->endio_hook_pool) { *error = "Error creating pool's endio_hook mempool"; err_p = ERR_PTR(-ENOMEM); @@ -2748,7 +2752,42 @@ static int __init dm_thin_init(void) r = dm_register_target(&pool_target); if (r) - dm_unregister_target(&thin_target); + goto bad_pool_target; + + _cell_cache = kmem_cache_create("dm_bio_prison_cell", + sizeof(struct cell), + __alignof__(struct cell), 0, NULL); + if (!_cell_cache) { + r = -ENOMEM; + goto bad_cell_cache; + } + + _new_mapping_cache = kmem_cache_create("dm_thin_new_mapping", + sizeof(struct new_mapping), + __alignof__(struct new_mapping), 0, NULL); + if (!_new_mapping_cache) { + r = -ENOMEM; + goto bad_new_mapping_cache; + } + + _endio_hook_cache = kmem_cache_create("dm_thin_endio_hook", + sizeof(struct endio_hook), + __alignof__(struct endio_hook), 0, NULL); + if (!_endio_hook_cache) { + r = -ENOMEM; + goto bad_endio_hook_cache; + } + + return 0; + +bad_endio_hook_cache: + kmem_cache_destroy(_new_mapping_cache); +bad_new_mapping_cache: + kmem_cache_destroy(_cell_cache); +bad_cell_cache: + dm_unregister_target(&pool_target); +bad_pool_target: + dm_unregister_target(&thin_target); return r; } @@ -2757,6 +2796,9 @@ static void dm_thin_exit(void) { dm_unregister_target(&thin_target); dm_unregister_target(&pool_target); + kmem_cache_destroy(_cell_cache); + kmem_cache_destroy(_new_mapping_cache); + kmem_cache_destroy(_endio_hook_cache); } module_init(dm_thin_init); -- 1.7.4.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] dm thin: fix memory leak of singleton bio-prison cell 2012-04-12 22:34 [PATCH 1/2] dm thin: fix memory leak of singleton bio-prison cell Mike Snitzer 2012-04-12 22:34 ` [PATCH 2/2] dm thin: use slab_pool for caches Mike Snitzer @ 2012-04-23 7:24 ` Joe Thornber 2012-04-23 10:02 ` Alasdair G Kergon 1 sibling, 1 reply; 6+ messages in thread From: Joe Thornber @ 2012-04-23 7:24 UTC (permalink / raw) To: Mike Snitzer; +Cc: dm-devel, ejt On Thu, Apr 12, 2012 at 06:34:34PM -0400, Mike Snitzer wrote: > Add missing mempool_free() to __cell_release_singleton(). > > This is a pretty significant leak that will accumulate to over 2GB of > leaked memory just from running the full thinp-test-suite. When was this introduced? With agk's tweaks for 3.4? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] dm thin: fix memory leak of singleton bio-prison cell 2012-04-23 7:24 ` [PATCH 1/2] dm thin: fix memory leak of singleton bio-prison cell Joe Thornber @ 2012-04-23 10:02 ` Alasdair G Kergon 2012-04-23 9:26 ` Joe Thornber 0 siblings, 1 reply; 6+ messages in thread From: Alasdair G Kergon @ 2012-04-23 10:02 UTC (permalink / raw) To: ejt; +Cc: dm-devel On Mon, Apr 23, 2012 at 08:24:08AM +0100, Joe Thornber wrote: > On Thu, Apr 12, 2012 at 06:34:34PM -0400, Mike Snitzer wrote: > > Add missing mempool_free() to __cell_release_singleton(). > > > > This is a pretty significant leak that will accumulate to over 2GB of > > leaked memory just from running the full thinp-test-suite. > When was this introduced? With agk's tweaks for 3.4? I think it was here: http://www.redhat.com/archives/dm-devel/2012-March/msg00080.html Localised fix: http://people.redhat.com/agk/patches/linux/editing/dm-thin-reinstate-missing-mempool_free-in-cell_release_singleton.patch Alasdair ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] dm thin: fix memory leak of singleton bio-prison cell 2012-04-23 10:02 ` Alasdair G Kergon @ 2012-04-23 9:26 ` Joe Thornber 0 siblings, 0 replies; 6+ messages in thread From: Joe Thornber @ 2012-04-23 9:26 UTC (permalink / raw) To: ejt, dm-devel On Mon, Apr 23, 2012 at 11:02:46AM +0100, Alasdair G Kergon wrote: > On Mon, Apr 23, 2012 at 08:24:08AM +0100, Joe Thornber wrote: > > On Thu, Apr 12, 2012 at 06:34:34PM -0400, Mike Snitzer wrote: > > > Add missing mempool_free() to __cell_release_singleton(). > > > > > > This is a pretty significant leak that will accumulate to over 2GB of > > > leaked memory just from running the full thinp-test-suite. > > When was this introduced? With agk's tweaks for 3.4? > > I think it was here: > http://www.redhat.com/archives/dm-devel/2012-March/msg00080.html Yep, this should call mempool_release: @@ -305,22 +310,45 @@ static void cell_release(struct cell *cell, struct bio_list *bios) * bio may be in the cell. This function releases the cell, and also does * a sanity check. */ +static void __cell_release_singleton(struct cell *cell, struct bio *bio) +{ + hlist_del(&cell->list); + BUG_ON(cell->holder != bio); + BUG_ON(!bio_list_empty(&cell->bios)); +} + ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-04-23 10:02 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-04-12 22:34 [PATCH 1/2] dm thin: fix memory leak of singleton bio-prison cell Mike Snitzer 2012-04-12 22:34 ` [PATCH 2/2] dm thin: use slab_pool for caches Mike Snitzer 2012-04-12 22:39 ` [PATCH 2/2 v2] dm thin: use slab mempools with local caches Mike Snitzer 2012-04-23 7:24 ` [PATCH 1/2] dm thin: fix memory leak of singleton bio-prison cell Joe Thornber 2012-04-23 10:02 ` Alasdair G Kergon 2012-04-23 9:26 ` Joe Thornber
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.