diff mbox series

[1/8] btrfs: make the extent buffer leak check per fs info

Message ID 20200214211147.24610-2-josef@toxicpanda.com (mailing list archive)
State New, archived
Headers show
Series Cleanup how we handle root refs, part 2 | expand

Commit Message

Josef Bacik Feb. 14, 2020, 9:11 p.m. UTC
I'm going to make the entire destruction of btrfs_root's controlled by
their refcount, so it will be helpful to notice if we're leaking their
eb's on umount.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/ctree.h     |  3 +++
 fs/btrfs/disk-io.c   |  3 +++
 fs/btrfs/extent_io.c | 45 ++++++++++++++++++++++----------------------
 fs/btrfs/extent_io.h |  7 +++++++
 4 files changed, 36 insertions(+), 22 deletions(-)

Comments

Nikolay Borisov Feb. 19, 2020, 1:59 p.m. UTC | #1
On 14.02.20 г. 23:11 ч., Josef Bacik wrote:
> I'm going to make the entire destruction of btrfs_root's controlled by
> their refcount, so it will be helpful to notice if we're leaking their
> eb's on umount.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Overall looks good, one minor nit below though:

> ---
>  fs/btrfs/ctree.h     |  3 +++
>  fs/btrfs/disk-io.c   |  3 +++
>  fs/btrfs/extent_io.c | 45 ++++++++++++++++++++++----------------------
>  fs/btrfs/extent_io.h |  7 +++++++
>  4 files changed, 36 insertions(+), 22 deletions(-)
> 

<snip>

> @@ -35,42 +35,45 @@ static inline bool extent_state_in_tree(const struct extent_state *state)
>  }
>  
>  #ifdef CONFIG_BTRFS_DEBUG
> -static LIST_HEAD(buffers);
>  static LIST_HEAD(states);
> -
>  static DEFINE_SPINLOCK(leak_lock);

This lock should be renamed to extent_state_leak_lock, otherwise it's
not clear what it protects. I had to through its usage to figure it out.
To take this further, why don't you make it a a per-fs_info lock as
well? Extent states are per fs themselves.

<snip>
Josef Bacik Feb. 20, 2020, 3:49 p.m. UTC | #2
On 2/19/20 8:59 AM, Nikolay Borisov wrote:
> 
> 
> On 14.02.20 г. 23:11 ч., Josef Bacik wrote:
>> I'm going to make the entire destruction of btrfs_root's controlled by
>> their refcount, so it will be helpful to notice if we're leaking their
>> eb's on umount.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> Overall looks good, one minor nit below though:
> 
>> ---
>>   fs/btrfs/ctree.h     |  3 +++
>>   fs/btrfs/disk-io.c   |  3 +++
>>   fs/btrfs/extent_io.c | 45 ++++++++++++++++++++++----------------------
>>   fs/btrfs/extent_io.h |  7 +++++++
>>   4 files changed, 36 insertions(+), 22 deletions(-)
>>
> 
> <snip>
> 
>> @@ -35,42 +35,45 @@ static inline bool extent_state_in_tree(const struct extent_state *state)
>>   }
>>   
>>   #ifdef CONFIG_BTRFS_DEBUG
>> -static LIST_HEAD(buffers);
>>   static LIST_HEAD(states);
>> -
>>   static DEFINE_SPINLOCK(leak_lock);
> 
> This lock should be renamed to extent_state_leak_lock, otherwise it's
> not clear what it protects. I had to through its usage to figure it out.
> To take this further, why don't you make it a a per-fs_info lock as
> well? Extent states are per fs themselves.
> 

Yeah I can follow up once this set is merged.  Thanks,

Josef
diff mbox series

Patch

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index ffd99c3f64db..000ad54629e3 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -948,6 +948,9 @@  struct btrfs_fs_info {
 	struct kobject *debug_kobj;
 	struct kobject *discard_debug_kobj;
 	struct list_head allocated_roots;
+
+	spinlock_t eb_leak_lock;
+	struct list_head alloced_ebs;
 #endif
 };
 
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 018681ec159b..2a237aecc6d7 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1574,6 +1574,7 @@  void btrfs_free_fs_info(struct btrfs_fs_info *fs_info)
 	btrfs_put_root(fs_info->free_space_root);
 	btrfs_put_root(fs_info->fs_root);
 	btrfs_check_leaked_roots(fs_info);
+	btrfs_extent_buffer_leak_debug_check(fs_info);
 	kfree(fs_info->super_copy);
 	kfree(fs_info->super_for_commit);
 	kvfree(fs_info);
@@ -2702,6 +2703,8 @@  void btrfs_init_fs_info(struct btrfs_fs_info *fs_info)
 	INIT_LIST_HEAD(&fs_info->unused_bgs);
 #ifdef CONFIG_BTRFS_DEBUG
 	INIT_LIST_HEAD(&fs_info->allocated_roots);
+	INIT_LIST_HEAD(&fs_info->alloced_ebs);
+	spin_lock_init(&fs_info->eb_leak_lock);
 #endif
 	extent_map_tree_init(&fs_info->mapping_tree);
 	btrfs_init_block_rsv(&fs_info->global_block_rsv,
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 6f9638350470..e82e3817e811 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -35,42 +35,45 @@  static inline bool extent_state_in_tree(const struct extent_state *state)
 }
 
 #ifdef CONFIG_BTRFS_DEBUG
-static LIST_HEAD(buffers);
 static LIST_HEAD(states);
