[PATCH 0/7] btrfs: avoid some unnecessary commit of empty transactions

[PATCH 0/7] btrfs: avoid some unnecessary commit of empty transactions

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

A few places can unnecessarily create an empty transaction and then commit
it, when the goal is just to catch the current transaction and wait for
its commit to complete. This results in wasting IO, time and rotation of
the precious backup roots in the super block. Details in the change logs.
The patches are all independent, except patch 4 that applies on top of
patch 3 (but could have been done in any order really, they are independent).

Filipe Manana (7):
  btrfs: qgroup: avoid start/commit empty transaction when flushing reservations
  btrfs: avoid create and commit empty transaction when committing super
  btrfs: send: make ensure_commit_roots_uptodate() simpler and more efficient
  btrfs: send: avoid create/commit empty transaction at ensure_commit_roots_uptodate()
  btrfs: scrub: avoid create/commit empty transaction at finish_extent_writes_for_zoned()
  btrfs: add and use helper to commit the current transaction
  btrfs: send: get rid of the label and gotos at ensure_commit_roots_uptodate()

 fs/btrfs/disk-io.c     |  8 +-------
 fs/btrfs/qgroup.c      | 31 +++++--------------------------
 fs/btrfs/scrub.c       |  6 +-----
 fs/btrfs/send.c        | 32 ++++++++------------------------
 fs/btrfs/space-info.c  |  9 +--------
 fs/btrfs/super.c       | 11 +----------
 fs/btrfs/transaction.c | 19 +++++++++++++++++++
 fs/btrfs/transaction.h |  1 +
 8 files changed, 37 insertions(+), 80 deletions(-)

-- 
2.43.0

[PATCH 1/7] btrfs: qgroup: avoid start/commit empty transaction when flushing reservations

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When flushing reservations we are using btrfs_join_transaction() to get a
handle for the current transaction and then commit it to try to release
space. However btrfs_join_transaction() has some undesirable consequences:

1) If there's no running transaction, it will create one, and we will
   commit it right after. This is unncessary because it will not release
   any space, and it will result in unnecessary IO and rotation of backup
   roots in the superblock;

2) If there's a current transaction and that transaction is committing
   (its state is >= TRANS_STATE_COMMIT_DOING), it will wait for that
   transaction to almost finish its commit (for its state to be >=
   TRANS_STATE_UNBLOCKED) and then start and return a new transaction.

   We will then commit that new transaction, which is pointless because
   all we wanted was to wait for the current (previous) transaction to
   fully finish its commit (state == TRANS_STATE_COMPLETED), and by
   starting and committing a new transaction we are wasting IO too and
   causing unnecessary rotation of backup roots in the superblock.

So improve this by using btrfs_attach_transaction_barrier() instead, which
does not create a new transaction if there's none running, and if there's
a current transaction that is committing, it will wait for it to fully
commit and not create a new transaction.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/qgroup.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index a2e329710287..391af9e79dd6 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1341,12 +1341,14 @@ static int flush_reservations(struct btrfs_fs_info *fs_info)
 	if (ret)
 		return ret;
 	btrfs_wait_ordered_roots(fs_info, U64_MAX, NULL);
-	trans = btrfs_join_transaction(fs_info->tree_root);
-	if (IS_ERR(trans))
-		return PTR_ERR(trans);
-	ret = btrfs_commit_transaction(trans);
 
-	return ret;
+	trans = btrfs_attach_transaction_barrier(fs_info->tree_root);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		return (ret == -ENOENT) ? 0 : ret;
+	}
+
+	return btrfs_commit_transaction(trans);
 }
 
 int btrfs_quota_disable(struct btrfs_fs_info *fs_info)
-- 
2.43.0

[PATCH 2/7] btrfs: avoid create and commit empty transaction when committing super

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

At btrfs_commit_super(), called in a few contextes such as when unmounting
a filesystem, we use btrfs_join_transaction() to catch any running
transaction and then commit it. This will however create a new and empty
transaction in case there's no running transaction or there's a running
transaction with a state >= TRANS_STATE_UNBLOCKED.

As we just want to be sure that any existing transaction is fully
committed, we can use btrfs_attach_transaction_barrier() instead of
btrfs_join_transaction(), therefore avoiding the creation and commit of
empty transactions, which only waste IO and causes rotation of the
precious backup roots.

