Bisected: Massive memory leak in dm-snapshot in 3.1 development introduced

PATCH New Michael Leun 4 participants 6 messages Oct 23, 2011 – Oct 25, 2011

Subsystems (4)

Memory management Library code Device mapper Slab allocator

lore

Share & tools

lore

Michael Leun

author

2011-10-23 09:21 UTC #0 lore raw

Hi,

How to reproduce:

lvcreate -L30G -n testsnap vg1 # of course substitute VG as appropriate dd if=/dev/zero of=/dev/vg1/testsnap bs=2M # to make things clear lvcreate -L15G -s /dev/vg1/test -n testsnap
dd if=/dev/zero of=/dev/vg1/testsnap bs=2M &
watch free

I noticed roughly 1GB memory vanishing from free / -/+ buffers/cache per 2GB copied.

Bisecting yielded a6e50b409d3f9e0833e69c3c9cca822e8fa4adbb, reverting that from git master cures the memory leak

Signature · 3 lines

MfG,

Michael Leun

Linus Torvalds

2011-10-23 09:52 UTC #1 lore raw

Making sure that Alasdair sees this too (hopefully he picked up on it from dm-devel, but best send things directly too)

Mikulas, Alasdair? I see the mempool_free() for the "master job", what about everything else? Does the dm_kcopyd_prepare_callback() perhaps need to do a

    job->master_job = job;

or similar?

Linus

Quoted 22 lines · Michael Leun

Hi,
How to reproduce:
lvcreate -L30G -n testsnap vg1 # of course substitute VG as appropriate dd if=/dev/zero of=/dev/vg1/testsnap bs=2M # to make things clear lvcreate -L15G -s /dev/vg1/test -n testsnap
dd if=/dev/zero of=/dev/vg1/testsnap bs=2M &
watch free
I noticed roughly 1GB memory vanishing from free / -/+ buffers/cache per 2GB copied.
Bisecting yielded a6e50b409d3f9e0833e69c3c9cca822e8fa4adbb, reverting that from git master cures the memory leak
--
MfG,
Michael Leun

Alasdair G Kergon

maintainer

2011-10-23 13:44 UTC #2 lore raw

Quoted 7 lines · Linus Torvalds

Mikulas, Alasdair? I see the mempool_free() for the "master job", what about everything else? Does the dm_kcopyd_prepare_callback() perhaps need to do a
    job->master_job = job;
or similar?

If you look at the other place that performs that mempool_alloc, it's indeed present there.

Michael - can you test if this solves the problem?

Alasdair

drivers/md/dm-kcopyd.c +1 -0