-
 static DEFINE_SPINLOCK(leak_lock);
 
-static inline
-void btrfs_leak_debug_add(struct list_head *new, struct list_head *head)
+static inline void btrfs_leak_debug_add(spinlock_t *lock,
+					struct list_head *new,
+					struct list_head *head)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&leak_lock, flags);
+	spin_lock_irqsave(lock, flags);
 	list_add(new, head);
-	spin_unlock_irqrestore(&leak_lock, flags);
+	spin_unlock_irqrestore(lock, flags);
 }
 
-static inline
-void btrfs_leak_debug_del(struct list_head *entry)
+static inline void btrfs_leak_debug_del(spinlock_t *lock,
+					struct list_head *entry)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&leak_lock, flags);
+	spin_lock_irqsave(lock, flags);
 	list_del(entry);
-	spin_unlock_irqrestore(&leak_lock, flags);
+	spin_unlock_irqrestore(lock, flags);
 }
 
-static inline void btrfs_extent_buffer_leak_debug_check(void)
+void btrfs_extent_buffer_leak_debug_check(struct btrfs_fs_info *fs_info)
 {
 	struct extent_buffer *eb;
+	unsigned long flags;
 
-	while (!list_empty(&buffers)) {
-		eb = list_entry(buffers.next, struct extent_buffer, leak_list);
+	spin_lock_irqsave(&fs_info->eb_leak_lock, flags);
+	while (!list_empty(&fs_info->alloced_ebs)) {
+		eb = list_first_entry(&fs_info->alloced_ebs,
+				      struct extent_buffer, leak_list);
 		pr_err("BTRFS: buffer leak start %llu len %lu refs %d bflags %lu\n",
 		       eb->start, eb->len, atomic_read(&eb->refs), eb->bflags);
 		list_del(&eb->leak_list);
 		kmem_cache_free(extent_buffer_cache, eb);
 	}
+	spin_unlock_irqrestore(&fs_info->eb_leak_lock, flags);
 }
 
 static inline void btrfs_extent_state_leak_debug_check(void)
@@ -107,9 +110,8 @@  static inline void __btrfs_debug_check_extent_io_range(const char *caller,
 	}
 }
 #else
-#define btrfs_leak_debug_add(new, head)	do {} while (0)
-#define btrfs_leak_debug_del(entry)	do {} while (0)
-#define btrfs_extent_buffer_leak_debug_check()	do {} while (0)
+#define btrfs_leak_debug_add(lock, new, head)	do {} while (0)
+#define btrfs_leak_debug_del(lock, entry)	do {} while (0)
 #define btrfs_extent_state_leak_debug_check()	do {} while (0)
 #define btrfs_debug_check_extent_io_range(c, s, e)	do {} while (0)
 #endif
@@ -245,8 +247,6 @@  void __cold extent_state_cache_exit(void)
 
 void __cold extent_io_exit(void)
 {
-	btrfs_extent_buffer_leak_debug_check();
-
 	/*
 	 * Make sure all delayed rcu free are flushed before we
 	 * destroy caches.
@@ -324,7 +324,7 @@  static struct extent_state *alloc_extent_state(gfp_t mask)
 	state->state = 0;
 	state->failrec = NULL;
 	RB_CLEAR_NODE(&state->rb_node);
-	btrfs_leak_debug_add(&state->leak_list, &states);
+	btrfs_leak_debug_add(&leak_lock, &state->leak_list, &states);
 	refcount_set(&state->refs, 1);
 	init_waitqueue_head(&state->wq);
 	trace_alloc_extent_state(state, mask, _RET_IP_);
@@ -337,7 +337,7 @@  void free_extent_state(struct extent_state *state)
 		return;
 	if (refcount_dec_and_test(&state->refs)) {
 		WARN_ON(extent_state_in_tree(state));
-		btrfs_leak_debug_del(&state->leak_list);
+		btrfs_leak_debug_del(&leak_lock, &state->leak_list);
 		trace_free_extent_state(state, _RET_IP_);
 		kmem_cache_free(extent_state_cache, state);
 	}
@@ -4795,7 +4795,7 @@  int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 
 static void __free_extent_buffer(struct extent_buffer *eb)
 {
-	btrfs_leak_debug_del(&eb->leak_list);
+	btrfs_leak_debug_del(&eb->fs_info->eb_leak_lock, &eb->leak_list);
 	kmem_cache_free(extent_buffer_cache, eb);
 }
 
@@ -4882,7 +4882,8 @@  __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
 	init_waitqueue_head(&eb->write_lock_wq);
 	init_waitqueue_head(&eb->read_lock_wq);
 
-	btrfs_leak_debug_add(&eb->leak_list, &buffers);
+	btrfs_leak_debug_add(&fs_info->eb_leak_lock, &eb->leak_list,
+			     &fs_info->alloced_ebs);
 
 	spin_lock_init(&eb->refs_lock);
 	atomic_set(&eb->refs, 1);
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 234622101230..2ed65bd0760e 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -325,4 +325,11 @@  bool find_lock_delalloc_range(struct inode *inode,
 #endif
 struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,
 					       u64 start);
+
+#ifdef CONFIG_BTRFS_DEBUG
+void btrfs_extent_buffer_leak_debug_check(struct btrfs_fs_info *fs_info);
+#else
+#define btrfs_extent_buffer_leak_debug_check(fs_info)	do {} while (0)
+#endif
+
 #endif