[PATCH 0/7] btrfs: various qg meta rsv leak fixes

[PATCH 0/7] btrfs: various qg meta rsv leak fixes

[Boris Burkov]

generic/269 and generic/475 expose a number of reservation accounting
issues in the btrfs quotas code that is shared between qgroups and
squotas. In particular, error paths for failed transactions and errors
in start_transaction and other critical functions for root per-trans
accounting.

These semi-related patches fix up a number of such issues. With them,
generic/269 with -O squota passed 1000+ times in a row for me and
generic/475 has run hundreds of iterations without ever failing on a
metadata reservation leak warning. generic/475 does still see issues
with qgroup data reservation accounting and only passes ~9/10 times
on my system.

Boris Burkov (7):
  btrfs: correctly model root qgroup rsv in convert
  btrfs: fix qgroup prealloc rsv leak in subvolume operations
  btrfs: record delayed inode root in transaction
  btrfs: convert PREALLOC to PERTRANS after record_root_in_trans
  btrfs: free pertrans at end of cleanup_transaction
  btrfs: btrfs_clear_delalloc_extent frees rsv
  btrfs: always clear meta pertrans during commit

 fs/btrfs/delayed-inode.c |  3 +++
 fs/btrfs/disk-io.c       |  3 +--
 fs/btrfs/inode.c         | 15 +++++++++++++--
 fs/btrfs/ioctl.c         | 37 ++++++++++++++++++++++++++++---------
 fs/btrfs/qgroup.c        |  2 ++
 fs/btrfs/root-tree.c     | 10 ----------
 fs/btrfs/root-tree.h     |  2 --
 fs/btrfs/transaction.c   | 19 +++++++++----------
 8 files changed, 56 insertions(+), 35 deletions(-)

-- 
2.44.0

[PATCH 1/7] btrfs: correctly model root qgroup rsv in convert

[Boris Burkov]

We use add_root_meta_rsv and sub_root_meta_rsv to track prealloc and
pertrans reservations for subvols when quotas are enabled. The convert
function does not properly increment pertrans after decrementing
prealloc, so the count is not accurate.

Note: we check that the fs is not read-only to mirror the logic in
qgroup_convert_meta, which checks that before adding to the pertrans rsv.

Fixes: 8287475a2055 ("btrfs: qgroup: Use root::qgroup_meta_rsv_* to record qgroup meta reserved space")
Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/qgroup.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index a8197e25192c..2cba6451d164 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -4492,6 +4492,8 @@ void btrfs_qgroup_convert_reserved_meta(struct btrfs_root *root, int num_bytes)
 				      BTRFS_QGROUP_RSV_META_PREALLOC);
 	trace_qgroup_meta_convert(root, num_bytes);
 	qgroup_convert_meta(fs_info, root->root_key.objectid, num_bytes);
+	if (!sb_rdonly(fs_info->sb))
+		add_root_meta_rsv(root, num_bytes, BTRFS_QGROUP_RSV_META_PERTRANS);
 }
 
 /*
-- 
2.44.0

[PATCH 2/7] btrfs: fix qgroup prealloc rsv leak in subvolume operations

[Boris Burkov]

Create subvol, create snapshot and delete subvol all use
btrfs_subvolume_reserve_metadata to reserve metadata for the changes
done to the parent subvolume's fs tree, which cannot be mediated in the
normal way via start_transaction. When quota groups (squota or qgroups)
are enabled, this reserves qgroup metadata of type PREALLOC. Once the
operation is associated to a transaction, we convert PREALLOC to
PERTRANS, which gets cleared in bulk at the end of the transaction.

However, the error paths of these three operations were not implementing
this lifecycle correctly. They unconditionally converted the PREALLOC to
PERTRANS in a generic cleanup step regardless of errors or whether the
operation was fully associated to a transaction or not. This resulted in
error paths occasionally converting this rsv to PERTRANS without calling
record_root_in_trans successfully, which meant that unless that root got
recorded in the transaction by some other thread, the end of the
transaction would not free that root's PERTRANS, leaking it. Ultimately,
this resulted in hitting a WARN in CONFIG_BTRFS_DEBUG builds at unmount
for the leaked reservation.

The fix is to ensure that every qgroup PREALLOC reservation observes the
following properties:
1. any failure before record_root_in_trans is called successfully
   results in freeing the PREALLOC reservation.
2. after record_root_in_trans, we convert to PERTRANS, and now the
   transaction owns freeing the reservation.

This patch enforces those properties on the three operations. Without
it, generic/269 with squotas enabled at MKFS time would fail in ~5-10
runs on my system. With this patch, it ran successfully 1000 times in a
row.

Fixes: e85fde5162bf ("btrfs: qgroup: fix qgroup meta rsv leak for subvolume operations")
Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/inode.c     | 13 ++++++++++++-
 fs/btrfs/ioctl.c     | 37 ++++++++++++++++++++++++++++---------
 fs/btrfs/root-tree.c | 10 ----------
 fs/btrfs/root-tree.h |  2 --
 4 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 09f0a20b5ce8..2587a2e25e44 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4503,6 +4503,7 @@ int btrfs_delete_subvolume(struct btrfs_inode *dir, struct dentry *dentry)
 	struct btrfs_trans_handle *trans;
 	struct btrfs_block_rsv block_rsv;
 	u64 root_flags;
+	u64 qgroup_reserved = 0;
 	int ret;
 
 	down_write(&fs_info->subvol_sem);
@@ -4547,12 +4548,20 @@ int btrfs_delete_subvolume(struct btrfs_inode *dir, struct dentry *dentry)
 	ret = btrfs_subvolume_reserve_metadata(root, &block_rsv, 5, true);
 	if (ret)
 		goto out_undead;
+	qgroup_reserved = block_rsv.qgroup_rsv_reserved;
 
 	trans = btrfs_start_transaction(root, 0);
 	if (IS_ERR(trans)) {
 		ret = PTR_ERR(trans);
 		goto out_release;
 	}
+	ret = btrfs_record_root_in_trans(trans, root);
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		goto out_end_trans;
+	}
+	btrfs_qgroup_convert_reserved_meta(root, qgroup_reserved);
+	qgroup_reserved = 0;
 	trans->block_rsv = &block_rsv;
 	trans->bytes_reserved = block_rsv.size;
 
@@ -4611,7 +4620,9 @@ int btrfs_delete_subvolume(struct btrfs_inode *dir, struct dentry *dentry)
 	ret = btrfs_end_transaction(trans);
 	inode->i_flags |= S_DEAD;
 out_release:
-	btrfs_subvolume_release_metadata(root, &block_rsv);
+	btrfs_block_rsv_release(fs_info, &block_rsv, (u64)-1, NULL);
+	if (qgroup_reserved)
+		btrfs_qgroup_free_meta_prealloc(root, qgroup_reserved);
 out_undead:
 	if (ret) {
 		spin_lock(&dest->root_item_lock);
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index e3a72292eda9..888dc92c6c75 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -613,6 +613,7 @@ static noinline int create_subvol(struct mnt_idmap *idmap,
 	int ret;
 	dev_t anon_dev;
 	u64 objectid;
+	u64 qgroup_reserved = 0;
 
 	root_item = kzalloc(sizeof(*root_item), GFP_KERNEL);
 	if (!root_item)
@@ -650,13 +651,18 @@ static noinline int create_subvol(struct mnt_idmap *idmap,
 					       trans_num_items, false);
 	if (ret)
 		goto out_new_inode_args;
+	qgroup_reserved = block_rsv.qgroup_rsv_reserved;
 
 	trans = btrfs_start_transaction(root, 0);
 	if (IS_ERR(trans)) {
 		ret = PTR_ERR(trans);
-		btrfs_subvolume_release_metadata(root, &block_rsv);
-		goto out_new_inode_args;
+		goto out_release_rsv;
 	}
+	ret = btrfs_record_root_in_trans(trans, BTRFS_I(dir)->root);
+	if (ret)
+		goto out;
+	btrfs_qgroup_convert_reserved_meta(root, qgroup_reserved);
+	qgroup_reserved = 0;
 	trans->block_rsv = &block_rsv;
 	trans->bytes_reserved = block_rsv.size;
 	/* Tree log can't currently deal with an inode which is a new root. */