Example where we create and commit a pointless empty transaction:

  $ mkfs.btrfs -f /dev/sdj
  $ btrfs inspect-internal dump-super /dev/sdj | grep -e '^generation'
  generation            6

  $ mount /dev/sdj /mnt/sdj
  $ touch /mnt/sdj/foo

  # Commit the currently open transaction. Just 'sync' or wait ~30
  # seconds for the transaction kthread to commit it.
  $ sync

  $ btrfs inspect-internal dump-super /dev/sdj | grep -e '^generation'
  generation            7

  $ umount /mnt/sdj

  $ btrfs inspect-internal dump-super /dev/sdj | grep -e '^generation'
  generation            8

The transaction with id 8 was pointless, an empty transaction that did
not achieve anything.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/disk-io.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index eec5bb392b8e..2f56f967beb8 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4156,9 +4156,13 @@ int btrfs_commit_super(struct btrfs_fs_info *fs_info)
 	down_write(&fs_info->cleanup_work_sem);
 	up_write(&fs_info->cleanup_work_sem);
 
-	trans = btrfs_join_transaction(root);
-	if (IS_ERR(trans))
-		return PTR_ERR(trans);
+	trans = btrfs_attach_transaction_barrier(root);
+	if (IS_ERR(trans)) {
+		int ret = PTR_ERR(trans);
+
+		return (ret == -ENOENT) ? 0 : ret;
+	}
+
 	return btrfs_commit_transaction(trans);
 }
 
-- 
2.43.0

[PATCH 3/7] btrfs: send: make ensure_commit_roots_uptodate() simpler and more efficient

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Before starting a send operation we have to make sure that every root has
its commit root matching the regular root, to that send doesn't find stale
inodes in the commit root (inodes that were deleted in the regular root)
and fails the inode lookups with -ESTALE.

Currently we keep looking for roots used by the send operation and as soon
as we find one we commit the current transaction (or a new one since
btrfs_join_transaction() creates one if there isn't any running or the
running one is in a state >= TRANS_STATE_UNBLOCKED). It's pointless to
keep looking until we don't find any, because after the first transaction
commit all the other roots are updated too, as they were already tagged in
the fs_info->fs_roots_radix radix tree when they were modified in order to
have a commit root different from the regular root.

Currently we are also always passing the main send root into
btrfs_join_transaction(), which despite not having any functional issue,
it is not optimal because in case the root wasn't modified we end up
adding it to fs_info->fs_roots_radix and then update its root item in the
root tree when comitting the transaction, causing unnecessary work.

So simplify and make this more efficient by removing the looping and by
passing the first root we found that is modified as the argument to
btrfs_join_transaction().

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/send.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index c69743233be5..2c46bd1c90d3 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -7998,32 +7998,29 @@ static int send_subvol(struct send_ctx *sctx)
  */
 static int ensure_commit_roots_uptodate(struct send_ctx *sctx)
 {
-	int i;
-	struct btrfs_trans_handle *trans = NULL;
+	struct btrfs_trans_handle *trans;
+	struct btrfs_root *root = sctx->parent_root;
 
-again:
-	if (sctx->parent_root &&
-	    sctx->parent_root->node != sctx->parent_root->commit_root)
+	if (root && root->node != root->commit_root)
 		goto commit_trans;
 
-	for (i = 0; i < sctx->clone_roots_cnt; i++)
-		if (sctx->clone_roots[i].root->node !=
-		    sctx->clone_roots[i].root->commit_root)
+	for (int i = 0; i < sctx->clone_roots_cnt; i++) {
+		root = sctx->clone_roots[i].root;
+		if (root->node != root->commit_root)
 			goto commit_trans;
-
-	if (trans)
-		return btrfs_end_transaction(trans);
+	}
 
 	return 0;
 
 commit_trans:
-	/* Use any root, all fs roots will get their commit roots updated. */
-	if (!trans) {
-		trans = btrfs_join_transaction(sctx->send_root);
-		if (IS_ERR(trans))
-			return PTR_ERR(trans);
-		goto again;
-	}
+	/*
+	 * Use the first root we found. We could use any but that would cause
+	 * an unnecessary update of the root's item in the root tree when
+	 * committing the transaction if that root wasn't changed before.
+	 */
+	trans = btrfs_join_transaction(root);
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
 
 	return btrfs_commit_transaction(trans);
 }
