[PATCH v2 0/3] btrfs: further cleanup on dirty_cowonly_roots and switch_commits

[PATCH v2 0/3] btrfs: further cleanup on dirty_cowonly_roots and switch_commits

[Qu Wenruo]

[CHANGELOG]
v2:
- Add a patch to release qgroup swapped blocks during btrfs_put_root()
  This is addressing a real but rare memory leak.

- Replace the btrfs_extent_io_tree_release() with an ASSERT()
  As cowonly roots should never have dirty_log_pages populated.
  That is only utilized by log trees, and log trees are already poperly
  cleaned up in btrfs_drop_all_logs()

- Remove the btrfs_qgroup_clean_swapped_blocks() call in
  btrfs_free_fs_roots()
  As we have a dedicated cleanup during btrfs_put_root(), no need to do
  it inside btrfs_free_fs_roots()

Sashiko mentioned that dirty_cowonly_roots can still be non-empty if a
transaction aborts during commit_cowonly_roots().

However it should not cause any problem, as after a transaction is
aborted, no one will ever touch dirty_cowonly_roots.
So even if it has some dangling pointers, as long as no one is reading
it, it's completely fine.

Furthermore, dirty_cowonly_roots contains roots that are not subvolume
nor log trees, thus they should not have swapped qgroup blocks nor
dirty_log_pages. This means no memory leak from those two members
either.

But we're still depending on such quirky behavior, and it's only going
to cause more false alerts from various LLMs, e.g. Sashiko is already
reporting problems on this.

This series address all those minor problems by:

- Remove quota_root from dirty_cowonly_roots during
  btrfs_quota_disable()
  This prepares for the final patch as we're adding a reader for
  dirty_cowonly_roots during unmount.

- Cleanup qgroup swapped blocks during btrfs_put_root()
  This is to address a real possible memory leak, that some dirty
  subvolumes can have qgroup swapped blocks, and that can leak memory
  if not properly handled.

- Cleanup dirty_cowonly_roots during btrfs_free_fs_roots()
  This to remove the quirky dirty_cowonly_roots handling, and relies on
  explicit removal during unmount.

  Also add a new ASSERT() to make sure that roots linked into
  dirty_cowonly_roots() has no dirty_log_pages.


*** BLURB HERE ***

Qu Wenruo (3):
  btrfs: always remove quota_root from dirty_cowonly_roots
  btrfs: cleanup qgroup swapped blocks at root release
  btrfs: cleanup dirty_cowonly_roots during btrfs_free_fs_roots()

 fs/btrfs/disk-io.c | 20 ++++++++++++++++++++
 fs/btrfs/qgroup.c  | 10 ++++++----
 2 files changed, 26 insertions(+), 4 deletions(-)

-- 
2.56.0

[PATCH v2 1/3] btrfs: always remove quota_root from dirty_cowonly_roots

[Qu Wenruo]

[ERROR HANDLING QUIRK]
When btrfs_quota_disable() failed at btrfs_clean_quota_tree() or
btrfs_del_root(), the quota_root->dirty_list is not removed from
fs_info->dirty_cowonly_roots, but the quota_root itself will be freed.

This leaves a dangling pointer in the dirty_cowonly_roots.

This is not a huge problem, as the transaction is aborted thus no one
will read the dirty_cowonly_roots for now.

Furthermore, quota_root should never be logged nor have qgroup swapped
tree blocks, thus it won't cause leaks from extent io tree nor qgroup
swapped blocks.

[ENHANCEMENT]
For the incoming cleanup of dirty_cowonly_roots during
btrfs_free_fs_roots(), any root that is still linked to
dirty_cowonly_roots will be read again, and this existing dangling
pointer is going to cause problem.

To prepare for that cleanup, move the list_del() call into the out
label, so that even if the transaction is aborted, we still unlink
quota_root from dirty_cowonly_roots before freeing it.

Assisted-by: LLM (exposing the bug during review)
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index a0651fbfe778..8bd3abdf4805 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1440,10 +1440,6 @@ int btrfs_quota_disable(struct btrfs_fs_info *fs_info)
 		goto out;
 	}
 
-	spin_lock(&fs_info->trans_lock);
-	list_del(&quota_root->dirty_list);
-	spin_unlock(&fs_info->trans_lock);
-
 	btrfs_tree_lock(quota_root->node);
 	btrfs_clear_buffer_dirty(trans, quota_root->node);
 	btrfs_tree_unlock(quota_root->node);
@@ -1454,6 +1450,12 @@ int btrfs_quota_disable(struct btrfs_fs_info *fs_info)
 		btrfs_abort_transaction(trans, ret);
 
 out:
+	if (quota_root) {
+		spin_lock(&fs_info->trans_lock);
+		list_del(&quota_root->dirty_list);
+		spin_unlock(&fs_info->trans_lock);
+	}
+
 	btrfs_put_root(quota_root);
 	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	if (ret && trans)
-- 
2.56.0

[PATCH v2 2/3] btrfs: cleanup qgroup swapped blocks at root release

[Qu Wenruo]

Normally btrfs_qgroup_clean_swapped_blocks() is called when switching
the commit root of a subvolume tree.

However if a transaction is aborted before all dirty subvolumes are
linked to switch_commits, those left-over dirty subvolumes won't get
their qgroup swapped blocks cleaned up.

And since a transaction is already aborted, no new transaction is
allowed to be started, thus no way to cleanup the qgroup swapped blocks
for those subvolume trees.

Fix the problem by calling btrfs_qgroup_clean_swapped_blocks() inside
btrfs_put_root() when the last ref is dropped, so that at unmount time
those left-over qgroup swapped blocks are properly released.

This fix is inspired by the recent Sashiko reports about similar cases
on cowonly roots.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 02adda065097..70c1ee40bf02 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1848,6 +1848,7 @@ void btrfs_put_root(struct btrfs_root *root)
 		if (root->anon_dev)
 			free_anon_bdev(root->anon_dev);
 		free_root_extent_buffers(root);
+		btrfs_qgroup_clean_swapped_blocks(root);
 #ifdef CONFIG_BTRFS_DEBUG
 		spin_lock(&root->fs_info->fs_roots_radix_lock);
 		list_del_init(&root->leak_list);
-- 
2.56.0

[PATCH v2 3/3] btrfs: cleanup dirty_cowonly_roots during btrfs_free_fs_roots()

[Qu Wenruo]

[EXISTING ERROR HANDLING QUIRK]
Sashiko mentioned an existing problem that, if btrfs_abort_transaction()
is triggered during the following call site, some roots will be attached
to dirty_cowonly_roots without being cleaned up:

 btrfs_commit_transaction()
 |- commit_cowonly_roots()
    |- list_move_tail();
    |  One root's dirty_list is moved from dirty_cowonly_roots to
    |  switch_commits.
    |  But there are still other dirty roots in dirty_cowonly_roots.
    |
    |- update_cowonly_root()
    |  |- btrfs_update_root()
    |     |- btrfs_abort_transaction()
    |
    |- return ret;

In that case, those dirty roots linked into dirty_cowonly_roots will
never be removed from that list.
As normally we require a transaction commit to move them, but since the
fs is already marked error, no new transaction can be started.

For roots that are already moved to switch_commits list, they are
cleaned up by btrfs_cleanup_one_transaction().

This is not going to cause memory leak on the btrfs_root structure
itself, as after a transaction is aborted, no one will read
dirty_cowonly_roots.

Furthermore dirty_cowonly_roots only tracks trees that are not
subvolume or log trees. They should not have root->dirty_log_pages.
And qgroup swapped blocks are going to be released during
btrfs_put_root().

Thus no memory leak from those two members either.

But we're still relying on a hidden behavior to keep us from leaking
memory.

[ENHANCEMENT]
Unlink all roots that are still linked to dirty_cowonly_roots during
btrfs_free_fs_roots(), and do one extra ASSERT() to make sure that their
dirty_log_pages are empty.

At that timing there is no other thread thus we're safe to unlink roots
without holding trans_lock.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 70c1ee40bf02..91eceb1ab4f9 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1864,6 +1864,25 @@ void btrfs_free_fs_roots(struct btrfs_fs_info *fs_info)
 	struct btrfs_root *gang[8];
 	int i;
 
+	/*
+	 * Some dirty roots may still be linked to dirty_cowonly_roots after
+	 * the transaction is aborted. Cleanup them here.
+	 */
+	while (!list_empty(&fs_info->dirty_cowonly_roots)) {
+		struct btrfs_root *root;
+
+		root = list_first_entry(&fs_info->dirty_cowonly_roots,
+					struct btrfs_root, dirty_list);
+		list_del_init(&root->dirty_list);
+		/*
+		 * The dirty_log_pages should never be populated for
+		 * cowonly trees.
+		 */
+		spin_lock(&root->dirty_log_pages.lock);
+		ASSERT(RB_EMPTY_ROOT(&root->dirty_log_pages.state));
+		spin_unlock(&root->dirty_log_pages.lock);
+	}
+
 	while (!list_empty(&fs_info->dead_roots)) {
 		gang[0] = list_first_entry(&fs_info->dead_roots,
 					   struct btrfs_root, root_list);
-- 
2.56.0