@@ -767,9 +773,11 @@ static noinline int create_subvol(struct mnt_idmap *idmap,
 out:
 	trans->block_rsv = NULL;
 	trans->bytes_reserved = 0;
-	btrfs_subvolume_release_metadata(root, &block_rsv);
-
 	btrfs_end_transaction(trans);
+out_release_rsv:
+	btrfs_block_rsv_release(fs_info, &block_rsv, (u64)-1, NULL);
+	if (qgroup_reserved)
+		btrfs_qgroup_free_meta_prealloc(root, qgroup_reserved);
 out_new_inode_args:
 	btrfs_new_inode_args_destroy(&new_inode_args);
 out_inode:
@@ -791,6 +799,8 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
 	struct btrfs_pending_snapshot *pending_snapshot;
 	unsigned int trans_num_items;
 	struct btrfs_trans_handle *trans;
+	struct btrfs_block_rsv *block_rsv;
+	u64 qgroup_reserved = 0;
 	int ret;
 
 	/* We do not support snapshotting right now. */
@@ -827,19 +837,19 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
 		goto free_pending;
 	}
 
-	btrfs_init_block_rsv(&pending_snapshot->block_rsv,
-			     BTRFS_BLOCK_RSV_TEMP);
+	block_rsv = &pending_snapshot->block_rsv;
+	btrfs_init_block_rsv(block_rsv, BTRFS_BLOCK_RSV_TEMP);
 	/*
 	 * 1 to add dir item
 	 * 1 to add dir index
 	 * 1 to update parent inode item
 	 */
 	trans_num_items = create_subvol_num_items(inherit) + 3;
-	ret = btrfs_subvolume_reserve_metadata(BTRFS_I(dir)->root,
-					       &pending_snapshot->block_rsv,
+	ret = btrfs_subvolume_reserve_metadata(BTRFS_I(dir)->root, block_rsv,
 					       trans_num_items, false);
 	if (ret)
 		goto free_pending;
+	qgroup_reserved = block_rsv->qgroup_rsv_reserved;
 
 	pending_snapshot->dentry = dentry;
 	pending_snapshot->root = root;
@@ -852,6 +862,13 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
 		ret = PTR_ERR(trans);
 		goto fail;
 	}
+	ret = btrfs_record_root_in_trans(trans, BTRFS_I(dir)->root);
+	if (ret) {
+		btrfs_end_transaction(trans);
+		goto fail;
+	}
+	btrfs_qgroup_convert_reserved_meta(root, qgroup_reserved);
+	qgroup_reserved = 0;
 
 	trans->pending_snapshot = pending_snapshot;
 
@@ -881,7 +898,9 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
 	if (ret && pending_snapshot->snap)
 		pending_snapshot->snap->anon_dev = 0;
 	btrfs_put_root(pending_snapshot->snap);
-	btrfs_subvolume_release_metadata(root, &pending_snapshot->block_rsv);
+	btrfs_block_rsv_release(fs_info, block_rsv, (u64)-1, NULL);
+	if (qgroup_reserved)
+		btrfs_qgroup_free_meta_prealloc(root, qgroup_reserved);
 free_pending:
 	if (pending_snapshot->anon_dev)
 		free_anon_bdev(pending_snapshot->anon_dev);
diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index 4bb538a372ce..7007f9e0c972 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -548,13 +548,3 @@ int btrfs_subvolume_reserve_metadata(struct btrfs_root *root,
 	}
 	return ret;
 }
-
-void btrfs_subvolume_release_metadata(struct btrfs_root *root,
-				      struct btrfs_block_rsv *rsv)
-{
-	struct btrfs_fs_info *fs_info = root->fs_info;
-	u64 qgroup_to_release;
-
-	btrfs_block_rsv_release(fs_info, rsv, (u64)-1, &qgroup_to_release);
-	btrfs_qgroup_convert_reserved_meta(root, qgroup_to_release);
-}
diff --git a/fs/btrfs/root-tree.h b/fs/btrfs/root-tree.h
index 6f929cf3bd49..8f5739e732b9 100644
--- a/fs/btrfs/root-tree.h
+++ b/fs/btrfs/root-tree.h
@@ -18,8 +18,6 @@ struct btrfs_trans_handle;
 int btrfs_subvolume_reserve_metadata(struct btrfs_root *root,
 				     struct btrfs_block_rsv *rsv,
 				     int nitems, bool use_global_rsv);
-void btrfs_subvolume_release_metadata(struct btrfs_root *root,
-				      struct btrfs_block_rsv *rsv);
 int btrfs_add_root_ref(struct btrfs_trans_handle *trans, u64 root_id,
 		       u64 ref_id, u64 dirid, u64 sequence,
 		       const struct fscrypt_str *name);
-- 
2.44.0

[PATCH 3/7] btrfs: record delayed inode root in transaction

[Boris Burkov]

When running delayed inode updates, we do not record the inode's root in
the transaction, but we do allocate PREALLOC and thus converted PERTRANS
space for it. To be sure we free that PERTRANS meta rsv, we must ensure
that we record the root in the transaction.

Fixes: 4f5427ccce5d ("btrfs: delayed-inode: Use new qgroup meta rsv for delayed inode and item")
Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/delayed-inode.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index dd6f566a383f..121ab890bd05 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -1133,6 +1133,9 @@ __btrfs_commit_inode_delayed_items(struct btrfs_trans_handle *trans,
 	if (ret)
 		return ret;
 
+	ret = btrfs_record_root_in_trans(trans, node->root);
+	if (ret)
+		return ret;
 	ret = btrfs_update_delayed_inode(trans, node->root, path, node);
 	return ret;
 }