-- 
2.43.0

[PATCH 4/7] btrfs: send: avoid create/commit empty transaction at ensure_commit_roots_uptodate()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

At ensure_commit_roots_uptodate() we use btrfs_join_transaction() to
catch any running transaction and then commit it. This will however create
a new and empty transaction in case there's no running transaction anymore
(got committed by the transaction kthread or other task for example) or
there's a running transaction finishing its commit and with a state >=
TRANS_STATE_UNBLOCKED. In the former case we don't need to do anything
while in the second case we just need to wait for the transaction to
complete its commit.

So improve this by using btrfs_attach_transaction_barrier() instead, which
does not create a new transaction if there's none running, and if there's
a current transaction that is committing, it will wait for it to fully
commit and not create a new transaction. This helps avoiding creating and
committing empty transactions, saving IO, time and unnecessary rotation of
the backup roots in the super block.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/send.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 2c46bd1c90d3..289e5e6a6c56 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -8018,9 +8018,12 @@ static int ensure_commit_roots_uptodate(struct send_ctx *sctx)
 	 * an unnecessary update of the root's item in the root tree when
 	 * committing the transaction if that root wasn't changed before.
 	 */
-	trans = btrfs_join_transaction(root);
-	if (IS_ERR(trans))
-		return PTR_ERR(trans);
+	trans = btrfs_attach_transaction_barrier(root);
+	if (IS_ERR(trans)) {
+		int ret = PTR_ERR(trans);
+
+		return (ret == -ENOENT) ? 0 : ret;
+	}
 
 	return btrfs_commit_transaction(trans);
 }
-- 
2.43.0

[PATCH 5/7] btrfs: scrub: avoid create/commit empty transaction at finish_extent_writes_for_zoned()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

At finish_extent_writes_for_zoned() we use btrfs_join_transaction() to
catch any running transaction and then commit it. This will however create
a new and empty transaction in case there's no running transaction anymore
(got committed by the transaction kthread or other task for example) or
there's a running transaction finishing its commit and with a state >=
TRANS_STATE_UNBLOCKED. In the former case we don't need to do anything
while in the second case we just need to wait for the transaction to
complete its commit.

So improve this by using btrfs_attach_transaction_barrier() instead, which
does not create a new transaction if there's none running, and if there's
a current transaction that is committing, it will wait for it to fully
commit and not create a new transaction. This helps avoiding creating and
committing empty transactions, saving IO, time and unnecessary rotation of
the backup roots in the super block.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/scrub.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 376c5c2e9aed..6c7b5d52591e 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -2446,9 +2446,13 @@ static int finish_extent_writes_for_zoned(struct btrfs_root *root,
 	btrfs_wait_nocow_writers(cache);
 	btrfs_wait_ordered_roots(fs_info, U64_MAX, cache);
 
-	trans = btrfs_join_transaction(root);
-	if (IS_ERR(trans))
-		return PTR_ERR(trans);
+	trans = btrfs_attach_transaction_barrier(root);
+	if (IS_ERR(trans)) {
+		int ret = PTR_ERR(trans);
+
+		return (ret == -ENOENT) ? 0 : ret;
+	}
+
 	return btrfs_commit_transaction(trans);
 }
 
-- 
2.43.0