@@ -628,6 +628,7 @@ void *dm_kcopyd_prepare_callback(struct dm_kcopyd_client *kc,
 	job->kc = kc;
 	job->fn = fn;
 	job->context = context;
+	job->master_job = job;
 
 	atomic_inc(&kc->nr_jobs);

Michael Leun

author

2011-10-23 14:30 UTC #3 lore raw

Quoted 20 lines · On Sun, 23 Oct 2011 14:44:49 +0100 Alasdair G Kergon <[email protected]> wrote:

Quoted 7 lines · Linus Torvalds
Mikulas, Alasdair? I see the mempool_free() for the "master job", what about everything else? Does the dm_kcopyd_prepare_callback() perhaps need to do a
    job->master_job = job;
or similar?
If you look at the other place that performs that mempool_alloc, it's indeed present there.
Michael - can you test if this solves the problem?
Alasdair

drivers/md/dm-kcopyd.c void *dm_kcopyd_prepare_callback(struct · line 634

Quoted 7 lines

drivers/md/dm-kcopyd.c +1 -0

@@ -628,6 +628,7 @@ void *dm_kcopyd_prepare_callback(struct
 dm_kcopyd_client *kc, job->kc = kc;
 	job->fn = fn;
 	job->context = context;
+	job->master_job = job;
 
 	atomic_inc(&kc->nr_jobs);

Yup, it indeed does.

Signature · 3 lines

MfG,

Michael Leun

Alasdair G Kergon

maintainer

2011-10-23 20:19 UTC #4 lore raw

Quoted 3 lines · Michael Leun

Quoted 1 line · On Sun, 23 Oct 2011 14:44:49 +0100 Alasdair G Kergon <[email protected]> wrote:
Michael - can you test if this solves the problem?

OK, let's go with this patch then.

Linus, you can pull it from here:

http://people.redhat.com/agk/git/linux-dm for-linus

Alasdair G Kergon (1):
dm kcopyd: fix job_pool leak

 drivers/md/dm-kcopyd.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

Mikulas Patocka

maintainer

2011-10-25 17:45 UTC #5 lore raw

PATCH 1/1 check for mempool memory leaks

In reply to Linus Torvalds

Quoted 7 lines · Linus Torvalds

Making sure that Alasdair sees this too (hopefully he picked up on it from dm-devel, but best send things directly too)
Mikulas, Alasdair? I see the mempool_free() for the "master job", what about everything else? Does the dm_kcopyd_prepare_callback() perhaps need to do a

Quoted 30 lines

    job->master_job = job;
or similar?
Linus
Quoted 22 lines · Michael Leun
Hi,
How to reproduce:
lvcreate -L30G -n testsnap vg1 # of course substitute VG as appropriate dd if=/dev/zero of=/dev/vg1/testsnap bs=2M # to make things clear lvcreate -L15G -s /dev/vg1/test -n testsnap
dd if=/dev/zero of=/dev/vg1/testsnap bs=2M &
watch free
I noticed roughly 1GB memory vanishing from free / -/+ buffers/cache per 2GB copied.
Bisecting yielded a6e50b409d3f9e0833e69c3c9cca822e8fa4adbb, reverting that from git master cures the memory leak
--
MfG,
Michael Leun

BTW. would you like to accept this patch that enables tracking the number of objects allocated in a mempool and reporting a warning? So that such bugs could be automatically detected in the future?

---

Introduce an option DEBUG_MEMPOOL. It will check that a mempool is empty when destroying the mempool.

Signed-off-by Mikulas Patocka <[email protected]>

---
 include/linux/mempool.h |    6 ++++++
 lib/Kconfig.debug       |    8 ++++++++
 mm/mempool.c            |   25 +++++++++++++++++++++++--
 3 files changed, 37 insertions(+), 2 deletions(-)

lib/Kconfig.debug +8 -0

Index: linux-3.1-fast/lib/Kconfig.debug
===================================================================
@@ -411,6 +411,14 @@ config SLUB_STATS
 	  out which slabs are relevant to a particular load.
 	  Try running: slabinfo -DA
 
+config DEBUG_MEMPOOL
+	bool "Check memory leaks in mempools"
+	depends on DEBUG_KERNEL
+	help
+	  Enable debugging memory leaks in mempools. This options makes
+	  the kernel count the number of objects allocated in a mempool
+	  and check if this number is zero when the mempool is destroyed.
+
 config DEBUG_KMEMLEAK
 	bool "Kernel memory leak detector"
 	depends on DEBUG_KERNEL && EXPERIMENTAL && !MEMORY_HOTPLUG && \

mm/mempool.c +23 -2

Index: linux-3.1-fast/mm/mempool.c
===================================================================
@@ -71,6 +71,9 @@ mempool_t *mempool_create_node(int min_n
 		kfree(pool);
 		return NULL;
 	}
+#ifdef CONFIG_DEBUG_MEMPOOL
+	atomic_long_set(&pool->nr_allocated, 0);
+#endif
 	spin_lock_init(&pool->lock);
 	pool->min_nr = min_nr;
 	pool->pool_data = pool_data;

@@ -183,7 +186,13 @@ EXPORT_SYMBOL(mempool_resize);
 void mempool_destroy(mempool_t *pool)
 {
 	/* Check for outstanding elements */
-	BUG_ON(pool->curr_nr != pool->min_nr);
+#ifdef CONFIG_DEBUG_MEMPOOL
+	WARN(atomic_long_read(&pool->nr_allocated),
+	     "%ld objects leaked in mempool",
+	     atomic_long_read(&pool->nr_allocated));
+#else
+	WARN_ON(pool->curr_nr != pool->min_nr);
+#endif
 	free_pool(pool);
 }
 EXPORT_SYMBOL(mempool_destroy);

@@ -217,13 +226,20 @@ void * mempool_alloc(mempool_t *pool, gf
 repeat_alloc:
 
 	element = pool->alloc(gfp_temp, pool->pool_data);
-	if (likely(element != NULL))
+	if (likely(element != NULL)) {
+#ifdef CONFIG_DEBUG_MEMPOOL
+		atomic_long_inc(&pool->nr_allocated);
+#endif
 		return element;
+	}
 
 	spin_lock_irqsave(&pool->lock, flags);
 	if (likely(pool->curr_nr)) {
 		element = remove_element(pool);
 		spin_unlock_irqrestore(&pool->lock, flags);
+#ifdef CONFIG_DEBUG_MEMPOOL
+		atomic_long_inc(&pool->nr_allocated);
+#endif
 		return element;
 	}
 	spin_unlock_irqrestore(&pool->lock, flags);

@@ -265,6 +281,11 @@ void mempool_free(void *element, mempool
 	if (unlikely(element == NULL))
 		return;
 
+#ifdef CONFIG_DEBUG_MEMPOOL
+	BUG_ON(!atomic_long_read(&pool->nr_allocated));
+	atomic_long_dec(&pool->nr_allocated);
+#endif
+
 	smp_mb();
 	if (pool->curr_nr < pool->min_nr) {
 		spin_lock_irqsave(&pool->lock, flags);

include/linux/mempool.h +6 -0

Index: linux-3.1-fast/include/linux/mempool.h
===================================================================
@@ -5,6 +5,9 @@
 #define _LINUX_MEMPOOL_H
 
 #include <linux/wait.h>
+#ifdef CONFIG_DEBUG_MEMPOOLS
+#include <linux/atomic.h>
+#endif
 
 struct kmem_cache;

@@ -12,6 +15,9 @@ typedef void * (mempool_alloc_t)(gfp_t g
 typedef void (mempool_free_t)(void *element, void *pool_data);
 
 typedef struct mempool_s {
+#ifdef CONFIG_DEBUG_MEMPOOL
+	atomic_long_t nr_allocated;
+#endif
 	spinlock_t lock;
 	int min_nr;		/* nr of elements at *elements */
 	int curr_nr;		/* Current nr of elements at *elements */