-- 
2.44.0

[PATCH 4/7] btrfs: convert PREALLOC to PERTRANS after record_root_in_trans

[Boris Burkov]

The transaction is only able to free PERTRANS reservations for a root
once that root has been recorded with the TRANS tag on the roots radix
tree. Therefore, until we are sure that this root will get tagged, it
isn't safe to convert. Generally, this is not an issue as *some*
transaction will likely tag the root before long and this reservation
will get freed in that transaction, but technically it could stick
around until unmount and result in a warning about leaked metadata
reservation space.

This path is most exercised by running the generic/269 xfstest with
CONFIG_BTRFS_DEBUG.

Fixes: a6496849671a ("btrfs: fix start transaction qgroup rsv double free")
Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/transaction.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index feffff91c6fe..1c449d1cea1b 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -745,14 +745,6 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
 		h->reloc_reserved = reloc_reserved;
 	}
 
-	/*
-	 * Now that we have found a transaction to be a part of, convert the
-	 * qgroup reservation from prealloc to pertrans. A different transaction
-	 * can't race in and free our pertrans out from under us.
-	 */
-	if (qgroup_reserved)
-		btrfs_qgroup_convert_reserved_meta(root, qgroup_reserved);
-
 got_it:
 	if (!current->journal_info)
 		current->journal_info = h;
@@ -786,8 +778,15 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
 		 * not just freed.
 		 */
 		btrfs_end_transaction(h);
-		return ERR_PTR(ret);
+		goto reserve_fail;
 	}
+	/*
+	 * Now that we have found a transaction to be a part of, convert the
+	 * qgroup reservation from prealloc to pertrans. A different transaction
+	 * can't race in and free our pertrans out from under us.
+	 */
+	if (qgroup_reserved)
+		btrfs_qgroup_convert_reserved_meta(root, qgroup_reserved);
 
 	return h;
 
-- 
2.44.0

[PATCH 5/7] btrfs: free pertrans at end of cleanup_transaction

[Boris Burkov]

Some of the operations after the free might convert more pertrans
metadata. Do the freeing as late as possible to eliminate a source of
leaked pertrans metadata.

Helps with the pass rate of generic/269 and generic/475.

Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/disk-io.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 3df5477d48a8..4d7893cc0d4e 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4850,8 +4850,6 @@ void btrfs_cleanup_one_transaction(struct btrfs_transaction *cur_trans,
 				     EXTENT_DIRTY);
 	btrfs_destroy_pinned_extent(fs_info, &cur_trans->pinned_extents);
 
-	btrfs_free_all_qgroup_pertrans(fs_info);
-
 	cur_trans->state =TRANS_STATE_COMPLETED;
 	wake_up(&cur_trans->commit_wait);
 }
@@ -4904,6 +4902,7 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
 	btrfs_assert_delayed_root_empty(fs_info);
 	btrfs_destroy_all_delalloc_inodes(fs_info);
 	btrfs_drop_all_logs(fs_info);
+	btrfs_free_all_qgroup_pertrans(fs_info);
 	mutex_unlock(&fs_info->transaction_kthread_mutex);
 
 	return 0;
-- 
2.44.0

[PATCH 6/7] btrfs: btrfs_clear_delalloc_extent frees rsv

[Boris Burkov]

Currently, this callsite only converts the reservation. We are marking
it not delalloc, so I don't think it makes sense to keep the rsv around.
This is a path where we are not sure to join a transaction, so it leads
to incorrect free-ing during umount.

Helps with the pass rate of generic/269 and generic/475

Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 2587a2e25e44..273adbb6b812 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2533,7 +2533,7 @@ void btrfs_clear_delalloc_extent(struct btrfs_inode *inode,
 		 */
 		if (bits & EXTENT_CLEAR_META_RESV &&
 		    root != fs_info->tree_root)
-			btrfs_delalloc_release_metadata(inode, len, false);
+			btrfs_delalloc_release_metadata(inode, len, true);
 
 		/* For sanity tests. */
 		if (btrfs_is_testing(fs_info))
-- 
2.44.0

[PATCH 7/7] btrfs: always clear meta pertrans during commit

[Boris Burkov]

It is possible to clear a root's IN_TRANS tag from the radix tree, but
not clear its pertrans, if there is some error in between. Eliminate
that possibility by moving the free up to where we clear the tag.

Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/transaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 1c449d1cea1b..df2e58aa824a 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1494,6 +1494,7 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
 			radix_tree_tag_clear(&fs_info->fs_roots_radix,
 					(unsigned long)root->root_key.objectid,
 					BTRFS_ROOT_TRANS_TAG);
+			btrfs_qgroup_free_meta_all_pertrans(root);
 			spin_unlock(&fs_info->fs_roots_radix_lock);
 
 			btrfs_free_log(trans, root);
@@ -1518,7 +1519,6 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
 			if (ret2)
 				return ret2;
 			spin_lock(&fs_info->fs_roots_radix_lock);
-			btrfs_qgroup_free_meta_all_pertrans(root);
 		}
 	}
 	spin_unlock(&fs_info->fs_roots_radix_lock);
-- 
2.44.0

Re: [PATCH 1/7] btrfs: correctly model root qgroup rsv in convert

[Qu Wenruo]

在 2024/3/27 08:09, Boris Burkov 写道:
> We use add_root_meta_rsv and sub_root_meta_rsv to track prealloc and
> pertrans reservations for subvols when quotas are enabled. The convert
> function does not properly increment pertrans after decrementing
> prealloc, so the count is not accurate.
> 
> Note: we check that the fs is not read-only to mirror the logic in
> qgroup_convert_meta, which checks that before adding to the pertrans rsv.
> 
> Fixes: 8287475a2055 ("btrfs: qgroup: Use root::qgroup_meta_rsv_* to record qgroup meta reserved space")
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>   fs/btrfs/qgroup.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index a8197e25192c..2cba6451d164 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -4492,6 +4492,8 @@ void btrfs_qgroup_convert_reserved_meta(struct btrfs_root *root, int num_bytes)
>   				      BTRFS_QGROUP_RSV_META_PREALLOC);
>   	trace_qgroup_meta_convert(root, num_bytes);
>   	qgroup_convert_meta(fs_info, root->root_key.objectid, num_bytes);
> +	if (!sb_rdonly(fs_info->sb))
> +		add_root_meta_rsv(root, num_bytes, BTRFS_QGROUP_RSV_META_PERTRANS);

Don't we already call qgroup_rsv_add() inside qgroup_convert_meta()?
This sounds like a double add here.

And if you have any example to show the problem in a more detailed way, 
it would be great help.