[PATCH 6/7] btrfs: add and use helper to commit the current transaction

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We have several places that attach to the current transaction with
btrfs_attach_transaction_barrier() and then commit the transaction if
there is one. Add a helper and use it to deduplicate this pattern.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/disk-io.c     | 12 +-----------
 fs/btrfs/qgroup.c      | 33 +++++----------------------------
 fs/btrfs/scrub.c       | 10 +---------
 fs/btrfs/send.c        | 10 +---------
 fs/btrfs/space-info.c  |  9 +--------
 fs/btrfs/super.c       | 11 +----------
 fs/btrfs/transaction.c | 19 +++++++++++++++++++
 fs/btrfs/transaction.h |  1 +
 8 files changed, 30 insertions(+), 75 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 2f56f967beb8..78d3966232ae 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4144,9 +4144,6 @@ void btrfs_drop_and_free_fs_root(struct btrfs_fs_info *fs_info,
 
 int btrfs_commit_super(struct btrfs_fs_info *fs_info)
 {
-	struct btrfs_root *root = fs_info->tree_root;
-	struct btrfs_trans_handle *trans;
-
 	mutex_lock(&fs_info->cleaner_mutex);
 	btrfs_run_delayed_iputs(fs_info);
 	mutex_unlock(&fs_info->cleaner_mutex);
@@ -4156,14 +4153,7 @@ int btrfs_commit_super(struct btrfs_fs_info *fs_info)
 	down_write(&fs_info->cleanup_work_sem);
 	up_write(&fs_info->cleanup_work_sem);
 
-	trans = btrfs_attach_transaction_barrier(root);
-	if (IS_ERR(trans)) {
-		int ret = PTR_ERR(trans);
-
-		return (ret == -ENOENT) ? 0 : ret;
-	}
-
-	return btrfs_commit_transaction(trans);
+	return btrfs_commit_current_transaction(fs_info->tree_root);
 }
 
 static void warn_about_uncommitted_trans(struct btrfs_fs_info *fs_info)
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 391af9e79dd6..6864a8a201df 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1334,7 +1334,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
  */
 static int flush_reservations(struct btrfs_fs_info *fs_info)
 {
-	struct btrfs_trans_handle *trans;
 	int ret;
 
 	ret = btrfs_start_delalloc_roots(fs_info, LONG_MAX, false);
@@ -1342,13 +1341,7 @@ static int flush_reservations(struct btrfs_fs_info *fs_info)
 		return ret;
 	btrfs_wait_ordered_roots(fs_info, U64_MAX, NULL);
 
-	trans = btrfs_attach_transaction_barrier(fs_info->tree_root);
-	if (IS_ERR(trans)) {
-		ret = PTR_ERR(trans);
-		return (ret == -ENOENT) ? 0 : ret;
-	}
-
-	return btrfs_commit_transaction(trans);
+	return btrfs_commit_current_transaction(fs_info->tree_root);
 }
 
 int btrfs_quota_disable(struct btrfs_fs_info *fs_info)
@@ -4028,7 +4021,6 @@ int
 btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info)
 {
 	int ret = 0;
-	struct btrfs_trans_handle *trans;
 
 	ret = qgroup_rescan_init(fs_info, 0, 1);
 	if (ret)
@@ -4045,16 +4037,10 @@ btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info)
 	 * going to clear all tracking information for a clean start.
 	 */
 
