| Message ID | 1357656561-24604-1-git-send-email-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> 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 -- 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
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 -- 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
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 -- 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
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 -- 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
> > 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 -- 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
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 -- 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
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 -- 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
> 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 -- 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
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 -- 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
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 {
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(-)