Thanks,
Qu
>   }
>   
>   /*

Re: [PATCH 2/7] btrfs: fix qgroup prealloc rsv leak in subvolume operations

[Qu Wenruo]

在 2024/3/27 08:09, Boris Burkov 写道:
> Create subvol, create snapshot and delete subvol all use
> btrfs_subvolume_reserve_metadata to reserve metadata for the changes
> done to the parent subvolume's fs tree, which cannot be mediated in the
> normal way via start_transaction. When quota groups (squota or qgroups)
> are enabled, this reserves qgroup metadata of type PREALLOC. Once the
> operation is associated to a transaction, we convert PREALLOC to
> PERTRANS, which gets cleared in bulk at the end of the transaction.
> 
> However, the error paths of these three operations were not implementing
> this lifecycle correctly. They unconditionally converted the PREALLOC to
> PERTRANS in a generic cleanup step regardless of errors or whether the
> operation was fully associated to a transaction or not. This resulted in
> error paths occasionally converting this rsv to PERTRANS without calling
> record_root_in_trans successfully, which meant that unless that root got
> recorded in the transaction by some other thread, the end of the
> transaction would not free that root's PERTRANS, leaking it. Ultimately,
> this resulted in hitting a WARN in CONFIG_BTRFS_DEBUG builds at unmount
> for the leaked reservation.
> 
> The fix is to ensure that every qgroup PREALLOC reservation observes the
> following properties:
> 1. any failure before record_root_in_trans is called successfully
>     results in freeing the PREALLOC reservation.
> 2. after record_root_in_trans, we convert to PERTRANS, and now the
>     transaction owns freeing the reservation.
> 
> This patch enforces those properties on the three operations. Without
> it, generic/269 with squotas enabled at MKFS time would fail in ~5-10
> runs on my system. With this patch, it ran successfully 1000 times in a
> row.
> 
> Fixes: e85fde5162bf ("btrfs: qgroup: fix qgroup meta rsv leak for subvolume operations")

Thanks for pinning down the bug, and it looks fine to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>   fs/btrfs/inode.c     | 13 ++++++++++++-
>   fs/btrfs/ioctl.c     | 37 ++++++++++++++++++++++++++++---------
>   fs/btrfs/root-tree.c | 10 ----------
>   fs/btrfs/root-tree.h |  2 --
>   4 files changed, 40 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 09f0a20b5ce8..2587a2e25e44 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4503,6 +4503,7 @@ int btrfs_delete_subvolume(struct btrfs_inode *dir, struct dentry *dentry)
>   	struct btrfs_trans_handle *trans;
>   	struct btrfs_block_rsv block_rsv;
>   	u64 root_flags;
> +	u64 qgroup_reserved = 0;
>   	int ret;
>   
>   	down_write(&fs_info->subvol_sem);
> @@ -4547,12 +4548,20 @@ int btrfs_delete_subvolume(struct btrfs_inode *dir, struct dentry *dentry)
>   	ret = btrfs_subvolume_reserve_metadata(root, &block_rsv, 5, true);
>   	if (ret)
>   		goto out_undead;
> +	qgroup_reserved = block_rsv.qgroup_rsv_reserved;
>   
>   	trans = btrfs_start_transaction(root, 0);
>   	if (IS_ERR(trans)) {
>   		ret = PTR_ERR(trans);
>   		goto out_release;
>   	}
> +	ret = btrfs_record_root_in_trans(trans, root);
> +	if (ret) {
> +		btrfs_abort_transaction(trans, ret);
> +		goto out_end_trans;
> +	}
> +	btrfs_qgroup_convert_reserved_meta(root, qgroup_reserved);
> +	qgroup_reserved = 0;
>   	trans->block_rsv = &block_rsv;
>   	trans->bytes_reserved = block_rsv.size;
>   
> @@ -4611,7 +4620,9 @@ int btrfs_delete_subvolume(struct btrfs_inode *dir, struct dentry *dentry)
>   	ret = btrfs_end_transaction(trans);
>   	inode->i_flags |= S_DEAD;
>   out_release:
> -	btrfs_subvolume_release_metadata(root, &block_rsv);
> +	btrfs_block_rsv_release(fs_info, &block_rsv, (u64)-1, NULL);
> +	if (qgroup_reserved)
> +		btrfs_qgroup_free_meta_prealloc(root, qgroup_reserved);
>   out_undead:
>   	if (ret) {
>   		spin_lock(&dest->root_item_lock);
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index e3a72292eda9..888dc92c6c75 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -613,6 +613,7 @@ static noinline int create_subvol(struct mnt_idmap *idmap,
>   	int ret;
>   	dev_t anon_dev;
>   	u64 objectid;
> +	u64 qgroup_reserved = 0;
>   
>   	root_item = kzalloc(sizeof(*root_item), GFP_KERNEL);
>   	if (!root_item)
> @@ -650,13 +651,18 @@ static noinline int create_subvol(struct mnt_idmap *idmap,
>   					       trans_num_items, false);
>   	if (ret)
>   		goto out_new_inode_args;
> +	qgroup_reserved = block_rsv.qgroup_rsv_reserved;
>   
>   	trans = btrfs_start_transaction(root, 0);
>   	if (IS_ERR(trans)) {
>   		ret = PTR_ERR(trans);
> -		btrfs_subvolume_release_metadata(root, &block_rsv);
> -		goto out_new_inode_args;
> +		goto out_release_rsv;
>   	}
> +	ret = btrfs_record_root_in_trans(trans, BTRFS_I(dir)->root);
> +	if (ret)
> +		goto out;
> +	btrfs_qgroup_convert_reserved_meta(root, qgroup_reserved);
> +	qgroup_reserved = 0;
>   	trans->block_rsv = &block_rsv;
>   	trans->bytes_reserved = block_rsv.size;
>   	/* Tree log can't currently deal with an inode which is a new root. */
> @@ -767,9 +773,11 @@ static noinline int create_subvol(struct mnt_idmap *idmap,
>   out:
>   	trans->block_rsv = NULL;
>   	trans->bytes_reserved = 0;
> -	btrfs_subvolume_release_metadata(root, &block_rsv);
> -
>   	btrfs_end_transaction(trans);
> +out_release_rsv:
> +	btrfs_block_rsv_release(fs_info, &block_rsv, (u64)-1, NULL);
> +	if (qgroup_reserved)
> +		btrfs_qgroup_free_meta_prealloc(root, qgroup_reserved);
>   out_new_inode_args:
>   	btrfs_new_inode_args_destroy(&new_inode_args);
>   out_inode:
> @@ -791,6 +799,8 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
>   	struct btrfs_pending_snapshot *pending_snapshot;
>   	unsigned int trans_num_items;
>   	struct btrfs_trans_handle *trans;
> +	struct btrfs_block_rsv *block_rsv;
> +	u64 qgroup_reserved = 0;
>   	int ret;
>   
>   	/* We do not support snapshotting right now. */
> @@ -827,19 +837,19 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
>   		goto free_pending;
>   	}
>   
> -	btrfs_init_block_rsv(&pending_snapshot->block_rsv,
> -			     BTRFS_BLOCK_RSV_TEMP);
> +	block_rsv = &pending_snapshot->block_rsv;
> +	btrfs_init_block_rsv(block_rsv, BTRFS_BLOCK_RSV_TEMP);
>   	/*
>   	 * 1 to add dir item
>   	 * 1 to add dir index
>   	 * 1 to update parent inode item
>   	 */
>   	trans_num_items = create_subvol_num_items(inherit) + 3;
> -	ret = btrfs_subvolume_reserve_metadata(BTRFS_I(dir)->root,
> -					       &pending_snapshot->block_rsv,
> +	ret = btrfs_subvolume_reserve_metadata(BTRFS_I(dir)->root, block_rsv,
>   					       trans_num_items, false);
>   	if (ret)
>   		goto free_pending;
> +	qgroup_reserved = block_rsv->qgroup_rsv_reserved;
>   
>   	pending_snapshot->dentry = dentry;
>   	pending_snapshot->root = root;
> @@ -852,6 +862,13 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
>   		ret = PTR_ERR(trans);
>   		goto fail;
>   	}
> +	ret = btrfs_record_root_in_trans(trans, BTRFS_I(dir)->root);
> +	if (ret) {
> +		btrfs_end_transaction(trans);
> +		goto fail;
> +	}
> +	btrfs_qgroup_convert_reserved_meta(root, qgroup_reserved);
> +	qgroup_reserved = 0;
>   
>   	trans->pending_snapshot = pending_snapshot;
>   
> @@ -881,7 +898,9 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
>   	if (ret && pending_snapshot->snap)
>   		pending_snapshot->snap->anon_dev = 0;
>   	btrfs_put_root(pending_snapshot->snap);
> -	btrfs_subvolume_release_metadata(root, &pending_snapshot->block_rsv);
> +	btrfs_block_rsv_release(fs_info, block_rsv, (u64)-1, NULL);
> +	if (qgroup_reserved)
> +		btrfs_qgroup_free_meta_prealloc(root, qgroup_reserved);
>   free_pending:
>   	if (pending_snapshot->anon_dev)
>   		free_anon_bdev(pending_snapshot->anon_dev);
> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> index 4bb538a372ce..7007f9e0c972 100644
> --- a/fs/btrfs/root-tree.c
> +++ b/fs/btrfs/root-tree.c
> @@ -548,13 +548,3 @@ int btrfs_subvolume_reserve_metadata(struct btrfs_root *root,
>   	}
>   	return ret;
>   }
> -
> -void btrfs_subvolume_release_metadata(struct btrfs_root *root,
> -				      struct btrfs_block_rsv *rsv)
> -{
> -	struct btrfs_fs_info *fs_info = root->fs_info;
> -	u64 qgroup_to_release;
> -
> -	btrfs_block_rsv_release(fs_info, rsv, (u64)-1, &qgroup_to_release);
> -	btrfs_qgroup_convert_reserved_meta(root, qgroup_to_release);
> -}
> diff --git a/fs/btrfs/root-tree.h b/fs/btrfs/root-tree.h
> index 6f929cf3bd49..8f5739e732b9 100644
> --- a/fs/btrfs/root-tree.h
> +++ b/fs/btrfs/root-tree.h
> @@ -18,8 +18,6 @@ struct btrfs_trans_handle;
>   int btrfs_subvolume_reserve_metadata(struct btrfs_root *root,
>   				     struct btrfs_block_rsv *rsv,
>   				     int nitems, bool use_global_rsv);
> -void btrfs_subvolume_release_metadata(struct btrfs_root *root,
> -				      struct btrfs_block_rsv *rsv);
>   int btrfs_add_root_ref(struct btrfs_trans_handle *trans, u64 root_id,
>   		       u64 ref_id, u64 dirid, u64 sequence,
>   		       const struct fscrypt_str *name);

