There was a time when sprinkling #ifdefs around was
bad form. ;) We can clean up the code a bit by moving
the leak-list adds/removes into functions and make things
more readable.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
p.s. maybe _debug_ should be in the function names?
*shrug*
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 223aa6a..c4f163c 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -30,6 +30,29 @@ static LIST_HEAD(states);
#define LEAK_DEBUG 0
#if LEAK_DEBUG
static DEFINE_SPINLOCK(leak_lock);
+
+static inline
+void btrfs_leak_list_add(struct list_head *new, struct list_head *head)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&leak_lock, flags);
+ list_add(new, head);
+ spin_unlock_irqrestore(&leak_lock, flags);
+}
+
+static inline
+void btrfs_leak_list_del(struct list_head *entry)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&leak_lock, flags);
+ list_del(entry);
+ spin_unlock_irqrestore(&leak_lock, flags);
+}
+#else
+#define btrfs_leak_list_add(new, head) do {} while (0);
+#define btrfs_leak_list_del(entry) do {} while (0);
#endif
#define BUFFER_LRU_MAX 64
@@ -96,7 +119,6 @@ void extent_io_exit(void)
state->state, state->tree, atomic_read(&state->refs));
list_del(&state->leak_list);
kmem_cache_free(extent_state_cache, state);
-
}
while (!list_empty(&buffers)) {
@@ -134,9 +156,6 @@ void extent_io_tree_init(struct extent_io_tree *tree,
static struct extent_state *alloc_extent_state(gfp_t mask)
{
struct extent_state *state;
-#if LEAK_DEBUG
- unsigned long flags;
-#endif
state = kmem_cache_alloc(extent_state_cache, mask);
if (!state)
@@ -144,11 +163,7 @@ static struct extent_state *alloc_extent_state(gfp_t mask)
state->state = 0;
state->private = 0;
state->tree = NULL;
-#if LEAK_DEBUG
- spin_lock_irqsave(&leak_lock, flags);
- list_add(&state->leak_list, &states);
- spin_unlock_irqrestore(&leak_lock, flags);
-#endif
+ btrfs_leak_list_add(&state->leak_list, &states);
atomic_set(&state->refs, 1);
init_waitqueue_head(&state->wq);
trace_alloc_extent_state(state, mask, _RET_IP_);
@@ -160,15 +175,8 @@ void free_extent_state(struct extent_state *state)
if (!state)
return;
if (atomic_dec_and_test(&state->refs)) {
-#if LEAK_DEBUG
- unsigned long flags;
-#endif
WARN_ON(state->tree);
-#if LEAK_DEBUG
- spin_lock_irqsave(&leak_lock, flags);
- list_del(&state->leak_list);
- spin_unlock_irqrestore(&leak_lock, flags);
-#endif
+ btrfs_leak_list_del(&state->leak_list);
trace_free_extent_state(state, _RET_IP_);
kmem_cache_free(extent_state_cache, state);
}
@@ -4010,12 +4018,7 @@ out:
static void __free_extent_buffer(struct extent_buffer *eb)
{
-#if LEAK_DEBUG
- unsigned long flags;
- spin_lock_irqsave(&leak_lock, flags);
- list_del(&eb->leak_list);
- spin_unlock_irqrestore(&leak_lock, flags);
-#endif
+ btrfs_leak_list_del(&eb->leak_list);
kmem_cache_free(extent_buffer_cache, eb);
}
@@ -4025,9 +4028,6 @@ static struct extent_buffer *__alloc_extent_buffer(struct
extent_io_tree *tree,
gfp_t mask)
{
struct extent_buffer *eb = NULL;
-#if LEAK_DEBUG
- unsigned long flags;
-#endif
eb = kmem_cache_zalloc(extent_buffer_cache, mask);
if (eb == NULL)
@@ -4047,11 +4047,8 @@ static struct extent_buffer *__alloc_extent_buffer(struct
extent_io_tree *tree,
init_waitqueue_head(&eb->write_lock_wq);
init_waitqueue_head(&eb->read_lock_wq);
-#if LEAK_DEBUG
- spin_lock_irqsave(&leak_lock, flags);
- list_add(&eb->leak_list, &buffers);
- spin_unlock_irqrestore(&leak_lock, flags);
-#endif
+ btrfs_leak_list_add(&eb->leak_list, &buffers);
+
spin_lock_init(&eb->refs_lock);
atomic_set(&eb->refs, 1);
atomic_set(&eb->io_pages, 0);
--
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
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 20/04/13 23:32, Eric Sandeen wrote:> +#define btrfs_leak_list_add(new, head) do {} while (0); +#define > btrfs_leak_list_del(entry) do {} while (0);Shouldn''t the trailing semi-colons be omitted? Roger -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iEYEARECAAYFAlF0uAsACgkQmOOfHg372QSuKACfSSd2ufXyZHzuD8xLhGhcBYnp 6iMAn3NbhUgzIxbrVxHDAiWMChQOPJUx =n9GF -----END PGP SIGNATURE----- -- 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
There was a time when sprinkling #ifdefs around was
bad form. We can clean up the code a bit by moving
the leak-list adds/removes into functions and make things
more readable.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
V2: remove extra semicolons on no-op #defines,
thanks Roger!
p.s. maybe _debug_ should be in the function names?
*shrug*
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 223aa6a..c4f163c 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -30,6 +30,29 @@ static LIST_HEAD(states);
#define LEAK_DEBUG 0
#if LEAK_DEBUG
static DEFINE_SPINLOCK(leak_lock);
+
+static inline
+void btrfs_leak_list_add(struct list_head *new, struct list_head *head)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&leak_lock, flags);
+ list_add(new, head);
+ spin_unlock_irqrestore(&leak_lock, flags);
+}
+
+static inline
+void btrfs_leak_list_del(struct list_head *entry)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&leak_lock, flags);
+ list_del(entry);
+ spin_unlock_irqrestore(&leak_lock, flags);
+}
+#else
+#define btrfs_leak_list_add(new, head) do {} while (0)
+#define btrfs_leak_list_del(entry) do {} while (0)
#endif
#define BUFFER_LRU_MAX 64
@@ -96,7 +119,6 @@ void extent_io_exit(void)
state->state, state->tree, atomic_read(&state->refs));
list_del(&state->leak_list);
kmem_cache_free(extent_state_cache, state);
-
}
while (!list_empty(&buffers)) {
@@ -134,9 +156,6 @@ void extent_io_tree_init(struct extent_io_tree *tree,
static struct extent_state *alloc_extent_state(gfp_t mask)
{
struct extent_state *state;
-#if LEAK_DEBUG
- unsigned long flags;
-#endif
state = kmem_cache_alloc(extent_state_cache, mask);
if (!state)
@@ -144,11 +163,7 @@ static struct extent_state *alloc_extent_state(gfp_t mask)
state->state = 0;
state->private = 0;
state->tree = NULL;
-#if LEAK_DEBUG
- spin_lock_irqsave(&leak_lock, flags);
- list_add(&state->leak_list, &states);
- spin_unlock_irqrestore(&leak_lock, flags);
-#endif
+ btrfs_leak_list_add(&state->leak_list, &states);
atomic_set(&state->refs, 1);
init_waitqueue_head(&state->wq);
trace_alloc_extent_state(state, mask, _RET_IP_);
@@ -160,15 +175,8 @@ void free_extent_state(struct extent_state *state)
if (!state)
return;
if (atomic_dec_and_test(&state->refs)) {
-#if LEAK_DEBUG
- unsigned long flags;
-#endif
WARN_ON(state->tree);
-#if LEAK_DEBUG
- spin_lock_irqsave(&leak_lock, flags);
- list_del(&state->leak_list);
- spin_unlock_irqrestore(&leak_lock, flags);
-#endif
+ btrfs_leak_list_del(&state->leak_list);
trace_free_extent_state(state, _RET_IP_);
kmem_cache_free(extent_state_cache, state);
}
@@ -4010,12 +4018,7 @@ out:
static void __free_extent_buffer(struct extent_buffer *eb)
{
-#if LEAK_DEBUG
- unsigned long flags;
- spin_lock_irqsave(&leak_lock, flags);
- list_del(&eb->leak_list);
- spin_unlock_irqrestore(&leak_lock, flags);
-#endif
+ btrfs_leak_list_del(&eb->leak_list);
kmem_cache_free(extent_buffer_cache, eb);
}
@@ -4025,9 +4028,6 @@ static struct extent_buffer *__alloc_extent_buffer(struct
extent_io_tree *tree,
gfp_t mask)
{
struct extent_buffer *eb = NULL;
-#if LEAK_DEBUG
- unsigned long flags;
-#endif
eb = kmem_cache_zalloc(extent_buffer_cache, mask);
if (eb == NULL)
@@ -4047,11 +4047,8 @@ static struct extent_buffer *__alloc_extent_buffer(struct
extent_io_tree *tree,
init_waitqueue_head(&eb->write_lock_wq);
init_waitqueue_head(&eb->read_lock_wq);
-#if LEAK_DEBUG
- spin_lock_irqsave(&leak_lock, flags);
- list_add(&eb->leak_list, &buffers);
- spin_unlock_irqrestore(&leak_lock, flags);
-#endif
+ btrfs_leak_list_add(&eb->leak_list, &buffers);
+
spin_lock_init(&eb->refs_lock);
atomic_set(&eb->refs, 1);
atomic_set(&eb->io_pages, 0);
--
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
David Sterba
2013-Apr-22 15:22 UTC
Re: [PATCH V2] btrfs: move leak debug code to functions
On Mon, Apr 22, 2013 at 12:18:37AM -0500, Eric Sandeen wrote:> There was a time when sprinkling #ifdefs around was > bad form. We can clean up the code a bit by moving > the leak-list adds/removes into functions and make things > more readable. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > V2: remove extra semicolons on no-op #defines, > thanks Roger! > > p.s. maybe _debug_ should be in the function names? > *shrug*I agree with that, makes things clear in the code, one does not need to look into that function. The leak_list members of extent_state and extent_buffer are present even without LEAK_DEBUG and are taking space, I''d like to see them moved under an #ifdef, but as the define is now local to the .c file this would need to add eg. a CONFIG_ option or a comment that describes how it should be used. Bug I''m fine with changing just the function names right now, the rest is optional and nice to have. Reviewed-by: David Sterba <dsterba@suse.cz> -- 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
Eric Sandeen
2013-Apr-22 15:27 UTC
Re: [PATCH V2] btrfs: move leak debug code to functions
On 4/22/13 10:22 AM, David Sterba wrote:> On Mon, Apr 22, 2013 at 12:18:37AM -0500, Eric Sandeen wrote: >> There was a time when sprinkling #ifdefs around was >> bad form. We can clean up the code a bit by moving >> the leak-list adds/removes into functions and make things >> more readable. >> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> >> V2: remove extra semicolons on no-op #defines, >> thanks Roger! >> >> p.s. maybe _debug_ should be in the function names? >> *shrug* > > I agree with that, makes things clear in the code, one does not need to > look into that function. > > The leak_list members of extent_state and extent_buffer are present even > without LEAK_DEBUG and are taking space, I''d like to see them moved > under an #ifdef, but as the define is now local to the .c file this > would need to add eg. a CONFIG_ option or a comment that describes how > it should be used.Yep I noticed that too, I''d be happy to elevate it to a CONFIG_ option if it''s really something that will be kept around for a while. Could maybe be lumped in to a CONFIG_BTRFS_DEBUG that catches other #define DEBUG behaviors as well? -Eric> Bug I''m fine with changing just the function names right now, the rest > is optional and nice to have. > > Reviewed-by: David Sterba <dsterba@suse.cz> >-- 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
David Sterba
2013-Apr-22 15:36 UTC
Re: [PATCH V2] btrfs: move leak debug code to functions
On Mon, Apr 22, 2013 at 10:27:11AM -0500, Eric Sandeen wrote:> Yep I noticed that too, I''d be happy to elevate it to a CONFIG_ option if > it''s really something that will be kept around for a while.We have (and will have) more code for debugging aid.> Could maybe be lumped in to a CONFIG_BTRFS_DEBUG that catches other > #define DEBUG behaviors as well?Yes please, feel free to use this https://patchwork.kernel.org/patch/846462/ If there are really heavy debug checks we may separate them, but a single option should be fine for now. thanks, 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
Clean up the leak debugging in extent_io.c by moving
the debug code into functions. This also removes the
list_heads used for debugging from the extent_buffer
and extent_state structures when debug is not enabled.
Since we need a global debug config to do that last
part, implement CONFIG_BTRFS_DEBUG to accommodate.
Thanks to Dave Sterba for the Kconfig bit.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
Now we can move other #ifdef DEBUG under this same
config, but not all of them; some look like they
would be way too chatty. I''ll need to review those
before ending another patch to add any of them
under CONFIG_BTRFS_DEBUG.
Thanks,
-Eric
diff --git a/fs/btrfs/Kconfig b/fs/btrfs/Kconfig
index 9a8622a..42dc602 100644
--- a/fs/btrfs/Kconfig
+++ b/fs/btrfs/Kconfig
@@ -52,3 +52,13 @@ config BTRFS_FS_CHECK_INTEGRITY
In most cases, unless you are a btrfs developer who needs
to verify the integrity of (super)-block write requests
during the run of a regression test, say N
+
+config BTRFS_DEBUG
+ bool "Btrfs debugging support"
+ depends on BTRFS_FS
+ help
+ Enable run-time debugging support for the btrfs filesystem. This may
+ enable additional and expensive checks with negative impact on
+ performance, or export extra information via sysfs.
+
+ If unsure, say N.
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 223aa6a..6f5eda8 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -24,12 +24,62 @@
static struct kmem_cache *extent_state_cache;
static struct kmem_cache *extent_buffer_cache;
+#ifdef CONFIG_BTRFS_DEBUG
static LIST_HEAD(buffers);
static LIST_HEAD(states);
-#define LEAK_DEBUG 0
-#if LEAK_DEBUG
static DEFINE_SPINLOCK(leak_lock);
+
+static inline
+void btrfs_leak_debug_add(struct list_head *new, struct list_head *head)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&leak_lock, flags);
+ list_add(new, head);
+ spin_unlock_irqrestore(&leak_lock, flags);
+}
+
+static inline
+void btrfs_leak_debug_del(struct list_head *entry)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&leak_lock, flags);
+ list_del(entry);
+ spin_unlock_irqrestore(&leak_lock, flags);
+}
+
+static inline
+void btrfs_leak_debug_check()
+{
+ struct extent_state *state;
+ struct extent_buffer *eb;
+
+ while (!list_empty(&states)) {
+ state = list_entry(states.next, struct extent_state, leak_list);
+ printk(KERN_ERR "btrfs state leak: start %llu end %llu "
+ "state %lu in tree %p refs %d\n",
+ (unsigned long long)state->start,
+ (unsigned long long)state->end,
+ state->state, state->tree, atomic_read(&state->refs));
+ list_del(&state->leak_list);
+ kmem_cache_free(extent_state_cache, state);
+ }
+
+ while (!list_empty(&buffers)) {
+ eb = list_entry(buffers.next, struct extent_buffer, leak_list);
+ printk(KERN_ERR "btrfs buffer leak start %llu len %lu "
+ "refs %d\n", (unsigned long long)eb->start,
+ eb->len, atomic_read(&eb->refs));
+ list_del(&eb->leak_list);
+ kmem_cache_free(extent_buffer_cache, eb);
+ }
+}
+#else
+#define btrfs_leak_debug_add(new, head) do {} while (0)
+#define btrfs_leak_debug_del(entry) do {} while (0)
+#define btrfs_leak_debug_check(entry) do {} while (0)
#endif
#define BUFFER_LRU_MAX 64
@@ -84,29 +134,7 @@ free_state_cache:
void extent_io_exit(void)
{
- struct extent_state *state;
- struct extent_buffer *eb;
-
- while (!list_empty(&states)) {
- state = list_entry(states.next, struct extent_state, leak_list);
- printk(KERN_ERR "btrfs state leak: start %llu end %llu "
- "state %lu in tree %p refs %d\n",
- (unsigned long long)state->start,
- (unsigned long long)state->end,
- state->state, state->tree, atomic_read(&state->refs));
- list_del(&state->leak_list);
- kmem_cache_free(extent_state_cache, state);
-
- }
-
- while (!list_empty(&buffers)) {
- eb = list_entry(buffers.next, struct extent_buffer, leak_list);
- printk(KERN_ERR "btrfs buffer leak start %llu len %lu "
- "refs %d\n", (unsigned long long)eb->start,
- eb->len, atomic_read(&eb->refs));
- list_del(&eb->leak_list);
- kmem_cache_free(extent_buffer_cache, eb);
- }
+ btrfs_leak_debug_check();
/*
* Make sure all delayed rcu free are flushed before we
@@ -134,9 +162,6 @@ void extent_io_tree_init(struct extent_io_tree *tree,
static struct extent_state *alloc_extent_state(gfp_t mask)
{
struct extent_state *state;
-#if LEAK_DEBUG
- unsigned long flags;
-#endif
state = kmem_cache_alloc(extent_state_cache, mask);
if (!state)
@@ -144,11 +169,7 @@ static struct extent_state *alloc_extent_state(gfp_t mask)
state->state = 0;
state->private = 0;
state->tree = NULL;
-#if LEAK_DEBUG
- spin_lock_irqsave(&leak_lock, flags);
- list_add(&state->leak_list, &states);
- spin_unlock_irqrestore(&leak_lock, flags);
-#endif
+ btrfs_leak_debug_add(&state->leak_list, &states);
atomic_set(&state->refs, 1);
init_waitqueue_head(&state->wq);
trace_alloc_extent_state(state, mask, _RET_IP_);
@@ -160,15 +181,8 @@ void free_extent_state(struct extent_state *state)
if (!state)
return;
if (atomic_dec_and_test(&state->refs)) {
-#if LEAK_DEBUG
- unsigned long flags;
-#endif
WARN_ON(state->tree);
-#if LEAK_DEBUG
- spin_lock_irqsave(&leak_lock, flags);
- list_del(&state->leak_list);
- spin_unlock_irqrestore(&leak_lock, flags);
-#endif
+ btrfs_leak_debug_del(&state->leak_list);
trace_free_extent_state(state, _RET_IP_);
kmem_cache_free(extent_state_cache, state);
}
@@ -4010,12 +4024,7 @@ out:
static void __free_extent_buffer(struct extent_buffer *eb)
{
-#if LEAK_DEBUG
- unsigned long flags;
- spin_lock_irqsave(&leak_lock, flags);
- list_del(&eb->leak_list);
- spin_unlock_irqrestore(&leak_lock, flags);
-#endif
+ btrfs_leak_debug_del(&eb->leak_list);
kmem_cache_free(extent_buffer_cache, eb);
}
@@ -4025,9 +4034,6 @@ static struct extent_buffer *__alloc_extent_buffer(struct
extent_io_tree *tree,
gfp_t mask)
{
struct extent_buffer *eb = NULL;
-#if LEAK_DEBUG
- unsigned long flags;
-#endif
eb = kmem_cache_zalloc(extent_buffer_cache, mask);
if (eb == NULL)
@@ -4047,11 +4053,8 @@ static struct extent_buffer *__alloc_extent_buffer(struct
extent_io_tree *tree,
init_waitqueue_head(&eb->write_lock_wq);
init_waitqueue_head(&eb->read_lock_wq);
-#if LEAK_DEBUG
- spin_lock_irqsave(&leak_lock, flags);
- list_add(&eb->leak_list, &buffers);
- spin_unlock_irqrestore(&leak_lock, flags);
-#endif
+ btrfs_leak_debug_add(&eb->leak_list, &buffers);
+
spin_lock_init(&eb->refs_lock);
atomic_set(&eb->refs, 1);
atomic_set(&eb->io_pages, 0);
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 295c888..a478250 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -116,7 +116,9 @@ struct extent_state {
/* for use by the FS */
u64 private;
+#ifdef CONFIG_BTRFS_DEBUG
struct list_head leak_list;
+#endif
};
#define INLINE_EXTENT_BUFFER_PAGES 16
@@ -132,7 +134,6 @@ struct extent_buffer {
atomic_t refs;
atomic_t io_pages;
int read_mirror;
- struct list_head leak_list;
struct rcu_head rcu_head;
pid_t lock_owner;
@@ -159,6 +160,9 @@ struct extent_buffer {
wait_queue_head_t read_lock_wq;
wait_queue_head_t lock_wq;
struct page *pages[INLINE_EXTENT_BUFFER_PAGES];
+#ifdef CONFIG_BTRFS_DEBUG
+ struct list_head leak_list;
+#endif
};
static inline void extent_set_compress_type(unsigned long *bio_flags,
--
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
David Sterba
2013-Apr-23 17:05 UTC
Re: [PATCH V3] btrfs: move leak debug code to functions
On Mon, Apr 22, 2013 at 11:12:31AM -0500, Eric Sandeen wrote:> Clean up the leak debugging in extent_io.c by moving > the debug code into functions. This also removes the > list_heads used for debugging from the extent_buffer > and extent_state structures when debug is not enabled. > > Since we need a global debug config to do that last > part, implement CONFIG_BTRFS_DEBUG to accommodate. > > Thanks to Dave Sterba for the Kconfig bit. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com>Reviewed-by: David Sterba <dsterba@suse.cz>> Now we can move other #ifdef DEBUG under this same > config, but not all of them; some look like they > would be way too chatty. I''ll need to review those > before ending another patch to add any of them > under CONFIG_BTRFS_DEBUG.Agreed. The number of extra checks should be reasonable so the fs is still usable. 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