-	trans = btrfs_attach_transaction_barrier(fs_info->fs_root);
-	if (IS_ERR(trans) && trans != ERR_PTR(-ENOENT)) {
+	ret = btrfs_commit_current_transaction(fs_info->fs_root);
+	if (ret) {
 		fs_info->qgroup_flags &= ~BTRFS_QGROUP_STATUS_FLAG_RESCAN;
-		return PTR_ERR(trans);
-	} else if (trans != ERR_PTR(-ENOENT)) {
-		ret = btrfs_commit_transaction(trans);
-		if (ret) {
-			fs_info->qgroup_flags &= ~BTRFS_QGROUP_STATUS_FLAG_RESCAN;
-			return ret;
-		}
+		return ret;
 	}
 
 	qgroup_rescan_zero_tracking(fs_info);
@@ -4190,7 +4176,6 @@ static int qgroup_unreserve_range(struct btrfs_inode *inode,
  */
 static int try_flush_qgroup(struct btrfs_root *root)
 {
-	struct btrfs_trans_handle *trans;
 	int ret;
 
 	/* Can't hold an open transaction or we run the risk of deadlocking. */
@@ -4213,15 +4198,7 @@ static int try_flush_qgroup(struct btrfs_root *root)
 		goto out;
 	btrfs_wait_ordered_extents(root, U64_MAX, NULL);
 
-	trans = btrfs_attach_transaction_barrier(root);
-	if (IS_ERR(trans)) {
-		ret = PTR_ERR(trans);
-		if (ret == -ENOENT)
-			ret = 0;
-		goto out;
-	}
-
-	ret = btrfs_commit_transaction(trans);
+	ret = btrfs_commit_current_transaction(root);
 out:
 	clear_bit(BTRFS_ROOT_QGROUP_FLUSHING, &root->state);
 	wake_up(&root->qgroup_flush_wait);
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 6c7b5d52591e..188a9c42c9eb 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -2437,7 +2437,6 @@ static int finish_extent_writes_for_zoned(struct btrfs_root *root,
 					  struct btrfs_block_group *cache)
 {
 	struct btrfs_fs_info *fs_info = cache->fs_info;
-	struct btrfs_trans_handle *trans;
 
 	if (!btrfs_is_zoned(fs_info))
 		return 0;
@@ -2446,14 +2445,7 @@ static int finish_extent_writes_for_zoned(struct btrfs_root *root,
 	btrfs_wait_nocow_writers(cache);
 	btrfs_wait_ordered_roots(fs_info, U64_MAX, cache);
 
-	trans = btrfs_attach_transaction_barrier(root);
-	if (IS_ERR(trans)) {
-		int ret = PTR_ERR(trans);
-
-		return (ret == -ENOENT) ? 0 : ret;
-	}
-
-	return btrfs_commit_transaction(trans);
+	return btrfs_commit_current_transaction(root);
 }
 
 static noinline_for_stack
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 289e5e6a6c56..7a82132500a8 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -7998,7 +7998,6 @@ static int send_subvol(struct send_ctx *sctx)
  */
 static int ensure_commit_roots_uptodate(struct send_ctx *sctx)
 {
-	struct btrfs_trans_handle *trans;
 	struct btrfs_root *root = sctx->parent_root;
 
 	if (root && root->node != root->commit_root)
@@ -8018,14 +8017,7 @@ static int ensure_commit_roots_uptodate(struct send_ctx *sctx)
 	 * an unnecessary update of the root's item in the root tree when
 	 * committing the transaction if that root wasn't changed before.
 	 */
-	trans = btrfs_attach_transaction_barrier(root);
-	if (IS_ERR(trans)) {
-		int ret = PTR_ERR(trans);
-
-		return (ret == -ENOENT) ? 0 : ret;
-	}
-
-	return btrfs_commit_transaction(trans);
+	return btrfs_commit_current_transaction(root);
 }
 
 /*
diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 69760e1e726f..0283ee9bf813 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -805,14 +805,7 @@ static void flush_space(struct btrfs_fs_info *fs_info,
 		 * because that does not wait for a transaction to fully commit
 		 * (only for it to be unblocked, state TRANS_STATE_UNBLOCKED).
 		 */
-		trans = btrfs_attach_transaction_barrier(root);
-		if (IS_ERR(trans)) {
-			ret = PTR_ERR(trans);
-			if (ret == -ENOENT)
-				ret = 0;
-			break;
-		}
-		ret = btrfs_commit_transaction(trans);
+		ret = btrfs_commit_current_transaction(root);
 		break;
 	default:
 		ret = -ENOSPC;
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 117a355dbd7a..21d986e07500 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2257,9 +2257,7 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
 
 static int btrfs_freeze(struct super_block *sb)
 {
-	struct btrfs_trans_handle *trans;
 	struct btrfs_fs_info *fs_info = btrfs_sb(sb);
-	struct btrfs_root *root = fs_info->tree_root;
 
 	set_bit(BTRFS_FS_FROZEN, &fs_info->flags);
 	/*
@@ -2268,14 +2266,7 @@ static int btrfs_freeze(struct super_block *sb)
 	 * we want to avoid on a frozen filesystem), or do the commit
 	 * ourselves.
 	 */
-	trans = btrfs_attach_transaction_barrier(root);
-	if (IS_ERR(trans)) {
-		/* no transaction, don't bother */
-		if (PTR_ERR(trans) == -ENOENT)
-			return 0;
-		return PTR_ERR(trans);
-	}
-	return btrfs_commit_transaction(trans);
+	return btrfs_commit_current_transaction(fs_info->tree_root);
 }
 
 static int check_dev_super(struct btrfs_device *dev)
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 639755f025b4..9590a1899b9d 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1989,6 +1989,25 @@ void btrfs_commit_transaction_async(struct btrfs_trans_handle *trans)
 	btrfs_put_transaction(cur_trans);
 }
 
+/*
+ * If there is a running transaction commit it or if it's already committing,
+ * wait for its commit to complete. Does not start and commit a new transaction
+ * if there isn't any running.
+ */
+int btrfs_commit_current_transaction(struct btrfs_root *root)
+{
+	struct btrfs_trans_handle *trans;
+
+	trans = btrfs_attach_transaction_barrier(root);
+	if (IS_ERR(trans)) {
+		int ret = PTR_ERR(trans);
+
+		return (ret == -ENOENT) ? 0 : ret;
+	}
+
+	return btrfs_commit_transaction(trans);
+}
+
 static void cleanup_transaction(struct btrfs_trans_handle *trans, int err)
 {
 	struct btrfs_fs_info *fs_info = trans->fs_info;
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index 90b987941dd1..81da655b5ee7 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -268,6 +268,7 @@ void btrfs_maybe_wake_unfinished_drop(struct btrfs_fs_info *fs_info);
 int btrfs_clean_one_deleted_snapshot(struct btrfs_fs_info *fs_info);
 int btrfs_commit_transaction(struct btrfs_trans_handle *trans);
 void btrfs_commit_transaction_async(struct btrfs_trans_handle *trans);
+int btrfs_commit_current_transaction(struct btrfs_root *root);
 int btrfs_end_transaction_throttle(struct btrfs_trans_handle *trans);
 bool btrfs_should_end_transaction(struct btrfs_trans_handle *trans);
 void btrfs_throttle(struct btrfs_fs_info *fs_info);
-- 
2.43.0

[PATCH 7/7] btrfs: send: get rid of the label and gotos at ensure_commit_roots_uptodate()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Now that there is a helper to commit the current transaction and we are
using it, there's no need for the label and goto statements at
ensure_commit_roots_uptodate(). So replace them with direct return
statements that call btrfs_commit_current_transaction().

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/send.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 7a82132500a8..2099b5f8c022 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -8001,23 +8001,15 @@ static int ensure_commit_roots_uptodate(struct send_ctx *sctx)
 	struct btrfs_root *root = sctx->parent_root;
 
 	if (root && root->node != root->commit_root)
-		goto commit_trans;
+		return btrfs_commit_current_transaction(root);
 
 	for (int i = 0; i < sctx->clone_roots_cnt; i++) {
 		root = sctx->clone_roots[i].root;
 		if (root->node != root->commit_root)
-			goto commit_trans;
+			return btrfs_commit_current_transaction(root);
 	}
 
 	return 0;
-
-commit_trans:
-	/*
-	 * Use the first root we found. We could use any but that would cause
-	 * an unnecessary update of the root's item in the root tree when
-	 * committing the transaction if that root wasn't changed before.
-	 */
-	return btrfs_commit_current_transaction(root);
 }
 
 /*
-- 
2.43.0

Re: [PATCH 0/7] btrfs: avoid some unnecessary commit of empty transactions

[Josef Bacik]

On Wed, May 22, 2024 at 03:36:28PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> A few places can unnecessarily create an empty transaction and then commit
> it, when the goal is just to catch the current transaction and wait for
> its commit to complete. This results in wasting IO, time and rotation of
> the precious backup roots in the super block. Details in the change logs.
> The patches are all independent, except patch 4 that applies on top of
> patch 3 (but could have been done in any order really, they are independent).

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

Thanks,

Josef

Re: [PATCH 0/7] btrfs: avoid some unnecessary commit of empty transactions

[Qu Wenruo]

在 2024/5/23 00:06, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
>
> A few places can unnecessarily create an empty transaction and then commit
> it, when the goal is just to catch the current transaction and wait for
> its commit to complete. This results in wasting IO, time and rotation of
> the precious backup roots in the super block. Details in the change logs.
> The patches are all independent, except patch 4 that applies on top of
> patch 3 (but could have been done in any order really, they are independent).

Looks good to me.

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

Have you considered outputting a warning if we're committing an empty
transaction (for debug build)?

That would prevent such problem from happening again.

Thanks,
Qu
>
> Filipe Manana (7):
>    btrfs: qgroup: avoid start/commit empty transaction when flushing reservations
>    btrfs: avoid create and commit empty transaction when committing super
>    btrfs: send: make ensure_commit_roots_uptodate() simpler and more efficient
>    btrfs: send: avoid create/commit empty transaction at ensure_commit_roots_uptodate()
>    btrfs: scrub: avoid create/commit empty transaction at finish_extent_writes_for_zoned()
>    btrfs: add and use helper to commit the current transaction
>    btrfs: send: get rid of the label and gotos at ensure_commit_roots_uptodate()
>
>   fs/btrfs/disk-io.c     |  8 +-------
>   fs/btrfs/qgroup.c      | 31 +++++--------------------------
>   fs/btrfs/scrub.c       |  6 +-----
>   fs/btrfs/send.c        | 32 ++++++++------------------------
>   fs/btrfs/space-info.c  |  9 +--------
>   fs/btrfs/super.c       | 11 +----------
>   fs/btrfs/transaction.c | 19 +++++++++++++++++++
>   fs/btrfs/transaction.h |  1 +
>   8 files changed, 37 insertions(+), 80 deletions(-)
>

Re: [PATCH 0/7] btrfs: avoid some unnecessary commit of empty transactions

[Filipe Manana]

On Wed, May 22, 2024 at 11:21 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> 在 2024/5/23 00:06, fdmanana@kernel.org 写道:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > A few places can unnecessarily create an empty transaction and then commit
> > it, when the goal is just to catch the current transaction and wait for
> > its commit to complete. This results in wasting IO, time and rotation of
> > the precious backup roots in the super block. Details in the change logs.
> > The patches are all independent, except patch 4 that applies on top of
> > patch 3 (but could have been done in any order really, they are independent).
>
> Looks good to me.
>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
>
> Have you considered outputting a warning if we're committing an empty
> transaction (for debug build)?
>
> That would prevent such problem from happening again.

It's not really a bug, just inefficient behaviour with side effects
for this particular type of use case.

An empty transaction can happen in other in other scenarios too like:

btrfs_start_transaction()

do something that fails, call btrfs_end_transaction() and return error
to user space

In that case no transaction abort happens since we haven't modified
anything, and if no one else uses that transaction until it's
committed, it's an "empty" transaction.

So the warning is not feasible.

Thanks.

>
> Thanks,
> Qu
> >
> > Filipe Manana (7):
> >    btrfs: qgroup: avoid start/commit empty transaction when flushing reservations
> >    btrfs: avoid create and commit empty transaction when committing super
> >    btrfs: send: make ensure_commit_roots_uptodate() simpler and more efficient
> >    btrfs: send: avoid create/commit empty transaction at ensure_commit_roots_uptodate()
> >    btrfs: scrub: avoid create/commit empty transaction at finish_extent_writes_for_zoned()
> >    btrfs: add and use helper to commit the current transaction
> >    btrfs: send: get rid of the label and gotos at ensure_commit_roots_uptodate()
> >
> >   fs/btrfs/disk-io.c     |  8 +-------
> >   fs/btrfs/qgroup.c      | 31 +++++--------------------------
> >   fs/btrfs/scrub.c       |  6 +-----
> >   fs/btrfs/send.c        | 32 ++++++++------------------------
> >   fs/btrfs/space-info.c  |  9 +--------
> >   fs/btrfs/super.c       | 11 +----------
> >   fs/btrfs/transaction.c | 19 +++++++++++++++++++
> >   fs/btrfs/transaction.h |  1 +
> >   8 files changed, 37 insertions(+), 80 deletions(-)
> >

Re: [PATCH 0/7] btrfs: avoid some unnecessary commit of empty transactions

[David Sterba]

On Wed, May 22, 2024 at 03:36:28PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> A few places can unnecessarily create an empty transaction and then commit
> it, when the goal is just to catch the current transaction and wait for
> its commit to complete. This results in wasting IO, time and rotation of
> the precious backup roots in the super block. Details in the change logs.
> The patches are all independent, except patch 4 that applies on top of
> patch 3 (but could have been done in any order really, they are independent).
> 
> Filipe Manana (7):
>   btrfs: qgroup: avoid start/commit empty transaction when flushing reservations
>   btrfs: avoid create and commit empty transaction when committing super
>   btrfs: send: make ensure_commit_roots_uptodate() simpler and more efficient
>   btrfs: send: avoid create/commit empty transaction at ensure_commit_roots_uptodate()
>   btrfs: scrub: avoid create/commit empty transaction at finish_extent_writes_for_zoned()
>   btrfs: add and use helper to commit the current transaction
>   btrfs: send: get rid of the label and gotos at ensure_commit_roots_uptodate()

Reviewed-by: David Sterba <dsterba@suse.com>