Re: [PATCH 3/7] btrfs: record delayed inode root in transaction

[Qu Wenruo]

在 2024/3/27 08:09, Boris Burkov 写道:
> When running delayed inode updates, we do not record the inode's root in
> the transaction, but we do allocate PREALLOC and thus converted PERTRANS
> space for it. To be sure we free that PERTRANS meta rsv, we must ensure
> that we record the root in the transaction.
> 
> Fixes: 4f5427ccce5d ("btrfs: delayed-inode: Use new qgroup meta rsv for delayed inode and item")

Reviewed-by: Qu Wenruo <wqu@suse.com>

Just curious, do you have a case that hits this particular bug only?

Thanks,
Qu
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>   fs/btrfs/delayed-inode.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index dd6f566a383f..121ab890bd05 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -1133,6 +1133,9 @@ __btrfs_commit_inode_delayed_items(struct btrfs_trans_handle *trans,
>   	if (ret)
>   		return ret;
>   
> +	ret = btrfs_record_root_in_trans(trans, node->root);
> +	if (ret)
> +		return ret;
>   	ret = btrfs_update_delayed_inode(trans, node->root, path, node);
>   	return ret;
>   }

Re: [PATCH 4/7] btrfs: convert PREALLOC to PERTRANS after record_root_in_trans

[Qu Wenruo]

在 2024/3/27 08:09, Boris Burkov 写道:
> The transaction is only able to free PERTRANS reservations for a root
> once that root has been recorded with the TRANS tag on the roots radix
> tree. Therefore, until we are sure that this root will get tagged, it
> isn't safe to convert. Generally, this is not an issue as *some*
> transaction will likely tag the root before long and this reservation
> will get freed in that transaction, but technically it could stick
> around until unmount and result in a warning about leaked metadata
> reservation space.
> 
> This path is most exercised by running the generic/269 xfstest with
> CONFIG_BTRFS_DEBUG.
> 
> Fixes: a6496849671a ("btrfs: fix start transaction qgroup rsv double free")

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>   fs/btrfs/transaction.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index feffff91c6fe..1c449d1cea1b 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -745,14 +745,6 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
>   		h->reloc_reserved = reloc_reserved;
>   	}
>   
> -	/*
> -	 * Now that we have found a transaction to be a part of, convert the
> -	 * qgroup reservation from prealloc to pertrans. A different transaction
> -	 * can't race in and free our pertrans out from under us.
> -	 */
> -	if (qgroup_reserved)
> -		btrfs_qgroup_convert_reserved_meta(root, qgroup_reserved);
> -
>   got_it:
>   	if (!current->journal_info)
>   		current->journal_info = h;
> @@ -786,8 +778,15 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
>   		 * not just freed.
>   		 */
>   		btrfs_end_transaction(h);
> -		return ERR_PTR(ret);
> +		goto reserve_fail;
>   	}
> +	/*
> +	 * Now that we have found a transaction to be a part of, convert the
> +	 * qgroup reservation from prealloc to pertrans. A different transaction
> +	 * can't race in and free our pertrans out from under us.
> +	 */
> +	if (qgroup_reserved)
> +		btrfs_qgroup_convert_reserved_meta(root, qgroup_reserved);
>   
>   	return h;
>   

