* [PATCH 1/2] Btrfs: add leak debug for extent map
@ 2013-01-08 14:49 Liu Bo
2013-01-08 14:49 ` [PATCH 2/2] Btrfs: fix memory leak on extent map after fsync Liu Bo
2013-01-08 20:07 ` [PATCH 1/2] Btrfs: add leak debug for extent map Zach Brown
0 siblings, 2 replies; 15+ messages in thread
From: Liu Bo @ 2013-01-08 14:49 UTC (permalink / raw)
To: linux-btrfs
This is for detecting extent map leak.
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
fs/btrfs/extent_map.c | 31 +++++++++++++++++++++++++++++++
fs/btrfs/extent_map.h | 1 +
2 files changed, 32 insertions(+), 0 deletions(-)
diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index f169d6b..c025a7a 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -9,6 +9,13 @@
static struct kmem_cache *extent_map_cache;
+static LIST_HEAD(emaps);
+
+#define LEAK_DEBUG 0
+#if LEAK_DEBUG
+static DEFINE_SPINLOCK(map_leak_lock);
+#endif
+
int __init extent_map_init(void)
{
extent_map_cache = kmem_cache_create("btrfs_extent_map",
@@ -21,6 +28,16 @@ int __init extent_map_init(void)
void extent_map_exit(void)
{
+ struct extent_map *em;
+
+ while (!list_empty(&emaps)) {
+ em = list_entry(emaps.next, struct extent_map, leak_list);
+ printk(KERN_ERR "btrfs ext map leak: start %llu len %llu block %llu flags %llu refs %d in tree %d compress %d\n",
+ em->start, em->len, em->block_start, em->flags, atomic_read(&em->refs), em->in_tree, em->compress_type);
+ list_del(&em->leak_list);
+ kmem_cache_free(extent_map_cache, em);
+ }
+
if (extent_map_cache)
kmem_cache_destroy(extent_map_cache);
}
@@ -48,6 +65,9 @@ void extent_map_tree_init(struct extent_map_tree *tree)
*/
struct extent_map *alloc_extent_map(void)
{
+#if LEAK_DEBUG
+ unsigned long flags;
+#endif
struct extent_map *em;
em = kmem_cache_zalloc(extent_map_cache, GFP_NOFS);
if (!em)
@@ -58,6 +78,11 @@ struct extent_map *alloc_extent_map(void)
em->generation = 0;
atomic_set(&em->refs, 1);
INIT_LIST_HEAD(&em->list);
+#if LEAK_DEBUG
+ spin_lock_irqsave(&map_leak_lock, flags);
+ list_add(&em->leak_list, &emaps);
+ spin_unlock_irqrestore(&map_leak_lock, flags);
+#endif
return em;
}
@@ -74,6 +99,12 @@ void free_extent_map(struct extent_map *em)
return;
WARN_ON(atomic_read(&em->refs) == 0);
if (atomic_dec_and_test(&em->refs)) {
+#if LEAK_DEBUG
+ unsigned long flags;
+ spin_lock_irqsave(&map_leak_lock, flags);
+ list_del(&em->leak_list);
+ spin_unlock_irqrestore(&map_leak_lock, flags);
+#endif
WARN_ON(em->in_tree);
WARN_ON(!list_empty(&em->list));
kmem_cache_free(extent_map_cache, em);
diff --git a/fs/btrfs/extent_map.h b/fs/btrfs/extent_map.h
index 922943c..d07a841 100644
--- a/fs/btrfs/extent_map.h
+++ b/fs/btrfs/extent_map.h
@@ -35,6 +35,7 @@ struct extent_map {
unsigned int in_tree;
unsigned int compress_type;
struct list_head list;
+ struct list_head leak_list;
};
struct extent_map_tree {
--
1.7.7.6
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] Btrfs: fix memory leak on extent map after fsync
2013-01-08 14:49 [PATCH 1/2] Btrfs: add leak debug for extent map Liu Bo
@ 2013-01-08 14:49 ` Liu Bo
2013-01-24 16:44 ` Josef Bacik
2013-01-24 16:52 ` Josef Bacik
2013-01-08 20:07 ` [PATCH 1/2] Btrfs: add leak debug for extent map Zach Brown
1 sibling, 2 replies; 15+ messages in thread
From: Liu Bo @ 2013-01-08 14:49 UTC (permalink / raw)
To: linux-btrfs
During fsync, we put the changed parts(i.e. extent map) into the log tree,
and we ship these parts from a list of modified_extents to a local list
to process, of course, we must increment the refs of the extent maps to
avoid it from getting evicted from cache.
The problem is
we don't hold the tree writer lock all the time of iterating the local list,
and it is possible that other threads hack in and delete the extent map from
the local list silently. So we'll end up with memory leak here.
I hit this when testing xfstest 274 with mount options 'autodefrag,compress=zlib'.
With this fix, the memory leak has gone away.
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
fs/btrfs/extent_map.c | 5 +++--
fs/btrfs/extent_map.h | 4 ++--
fs/btrfs/tree-log.c | 12 +++++-------
3 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index c025a7a..4c6d271 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -78,6 +78,7 @@ struct extent_map *alloc_extent_map(void)
em->generation = 0;
atomic_set(&em->refs, 1);
INIT_LIST_HEAD(&em->list);
+ INIT_LIST_HEAD(&em->log_list);
#if LEAK_DEBUG
spin_lock_irqsave(&map_leak_lock, flags);
list_add(&em->leak_list, &emaps);
@@ -107,6 +108,7 @@ void free_extent_map(struct extent_map *em)
#endif
WARN_ON(em->in_tree);
WARN_ON(!list_empty(&em->list));
+ WARN_ON(!list_empty(&em->log_list));
kmem_cache_free(extent_map_cache, em);
}
}
@@ -433,8 +435,7 @@ int remove_extent_mapping(struct extent_map_tree *tree, struct extent_map *em)
WARN_ON(test_bit(EXTENT_FLAG_PINNED, &em->flags));
rb_erase(&em->rb_node, &tree->map);
- if (!test_bit(EXTENT_FLAG_LOGGING, &em->flags))
- list_del_init(&em->list);
+ list_del_init(&em->list);
em->in_tree = 0;
return ret;
}
diff --git a/fs/btrfs/extent_map.h b/fs/btrfs/extent_map.h
index d07a841..ac12389 100644
--- a/fs/btrfs/extent_map.h
+++ b/fs/btrfs/extent_map.h
@@ -13,8 +13,7 @@
#define EXTENT_FLAG_COMPRESSED 1
#define EXTENT_FLAG_VACANCY 2 /* no file extent item found */
#define EXTENT_FLAG_PREALLOC 3 /* pre-allocated extent */
-#define EXTENT_FLAG_LOGGING 4 /* Logging this extent */
-#define EXTENT_FLAG_FILLING 5 /* Filling in a preallocated extent */
+#define EXTENT_FLAG_FILLING 4 /* Filling in a preallocated extent */
struct extent_map {
struct rb_node rb_node;
@@ -35,6 +34,7 @@ struct extent_map {
unsigned int in_tree;
unsigned int compress_type;
struct list_head list;
+ struct list_head log_list;
struct list_head leak_list;
};
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 83186c7..c3ea5bd 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3145,8 +3145,8 @@ static int extent_cmp(void *priv, struct list_head *a, struct list_head *b)
{
struct extent_map *em1, *em2;
- em1 = list_entry(a, struct extent_map, list);
- em2 = list_entry(b, struct extent_map, list);
+ em1 = list_entry(a, struct extent_map, log_list);
+ em2 = list_entry(b, struct extent_map, log_list);
if (em1->start < em2->start)
return -1;
@@ -3400,17 +3400,15 @@ static int btrfs_log_changed_extents(struct btrfs_trans_handle *trans,
continue;
/* Need a ref to keep it from getting evicted from cache */
atomic_inc(&em->refs);
- set_bit(EXTENT_FLAG_LOGGING, &em->flags);
- list_add_tail(&em->list, &extents);
+ list_add_tail(&em->log_list, &extents);
}
list_sort(NULL, &extents, extent_cmp);
while (!list_empty(&extents)) {
- em = list_entry(extents.next, struct extent_map, list);
+ em = list_entry(extents.next, struct extent_map, log_list);
- list_del_init(&em->list);
- clear_bit(EXTENT_FLAG_LOGGING, &em->flags);
+ list_del_init(&em->log_list);
/*
* If we had an error we just need to delete everybody from our
--
1.7.7.6
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Btrfs: add leak debug for extent map
2013-01-08 14:49 [PATCH 1/2] Btrfs: add leak debug for extent map Liu Bo
2013-01-08 14:49 ` [PATCH 2/2] Btrfs: fix memory leak on extent map after fsync Liu Bo
@ 2013-01-08 20:07 ` Zach Brown
2013-01-10 2:05 ` Liu Bo
1 sibling, 1 reply; 15+ messages in thread
From: Zach Brown @ 2013-01-08 20:07 UTC (permalink / raw)
To: Liu Bo; +Cc: linux-btrfs
> This is for detecting extent map leak.
Hmm, I guess it's cool to get the allocation-specific decoding which you
don't get from the generic kernel leak tracking?
> +static LIST_HEAD(emaps);
> + while (!list_empty(&emaps)) {
> + em = list_entry(emaps.next, struct extent_map, leak_list);
> + printk(KERN_ERR "btrfs ext map leak: start %llu len %llu block %llu flags %llu refs %d in tree %d compress %d\n",
> + em->start, em->len, em->block_start, em->flags, atomic_read(&em->refs), em->in_tree, em->compress_type);
> + list_del(&em->leak_list);
> + kmem_cache_free(extent_map_cache, em);
> + struct list_head leak_list;
Might as well protect all that with ifdefs, too, if you're going to do
it that way?
- z
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Btrfs: add leak debug for extent map
2013-01-08 20:07 ` [PATCH 1/2] Btrfs: add leak debug for extent map Zach Brown
@ 2013-01-10 2:05 ` Liu Bo
2013-01-10 11:54 ` David Sterba
2013-01-10 17:06 ` Zach Brown
0 siblings, 2 replies; 15+ messages in thread
From: Liu Bo @ 2013-01-10 2:05 UTC (permalink / raw)
To: Zach Brown; +Cc: linux-btrfs
On Tue, Jan 08, 2013 at 12:07:34PM -0800, Zach Brown wrote:
> > This is for detecting extent map leak.
>
> Hmm, I guess it's cool to get the allocation-specific decoding which you
> don't get from the generic kernel leak tracking?
Hi Zach,
Thanks for the advice, but what allocation-specific decoding do you refer to?
Could you please show me any examples?
>
> > +static LIST_HEAD(emaps);
>
> > + while (!list_empty(&emaps)) {
> > + em = list_entry(emaps.next, struct extent_map, leak_list);
> > + printk(KERN_ERR "btrfs ext map leak: start %llu len %llu block %llu flags %llu refs %d in tree %d compress %d\n",
> > + em->start, em->len, em->block_start, em->flags, atomic_read(&em->refs), em->in_tree, em->compress_type);
> > + list_del(&em->leak_list);
> > + kmem_cache_free(extent_map_cache, em);
>
> > + struct list_head leak_list;
>
> Might as well protect all that with ifdefs, too, if you're going to do
> it that way?
All right, I'm happy to do that.
Thanks,
liubo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Btrfs: add leak debug for extent map
2013-01-10 2:05 ` Liu Bo
@ 2013-01-10 11:54 ` David Sterba
2013-01-10 13:11 ` Liu Bo
2013-01-10 17:06 ` Zach Brown
1 sibling, 1 reply; 15+ messages in thread
From: David Sterba @ 2013-01-10 11:54 UTC (permalink / raw)
To: Liu Bo; +Cc: Zach Brown, linux-btrfs
On Thu, Jan 10, 2013 at 10:05:39AM +0800, Liu Bo wrote:
> On Tue, Jan 08, 2013 at 12:07:34PM -0800, Zach Brown wrote:
> > > This is for detecting extent map leak.
> >
> > Hmm, I guess it's cool to get the allocation-specific decoding which you
> > don't get from the generic kernel leak tracking?
>
> Thanks for the advice, but what allocation-specific decoding do you refer to?
> Could you please show me any examples?
IMHO that there's a leak check that is targeted to one exact problem in
one subsystem (extent_map in btrfs), does not need to be poked to do a
scan-for-leaks so the leak can be reported immediatelly and not after
some time. It makes sense for such a core structure like extent_map.
Other structures are allocated from a slab so we can at least check for
leaks upon module unload.
david
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Btrfs: add leak debug for extent map
2013-01-10 11:54 ` David Sterba
@ 2013-01-10 13:11 ` Liu Bo
2013-01-11 15:31 ` David Sterba
0 siblings, 1 reply; 15+ messages in thread
From: Liu Bo @ 2013-01-10 13:11 UTC (permalink / raw)
To: David Sterba; +Cc: Zach Brown, linux-btrfs
On Thu, Jan 10, 2013 at 12:54:26PM +0100, David Sterba wrote:
> On Thu, Jan 10, 2013 at 10:05:39AM +0800, Liu Bo wrote:
> > On Tue, Jan 08, 2013 at 12:07:34PM -0800, Zach Brown wrote:
> > > > This is for detecting extent map leak.
> > >
> > > Hmm, I guess it's cool to get the allocation-specific decoding which you
> > > don't get from the generic kernel leak tracking?
> >
> > Thanks for the advice, but what allocation-specific decoding do you refer to?
> > Could you please show me any examples?
>
> IMHO that there's a leak check that is targeted to one exact problem in
> one subsystem (extent_map in btrfs), does not need to be poked to do a
> scan-for-leaks so the leak can be reported immediatelly and not after
> some time. It makes sense for such a core structure like extent_map.
> Other structures are allocated from a slab so we can at least check for
> leaks upon module unload.
Sorry, I don't get your point, but extent map is allocated from its
slab section as well.
The 'scan-for-leaks' is just for developers' debug purpose, which
can tell us some information about the leaked ones, like refs, type, etc.
I think I'm doing the same thing as leak debug for extent_state/extent_buffer.
We can disable it as default.
thanks,
liubo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Btrfs: add leak debug for extent map
2013-01-10 2:05 ` Liu Bo
2013-01-10 11:54 ` David Sterba
@ 2013-01-10 17:06 ` Zach Brown
2013-01-11 8:45 ` Liu Bo
1 sibling, 1 reply; 15+ messages in thread
From: Zach Brown @ 2013-01-10 17:06 UTC (permalink / raw)
To: Liu Bo; +Cc: linux-btrfs
> > Hmm, I guess it's cool to get the allocation-specific decoding which you
> > don't get from the generic kernel leak tracking?
I mean that by doing this in btrfs, instead of doing it generically in
the allocator, you get specific knowledge that btrfs knows about the
allocated objects:
> > > + printk(KERN_ERR "btrfs ext map leak: start %llu len %llu block %llu flags %llu refs %d in tree %d compress %d\n",
> > > + em->start, em->len, em->block_start, em->flags, atomic_read(&em->refs), em->in_tree, em->compress_type);
That's valuable. I understand that it's quick and easy to implement
this in btrfs. It's hard to argue with working code.
But the right way to do this would be to add a callback that
kmem_cache_destroy() can use to generate debugging output for the
allocated objects. Maybe you have a registration function that sets the
callback on the slab? Slab already has tracking of allocated objects so
you could always have this leak output on without runtime overhead.
And, of course, other callers can easy also get this functionality
instead of having to mess around with all the stuff btrfs did: ifdefs,
locks, and lists.
- z
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Btrfs: add leak debug for extent map
2013-01-10 17:06 ` Zach Brown
@ 2013-01-11 8:45 ` Liu Bo
2013-01-11 20:54 ` Zach Brown
0 siblings, 1 reply; 15+ messages in thread
From: Liu Bo @ 2013-01-11 8:45 UTC (permalink / raw)
To: Zach Brown; +Cc: linux-btrfs
On Thu, Jan 10, 2013 at 09:06:34AM -0800, Zach Brown wrote:
> > > Hmm, I guess it's cool to get the allocation-specific decoding which you
> > > don't get from the generic kernel leak tracking?
>
> I mean that by doing this in btrfs, instead of doing it generically in
> the allocator, you get specific knowledge that btrfs knows about the
> allocated objects:
>
> > > > + printk(KERN_ERR "btrfs ext map leak: start %llu len %llu block %llu flags %llu refs %d in tree %d compress %d\n",
> > > > + em->start, em->len, em->block_start, em->flags, atomic_read(&em->refs), em->in_tree, em->compress_type);
>
> That's valuable. I understand that it's quick and easy to implement
> this in btrfs. It's hard to argue with working code.
>
> But the right way to do this would be to add a callback that
> kmem_cache_destroy() can use to generate debugging output for the
> allocated objects. Maybe you have a registration function that sets the
> callback on the slab? Slab already has tracking of allocated objects so
> you could always have this leak output on without runtime overhead.
>
> And, of course, other callers can easy also get this functionality
> instead of having to mess around with all the stuff btrfs did: ifdefs,
> locks, and lists.
>
> - z
Yeah, adding a callback here is really a more graceful way!
But after flipping slab code, I find that another callback will disable
merging slabs when allocating a slab, so I'm not sure if it worth doing so...
What do you think about it?
thanks,
liubo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Btrfs: add leak debug for extent map
2013-01-10 13:11 ` Liu Bo
@ 2013-01-11 15:31 ` David Sterba
0 siblings, 0 replies; 15+ messages in thread
From: David Sterba @ 2013-01-11 15:31 UTC (permalink / raw)
To: Liu Bo; +Cc: David Sterba, Zach Brown, linux-btrfs
On Thu, Jan 10, 2013 at 09:11:32PM +0800, Liu Bo wrote:
> On Thu, Jan 10, 2013 at 12:54:26PM +0100, David Sterba wrote:
> > On Thu, Jan 10, 2013 at 10:05:39AM +0800, Liu Bo wrote:
> > > On Tue, Jan 08, 2013 at 12:07:34PM -0800, Zach Brown wrote:
> > > > > This is for detecting extent map leak.
> > > >
> > > > Hmm, I guess it's cool to get the allocation-specific decoding which you
> > > > don't get from the generic kernel leak tracking?
> > >
> > > Thanks for the advice, but what allocation-specific decoding do you refer to?
> > > Could you please show me any examples?
> >
> > IMHO that there's a leak check that is targeted to one exact problem in
> > one subsystem (extent_map in btrfs), does not need to be poked to do a
> > scan-for-leaks so the leak can be reported immediatelly and not after
> > some time. It makes sense for such a core structure like extent_map.
> > Other structures are allocated from a slab so we can at least check for
> > leaks upon module unload.
>
> Sorry, I don't get your point, but extent map is allocated from its
> slab section as well.
>
> The 'scan-for-leaks' is just for developers' debug purpose, which
> can tell us some information about the leaked ones, like refs, type, etc.
Yeah, and it's a good thing.
> I think I'm doing the same thing as leak debug for extent_state/extent_buffer.
> We can disable it as default.
We've now gathered several debugging helpers so I'll resend the patch to
add CONFIG_BTRFS_DEBUG and we can put such things under that. Zach
already explained his concerns, mine was a bit off-track sorry :)
david
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Btrfs: add leak debug for extent map
2013-01-11 8:45 ` Liu Bo
@ 2013-01-11 20:54 ` Zach Brown
2013-01-13 12:18 ` Liu Bo
0 siblings, 1 reply; 15+ messages in thread
From: Zach Brown @ 2013-01-11 20:54 UTC (permalink / raw)
To: Liu Bo; +Cc: linux-btrfs
> But after flipping slab code, I find that another callback will disable
> merging slabs when allocating a slab, so I'm not sure if it worth doing so...
Do you mean the find_mergeable() stuff in SLUB?
> What do you think about it?
I don't know, pass in a callback to destruction?
void kmem_cache_destroy_inuse_cb(struct kmem_cache *s,
void (*objcb)(void *));
I'd try to spend as little time on this as possible. Get the most basic
thing working to demonstrate the idea and send it to lkml to get
feedback.
- z
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Btrfs: add leak debug for extent map
2013-01-11 20:54 ` Zach Brown
@ 2013-01-13 12:18 ` Liu Bo
0 siblings, 0 replies; 15+ messages in thread
From: Liu Bo @ 2013-01-13 12:18 UTC (permalink / raw)
To: Zach Brown; +Cc: linux-btrfs
On Fri, Jan 11, 2013 at 12:54:32PM -0800, Zach Brown wrote:
> > But after flipping slab code, I find that another callback will disable
> > merging slabs when allocating a slab, so I'm not sure if it worth doing so...
>
> Do you mean the find_mergeable() stuff in SLUB?
Yes, that's what I'm worried about.
>
> > What do you think about it?
>
> I don't know, pass in a callback to destruction?
>
> void kmem_cache_destroy_inuse_cb(struct kmem_cache *s,
> void (*objcb)(void *));
>
> I'd try to spend as little time on this as possible. Get the most basic
> thing working to demonstrate the idea and send it to lkml to get
> feedback.
>
> - z
Okay, I'll send a RFC, and thanks for the suggestion :)
thanks,
liubo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] Btrfs: fix memory leak on extent map after fsync
2013-01-08 14:49 ` [PATCH 2/2] Btrfs: fix memory leak on extent map after fsync Liu Bo
@ 2013-01-24 16:44 ` Josef Bacik
2013-01-25 1:38 ` Liu Bo
2013-01-24 16:52 ` Josef Bacik
1 sibling, 1 reply; 15+ messages in thread
From: Josef Bacik @ 2013-01-24 16:44 UTC (permalink / raw)
To: Liu Bo; +Cc: linux-btrfs@vger.kernel.org
On Tue, Jan 08, 2013 at 07:49:21AM -0700, Liu Bo wrote:
> During fsync, we put the changed parts(i.e. extent map) into the log tree,
> and we ship these parts from a list of modified_extents to a local list
> to process, of course, we must increment the refs of the extent maps to
> avoid it from getting evicted from cache.
>
> The problem is
> we don't hold the tree writer lock all the time of iterating the local list,
> and it is possible that other threads hack in and delete the extent map from
> the local list silently. So we'll end up with memory leak here.
>
> I hit this when testing xfstest 274 with mount options 'autodefrag,compress=zlib'.
>
> With this fix, the memory leak has gone away.
This isn't going to work, we use the LOGGING flag to make sure the em isn't
merged as well. Thanks,
Josef
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] Btrfs: fix memory leak on extent map after fsync
2013-01-08 14:49 ` [PATCH 2/2] Btrfs: fix memory leak on extent map after fsync Liu Bo
2013-01-24 16:44 ` Josef Bacik
@ 2013-01-24 16:52 ` Josef Bacik
1 sibling, 0 replies; 15+ messages in thread
From: Josef Bacik @ 2013-01-24 16:52 UTC (permalink / raw)
To: Liu Bo; +Cc: linux-btrfs@vger.kernel.org
On Tue, Jan 08, 2013 at 07:49:21AM -0700, Liu Bo wrote:
> During fsync, we put the changed parts(i.e. extent map) into the log tree,
> and we ship these parts from a list of modified_extents to a local list
> to process, of course, we must increment the refs of the extent maps to
> avoid it from getting evicted from cache.
>
> The problem is
> we don't hold the tree writer lock all the time of iterating the local list,
> and it is possible that other threads hack in and delete the extent map from
> the local list silently. So we'll end up with memory leak here.
>
> I hit this when testing xfstest 274 with mount options 'autodefrag,compress=zlib'.
>
> With this fix, the memory leak has gone away.
>
And actually I fixed this in one of my other fsync patches that didn't get
pulled in yet, so I'll break it out of that patch and we can send that along.
Thanks,
Josef
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] Btrfs: fix memory leak on extent map after fsync
2013-01-24 16:44 ` Josef Bacik
@ 2013-01-25 1:38 ` Liu Bo
2013-01-25 2:23 ` Liu Bo
0 siblings, 1 reply; 15+ messages in thread
From: Liu Bo @ 2013-01-25 1:38 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs@vger.kernel.org
On Thu, Jan 24, 2013 at 11:44:33AM -0500, Josef Bacik wrote:
> On Tue, Jan 08, 2013 at 07:49:21AM -0700, Liu Bo wrote:
> > During fsync, we put the changed parts(i.e. extent map) into the log tree,
> > and we ship these parts from a list of modified_extents to a local list
> > to process, of course, we must increment the refs of the extent maps to
> > avoid it from getting evicted from cache.
> >
> > The problem is
> > we don't hold the tree writer lock all the time of iterating the local list,
> > and it is possible that other threads hack in and delete the extent map from
> > the local list silently. So we'll end up with memory leak here.
> >
> > I hit this when testing xfstest 274 with mount options 'autodefrag,compress=zlib'.
> >
> > With this fix, the memory leak has gone away.
>
> This isn't going to work, we use the LOGGING flag to make sure the em isn't
> merged as well. Thanks,
A quick grep shows,
1 16 fs/btrfs/extent_map.h <<GLOBAL>>
#define EXTENT_FLAG_LOGGING 4
2 406 fs/btrfs/extent_map.c <<remove_extent_mapping>>
if (!test_bit(EXTENT_FLAG_LOGGING, &em->flags))
3 3403 fs/btrfs/tree-log.c <<btrfs_log_changed_extents>>
set_bit(EXTENT_FLAG_LOGGING, &em->flags);
4 3413 fs/btrfs/tree-log.c <<btrfs_log_changed_extents>>
clear_bit(EXTENT_FLAG_LOGGING, &em->flags);
how does the flag avoid merging em?
Seems we lost the check.
static int mergable_maps(struct extent_map *prev, struct extent_map *next)
{
if (test_bit(EXTENT_FLAG_PINNED, &prev->flags))
return 0;
/*
* don't merge compressed extents, we need to know their
* actual size
*/
if (test_bit(EXTENT_FLAG_COMPRESSED, &prev->flags))
return 0;
...
}
thanks,
liubo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] Btrfs: fix memory leak on extent map after fsync
2013-01-25 1:38 ` Liu Bo
@ 2013-01-25 2:23 ` Liu Bo
0 siblings, 0 replies; 15+ messages in thread
From: Liu Bo @ 2013-01-25 2:23 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs@vger.kernel.org
On Fri, Jan 25, 2013 at 09:38:06AM +0800, Liu Bo wrote:
> On Thu, Jan 24, 2013 at 11:44:33AM -0500, Josef Bacik wrote:
> > On Tue, Jan 08, 2013 at 07:49:21AM -0700, Liu Bo wrote:
> > > During fsync, we put the changed parts(i.e. extent map) into the log tree,
> > > and we ship these parts from a list of modified_extents to a local list
> > > to process, of course, we must increment the refs of the extent maps to
> > > avoid it from getting evicted from cache.
> > >
> > > The problem is
> > > we don't hold the tree writer lock all the time of iterating the local list,
> > > and it is possible that other threads hack in and delete the extent map from
> > > the local list silently. So we'll end up with memory leak here.
> > >
> > > I hit this when testing xfstest 274 with mount options 'autodefrag,compress=zlib'.
> > >
> > > With this fix, the memory leak has gone away.
> >
> > This isn't going to work, we use the LOGGING flag to make sure the em isn't
> > merged as well. Thanks,
Well, never mind, I've seen the fix in your btrfs-next. But it'd be
better if you can also send it to the list where everyone can review it
easily.
thanks,
liubo
>
> A quick grep shows,
>
> 1 16 fs/btrfs/extent_map.h <<GLOBAL>>
> #define EXTENT_FLAG_LOGGING 4
> 2 406 fs/btrfs/extent_map.c <<remove_extent_mapping>>
> if (!test_bit(EXTENT_FLAG_LOGGING, &em->flags))
> 3 3403 fs/btrfs/tree-log.c <<btrfs_log_changed_extents>>
> set_bit(EXTENT_FLAG_LOGGING, &em->flags);
> 4 3413 fs/btrfs/tree-log.c <<btrfs_log_changed_extents>>
> clear_bit(EXTENT_FLAG_LOGGING, &em->flags);
>
> how does the flag avoid merging em?
>
> Seems we lost the check.
>
> static int mergable_maps(struct extent_map *prev, struct extent_map *next)
> {
> if (test_bit(EXTENT_FLAG_PINNED, &prev->flags))
> return 0;
>
> /*
> * don't merge compressed extents, we need to know their
> * actual size
> */
> if (test_bit(EXTENT_FLAG_COMPRESSED, &prev->flags))
> return 0;
>
> ...
> }
>
> thanks,
> liubo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 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] 15+ messages in thread
end of thread, other threads:[~2013-01-25 2:26 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-08 14:49 [PATCH 1/2] Btrfs: add leak debug for extent map Liu Bo
2013-01-08 14:49 ` [PATCH 2/2] Btrfs: fix memory leak on extent map after fsync Liu Bo
2013-01-24 16:44 ` Josef Bacik
2013-01-25 1:38 ` Liu Bo
2013-01-25 2:23 ` Liu Bo
2013-01-24 16:52 ` Josef Bacik
2013-01-08 20:07 ` [PATCH 1/2] Btrfs: add leak debug for extent map Zach Brown
2013-01-10 2:05 ` Liu Bo
2013-01-10 11:54 ` David Sterba
2013-01-10 13:11 ` Liu Bo
2013-01-11 15:31 ` David Sterba
2013-01-10 17:06 ` Zach Brown
2013-01-11 8:45 ` Liu Bo
2013-01-11 20:54 ` Zach Brown
2013-01-13 12:18 ` Liu Bo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).