[PATCH 0/7] btrfs: some optimizations and cleanups for log syncing

[PATCH 0/7] btrfs: some optimizations and cleanups for log syncing

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Some optimizations for log syncing, specially for non-sdd mounts, and
some cleanups. Details in the changelogs of each patch.

Filipe Manana (7):
  btrfs: stop sleeping for one jiffy in non-ssd mounts during log commit
  btrfs: remove log batch counter use for fsync
  btrfs: do an initial lockless committed log transaction check during log syncing
  btrfs: check for log writers count first in wait_for_writer()
  btrfs: move condition for log commit wait into wait_log_commit()
  btrfs: check for exit condition after waking in wait_log_commit()
  btrfs: use simple booleans for log_commit field in struct btrfs_root

 fs/btrfs/ctree.h             |  6 +--
 fs/btrfs/disk-io.c           |  3 --
 fs/btrfs/file.c              |  4 --
 fs/btrfs/transaction.c       |  8 +--
 fs/btrfs/tree-log.c          | 98 ++++++++++++++----------------------
 include/trace/events/btrfs.h |  4 +-
 6 files changed, 42 insertions(+), 81 deletions(-)

-- 
2.47.2

[PATCH 1/7] btrfs: stop sleeping for one jiffy in non-ssd mounts during log commit

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Joining/starting a log transaction tracks if we ever had more than one task
concurrently logging by setting the flag BTRFS_ROOT_MULTI_LOG_TASKS in the
respective root. Once set, this flag remains for the rest of the lifetime
of the transaction, only cleared when we don't have a log root and need to
create a new one (transaction commits drop log roots).

During log commit, if we are not on a ssd mount (or use the -o nossd mount
option) and the BTRFS_ROOT_MULTI_LOG_TASKS flag is set, we sleep for one
jiffy with the excuse to allow future log writers to join and log inodes
and then commit a larger log transaction to reduce overall IO. However
this is extremely inefficient because:

1) If at some point we had multiple tasks logging concurrently but now
   we have only one task at a time, we force it to wait for 1 jiffy;

2) One jiffy can vary between 1ms to 10ms, depending on the kernel
   config option CONFIG_HZ, which by default has a value of 250HZ and
   that corresponds to 4ms - that is a lot.

This massively reduces the latency of fsyncs for non-ssd mounts, even
on consumer grade spinning disks.

Remove this mechanism to track if we have (or ever had) multiple tasks
logging and wait for 1 jiffy.