Re: [PATCH 5/7] btrfs: free pertrans at end of cleanup_transaction

[Qu Wenruo]

在 2024/3/27 08:09, Boris Burkov 写道:
> Some of the operations after the free might convert more pertrans
> metadata. Do the freeing as late as possible to eliminate a source of
> leaked pertrans metadata.
> 
> Helps with the pass rate of generic/269 and generic/475.
> 
> Signed-off-by: Boris Burkov <boris@bur.io>

Well, you can also move other fs level cleanup out of the 
btrfs_cleanup_one_transaction() call.
(e.g. destory_delayed_inodes()).

For qgroup part, it looks fine to me as a precautious behavior.

Thanks,
Qu

> ---
>   fs/btrfs/disk-io.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 3df5477d48a8..4d7893cc0d4e 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4850,8 +4850,6 @@ void btrfs_cleanup_one_transaction(struct btrfs_transaction *cur_trans,
>   				     EXTENT_DIRTY);
>   	btrfs_destroy_pinned_extent(fs_info, &cur_trans->pinned_extents);
>   
> -	btrfs_free_all_qgroup_pertrans(fs_info);
> -
>   	cur_trans->state =TRANS_STATE_COMPLETED;
>   	wake_up(&cur_trans->commit_wait);
>   }
> @@ -4904,6 +4902,7 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
>   	btrfs_assert_delayed_root_empty(fs_info);
>   	btrfs_destroy_all_delalloc_inodes(fs_info);
>   	btrfs_drop_all_logs(fs_info);
> +	btrfs_free_all_qgroup_pertrans(fs_info);
>   	mutex_unlock(&fs_info->transaction_kthread_mutex);
>   
>   	return 0;

Re: [PATCH 7/7] btrfs: always clear meta pertrans during commit

[Qu Wenruo]

在 2024/3/27 08:09, Boris Burkov 写道:
> It is possible to clear a root's IN_TRANS tag from the radix tree, but
> not clear its pertrans, if there is some error in between. Eliminate
> that possibility by moving the free up to where we clear the tag.
> 
> Signed-off-by: Boris Burkov <boris@bur.io>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   fs/btrfs/transaction.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 1c449d1cea1b..df2e58aa824a 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1494,6 +1494,7 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
>   			radix_tree_tag_clear(&fs_info->fs_roots_radix,
>   					(unsigned long)root->root_key.objectid,
>   					BTRFS_ROOT_TRANS_TAG);
> +			btrfs_qgroup_free_meta_all_pertrans(root);
>   			spin_unlock(&fs_info->fs_roots_radix_lock);
>   
>   			btrfs_free_log(trans, root);
> @@ -1518,7 +1519,6 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
>   			if (ret2)
>   				return ret2;
>   			spin_lock(&fs_info->fs_roots_radix_lock);
> -			btrfs_qgroup_free_meta_all_pertrans(root);
>   		}
>   	}
>   	spin_unlock(&fs_info->fs_roots_radix_lock);

Re: [PATCH 6/7] btrfs: btrfs_clear_delalloc_extent frees rsv

[Qu Wenruo]

在 2024/3/27 08:09, Boris Burkov 写道:
> Currently, this callsite only converts the reservation. We are marking
> it not delalloc, so I don't think it makes sense to keep the rsv around.
> This is a path where we are not sure to join a transaction, so it leads
> to incorrect free-ing during umount.
> 
> Helps with the pass rate of generic/269 and generic/475

I guess the problem of all these ENOSPC/hutdown test cases is their 
reproducibility.

Unlike regular fsstress which can be very reproducible with its seed, 
it's pretty hard to reproduce a situation where you hit a certain qgroup 
leak.

Maybe the qgroup rsv leak detection is a little too strict for aborted 
transactions?

Anyway, the patch itself looks fine.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> 
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>   fs/btrfs/inode.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 2587a2e25e44..273adbb6b812 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2533,7 +2533,7 @@ void btrfs_clear_delalloc_extent(struct btrfs_inode *inode,
>   		 */
>   		if (bits & EXTENT_CLEAR_META_RESV &&
>   		    root != fs_info->tree_root)
> -			btrfs_delalloc_release_metadata(inode, len, false);
> +			btrfs_delalloc_release_metadata(inode, len, true);
>   
>   		/* For sanity tests. */
>   		if (btrfs_is_testing(fs_info))

Re: [PATCH 1/7] btrfs: correctly model root qgroup rsv in convert

[Boris Burkov]

On Wed, Mar 27, 2024 at 08:30:47AM +1030, Qu Wenruo wrote:
> 
> 
> 在 2024/3/27 08:09, Boris Burkov 写道:
> > We use add_root_meta_rsv and sub_root_meta_rsv to track prealloc and
> > pertrans reservations for subvols when quotas are enabled. The convert
> > function does not properly increment pertrans after decrementing
> > prealloc, so the count is not accurate.
> > 
> > Note: we check that the fs is not read-only to mirror the logic in
> > qgroup_convert_meta, which checks that before adding to the pertrans rsv.
> > 
> > Fixes: 8287475a2055 ("btrfs: qgroup: Use root::qgroup_meta_rsv_* to record qgroup meta reserved space")
> > Signed-off-by: Boris Burkov <boris@bur.io>
> > ---
> >   fs/btrfs/qgroup.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> > index a8197e25192c..2cba6451d164 100644
> > --- a/fs/btrfs/qgroup.c
> > +++ b/fs/btrfs/qgroup.c
> > @@ -4492,6 +4492,8 @@ void btrfs_qgroup_convert_reserved_meta(struct btrfs_root *root, int num_bytes)
> >   				      BTRFS_QGROUP_RSV_META_PREALLOC);
> >   	trace_qgroup_meta_convert(root, num_bytes);
> >   	qgroup_convert_meta(fs_info, root->root_key.objectid, num_bytes);
> > +	if (!sb_rdonly(fs_info->sb))
> > +		add_root_meta_rsv(root, num_bytes, BTRFS_QGROUP_RSV_META_PERTRANS);
> 
> Don't we already call qgroup_rsv_add() inside qgroup_convert_meta()?
> This sounds like a double add here.

qgroup_rsv_add doesn't call add_root_meta_rsv. AFAICT, this is the
separate accounting in root->qgroup_meta_rsv_pertrans to fixup underflow
errors as we free.

> 
> And if you have any example to show the problem in a more detailed way, it
> would be great help.

I don't have a reproducer for this, it was just something I noticed. I'm
fine to drop this patch if you don't think it's worth the churn (and
certainly if I'm just a dummy and didn't see where we already call it)

In fact, this counter only exists to avoid underflow, but PERTRANS is
cleared by exact amount, and not via btrfs_qgroup_free_meta_pertrans, so
it might just be moot to track it at all in this way.

