[PATCH 0/2] btrfs: further cleanup on fs_info->dirty_cowonly_roots

[PATCH 0/2] btrfs: further cleanup on fs_info->dirty_cowonly_roots

[Qu Wenruo]

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 change the behavior to a dedicated cleanup, so that we no
longer relies on those hidden quirks.

Qu Wenruo (2):
  btrfs: always remove quota_root from dirty_cowonly_roots
  btrfs: cleanup dirty_cowonly_roots during btrfs_free_fs_roots()

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

-- 
2.56.0

[PATCH 1/2] 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 2/2] 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
nor qgroup swapped blocks, 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 follow btrfs_cleanup_one_transaction() to
release dirty_log_pages and qgroup swapped blocks even they should be
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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 02adda065097..740c8b06d008 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1863,6 +1863,20 @@ 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);
+		btrfs_extent_io_tree_release(&root->dirty_log_pages);
+		btrfs_qgroup_clean_swapped_blocks(root);
+	}
+
 	while (!list_empty(&fs_info->dead_roots)) {
 		gang[0] = list_first_entry(&fs_info->dead_roots,
 					   struct btrfs_root, root_list);
-- 
2.56.0

[PATCH 2/2] 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
nor qgroup swapped blocks, 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 follow btrfs_cleanup_one_transaction() to
release dirty_log_pages and qgroup swapped blocks even they should be
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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 02adda065097..740c8b06d008 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1863,6 +1863,20 @@ 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);
+		btrfs_extent_io_tree_release(&root->dirty_log_pages);
+		btrfs_qgroup_clean_swapped_blocks(root);
+	}
+
 	while (!list_empty(&fs_info->dead_roots)) {
 		gang[0] = list_first_entry(&fs_info->dead_roots,
 					   struct btrfs_root, root_list);
-- 
2.56.0