diff mbox series

[v3,5/5] btrfs: qgroup: catch reserved space leakage at unmount time

Message ID 20200610010444.13583-6-wqu@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: qgroup: detect and fix leaked data reserved space | expand

Commit Message

Qu Wenruo June 10, 2020, 1:04 a.m. UTC
Before this patch, btrfs qgroup completely relies on per-inode extent io
tree to detect reserved data space leakage.

However previous bug has already shown how release page before
btrfs_finish_ordered_io() could lead to leakage, and since it's
QGROUP_RESERVED bit cleared without triggering qgroup rsv, it can't be
detected by per-inode extent io tree.

So this patch adds another (and hopefully the final) safe net to catch
qgroup data reserved space leakage.

At least the new safe net catches all the leakage during development, so
it should be pretty useful in the real world.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c |  5 +++++
 fs/btrfs/qgroup.c  | 43 +++++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/qgroup.h  |  2 +-
 3 files changed, 49 insertions(+), 1 deletion(-)

Comments

Josef Bacik June 12, 2020, 6:51 p.m. UTC | #1
On 6/9/20 9:04 PM, Qu Wenruo wrote:
> Before this patch, btrfs qgroup completely relies on per-inode extent io
> tree to detect reserved data space leakage.
> 
> However previous bug has already shown how release page before
> btrfs_finish_ordered_io() could lead to leakage, and since it's
> QGROUP_RESERVED bit cleared without triggering qgroup rsv, it can't be
> detected by per-inode extent io tree.
> 
> So this patch adds another (and hopefully the final) safe net to catch
> qgroup data reserved space leakage.
> 
> At least the new safe net catches all the leakage during development, so
> it should be pretty useful in the real world.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef
diff mbox series

Patch

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index f8ec2d8606fd..aaecaa4c64f5 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4058,6 +4058,11 @@  void __cold close_ctree(struct btrfs_fs_info *fs_info)
 	ASSERT(list_empty(&fs_info->delayed_iputs));
 	set_bit(BTRFS_FS_CLOSING_DONE, &fs_info->flags);
 
+	if (btrfs_qgroup_has_leak(fs_info)) {
+		WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
+		btrfs_err(fs_info, "qgroup reserved space leaked\n");
+	}
+
 	btrfs_free_qgroup_config(fs_info);
 	ASSERT(list_empty(&fs_info->delalloc_roots));
 
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 5bd4089ad0e1..f899b2167fa8 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -505,6 +505,49 @@  int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info)
 	return ret < 0 ? ret : 0;
 }
 
+static u64 btrfs_qgroup_subvolid(u64 qgroupid)
+{
+	return (qgroupid & ((1ULL << BTRFS_QGROUP_LEVEL_SHIFT) - 1));
+}
+/*
+ * Get called for close_ctree() when quota is still enabled.
+ * This verifies we don't leak some reserved space.
+ *
+ * Return false if no reserved space is left.
+ * Return true if some reserved space is leaked.
+ */
+bool btrfs_qgroup_has_leak(struct btrfs_fs_info *fs_info)
+{
+	struct rb_node *node;
+	bool ret = false;
+
+	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
+		return ret;
+	/*
+	 * Since we're unmounting, there is no race and no need to grab
+	 * qgroup lock.
+	 * And here we don't go post order to provide a more user friendly
+	 * sorted result.
+	 */
+	for (node = rb_first(&fs_info->qgroup_tree); node; node = rb_next(node)) {
+		struct btrfs_qgroup *qgroup;
+		int i;
+
+		qgroup = rb_entry(node, struct btrfs_qgroup, node);
+		for (i = 0; i < BTRFS_QGROUP_RSV_LAST; i++) {
+			if (qgroup->rsv.values[i]) {
+				ret = true;
+				btrfs_warn(fs_info,
+		"qgroup %llu/%llu has unreleased space, type=%d rsv=%llu",
+				   btrfs_qgroup_level(qgroup->qgroupid),
+				   btrfs_qgroup_subvolid(qgroup->qgroupid),
+				   i, qgroup->rsv.values[i]);
+			}
+		}
+	}
+	return ret;
+}
+
 /*
  * This is called from close_ctree() or open_ctree() or btrfs_quota_disable(),
  * first two are in single-threaded paths.And for the third one, we have set
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index 1bc654459469..e3e9f9df8320 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -415,5 +415,5 @@  int btrfs_qgroup_add_swapped_blocks(struct btrfs_trans_handle *trans,
 int btrfs_qgroup_trace_subtree_after_cow(struct btrfs_trans_handle *trans,
 		struct btrfs_root *root, struct extent_buffer *eb);
 void btrfs_qgroup_destroy_extent_records(struct btrfs_transaction *trans);
-
+bool btrfs_qgroup_has_leak(struct btrfs_fs_info *fs_info);
 #endif