> 
> Thanks,
> Qu
> >   }
> >   /*

Re: [PATCH 3/7] btrfs: record delayed inode root in transaction

[Boris Burkov]

On Wed, Mar 27, 2024 at 08:38:53AM +1030, Qu Wenruo wrote:
> 
> 
> 在 2024/3/27 08:09, Boris Burkov 写道:
> > When running delayed inode updates, we do not record the inode's root in
> > the transaction, but we do allocate PREALLOC and thus converted PERTRANS
> > space for it. To be sure we free that PERTRANS meta rsv, we must ensure
> > that we record the root in the transaction.
> > 
> > Fixes: 4f5427ccce5d ("btrfs: delayed-inode: Use new qgroup meta rsv for delayed inode and item")
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> Just curious, do you have a case that hits this particular bug only?

I tested all of these fixes just by running generic/269 and generic/475
in a loop and driving the meta rsv failures down to 0. I *believe* all
of them are necessary to get to fully 0.

> 
> Thanks,
> Qu
> > Signed-off-by: Boris Burkov <boris@bur.io>
> > ---
> >   fs/btrfs/delayed-inode.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> > index dd6f566a383f..121ab890bd05 100644
> > --- a/fs/btrfs/delayed-inode.c
> > +++ b/fs/btrfs/delayed-inode.c
> > @@ -1133,6 +1133,9 @@ __btrfs_commit_inode_delayed_items(struct btrfs_trans_handle *trans,
> >   	if (ret)
> >   		return ret;
> > +	ret = btrfs_record_root_in_trans(trans, node->root);
> > +	if (ret)
> > +		return ret;
> >   	ret = btrfs_update_delayed_inode(trans, node->root, path, node);
> >   	return ret;
> >   }

Re: [PATCH 5/7] btrfs: free pertrans at end of cleanup_transaction

[Boris Burkov]

On Wed, Mar 27, 2024 at 08:46:39AM +1030, Qu Wenruo wrote:
> 
> 
> 在 2024/3/27 08:09, Boris Burkov 写道:
> > Some of the operations after the free might convert more pertrans
> > metadata. Do the freeing as late as possible to eliminate a source of
> > leaked pertrans metadata.
> > 
> > Helps with the pass rate of generic/269 and generic/475.
> > 
> > Signed-off-by: Boris Burkov <boris@bur.io>
> 
> Well, you can also move other fs level cleanup out of the
> btrfs_cleanup_one_transaction() call.
> (e.g. destory_delayed_inodes()).
> 
> For qgroup part, it looks fine to me as a precautious behavior.

Since the call isn't per transaction, do you think it just makes more
sense to call it once per cleanup not once per trans per cleanup?

Or would you rather I refactored it some other way?

> 
> Thanks,
> Qu
> 
> > ---
> >   fs/btrfs/disk-io.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index 3df5477d48a8..4d7893cc0d4e 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -4850,8 +4850,6 @@ void btrfs_cleanup_one_transaction(struct btrfs_transaction *cur_trans,
> >   				     EXTENT_DIRTY);
> >   	btrfs_destroy_pinned_extent(fs_info, &cur_trans->pinned_extents);
> > -	btrfs_free_all_qgroup_pertrans(fs_info);
> > -
> >   	cur_trans->state =TRANS_STATE_COMPLETED;
> >   	wake_up(&cur_trans->commit_wait);
> >   }
> > @@ -4904,6 +4902,7 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
> >   	btrfs_assert_delayed_root_empty(fs_info);
> >   	btrfs_destroy_all_delalloc_inodes(fs_info);
> >   	btrfs_drop_all_logs(fs_info);
> > +	btrfs_free_all_qgroup_pertrans(fs_info);
> >   	mutex_unlock(&fs_info->transaction_kthread_mutex);
> >   	return 0;

Re: [PATCH 6/7] btrfs: btrfs_clear_delalloc_extent frees rsv

[Boris Burkov]

On Wed, Mar 27, 2024 at 08:56:20AM +1030, Qu Wenruo wrote:
> 
> 
> 在 2024/3/27 08:09, Boris Burkov 写道:
> > Currently, this callsite only converts the reservation. We are marking
> > it not delalloc, so I don't think it makes sense to keep the rsv around.
> > This is a path where we are not sure to join a transaction, so it leads
> > to incorrect free-ing during umount.
> > 
> > Helps with the pass rate of generic/269 and generic/475
> 
> I guess the problem of all these ENOSPC/hutdown test cases is their
> reproducibility.

Yeah, it is definitely annoying to have to run generic/269 and
generic/475 hundreds of times to hit various different flavors of bugs
and try to drive it to 0. :/ It's hard to be sure that you are actually
successful and which fixes are definitely 100% necessary.

> 
> Unlike regular fsstress which can be very reproducible with its seed, it's
> pretty hard to reproduce a situation where you hit a certain qgroup leak.
> 
> Maybe the qgroup rsv leak detection is a little too strict for aborted
> transactions?

I agree for aborted transactions. It feels like a cheat just to beat the
warning. There are many failure paths that don't end in an aborted
transaction that we probably do actually care about, though.

> 
> Anyway, the patch itself looks fine.

Thanks for all the review on this series, btw!

> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> Thanks,
> Qu
> > 
> > Signed-off-by: Boris Burkov <boris@bur.io>
> > ---
> >   fs/btrfs/inode.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 2587a2e25e44..273adbb6b812 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -2533,7 +2533,7 @@ void btrfs_clear_delalloc_extent(struct btrfs_inode *inode,
> >   		 */
> >   		if (bits & EXTENT_CLEAR_META_RESV &&
> >   		    root != fs_info->tree_root)
> > -			btrfs_delalloc_release_metadata(inode, len, false);
> > +			btrfs_delalloc_release_metadata(inode, len, true);
> >   		/* For sanity tests. */
> >   		if (btrfs_is_testing(fs_info))

Re: [PATCH 1/7] btrfs: correctly model root qgroup rsv in convert

[Qu Wenruo]