The following fio test was used to benchmark:

   $ cat fio-buffered-fsync.sh
   DEV=/dev/sdj
   MNT=/mnt/sdj
   MOUNT_OPTIONS=""
   MKFS_OPTIONS=""

   if [ $# -ne 6 ]; then
      echo "Use $0 NUM_JOBS FILE_SIZE IO_SIZE FSYNC_FREQ BLOCK_SIZE [write|randwrite]"
      exit 1
   fi

   NUM_JOBS=$1
   FILE_SIZE=$2
   IO_SIZE=$3
   FSYNC_FREQ=$4
   BLOCK_SIZE=$5
   WRITE_MODE=$6

   if [ "$WRITE_MODE" != "write" ] && [ "$WRITE_MODE" != "randwrite" ]; then
      echo "Invalid WRITE_MODE, must be 'write' or 'randwrite'"
      exit 1
   fi

   cat <<EOF > /tmp/fio-job.ini
   [writers]
   rw=$WRITE_MODE
   fsync=$FSYNC_FREQ
   fallocate=none
   group_reporting=1
   direct=0
   bs=$BLOCK_SIZE
   ioengine=psync
   filesize=$FILE_SIZE
   io_size=$IO_SIZE
   directory=$MNT
   numjobs=$NUM_JOBS
   EOF

   echo
   echo "Using config:"
   echo
   cat /tmp/fio-job.ini
   echo

   umount $MNT &> /dev/null
   mkfs.btrfs -f $MKFS_OPTIONS $DEV
   mount $MOUNT_OPTIONS $DEV $MNT

   fio /tmp/fio-job.ini
   umount $MNT

Running the script as:  ./fio-buffered-fsync.sh 8 64M 64M 1 4K randwrite

Before patch:

  WRITE: bw=2647KiB/s (2711kB/s), 2647KiB/s-2647KiB/s (2711kB/s-2711kB/s), io=512MiB (537MB), run=198055-198055msec

After patch:

  WRITE: bw=14.9MiB/s (15.6MB/s), 14.9MiB/s-14.9MiB/s (15.6MB/s-15.6MB/s), io=512MiB (537MB), run=34471-34471msec

That's about 5.7 times faster.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.h    |  2 --
 fs/btrfs/tree-log.c | 18 +-----------------
 2 files changed, 1 insertion(+), 19 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 6de7ad191e04..0f2653182405 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -131,7 +131,6 @@ enum {
 	BTRFS_ROOT_ORPHAN_ITEM_INSERTED,
 	BTRFS_ROOT_DEFRAG_RUNNING,
 	BTRFS_ROOT_FORCE_COW,
-	BTRFS_ROOT_MULTI_LOG_TASKS,
 	BTRFS_ROOT_DIRTY,
 	BTRFS_ROOT_DELETING,
 
@@ -216,7 +215,6 @@ struct btrfs_root {
 	 * to access this field.
 	 */
 	int last_log_commit;
-	pid_t log_start_pid;
 
 	u64 last_trans;
 
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 875e4ddc68ea..ccc262833a7c 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -316,13 +316,6 @@ static int start_log_trans(struct btrfs_trans_handle *trans,
 			wait_log_commit(root, root->log_transid - 1);
 			goto again;
 		}
-
-		if (!root->log_start_pid) {
-			clear_bit(BTRFS_ROOT_MULTI_LOG_TASKS, &root->state);
-			root->log_start_pid = current->pid;
-		} else if (root->log_start_pid != current->pid) {
-			set_bit(BTRFS_ROOT_MULTI_LOG_TASKS, &root->state);
-		}
 	} else {
 		/*
 		 * This means fs_info->log_root_tree was already created
@@ -340,8 +333,6 @@ static int start_log_trans(struct btrfs_trans_handle *trans,
 			goto out;
 
 		set_bit(BTRFS_ROOT_HAS_LOG_TREE, &root->state);
-		clear_bit(BTRFS_ROOT_MULTI_LOG_TASKS, &root->state);
-		root->log_start_pid = current->pid;
 	}
 
 	atomic_inc(&root->log_writers);
@@ -3347,13 +3338,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 
 	while (1) {
 		int batch = atomic_read(&root->log_batch);
-		/* when we're on an ssd, just kick the log commit out */
-		if (!btrfs_test_opt(fs_info, SSD) &&
-		    test_bit(BTRFS_ROOT_MULTI_LOG_TASKS, &root->state)) {
-			mutex_unlock(&root->log_mutex);
-			schedule_timeout_uninterruptible(1);
-			mutex_lock(&root->log_mutex);
-		}
+
 		wait_for_writer(root);
 		if (batch == atomic_read(&root->log_batch))
 			break;
@@ -3414,7 +3399,6 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 
 	btrfs_set_root_log_transid(root, root->log_transid + 1);
 	log->log_transid = root->log_transid;
-	root->log_start_pid = 0;
 	/*
 	 * IO has been started, blocks of the log tree have WRITTEN flag set
 	 * in their headers. new modifications of the log will be written to
-- 
2.47.2

[PATCH 2/7] btrfs: remove log batch counter use for fsync

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We have the log batch counter defined per root which is now useless after
the previous patch (titled: "btrfs: stop sleeping for one jiffy in non-ssd
mounts during log commit"). The counter is incremented early in the fsync
path, before and after flushing dellaloc and waiting for writeback, and
then the counter is read during the log sync path. The goal was to wait
for tasks that are about to join a log transaction, so that we could
reduce the amount of IO and log syncing (flush all log tree extent buffers
and write super blocks), but that mechanism does not work since if there
are currently no log writers, btrfs_sync_log() does not unlock the root's
log_mutex, so no new log writers can join the log transaction. Having
concurrent fsync tasks increasing the log_batch counter only makes us loop
unncessarily in btrfs_sync_log() - that is always true since the previous
patch mentioned above and was true before that patch only when not using
the "-o ssd" mount option (which is activated by default if the filesystem
does not have rotational devices).

So remove the log batch counter. No performance changes were observed
after removing it.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.h    | 2 --
 fs/btrfs/disk-io.c  | 1 -
 fs/btrfs/file.c     | 4 ----
 fs/btrfs/tree-log.c | 8 +-------
 4 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 0f2653182405..d5d8b3899258 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -196,8 +196,6 @@ struct btrfs_root {
 	/* Used only for log trees of subvolumes, not for the log root tree */
 	atomic_t log_writers;
 	atomic_t log_commit[2];
-	/* Used only for log trees of subvolumes, not for the log root tree */
-	atomic_t log_batch;
 	/*
 	 * Protected by the 'log_mutex' lock but can be read without holding
 	 * that lock to avoid unnecessary lock contention, in which case it
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 37fc0d6b960d..1ee0a19e40a8 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -669,7 +669,6 @@ static struct btrfs_root *btrfs_alloc_root(struct btrfs_fs_info *fs_info,
 	atomic_set(&root->log_commit[0], 0);
 	atomic_set(&root->log_commit[1], 0);
 	atomic_set(&root->log_writers, 0);
-	atomic_set(&root->log_batch, 0);
 	refcount_set(&root->refs, 1);
 	atomic_set(&root->snapshot_force_cow, 0);
 	atomic_set(&root->nr_swapfiles, 0);
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 8f078c58e940..aeecf1121b51 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1573,8 +1573,6 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	else
 		btrfs_inode_lock(inode, BTRFS_ILOCK_MMAP);
 
-	atomic_inc(&root->log_batch);
-
 	/*
 	 * Before we acquired the inode's lock and the mmap lock, someone may
 	 * have dirtied more pages in the target range. We need to make sure
@@ -1657,8 +1655,6 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	if (ret)
 		goto out_release_extents;
 
-	atomic_inc(&root->log_batch);
-
 	if (skip_inode_logging(&ctx)) {
 		/*
 		 * We've had everything committed since the last time we were
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index ccc262833a7c..580f2aabc065 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3336,13 +3336,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	if (atomic_read(&root->log_commit[(index1 + 1) % 2]))
 		wait_log_commit(root, log_transid - 1);
 
-	while (1) {
-		int batch = atomic_read(&root->log_batch);
-
-		wait_for_writer(root);
-		if (batch == atomic_read(&root->log_batch))
-			break;
-	}
+	wait_for_writer(root);
 
 	/* bail out if we need to do a full commit */
 	if (btrfs_need_log_full_commit(trans)) {
-- 
2.47.2

[PATCH 3/7] btrfs: do an initial lockless committed log transaction check during log syncing

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When calling btrfs_sync_log() one of the things we do first is to check if
the log transaction the given context belongs to was already committed
and return immediately if it was committed before. However we take the
root's log_mutex before doing the check, which adds lock contention in
case the log transaction was already committed, as this is a heavily used
mutex in case we have multiple tasks doing fsyncs or renaming and adding
hard links to files that were fsynced before in the current transaction.

So add an initial lockless check in btrfs_sync_log() to reduce lock
contention.

The following fio test was used to benchmark:

  $ cat fio-buffered-fsync.sh
  DEV=/dev/sdj
  MNT=/mnt/sdj
  MOUNT_OPTIONS=""
  MKFS_OPTIONS=""

  if [ $# -ne 6 ]; then
     echo "Use $0 NUM_JOBS FILE_SIZE IO_SIZE FSYNC_FREQ BLOCK_SIZE [write|randwrite]"
     exit 1
  fi

  NUM_JOBS=$1
  FILE_SIZE=$2
  IO_SIZE=$3
  FSYNC_FREQ=$4
  BLOCK_SIZE=$5
  WRITE_MODE=$6

  if [ "$WRITE_MODE" != "write" ] && [ "$WRITE_MODE" != "randwrite" ]; then
     echo "Invalid WRITE_MODE, must be 'write' or 'randwrite'"
     exit 1
  fi

  cat <<EOF > /tmp/fio-job.ini
  [writers]
  rw=$WRITE_MODE
  fsync=$FSYNC_FREQ
  fallocate=none
  group_reporting=1
  direct=0
  bs=$BLOCK_SIZE
  ioengine=psync
  filesize=$FILE_SIZE
  io_size=$IO_SIZE
  directory=$MNT
  numjobs=$NUM_JOBS
  EOF

  echo
  echo "Using config:"
  echo
  cat /tmp/fio-job.ini
  echo

  umount $MNT &> /dev/null
  mkfs.btrfs -f $MKFS_OPTIONS $DEV
  mount $MOUNT_OPTIONS $DEV $MNT

  fio /tmp/fio-job.ini
  umount $MNT

Running the script as:  ./fio-buffered-fsync.sh 12 64M 64M 1 4K randwrite

Before patch:

  WRITE: bw=16.6MiB/s (17.4MB/s), 16.6MiB/s-16.6MiB/s (17.4MB/s-17.4MB/s), io=768MiB (805MB), run=46235-46235msec

After patch:

  WRITE: bw=18.2MiB/s (19.0MB/s), 18.2MiB/s-18.2MiB/s (19.0MB/s-19.0MB/s), io=768MiB (805MB), run=42277-42277msec

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

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 580f2aabc065..191308982e9c 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3306,15 +3306,21 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	struct btrfs_root *log = root->log_root;
 	struct btrfs_root *log_root_tree = fs_info->log_root_tree;
 	struct btrfs_root_item new_root_item;
-	int log_transid = 0;
+	int log_transid = ctx->log_transid;
 	struct btrfs_log_ctx root_log_ctx;
 	struct blk_plug plug;
 	u64 log_root_start;
 	u64 log_root_level;
 
+	/* Avoid fist taking root->log_mutex, reduce lock contention. */
+	if (data_race(root->log_transid_committed) >= log_transid) {
+		trace_btrfs_sync_log_enter(trans, root, ctx);
+		trace_btrfs_sync_log_exit(trans, root, ctx, ctx->log_ret);
+		return ctx->log_ret;
+	}
+
 	mutex_lock(&root->log_mutex);
 	trace_btrfs_sync_log_enter(trans, root, ctx);
-	log_transid = ctx->log_transid;
 	if (root->log_transid_committed >= log_transid) {
 		trace_btrfs_sync_log_exit(trans, root, ctx, ctx->log_ret);
 		mutex_unlock(&root->log_mutex);
-- 
2.47.2

[PATCH 4/7] btrfs: check for log writers count first in wait_for_writer()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

In wait_for_writer() we first add the wait queue entry to the wait queue
and then check for the number of log writers. In case where right in the
first iteration of the loop we have no log writers, we end up wasting time
adding the wait queue entry to the wait queue (function call, locking the
queue with irq disable and enable on unlock, etc). So check first for the
number of log writers before adding to the wait queue.

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

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 191308982e9c..08af1e66c758 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3202,11 +3202,11 @@ static void wait_for_writer(struct btrfs_root *root)
 	DEFINE_WAIT(wait);
 
 	for (;;) {
-		prepare_to_wait(&root->log_writer_wait, &wait,
-				TASK_UNINTERRUPTIBLE);
-		if (!atomic_read(&root->log_writers))
+		if (atomic_read(&root->log_writers) == 0)
 			break;
 
+		prepare_to_wait(&root->log_writer_wait, &wait,
+				TASK_UNINTERRUPTIBLE);
 		mutex_unlock(&root->log_mutex);
 		schedule();
 		mutex_lock(&root->log_mutex);
-- 
2.47.2

[PATCH 5/7] btrfs: move condition for log commit wait into wait_log_commit()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Instead of having every caller check for root->log_commit[] being non-zero
and then call wait_log_commit(), move the check into wait_log_commit() and
have the callers call it unconditionally.

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

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 08af1e66c758..f33392da78ef 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -221,7 +221,7 @@ static int btrfs_log_inode(struct btrfs_trans_handle *trans,
 static int link_to_fixup_dir(struct walk_control *wc, u64 objectid);
 static noinline int replay_dir_deletes(struct walk_control *wc,
 				       u64 dirid, bool del_all);
-static void wait_log_commit(struct btrfs_root *root, int transid);
+static bool wait_log_commit(struct btrfs_root *root, int transid);
 
 /*
  * tree logging is a special write ahead log used to make sure that
@@ -305,17 +305,13 @@ static int start_log_trans(struct btrfs_trans_handle *trans,
 
 again:
 	if (root->log_root) {
-		int index = (root->log_transid + 1) % 2;
-
 		if (btrfs_need_log_full_commit(trans)) {
 			ret = BTRFS_LOG_FORCE_COMMIT;
 			goto out;
 		}
 
-		if (zoned && atomic_read(&root->log_commit[index])) {
-			wait_log_commit(root, root->log_transid - 1);
+		if (zoned && wait_log_commit(root, root->log_transid - 1))
 			goto again;
-		}
 	} else {
 		/*
 		 * This means fs_info->log_root_tree was already created
@@ -363,13 +359,9 @@ static int join_running_log_trans(struct btrfs_root *root)
 	mutex_lock(&root->log_mutex);
 again:
 	if (root->log_root) {
-		int index = (root->log_transid + 1) % 2;
-
 		ret = 0;
-		if (zoned && atomic_read(&root->log_commit[index])) {
-			wait_log_commit(root, root->log_transid - 1);
+		if (zoned && wait_log_commit(root, root->log_transid - 1))
 			goto again;
-		}
 		atomic_inc(&root->log_writers);
 	}
 	mutex_unlock(&root->log_mutex);
@@ -3172,11 +3164,15 @@ static int update_log_root(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-static void wait_log_commit(struct btrfs_root *root, int transid)
+/* Returns true if we had to wait, false otherwise. */
+static bool wait_log_commit(struct btrfs_root *root, int transid)
 {
 	DEFINE_WAIT(wait);
 	int index = transid % 2;
 
+	if (atomic_read(&root->log_commit[index]) == 0)
+		return false;
+
 	/*
 	 * we only allow two pending log transactions at a time,
 	 * so we know that if ours is more than 2 older than the
@@ -3195,6 +3191,8 @@ static void wait_log_commit(struct btrfs_root *root, int transid)
 		mutex_lock(&root->log_mutex);
 	}
 	finish_wait(&root->log_commit_wait[index], &wait);
+
+	return true;
 }
 
 static void wait_for_writer(struct btrfs_root *root)
@@ -3298,8 +3296,6 @@ static inline void btrfs_remove_all_log_ctxs(struct btrfs_root *root,
 int btrfs_sync_log(struct btrfs_trans_handle *trans,
 		   struct btrfs_root *root, struct btrfs_log_ctx *ctx)
 {
-	int index1;
-	int index2;
 	int mark;
 	int ret;
 	struct btrfs_fs_info *fs_info = root->fs_info;
@@ -3307,6 +3303,8 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	struct btrfs_root *log_root_tree = fs_info->log_root_tree;
 	struct btrfs_root_item new_root_item;
 	int log_transid = ctx->log_transid;
+	int index1 = log_transid % 2;
+	int index2;
 	struct btrfs_log_ctx root_log_ctx;
 	struct blk_plug plug;
 	u64 log_root_start;
@@ -3327,9 +3325,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 		return ctx->log_ret;
 	}
 
-	index1 = log_transid % 2;
-	if (atomic_read(&root->log_commit[index1])) {
-		wait_log_commit(root, log_transid);
+	if (wait_log_commit(root, log_transid)) {
 		trace_btrfs_sync_log_exit(trans, root, ctx, ctx->log_ret);
 		mutex_unlock(&root->log_mutex);
 		return ctx->log_ret;
@@ -3339,8 +3335,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	atomic_set(&root->log_commit[index1], 1);
 
 	/* wait for previous tree log sync to complete */
-	if (atomic_read(&root->log_commit[(index1 + 1) % 2]))
-		wait_log_commit(root, log_transid - 1);
+	wait_log_commit(root, log_transid - 1);
 
 	wait_for_writer(root);
 
@@ -3473,10 +3468,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 		root_log_ctx.log_transid, log_root_tree->log_transid);
 	atomic_set(&log_root_tree->log_commit[index2], 1);
 
-	if (atomic_read(&log_root_tree->log_commit[(index2 + 1) % 2])) {
-		wait_log_commit(log_root_tree,
-				root_log_ctx.log_transid - 1);
-	}
+	wait_log_commit(log_root_tree, root_log_ctx.log_transid - 1);
 
 	/*
 	 * now that we've moved on to the tree of log tree roots,
-- 
2.47.2

[PATCH 6/7] btrfs: check for exit condition after waking in wait_log_commit()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We check for the exit condition after we add ourselves to the wait queue
and before we unlock the root's log_mutex, sleep and lock again log_mutex.
This is not incorrect, but it's not optimal since in the first iteration
this is pointless because we already know that root->log_commit[index] is
not zero, so we should check the exit condition only after unlocking
log_mutex, sleeping, waking up and locking again the log_mutex.

So move the check for the exit condition to bottom of the loop, after we
were woken and locked log_mutex again.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/tree-log.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index f33392da78ef..f2d0f6fb3eae 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3182,13 +3182,13 @@ static bool wait_log_commit(struct btrfs_root *root, int transid)
 		prepare_to_wait(&root->log_commit_wait[index],
 				&wait, TASK_UNINTERRUPTIBLE);
 
-		if (!(root->log_transid_committed < transid &&
-		      atomic_read(&root->log_commit[index])))
-			break;
-
 		mutex_unlock(&root->log_mutex);
 		schedule();
 		mutex_lock(&root->log_mutex);
+
+		if (!(root->log_transid_committed < transid &&
+		      atomic_read(&root->log_commit[index]) != 0))
+			break;
 	}
 	finish_wait(&root->log_commit_wait[index], &wait);
 
-- 
2.47.2

[PATCH 7/7] btrfs: use simple booleans for log_commit field in struct btrfs_root

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We are using atomic types for the log_commit array of struct btrfs_root
but all we need is simple booleans. The log_commit array elements are
always protected by the root's log_mutex, both for writes and reads, so
we can use a simple boolean. The use of atomics if from the very early
days of the log tree code where the access to the fields was not protected
by any lock.

So switch to simple booleans, which results in cheaper code and slightly
reduces the object size too.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.h             |  2 +-
 fs/btrfs/disk-io.c           |  2 --
 fs/btrfs/transaction.c       |  8 ++------
 fs/btrfs/tree-log.c          | 16 ++++++++--------
 include/trace/events/btrfs.h |  4 ++--
 5 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index d5d8b3899258..22ba2b4505b3 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -195,7 +195,7 @@ struct btrfs_root {
 	struct list_head log_ctxs[2];
 	/* Used only for log trees of subvolumes, not for the log root tree */
 	atomic_t log_writers;
-	atomic_t log_commit[2];
+	bool log_commit[2];
 	/*
 	 * Protected by the 'log_mutex' lock but can be read without holding
 	 * that lock to avoid unnecessary lock contention, in which case it
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 1ee0a19e40a8..d17e087b9cbe 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -666,8 +666,6 @@ static struct btrfs_root *btrfs_alloc_root(struct btrfs_fs_info *fs_info,
 	init_waitqueue_head(&root->log_commit_wait[1]);
 	INIT_LIST_HEAD(&root->log_ctxs[0]);
 	INIT_LIST_HEAD(&root->log_ctxs[1]);
-	atomic_set(&root->log_commit[0], 0);
-	atomic_set(&root->log_commit[1], 0);
 	atomic_set(&root->log_writers, 0);
 	refcount_set(&root->refs, 1);
 	atomic_set(&root->snapshot_force_cow, 0);
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 45149d027740..91b41f55a3ce 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1517,12 +1517,8 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
 			ASSERT(atomic_read(&root->log_writers) == 0,
 			       "atomic_read(&root->log_writers)=%d",
 			       atomic_read(&root->log_writers));
-			ASSERT(atomic_read(&root->log_commit[0]) == 0,
-			       "atomic_read(&root->log_commit[0])=%d",
-			       atomic_read(&root->log_commit[0]));
-			ASSERT(atomic_read(&root->log_commit[1]) == 0,
-			       "atomic_read(&root->log_commit[1])=%d",
-			       atomic_read(&root->log_commit[1]));
+			ASSERT(!root->log_commit[0]);
+			ASSERT(!root->log_commit[1]);
 
 			radix_tree_tag_clear(&fs_info->fs_roots_radix,
 					(unsigned long)btrfs_root_id(root),
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index f2d0f6fb3eae..263e229c181c 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3170,7 +3170,7 @@ static bool wait_log_commit(struct btrfs_root *root, int transid)
 	DEFINE_WAIT(wait);
 	int index = transid % 2;
 
-	if (atomic_read(&root->log_commit[index]) == 0)
+	if (!root->log_commit[index])
 		return false;
 
 	/*
@@ -3187,7 +3187,7 @@ static bool wait_log_commit(struct btrfs_root *root, int transid)
 		mutex_lock(&root->log_mutex);
 
 		if (!(root->log_transid_committed < transid &&
-		      atomic_read(&root->log_commit[index]) != 0))
+		      root->log_commit[index]))
 			break;
 	}
 	finish_wait(&root->log_commit_wait[index], &wait);
@@ -3332,7 +3332,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	}
 	ASSERT(log_transid == root->log_transid,
 	       "log_transid=%d root->log_transid=%d", log_transid, root->log_transid);
-	atomic_set(&root->log_commit[index1], 1);
+	root->log_commit[index1] = true;
 
 	/* wait for previous tree log sync to complete */
 	wait_log_commit(root, log_transid - 1);
@@ -3452,7 +3452,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 		goto out;
 	}
 
-	if (atomic_read(&log_root_tree->log_commit[index2])) {
+	if (log_root_tree->log_commit[index2]) {
 		blk_finish_plug(&plug);
 		ret = btrfs_wait_tree_log_extents(log, mark);
 		wait_log_commit(log_root_tree,
@@ -3466,7 +3466,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	ASSERT(root_log_ctx.log_transid == log_root_tree->log_transid,
 	       "root_log_ctx.log_transid=%d log_root_tree->log_transid=%d",
 		root_log_ctx.log_transid, log_root_tree->log_transid);
-	atomic_set(&log_root_tree->log_commit[index2], 1);
+	log_root_tree->log_commit[index2] = true;
 
 	wait_log_commit(log_root_tree, root_log_ctx.log_transid - 1);
 
@@ -3566,7 +3566,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 
 	/*
 	 * We know there can only be one task here, since we have not yet set
-	 * root->log_commit[index1] to 0 and any task attempting to sync the
+	 * root->log_commit[index1] to false and any task attempting to sync the
 	 * log must wait for the previous log transaction to commit if it's
 	 * still in progress or wait for the current log transaction commit if
 	 * someone else already started it. We use <= and not < because the
@@ -3582,7 +3582,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	btrfs_remove_all_log_ctxs(log_root_tree, index2, ret);
 
 	log_root_tree->log_transid_committed++;
-	atomic_set(&log_root_tree->log_commit[index2], 0);
+	log_root_tree->log_commit[index2] = false;
 	mutex_unlock(&log_root_tree->log_mutex);
 
 	/*
@@ -3595,7 +3595,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	mutex_lock(&root->log_mutex);
 	btrfs_remove_all_log_ctxs(root, index1, ret);
 	root->log_transid_committed++;
-	atomic_set(&root->log_commit[index1], 0);
+	root->log_commit[index1] = false;
 	mutex_unlock(&root->log_mutex);
 
 	/*
diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
index 4c5c47c5edb7..083519ceecbf 100644
--- a/include/trace/events/btrfs.h
+++ b/include/trace/events/btrfs.h
@@ -1578,9 +1578,9 @@ TRACE_EVENT(btrfs_sync_log_enter,
 		__entry->log_transid_committed	=
 			data_race(root->log_transid_committed);
 		__entry->log_committing		=
-			atomic_read(&root->log_commit[ctx->log_transid % 2]);
+			root->log_commit[ctx->log_transid % 2];
 		__entry->log_committing_prev	=
-			atomic_read(&root->log_commit[(ctx->log_transid + 1) % 2]);
+			root->log_commit[(ctx->log_transid + 1) % 2];
 		__entry->log_writers		= atomic_read(&root->log_writers);
 	),
 
-- 
2.47.2

Re: [PATCH 0/7] btrfs: some optimizations and cleanups for log syncing

[Boris Burkov]

On Wed, Jul 22, 2026 at 01:41:36PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Some optimizations for log syncing, specially for non-sdd mounts, and
> some cleanups. Details in the changelogs of each patch.

Reviewed-by: Boris Burkov <boris@bur.io>

> 
> Filipe Manana (7):
>   btrfs: stop sleeping for one jiffy in non-ssd mounts during log commit
>   btrfs: remove log batch counter use for fsync
>   btrfs: do an initial lockless committed log transaction check during log syncing
>   btrfs: check for log writers count first in wait_for_writer()
>   btrfs: move condition for log commit wait into wait_log_commit()
>   btrfs: check for exit condition after waking in wait_log_commit()
>   btrfs: use simple booleans for log_commit field in struct btrfs_root
> 
>  fs/btrfs/ctree.h             |  6 +--
>  fs/btrfs/disk-io.c           |  3 --
>  fs/btrfs/file.c              |  4 --
>  fs/btrfs/transaction.c       |  8 +--
>  fs/btrfs/tree-log.c          | 98 ++++++++++++++----------------------
>  include/trace/events/btrfs.h |  4 +-
>  6 files changed, 42 insertions(+), 81 deletions(-)
> 
> -- 
> 2.47.2
> 

[PATCH v2 0/5] btrfs: some optimizations and cleanups for log syncing

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Some optimizations for log syncing, specially for non-sdd mounts, and
some cleanups. Details in the changelogs of each patch.

V2: Removed two patches which sashiko found problematic, the benefits of
    fixing them up versus the complexity required to fix them is not worth
    it. Updated last patch to add the data_race() annotation in the trace
    events.

Filipe Manana (5):
  btrfs: stop sleeping for one jiffy in non-ssd mounts during log commit
  btrfs: remove log batch counter use for fsync
  btrfs: move condition for log commit wait into wait_log_commit()
  btrfs: check for exit condition after waking in wait_log_commit()
  btrfs: use simple booleans for log_commit field in struct btrfs_root

 fs/btrfs/ctree.h             |  6 +--
 fs/btrfs/disk-io.c           |  3 --
 fs/btrfs/file.c              |  4 --
 fs/btrfs/transaction.c       |  8 +---
 fs/btrfs/tree-log.c          | 85 ++++++++++++------------------------
 include/trace/events/btrfs.h |  4 +-
 6 files changed, 32 insertions(+), 78 deletions(-)

-- 
2.47.2

[PATCH v2 1/5] btrfs: stop sleeping for one jiffy in non-ssd mounts during log commit

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Joining/starting a log transaction tracks if we ever had more than one task
concurrently logging by setting the flag BTRFS_ROOT_MULTI_LOG_TASKS in the
respective root. Once set, this flag remains for the rest of the lifetime
of the transaction, only cleared when we don't have a log root and need to
create a new one (transaction commits drop log roots).

During log commit, if we are not on a ssd mount (or use the -o nossd mount
option) and the BTRFS_ROOT_MULTI_LOG_TASKS flag is set, we sleep for one
jiffy with the excuse to allow future log writers to join and log inodes
and then commit a larger log transaction to reduce overall IO. However
this is extremely inefficient because:

1) If at some point we had multiple tasks logging concurrently but now
   we have only one task at a time, we force it to wait for 1 jiffy;

2) One jiffy can vary between 1ms to 10ms, depending on the kernel
   config option CONFIG_HZ, which by default has a value of 250HZ and
   that corresponds to 4ms - that is a lot.

This massively reduces the latency of fsyncs for non-ssd mounts, even
on consumer grade spinning disks.

Remove this mechanism to track if we have (or ever had) multiple tasks
logging and wait for 1 jiffy.

The following fio test was used to benchmark:

   $ cat fio-buffered-fsync.sh
   DEV=/dev/sdj
   MNT=/mnt/sdj
   MOUNT_OPTIONS=""
   MKFS_OPTIONS=""

   if [ $# -ne 6 ]; then
      echo "Use $0 NUM_JOBS FILE_SIZE IO_SIZE FSYNC_FREQ BLOCK_SIZE [write|randwrite]"
      exit 1
   fi

   NUM_JOBS=$1
   FILE_SIZE=$2
   IO_SIZE=$3
   FSYNC_FREQ=$4
   BLOCK_SIZE=$5
   WRITE_MODE=$6

   if [ "$WRITE_MODE" != "write" ] && [ "$WRITE_MODE" != "randwrite" ]; then
      echo "Invalid WRITE_MODE, must be 'write' or 'randwrite'"
      exit 1
   fi

   cat <<EOF > /tmp/fio-job.ini
   [writers]
   rw=$WRITE_MODE
   fsync=$FSYNC_FREQ
   fallocate=none
   group_reporting=1
   direct=0
   bs=$BLOCK_SIZE
   ioengine=psync
   filesize=$FILE_SIZE
   io_size=$IO_SIZE
   directory=$MNT
   numjobs=$NUM_JOBS
   EOF

   echo
   echo "Using config:"
   echo
   cat /tmp/fio-job.ini
   echo

   umount $MNT &> /dev/null
   mkfs.btrfs -f $MKFS_OPTIONS $DEV
   mount $MOUNT_OPTIONS $DEV $MNT

   fio /tmp/fio-job.ini
   umount $MNT

Running the script as:  ./fio-buffered-fsync.sh 8 64M 64M 1 4K randwrite

Before patch:

  WRITE: bw=2647KiB/s (2711kB/s), 2647KiB/s-2647KiB/s (2711kB/s-2711kB/s), io=512MiB (537MB), run=198055-198055msec

After patch:

  WRITE: bw=14.9MiB/s (15.6MB/s), 14.9MiB/s-14.9MiB/s (15.6MB/s-15.6MB/s), io=512MiB (537MB), run=34471-34471msec

That's about 5.7 times faster.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.h    |  2 --
 fs/btrfs/tree-log.c | 18 +-----------------
 2 files changed, 1 insertion(+), 19 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 6de7ad191e04..0f2653182405 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -131,7 +131,6 @@ enum {
 	BTRFS_ROOT_ORPHAN_ITEM_INSERTED,
 	BTRFS_ROOT_DEFRAG_RUNNING,
 	BTRFS_ROOT_FORCE_COW,
-	BTRFS_ROOT_MULTI_LOG_TASKS,
 	BTRFS_ROOT_DIRTY,
 	BTRFS_ROOT_DELETING,
 
@@ -216,7 +215,6 @@ struct btrfs_root {
 	 * to access this field.
 	 */
 	int last_log_commit;
-	pid_t log_start_pid;
 
 	u64 last_trans;
 
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 875e4ddc68ea..ccc262833a7c 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -316,13 +316,6 @@ static int start_log_trans(struct btrfs_trans_handle *trans,
 			wait_log_commit(root, root->log_transid - 1);
 			goto again;
 		}
-
-		if (!root->log_start_pid) {
-			clear_bit(BTRFS_ROOT_MULTI_LOG_TASKS, &root->state);
-			root->log_start_pid = current->pid;
-		} else if (root->log_start_pid != current->pid) {
-			set_bit(BTRFS_ROOT_MULTI_LOG_TASKS, &root->state);
-		}
 	} else {
 		/*
 		 * This means fs_info->log_root_tree was already created
@@ -340,8 +333,6 @@ static int start_log_trans(struct btrfs_trans_handle *trans,
 			goto out;
 
 		set_bit(BTRFS_ROOT_HAS_LOG_TREE, &root->state);
-		clear_bit(BTRFS_ROOT_MULTI_LOG_TASKS, &root->state);
-		root->log_start_pid = current->pid;
 	}
 
 	atomic_inc(&root->log_writers);
@@ -3347,13 +3338,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 
 	while (1) {
 		int batch = atomic_read(&root->log_batch);
-		/* when we're on an ssd, just kick the log commit out */
-		if (!btrfs_test_opt(fs_info, SSD) &&
-		    test_bit(BTRFS_ROOT_MULTI_LOG_TASKS, &root->state)) {
-			mutex_unlock(&root->log_mutex);
-			schedule_timeout_uninterruptible(1);
-			mutex_lock(&root->log_mutex);
-		}
+
 		wait_for_writer(root);
 		if (batch == atomic_read(&root->log_batch))
 			break;
@@ -3414,7 +3399,6 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 
 	btrfs_set_root_log_transid(root, root->log_transid + 1);
 	log->log_transid = root->log_transid;
-	root->log_start_pid = 0;
 	/*
 	 * IO has been started, blocks of the log tree have WRITTEN flag set
 	 * in their headers. new modifications of the log will be written to
-- 
2.47.2

[PATCH v2 2/5] btrfs: remove log batch counter use for fsync

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We have the log batch counter defined per root which is now useless after
the previous patch (titled: "btrfs: stop sleeping for one jiffy in non-ssd
mounts during log commit"). The counter is incremented early in the fsync
path, before and after flushing dellaloc and waiting for writeback, and
then the counter is read during the log sync path. The goal was to wait
for tasks that are about to join a log transaction, so that we could
reduce the amount of IO and log syncing (flush all log tree extent buffers
and write super blocks), but that mechanism does not work since if there
are currently no log writers, btrfs_sync_log() does not unlock the root's
log_mutex, so no new log writers can join the log transaction. Having
concurrent fsync tasks increasing the log_batch counter only makes us loop
unncessarily in btrfs_sync_log() - that is always true since the previous
patch mentioned above and was true before that patch only when not using
the "-o ssd" mount option (which is activated by default if the filesystem
does not have rotational devices).

So remove the log batch counter. No performance changes were observed
after removing it.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.h    | 2 --
 fs/btrfs/disk-io.c  | 1 -
 fs/btrfs/file.c     | 4 ----
 fs/btrfs/tree-log.c | 8 +-------
 4 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 0f2653182405..d5d8b3899258 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -196,8 +196,6 @@ struct btrfs_root {
 	/* Used only for log trees of subvolumes, not for the log root tree */
 	atomic_t log_writers;
 	atomic_t log_commit[2];
-	/* Used only for log trees of subvolumes, not for the log root tree */
-	atomic_t log_batch;
 	/*
 	 * Protected by the 'log_mutex' lock but can be read without holding
 	 * that lock to avoid unnecessary lock contention, in which case it
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 37fc0d6b960d..1ee0a19e40a8 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -669,7 +669,6 @@ static struct btrfs_root *btrfs_alloc_root(struct btrfs_fs_info *fs_info,
 	atomic_set(&root->log_commit[0], 0);
 	atomic_set(&root->log_commit[1], 0);
 	atomic_set(&root->log_writers, 0);
-	atomic_set(&root->log_batch, 0);
 	refcount_set(&root->refs, 1);
 	atomic_set(&root->snapshot_force_cow, 0);
 	atomic_set(&root->nr_swapfiles, 0);
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 8f078c58e940..aeecf1121b51 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1573,8 +1573,6 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	else
 		btrfs_inode_lock(inode, BTRFS_ILOCK_MMAP);
 
-	atomic_inc(&root->log_batch);
-
 	/*
 	 * Before we acquired the inode's lock and the mmap lock, someone may
 	 * have dirtied more pages in the target range. We need to make sure
@@ -1657,8 +1655,6 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	if (ret)
 		goto out_release_extents;
 
-	atomic_inc(&root->log_batch);
-
 	if (skip_inode_logging(&ctx)) {
 		/*
 		 * We've had everything committed since the last time we were
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index ccc262833a7c..580f2aabc065 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3336,13 +3336,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	if (atomic_read(&root->log_commit[(index1 + 1) % 2]))
 		wait_log_commit(root, log_transid - 1);
 
-	while (1) {
-		int batch = atomic_read(&root->log_batch);
-
-		wait_for_writer(root);
-		if (batch == atomic_read(&root->log_batch))
-			break;
-	}
+	wait_for_writer(root);
 
 	/* bail out if we need to do a full commit */
 	if (btrfs_need_log_full_commit(trans)) {
-- 
2.47.2

[PATCH v2 3/5] btrfs: move condition for log commit wait into wait_log_commit()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Instead of having every caller check for root->log_commit[] being non-zero
and then call wait_log_commit(), move the check into wait_log_commit() and
have the callers call it unconditionally.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/tree-log.c | 41 ++++++++++++++++-------------------------
 1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 580f2aabc065..b6ed6671ee26 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -221,7 +221,7 @@ static int btrfs_log_inode(struct btrfs_trans_handle *trans,
 static int link_to_fixup_dir(struct walk_control *wc, u64 objectid);
 static noinline int replay_dir_deletes(struct walk_control *wc,
 				       u64 dirid, bool del_all);
-static void wait_log_commit(struct btrfs_root *root, int transid);
+static bool wait_log_commit(struct btrfs_root *root, int transid);
 
 /*
  * tree logging is a special write ahead log used to make sure that
@@ -305,17 +305,13 @@ static int start_log_trans(struct btrfs_trans_handle *trans,
 
 again:
 	if (root->log_root) {
-		int index = (root->log_transid + 1) % 2;
-
 		if (btrfs_need_log_full_commit(trans)) {
 			ret = BTRFS_LOG_FORCE_COMMIT;
 			goto out;
 		}
 
-		if (zoned && atomic_read(&root->log_commit[index])) {
-			wait_log_commit(root, root->log_transid - 1);
+		if (zoned && wait_log_commit(root, root->log_transid - 1))
 			goto again;
-		}
 	} else {
 		/*
 		 * This means fs_info->log_root_tree was already created
@@ -363,13 +359,9 @@ static int join_running_log_trans(struct btrfs_root *root)
 	mutex_lock(&root->log_mutex);
 again:
 	if (root->log_root) {
-		int index = (root->log_transid + 1) % 2;
-
 		ret = 0;
-		if (zoned && atomic_read(&root->log_commit[index])) {
-			wait_log_commit(root, root->log_transid - 1);
+		if (zoned && wait_log_commit(root, root->log_transid - 1))
 			goto again;
-		}
 		atomic_inc(&root->log_writers);
 	}
 	mutex_unlock(&root->log_mutex);
@@ -3172,11 +3164,15 @@ static int update_log_root(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-static void wait_log_commit(struct btrfs_root *root, int transid)
+/* Returns true if we had to wait, false otherwise. */
+static bool wait_log_commit(struct btrfs_root *root, int transid)
 {
 	DEFINE_WAIT(wait);
 	int index = transid % 2;
 
+	if (atomic_read(&root->log_commit[index]) == 0)
+		return false;
+
 	/*
 	 * we only allow two pending log transactions at a time,
 	 * so we know that if ours is more than 2 older than the
@@ -3195,6 +3191,8 @@ static void wait_log_commit(struct btrfs_root *root, int transid)
 		mutex_lock(&root->log_mutex);
 	}
 	finish_wait(&root->log_commit_wait[index], &wait);
+
+	return true;
 }
 
 static void wait_for_writer(struct btrfs_root *root)
@@ -3298,15 +3296,15 @@ static inline void btrfs_remove_all_log_ctxs(struct btrfs_root *root,
 int btrfs_sync_log(struct btrfs_trans_handle *trans,
 		   struct btrfs_root *root, struct btrfs_log_ctx *ctx)
 {
-	int index1;
-	int index2;
 	int mark;
 	int ret;
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct btrfs_root *log = root->log_root;
 	struct btrfs_root *log_root_tree = fs_info->log_root_tree;
 	struct btrfs_root_item new_root_item;
-	int log_transid = 0;
+	int log_transid = ctx->log_transid;
+	int index1 = log_transid % 2;
+	int index2;
 	struct btrfs_log_ctx root_log_ctx;
 	struct blk_plug plug;
 	u64 log_root_start;
@@ -3314,16 +3312,13 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 
 	mutex_lock(&root->log_mutex);
 	trace_btrfs_sync_log_enter(trans, root, ctx);
-	log_transid = ctx->log_transid;
 	if (root->log_transid_committed >= log_transid) {
 		trace_btrfs_sync_log_exit(trans, root, ctx, ctx->log_ret);
 		mutex_unlock(&root->log_mutex);
 		return ctx->log_ret;
 	}
 
-	index1 = log_transid % 2;
-	if (atomic_read(&root->log_commit[index1])) {
-		wait_log_commit(root, log_transid);
+	if (wait_log_commit(root, log_transid)) {
 		trace_btrfs_sync_log_exit(trans, root, ctx, ctx->log_ret);
 		mutex_unlock(&root->log_mutex);
 		return ctx->log_ret;
@@ -3333,8 +3328,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	atomic_set(&root->log_commit[index1], 1);
 
 	/* wait for previous tree log sync to complete */
-	if (atomic_read(&root->log_commit[(index1 + 1) % 2]))
-		wait_log_commit(root, log_transid - 1);
+	wait_log_commit(root, log_transid - 1);
 
 	wait_for_writer(root);
 
@@ -3467,10 +3461,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 		root_log_ctx.log_transid, log_root_tree->log_transid);
 	atomic_set(&log_root_tree->log_commit[index2], 1);
 
-	if (atomic_read(&log_root_tree->log_commit[(index2 + 1) % 2])) {
-		wait_log_commit(log_root_tree,
-				root_log_ctx.log_transid - 1);
-	}
+	wait_log_commit(log_root_tree, root_log_ctx.log_transid - 1);
 
 	/*
 	 * now that we've moved on to the tree of log tree roots,
-- 
2.47.2

[PATCH v2 4/5] btrfs: check for exit condition after waking in wait_log_commit()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We check for the exit condition after we add ourselves to the wait queue
and before we unlock the root's log_mutex, sleep and lock again log_mutex.
This is not incorrect, but it's not optimal since in the first iteration
this is pointless because we already know that root->log_commit[index] is
not zero, so we should check the exit condition only after unlocking
log_mutex, sleeping, waking up and locking again the log_mutex.

So move the check for the exit condition to bottom of the loop, after we
were woken and locked log_mutex again.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/tree-log.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index b6ed6671ee26..fe96810d3363 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3182,13 +3182,13 @@ static bool wait_log_commit(struct btrfs_root *root, int transid)
 		prepare_to_wait(&root->log_commit_wait[index],
 				&wait, TASK_UNINTERRUPTIBLE);
 
-		if (!(root->log_transid_committed < transid &&
-		      atomic_read(&root->log_commit[index])))
-			break;
-
 		mutex_unlock(&root->log_mutex);
 		schedule();
 		mutex_lock(&root->log_mutex);
+
+		if (!(root->log_transid_committed < transid &&
+		      atomic_read(&root->log_commit[index]) != 0))
+			break;
 	}
 	finish_wait(&root->log_commit_wait[index], &wait);
 
-- 
2.47.2

[PATCH v2 5/5] btrfs: use simple booleans for log_commit field in struct btrfs_root

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We are using atomic types for the log_commit array of struct btrfs_root
but all we need is simple booleans. The log_commit array elements are
always protected by the root's log_mutex, both for writes and reads, so
we can use a simple boolean. The use of atomics if from the very early
days of the log tree code where the access to the fields was not protected
by any lock.

So switch to simple booleans, which results in cheaper code and slightly
reduces the object size too.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.h             |  2 +-
 fs/btrfs/disk-io.c           |  2 --
 fs/btrfs/transaction.c       |  8 ++------
 fs/btrfs/tree-log.c          | 16 ++++++++--------
 include/trace/events/btrfs.h |  4 ++--
 5 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index d5d8b3899258..22ba2b4505b3 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -195,7 +195,7 @@ struct btrfs_root {
 	struct list_head log_ctxs[2];
 	/* Used only for log trees of subvolumes, not for the log root tree */
 	atomic_t log_writers;
-	atomic_t log_commit[2];
+	bool log_commit[2];
 	/*
 	 * Protected by the 'log_mutex' lock but can be read without holding
 	 * that lock to avoid unnecessary lock contention, in which case it
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 1ee0a19e40a8..d17e087b9cbe 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -666,8 +666,6 @@ static struct btrfs_root *btrfs_alloc_root(struct btrfs_fs_info *fs_info,
 	init_waitqueue_head(&root->log_commit_wait[1]);
 	INIT_LIST_HEAD(&root->log_ctxs[0]);
 	INIT_LIST_HEAD(&root->log_ctxs[1]);
-	atomic_set(&root->log_commit[0], 0);
-	atomic_set(&root->log_commit[1], 0);
 	atomic_set(&root->log_writers, 0);
 	refcount_set(&root->refs, 1);
 	atomic_set(&root->snapshot_force_cow, 0);
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 45149d027740..91b41f55a3ce 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1517,12 +1517,8 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
 			ASSERT(atomic_read(&root->log_writers) == 0,
 			       "atomic_read(&root->log_writers)=%d",
 			       atomic_read(&root->log_writers));
-			ASSERT(atomic_read(&root->log_commit[0]) == 0,
-			       "atomic_read(&root->log_commit[0])=%d",
-			       atomic_read(&root->log_commit[0]));
-			ASSERT(atomic_read(&root->log_commit[1]) == 0,
-			       "atomic_read(&root->log_commit[1])=%d",
-			       atomic_read(&root->log_commit[1]));
+			ASSERT(!root->log_commit[0]);
+			ASSERT(!root->log_commit[1]);
 
 			radix_tree_tag_clear(&fs_info->fs_roots_radix,
 					(unsigned long)btrfs_root_id(root),
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index fe96810d3363..e59569770b7b 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3170,7 +3170,7 @@ static bool wait_log_commit(struct btrfs_root *root, int transid)
 	DEFINE_WAIT(wait);
 	int index = transid % 2;
 
-	if (atomic_read(&root->log_commit[index]) == 0)
+	if (!root->log_commit[index])
 		return false;
 
 	/*
@@ -3187,7 +3187,7 @@ static bool wait_log_commit(struct btrfs_root *root, int transid)
 		mutex_lock(&root->log_mutex);
 
 		if (!(root->log_transid_committed < transid &&
-		      atomic_read(&root->log_commit[index]) != 0))
+		      root->log_commit[index]))
 			break;
 	}
 	finish_wait(&root->log_commit_wait[index], &wait);
@@ -3325,7 +3325,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	}
 	ASSERT(log_transid == root->log_transid,
 	       "log_transid=%d root->log_transid=%d", log_transid, root->log_transid);
-	atomic_set(&root->log_commit[index1], 1);
+	root->log_commit[index1] = true;
 
 	/* wait for previous tree log sync to complete */
 	wait_log_commit(root, log_transid - 1);
@@ -3445,7 +3445,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 		goto out;
 	}
 
-	if (atomic_read(&log_root_tree->log_commit[index2])) {
+	if (log_root_tree->log_commit[index2]) {
 		blk_finish_plug(&plug);
 		ret = btrfs_wait_tree_log_extents(log, mark);
 		wait_log_commit(log_root_tree,
@@ -3459,7 +3459,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	ASSERT(root_log_ctx.log_transid == log_root_tree->log_transid,
 	       "root_log_ctx.log_transid=%d log_root_tree->log_transid=%d",
 		root_log_ctx.log_transid, log_root_tree->log_transid);
-	atomic_set(&log_root_tree->log_commit[index2], 1);
+	log_root_tree->log_commit[index2] = true;
 
 	wait_log_commit(log_root_tree, root_log_ctx.log_transid - 1);
 
@@ -3559,7 +3559,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 
 	/*
 	 * We know there can only be one task here, since we have not yet set
-	 * root->log_commit[index1] to 0 and any task attempting to sync the
+	 * root->log_commit[index1] to false and any task attempting to sync the
 	 * log must wait for the previous log transaction to commit if it's
 	 * still in progress or wait for the current log transaction commit if
 	 * someone else already started it. We use <= and not < because the
@@ -3575,7 +3575,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	btrfs_remove_all_log_ctxs(log_root_tree, index2, ret);
 
 	log_root_tree->log_transid_committed++;
-	atomic_set(&log_root_tree->log_commit[index2], 0);
+	log_root_tree->log_commit[index2] = false;
 	mutex_unlock(&log_root_tree->log_mutex);
 
 	/*
@@ -3588,7 +3588,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	mutex_lock(&root->log_mutex);
 	btrfs_remove_all_log_ctxs(root, index1, ret);
 	root->log_transid_committed++;
-	atomic_set(&root->log_commit[index1], 0);
+	root->log_commit[index1] = false;
 	mutex_unlock(&root->log_mutex);
 
 	/*
diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
index 4c5c47c5edb7..f9d22cd71768 100644
--- a/include/trace/events/btrfs.h
+++ b/include/trace/events/btrfs.h
@@ -1578,9 +1578,9 @@ TRACE_EVENT(btrfs_sync_log_enter,
 		__entry->log_transid_committed	=
 			data_race(root->log_transid_committed);
 		__entry->log_committing		=
-			atomic_read(&root->log_commit[ctx->log_transid % 2]);
+			data_race(root->log_commit[ctx->log_transid % 2]);
 		__entry->log_committing_prev	=
-			atomic_read(&root->log_commit[(ctx->log_transid + 1) % 2]);
+			data_race(root->log_commit[(ctx->log_transid + 1) % 2]);
 		__entry->log_writers		= atomic_read(&root->log_writers);
 	),
 
-- 
2.47.2

Re: [PATCH v2 0/5] btrfs: some optimizations and cleanups for log syncing

[Jeff Layton]

On Wed, 2026-07-22 at 16:24 +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Some optimizations for log syncing, specially for non-sdd mounts, and
> some cleanups. Details in the changelogs of each patch.
> 
> V2: Removed two patches which sashiko found problematic, the benefits of
>     fixing them up versus the complexity required to fix them is not worth
>     it. Updated last patch to add the data_race() annotation in the trace
>     events.
> 
> Filipe Manana (5):
>   btrfs: stop sleeping for one jiffy in non-ssd mounts during log commit
>   btrfs: remove log batch counter use for fsync
>   btrfs: move condition for log commit wait into wait_log_commit()
>   btrfs: check for exit condition after waking in wait_log_commit()
>   btrfs: use simple booleans for log_commit field in struct btrfs_root
> 
>  fs/btrfs/ctree.h             |  6 +--
>  fs/btrfs/disk-io.c           |  3 --
>  fs/btrfs/file.c              |  4 --
>  fs/btrfs/transaction.c       |  8 +---
>  fs/btrfs/tree-log.c          | 85 ++++++++++++------------------------
>  include/trace/events/btrfs.h |  4 +-
>  6 files changed, 32 insertions(+), 78 deletions(-)

These all look like good cleanups, and the diffstat is nice!

Reviewed-by: Jeff Layton <jlayton@kernel.org>