在 2024/3/28 03:50, Boris Burkov 写道:
> On Wed, Mar 27, 2024 at 08:30:47AM +1030, Qu Wenruo wrote:
>>
>>
>> 在 2024/3/27 08:09, Boris Burkov 写道:
>>> We use add_root_meta_rsv and sub_root_meta_rsv to track prealloc and
>>> pertrans reservations for subvols when quotas are enabled. The convert
>>> function does not properly increment pertrans after decrementing
>>> prealloc, so the count is not accurate.
>>>
>>> Note: we check that the fs is not read-only to mirror the logic in
>>> qgroup_convert_meta, which checks that before adding to the pertrans rsv.
>>>
>>> Fixes: 8287475a2055 ("btrfs: qgroup: Use root::qgroup_meta_rsv_* to record qgroup meta reserved space")
>>> Signed-off-by: Boris Burkov <boris@bur.io>
>>> ---
>>>    fs/btrfs/qgroup.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>> index a8197e25192c..2cba6451d164 100644
>>> --- a/fs/btrfs/qgroup.c
>>> +++ b/fs/btrfs/qgroup.c
>>> @@ -4492,6 +4492,8 @@ void btrfs_qgroup_convert_reserved_meta(struct btrfs_root *root, int num_bytes)
>>>    				      BTRFS_QGROUP_RSV_META_PREALLOC);
>>>    	trace_qgroup_meta_convert(root, num_bytes);
>>>    	qgroup_convert_meta(fs_info, root->root_key.objectid, num_bytes);
>>> +	if (!sb_rdonly(fs_info->sb))
>>> +		add_root_meta_rsv(root, num_bytes, BTRFS_QGROUP_RSV_META_PERTRANS);
>>
>> Don't we already call qgroup_rsv_add() inside qgroup_convert_meta()?
>> This sounds like a double add here.
>
> qgroup_rsv_add doesn't call add_root_meta_rsv. AFAICT, this is the
> separate accounting in root->qgroup_meta_rsv_pertrans to fixup underflow
> errors as we free.

My bad, it's true that we have extra per-root accounting that's not the
same as qgroup rsv.


>
>>
>> And if you have any example to show the problem in a more detailed way, it
>> would be great help.
>
> I don't have a reproducer for this, it was just something I noticed. I'm
> fine to drop this patch if you don't think it's worth the churn (and
> certainly if I'm just a dummy and didn't see where we already call it)

In that case, I think you're correct and the patch looks good to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

>
> In fact, this counter only exists to avoid underflow, but PERTRANS is
> cleared by exact amount, and not via btrfs_qgroup_free_meta_pertrans, so
> it might just be moot to track it at all in this way.
>
>>
>> Thanks,
>> Qu
>>>    }
>>>    /*
>

Re: [PATCH 6/7] btrfs: btrfs_clear_delalloc_extent frees rsv

[Qu Wenruo]

在 2024/3/28 03:56, Boris Burkov 写道:
> On Wed, Mar 27, 2024 at 08:56:20AM +1030, Qu Wenruo wrote:
>>
>>
>> 在 2024/3/27 08:09, Boris Burkov 写道:
>>> Currently, this callsite only converts the reservation. We are marking
>>> it not delalloc, so I don't think it makes sense to keep the rsv around.
>>> This is a path where we are not sure to join a transaction, so it leads
>>> to incorrect free-ing during umount.
>>>
>>> Helps with the pass rate of generic/269 and generic/475
>>
>> I guess the problem of all these ENOSPC/hutdown test cases is their
>> reproducibility.
>
> Yeah, it is definitely annoying to have to run generic/269 and
> generic/475 hundreds of times to hit various different flavors of bugs
> and try to drive it to 0. :/ It's hard to be sure that you are actually
> successful and which fixes are definitely 100% necessary.

I'm wondering if it's possible to add fsstress workload to inject errors
(to specified injection points).

IIRC we have error injection points for ENOSPC and ENOMEM, and fsstress
is so far the most reliable reproducer.

I hope that can greatly improve our reproducibility on the error paths.

>
>>
>> Unlike regular fsstress which can be very reproducible with its seed, it's
>> pretty hard to reproduce a situation where you hit a certain qgroup leak.
>>
>> Maybe the qgroup rsv leak detection is a little too strict for aborted
>> transactions?
>
> I agree for aborted transactions. It feels like a cheat just to beat the
> warning. There are many failure paths that don't end in an aborted
> transaction that we probably do actually care about, though.

Indeed, despite the aborted transactions, we still have a lot of ENOMEM
(less common though) and ENOSPC (much more common).

Thanks,
Qu
>
>>
>> Anyway, the patch itself looks fine.
>
> Thanks for all the review on this series, btw!
>
>>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>
>> Thanks,
>> Qu
>>>
>>> Signed-off-by: Boris Burkov <boris@bur.io>
>>> ---
>>>    fs/btrfs/inode.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index 2587a2e25e44..273adbb6b812 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -2533,7 +2533,7 @@ void btrfs_clear_delalloc_extent(struct btrfs_inode *inode,
>>>    		 */
>>>    		if (bits & EXTENT_CLEAR_META_RESV &&
>>>    		    root != fs_info->tree_root)
>>> -			btrfs_delalloc_release_metadata(inode, len, false);
>>> +			btrfs_delalloc_release_metadata(inode, len, true);
>>>    		/* For sanity tests. */
>>>    		if (btrfs_is_testing(fs_info))
>

Re: [PATCH 5/7] btrfs: free pertrans at end of cleanup_transaction

[Qu Wenruo]

在 2024/3/28 03:52, Boris Burkov 写道:
> On Wed, Mar 27, 2024 at 08:46:39AM +1030, Qu Wenruo wrote:
>>
>>
>> 在 2024/3/27 08:09, Boris Burkov 写道:
>>> Some of the operations after the free might convert more pertrans
>>> metadata. Do the freeing as late as possible to eliminate a source of
>>> leaked pertrans metadata.
>>>
>>> Helps with the pass rate of generic/269 and generic/475.
>>>
>>> Signed-off-by: Boris Burkov <boris@bur.io>
>>
>> Well, you can also move other fs level cleanup out of the
>> btrfs_cleanup_one_transaction() call.
>> (e.g. destory_delayed_inodes()).
>>
>> For qgroup part, it looks fine to me as a precautious behavior.
> 
> Since the call isn't per transaction, do you think it just makes more
> sense to call it once per cleanup not once per trans per cleanup?

Yes, just like what you did for btrfs_free_all_qgroup_pertrans().

> 
> Or would you rather I refactored it some other way?

Just an idea for future cleanups (moving all global cleanups out of 
btrfs_cleanup_one_transaction()).

Thanks,
Qu

> 
>>
>> Thanks,
>> Qu
>>
>>> ---
>>>    fs/btrfs/disk-io.c | 3 +--
>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>> index 3df5477d48a8..4d7893cc0d4e 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -4850,8 +4850,6 @@ void btrfs_cleanup_one_transaction(struct btrfs_transaction *cur_trans,
>>>    				     EXTENT_DIRTY);
>>>    	btrfs_destroy_pinned_extent(fs_info, &cur_trans->pinned_extents);
>>> -	btrfs_free_all_qgroup_pertrans(fs_info);
>>> -
>>>    	cur_trans->state =TRANS_STATE_COMPLETED;
>>>    	wake_up(&cur_trans->commit_wait);
>>>    }
>>> @@ -4904,6 +4902,7 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
>>>    	btrfs_assert_delayed_root_empty(fs_info);
>>>    	btrfs_destroy_all_delalloc_inodes(fs_info);
>>>    	btrfs_drop_all_logs(fs_info);
>>> +	btrfs_free_all_qgroup_pertrans(fs_info);
>>>    	mutex_unlock(&fs_info->transaction_kthread_mutex);
>>>    	return 0;