[PATCH v2 0/2] xfs: skip redundant fsync cache flushes

[PATCH v2 0/2] xfs: skip redundant fsync cache flushes

[Daejun Park via B4 Relay]

Concurrent fsyncs often wait for an iclog that another task wrote out,
and then send their own cache flush. Patch 1 numbers the cache flushes
on each buftarg. Patch 2 uses the numbers to skip that flush when a
later flush has completed.

16 fio jobs of 4k append + fsync in a QEMU guest: 1111 -> 4498
fsyncs/s. fstests -g quick shows no change, and a dm-log-writes crash
test on an internal log lost no data (patch 2).

---
Changes in v2:
- Zero ic_flush_seq after the bio setup and take the flush number
  where REQ_PREFLUSH is set
- Move the fio result into the commit log of patch 2, measured again
  for both kernels on the new base, and add the single job result
- Rename the argument of xfs_buftarg_flushed_since() and the fsync
  local to submit_seq, and describe the submit and completion ordering
  in the comments
- Keep the fsync comment on why the flush is needed
- In patch 2, shorten the DAX text in the comment and the commit log,
  and drop the commit log sentence on numbering the fsync's own flush
- Say below --- of patch 2 what changes for a failed flush on
  bio-based dm
- Rebase onto the current for-next (v7.4-rc2)

---
Daejun Park (2):
      xfs: number the cache flushes submitted to each buftarg
      xfs: skip the fsync flush when a later flush has completed

 fs/xfs/xfs_buf.c      | 40 ++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_buf.h      | 43 +++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_file.c     | 18 +++++++++++++++---
 fs/xfs/xfs_log.c      | 28 +++++++++++++++++++++-------
 fs/xfs/xfs_log_priv.h |  1 +
 5 files changed, 120 insertions(+), 10 deletions(-)
---
base-commit: 48326a2382d80715475d10ce98734ac65d2d2150
change-id: 20261105-xfs-fsync-flush-seq-83ae2cff6225

Best regards,
-- 
Daejun Park <daejun7.park@samsung.com>

[PATCH v2 1/2] xfs: number the cache flushes submitted to each buftarg

[Daejun Park via B4 Relay]

From: Daejun Park <daejun7.park@samsung.com>

fsync can only skip its own cache flush when its log force wrote out
the iclog. It cannot tell whether a flush that another task submitted
covers its data.

Number the cache flushes that the log code submits to each buftarg,
and record the highest number that completed without error. Nothing
reads the numbers yet.

Signed-off-by: Daejun Park <daejun7.park@samsung.com>
---
 fs/xfs/xfs_buf.c      | 13 +++++++++++++
 fs/xfs/xfs_buf.h      | 21 +++++++++++++++++++++
 fs/xfs/xfs_log.c      | 28 +++++++++++++++++++++-------
 fs/xfs/xfs_log_priv.h |  1 +
 4 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 6c93b4f5629..9025b177931 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1772,6 +1772,19 @@ xfs_init_buftarg(
 	return -ENOMEM;
 }
 
+/* Record that the flush numbered @seq completed successfully. */
+void
+xfs_buftarg_flush_done(
+	struct xfs_buftarg	*btp,
+	s64			seq)
+{
+	s64			old = atomic64_read(&btp->bt_flush_completed);
+
+	while (seq > old &&
+	       !atomic64_try_cmpxchg(&btp->bt_flush_completed, &old, seq))
+		;
+}
+
 struct xfs_buftarg *
 xfs_alloc_buftarg(
 	struct xfs_mount	*mp,
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index a4729253b56..6e6f0dbfef6 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -103,6 +103,17 @@ struct xfs_buftarg {
 	unsigned int		bt_awu_max;
 
 	struct rhashtable	bt_hash;
+
+	/*
+	 * Cache flush tracking: a flush that takes the next number from
+	 * xfs_buftarg_flush_start() before it is submitted records it here
+	 * once it completes without error, keeping the highest number. A
+	 * completed flush with a number above a sample taken after some
+	 * writes completed covers those writes. Flushes without a number
+	 * are not counted.
+	 */
+	atomic64_t		bt_flush_submitted;
+	atomic64_t		bt_flush_completed;
 };
 
 struct xfs_buf_map {
@@ -356,6 +367,16 @@ xfs_buf_update_cksum(struct xfs_buf *bp, unsigned long cksum_offset)
 struct xfs_buftarg *xfs_alloc_buftarg(struct xfs_mount *mp,
 		struct file *bdev_file);
 extern void xfs_free_buftarg(struct xfs_buftarg *);
+
+/* Call right before submitting a cache flush to @btp. */
+static inline s64
+xfs_buftarg_flush_start(
+	struct xfs_buftarg	*btp)
+{
+	return atomic64_inc_return(&btp->bt_flush_submitted);
+}
+
+void xfs_buftarg_flush_done(struct xfs_buftarg *btp, s64 seq);
 extern void xfs_buftarg_wait(struct xfs_buftarg *);
 extern void xfs_buftarg_drain(struct xfs_buftarg *);
 int xfs_configure_buftarg(struct xfs_buftarg *btp, unsigned int sectorsize,
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 74b324f915a..d99a5b678cf 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1211,6 +1211,8 @@ xlog_ioend_work(
 		xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
 	}
 
+	if (!error && iclog->ic_flush_seq)
+		xfs_buftarg_flush_done(log->l_targ, iclog->ic_flush_seq);
 	xlog_state_done_syncing(iclog);
 	bio_uninit(&iclog->ic_bio);
 
@@ -1570,19 +1572,21 @@ xlog_flush_end_io(
 	bio_put(bio);
 }
 
-static void
+static s64
 xlog_flush_async(
 	struct xlog_flush_done	*done,
-	struct block_device	*bdev)
+	struct xfs_buftarg	*btp)
 {
 	struct bio		*bio;
+	s64			seq = xfs_buftarg_flush_start(btp);
 
-	bio = bio_alloc(bdev, 0, REQ_OP_WRITE | REQ_PREFLUSH | REQ_SYNC,
+	bio = bio_alloc(btp->bt_bdev, 0, REQ_OP_WRITE | REQ_PREFLUSH | REQ_SYNC,
 			GFP_NOFS);
 	bio->bi_private = done;
 	bio->bi_end_io = xlog_flush_end_io;
 	atomic_inc(&done->pending);
 	submit_bio(bio);
+	return seq;
 }
 
 /*
@@ -1605,16 +1609,17 @@ xlog_flush_data_caches(
 		.pending	= ATOMIC_INIT(1),
 		.done		= COMPLETION_INITIALIZER_ONSTACK(done.done),
 	};
+	s64			dseq = 0, rseq = 0, lseq;
 
 	*did_flush = false;
 	if (mp->m_ddev_targp != mp->m_logdev_targp &&
 	    bdev_write_cache(mp->m_ddev_targp->bt_bdev)) {
-		xlog_flush_async(&done, mp->m_ddev_targp->bt_bdev);
+		dseq = xlog_flush_async(&done, mp->m_ddev_targp);
 		*did_flush = true;
 	}
 	if (mp->m_rtdev_targp && mp->m_rtdev_targp != mp->m_ddev_targp &&
 	    bdev_write_cache(mp->m_rtdev_targp->bt_bdev)) {
-		xlog_flush_async(&done, mp->m_rtdev_targp->bt_bdev);
+		rseq = xlog_flush_async(&done, mp->m_rtdev_targp);
 		*did_flush = true;
 	}
 
@@ -1623,12 +1628,17 @@ xlog_flush_data_caches(
 		 * If we flushed any other device, also use an async flush for
 		 * the log device so that all flushes happen in parallel.
 		 */
-		xlog_flush_async(&done, mp->m_logdev_targp->bt_bdev);
+		lseq = xlog_flush_async(&done, mp->m_logdev_targp);
 
 		xlog_flush_done(&done);
 		wait_for_completion(&done.done);
 		if (done.status)
 			return -EIO;
+		if (dseq)
+			xfs_buftarg_flush_done(mp->m_ddev_targp, dseq);
+		if (rseq)
+			xfs_buftarg_flush_done(mp->m_rtdev_targp, rseq);
+		xfs_buftarg_flush_done(mp->m_logdev_targp, lseq);
 	}
 
 	return 0;
@@ -1678,14 +1688,18 @@ xlog_write_iclog(
 	iclog->ic_bio.bi_iter.bi_sector = log->l_logBBstart + bno;
 	iclog->ic_bio.bi_end_io = xlog_bio_end_io;
 	iclog->ic_bio.bi_private = iclog;
+	iclog->ic_flush_seq = 0;
 
 	if (iclog->ic_flags & XLOG_ICL_NEED_FLUSH) {
 		if (xlog_flush_data_caches(log->l_mp, &did_flush))
 			goto shutdown;
 	}
 	if (!did_flush &&
-	    (iclog->ic_flags & (XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FLUSH_LOG)))
+	    (iclog->ic_flags &
+	     (XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FLUSH_LOG))) {
 		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
+		iclog->ic_flush_seq = xfs_buftarg_flush_start(log->l_targ);
+	}
 	if (iclog->ic_flags & XLOG_ICL_NEED_FUA)
 		iclog->ic_bio.bi_opf |= REQ_FUA;
 
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index 529720a4984..df0f13c4e04 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -222,6 +222,7 @@ struct xlog_in_core {
 #endif
 	struct semaphore	ic_sema;
 	struct work_struct	ic_end_io_work;
+	s64			ic_flush_seq;	/* buftarg flush number */
 	struct bio		ic_bio;
 	struct bio_vec		ic_bvec[];
 };

-- 
2.43.0

Re: [PATCH v2 1/2] xfs: number the cache flushes submitted to each buftarg

[Brian Foster]

On Wed, Nov 11, 2026 at 12:17:11PM +0900, Daejun Park via B4 Relay wrote:
> From: Daejun Park <daejun7.park@samsung.com>
> 
> fsync can only skip its own cache flush when its log force wrote out
> the iclog. It cannot tell whether a flush that another task submitted
> covers its data.
> 
> Number the cache flushes that the log code submits to each buftarg,
> and record the highest number that completed without error. Nothing
> reads the numbers yet.
> 
> Signed-off-by: Daejun Park <daejun7.park@samsung.com>
> ---
>  fs/xfs/xfs_buf.c      | 13 +++++++++++++
>  fs/xfs/xfs_buf.h      | 21 +++++++++++++++++++++
>  fs/xfs/xfs_log.c      | 28 +++++++++++++++++++++-------
>  fs/xfs/xfs_log_priv.h |  1 +
>  4 files changed, 56 insertions(+), 7 deletions(-)
> 
...
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 74b324f915a..d99a5b678cf 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1211,6 +1211,8 @@ xlog_ioend_work(
>  		xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
>  	}
>  
> +	if (!error && iclog->ic_flush_seq)
> +		xfs_buftarg_flush_done(log->l_targ, iclog->ic_flush_seq);

Taking another look at this I started to also wonder whether to store
this flush seq in the xlog_flush_done struct. It looks like we reuse
that for multiple async flushes in flight, however, so probably not
worth the complication to try and track all that there.

Thanks for the tweaks and this all looks reasonable to me:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  	xlog_state_done_syncing(iclog);
>  	bio_uninit(&iclog->ic_bio);
>  
> @@ -1570,19 +1572,21 @@ xlog_flush_end_io(
>  	bio_put(bio);
>  }
>  
> -static void
> +static s64
>  xlog_flush_async(
>  	struct xlog_flush_done	*done,
> -	struct block_device	*bdev)
> +	struct xfs_buftarg	*btp)
>  {
>  	struct bio		*bio;
> +	s64			seq = xfs_buftarg_flush_start(btp);
>  
> -	bio = bio_alloc(bdev, 0, REQ_OP_WRITE | REQ_PREFLUSH | REQ_SYNC,
> +	bio = bio_alloc(btp->bt_bdev, 0, REQ_OP_WRITE | REQ_PREFLUSH | REQ_SYNC,
>  			GFP_NOFS);
>  	bio->bi_private = done;
>  	bio->bi_end_io = xlog_flush_end_io;
>  	atomic_inc(&done->pending);
>  	submit_bio(bio);
> +	return seq;
>  }
>  
>  /*
> @@ -1605,16 +1609,17 @@ xlog_flush_data_caches(
>  		.pending	= ATOMIC_INIT(1),
>  		.done		= COMPLETION_INITIALIZER_ONSTACK(done.done),
>  	};
> +	s64			dseq = 0, rseq = 0, lseq;
>  
>  	*did_flush = false;
>  	if (mp->m_ddev_targp != mp->m_logdev_targp &&
>  	    bdev_write_cache(mp->m_ddev_targp->bt_bdev)) {
> -		xlog_flush_async(&done, mp->m_ddev_targp->bt_bdev);
> +		dseq = xlog_flush_async(&done, mp->m_ddev_targp);
>  		*did_flush = true;
>  	}
>  	if (mp->m_rtdev_targp && mp->m_rtdev_targp != mp->m_ddev_targp &&
>  	    bdev_write_cache(mp->m_rtdev_targp->bt_bdev)) {
> -		xlog_flush_async(&done, mp->m_rtdev_targp->bt_bdev);
> +		rseq = xlog_flush_async(&done, mp->m_rtdev_targp);
>  		*did_flush = true;
>  	}
>  
> @@ -1623,12 +1628,17 @@ xlog_flush_data_caches(
>  		 * If we flushed any other device, also use an async flush for
>  		 * the log device so that all flushes happen in parallel.
>  		 */
> -		xlog_flush_async(&done, mp->m_logdev_targp->bt_bdev);
> +		lseq = xlog_flush_async(&done, mp->m_logdev_targp);
>  
>  		xlog_flush_done(&done);
>  		wait_for_completion(&done.done);
>  		if (done.status)
>  			return -EIO;
> +		if (dseq)
> +			xfs_buftarg_flush_done(mp->m_ddev_targp, dseq);
> +		if (rseq)
> +			xfs_buftarg_flush_done(mp->m_rtdev_targp, rseq);
> +		xfs_buftarg_flush_done(mp->m_logdev_targp, lseq);
>  	}
>  
>  	return 0;
> @@ -1678,14 +1688,18 @@ xlog_write_iclog(
>  	iclog->ic_bio.bi_iter.bi_sector = log->l_logBBstart + bno;
>  	iclog->ic_bio.bi_end_io = xlog_bio_end_io;
>  	iclog->ic_bio.bi_private = iclog;
> +	iclog->ic_flush_seq = 0;
>  
>  	if (iclog->ic_flags & XLOG_ICL_NEED_FLUSH) {
>  		if (xlog_flush_data_caches(log->l_mp, &did_flush))
>  			goto shutdown;
>  	}
>  	if (!did_flush &&
> -	    (iclog->ic_flags & (XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FLUSH_LOG)))
> +	    (iclog->ic_flags &
> +	     (XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FLUSH_LOG))) {
>  		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
> +		iclog->ic_flush_seq = xfs_buftarg_flush_start(log->l_targ);
> +	}
>  	if (iclog->ic_flags & XLOG_ICL_NEED_FUA)
>  		iclog->ic_bio.bi_opf |= REQ_FUA;
>  
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 529720a4984..df0f13c4e04 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -222,6 +222,7 @@ struct xlog_in_core {
>  #endif
>  	struct semaphore	ic_sema;
>  	struct work_struct	ic_end_io_work;
> +	s64			ic_flush_seq;	/* buftarg flush number */
>  	struct bio		ic_bio;
>  	struct bio_vec		ic_bvec[];
>  };
> 
> -- 
> 2.43.0
> 
> 
> 

[PATCH v2 2/2] xfs: skip the fsync flush when a later flush has completed

[Daejun Park via B4 Relay]

From: Daejun Park <daejun7.park@samsung.com>

Since commit 47c7d0b19502 ("xfs: fix incorrect log_flushed on
fsync"), fsync sends its own cache flush whenever another task wrote
out the iclog it waited for. With concurrent fsyncs, these flushes can
limit throughput.

Skip the flush when a flush submitted after the data writes completed
has finished. DAX files keep the old behavior.

fio 4k append + fsync in a QEMU guest, virtio-blk (cache=none) on a
Samsung 990 PRO NVMe SSD without power loss protection, before ->
after:

  16 jobs: 1111 -> 4498 fsyncs/s, mean fsync latency 14.4 -> 3.6 ms
   1 job:  399 -> 396 fsyncs/s

Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Daejun Park <daejun7.park@samsung.com>
---
Christoph's 2017 mail with the sequence counter idea:
https://lore.kernel.org/20170830142134.GA650@lst.de

A dm-log-writes test written for this series (8 fsync writers,
internal log) lost no fsync-ed block in 528 replays. At least 33319
of 48048 fsyncs skipped their flush through the new check. A variant
that samples before the writeback loses 16 of 528. The test needs
"dm-log-writes: log earlier flushes' writes when a later flush
completes", sent to dm-devel.

With an internal log, an fsync that takes the new skip can miss a
failed flush on bio-based dm. dm drops the error of a preflush sent
with data, unless the table is linear or stripe on a single device.
The skip trusts the status of the iclog bio, as the log and the fsync
that wrote the iclog already do. Before this patch the other fsyncs
sent their own flush and returned its result. With 8 writers on a
two-device linear table whose second device fails every flush, 2590
of 4000 fsyncs returned EIO before and 37 do now. From the code,
bcache in writeback mode drops that error too. md reports no flush
error at all. Neither was tested.
---
 fs/xfs/xfs_buf.c  | 27 +++++++++++++++++++++++++++
 fs/xfs/xfs_buf.h  | 22 ++++++++++++++++++++++
 fs/xfs/xfs_file.c | 18 +++++++++++++++---
 3 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 9025b177931..2862cbdeda4 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1785,6 +1785,33 @@ xfs_buftarg_flush_done(
 		;
 }
 
+/*
+ * Return true if a flush has been submitted and completed since @submit_seq
+ * was sampled. If so, all writes that completed before @submit_seq was sampled
+ * are on stable storage.
+ */
+bool
+xfs_buftarg_flushed_since(
+	struct xfs_buftarg	*btp,
+	s64			submit_seq)
+{
+	return atomic64_read(&btp->bt_flush_completed) > submit_seq;
+}
+
+/* Flush the device cache and record it. */
+int
+xfs_buftarg_flush(
+	struct xfs_buftarg	*btp)
+{
+	s64			seq = xfs_buftarg_flush_start(btp);
+	int			error;
+
+	error = blkdev_issue_flush(btp->bt_bdev);
+	if (!error)
+		xfs_buftarg_flush_done(btp, seq);
+	return error;
+}
+
 struct xfs_buftarg *
 xfs_alloc_buftarg(
 	struct xfs_mount	*mp,
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 6e6f0dbfef6..08282e1b3c8 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -368,6 +368,26 @@ struct xfs_buftarg *xfs_alloc_buftarg(struct xfs_mount *mp,
 		struct file *bdev_file);
 extern void xfs_free_buftarg(struct xfs_buftarg *);
 
+/*
+ * Sample the flush submit counter of @btp. Call this after the writes that a
+ * flush must cover have completed. Flushes take their number before they are
+ * submitted, so a flush numbered above the sample was submitted after it and
+ * covers those writes once it succeeds. xfs_buftarg_flushed_since() checks
+ * for such a flush.
+ */
+static inline s64
+xfs_buftarg_flush_seq(
+	struct xfs_buftarg	*btp)
+{
+	/*
+	 * Order the read after the write completions the caller waited for.
+	 * Pairs with the fully ordered atomic64_inc_return() that
+	 * xfs_buftarg_flush_start() does before the flush is submitted.
+	 */
+	smp_mb();
+	return atomic64_read(&btp->bt_flush_submitted);
+}
+
 /* Call right before submitting a cache flush to @btp. */
 static inline s64
 xfs_buftarg_flush_start(
@@ -377,6 +397,8 @@ xfs_buftarg_flush_start(
 }
 
 void xfs_buftarg_flush_done(struct xfs_buftarg *btp, s64 seq);
+bool xfs_buftarg_flushed_since(struct xfs_buftarg *btp, s64 submit_seq);
+int xfs_buftarg_flush(struct xfs_buftarg *btp);
 extern void xfs_buftarg_wait(struct xfs_buftarg *);
 extern void xfs_buftarg_drain(struct xfs_buftarg *);
 int xfs_configure_buftarg(struct xfs_buftarg *btp, unsigned int sectorsize,
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 396aca8c66a..5af00520cbe 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -87,7 +87,9 @@ xfs_file_fsync(
 	int			datasync)
 {
 	struct xfs_inode	*ip = XFS_I(file->f_mapping->host);
+	struct xfs_buftarg	*btp = xfs_inode_buftarg(ip);
 	int			log_flushed = 0;
+	s64			submit_seq;
 	int			error;
 
 	trace_xfs_file_fsync(ip);
@@ -95,6 +97,7 @@ xfs_file_fsync(
 	error = file_write_and_wait_range(file, start, end);
 	if (error)
 		return error;
+	submit_seq = xfs_buftarg_flush_seq(btp);
 
 	if (xfs_is_shutdown(ip->i_mount))
 		return -EIO;
@@ -119,12 +122,21 @@ xfs_file_fsync(
 	 * file data target cache here. This can happen for fdatasync/O_DSYNC
 	 * when no metadata needed to be committed.
 	 *
+	 * Flush the file data target unless a flush submitted to it after the
+	 * data writes above completed has already finished. That is usually
+	 * the flush before an iclog written out by this log force or by
+	 * another task, or another fsync's flush.
+	 *
+	 * DAX writes are not block I/O, so DAX only skips the flush when
+	 * this log force flushed, as before.
+	 *
 	 * Use the inode's actual file data target rather than assuming the
 	 * main data device.
 	 */
-	if (!log_flushed)
-		return blkdev_issue_flush(xfs_inode_buftarg(ip)->bt_bdev);
-	return 0;
+	if (log_flushed ||
+	    (!IS_DAX(VFS_I(ip)) && xfs_buftarg_flushed_since(btp, submit_seq)))
+		return 0;
+	return xfs_buftarg_flush(btp);
 }
 
 static int

-- 
2.43.0

Re: [PATCH v2 2/2] xfs: skip the fsync flush when a later flush has completed

[Brian Foster]

On Wed, Nov 11, 2026 at 12:17:12PM +0900, Daejun Park via B4 Relay wrote:
> From: Daejun Park <daejun7.park@samsung.com>
> 
> Since commit 47c7d0b19502 ("xfs: fix incorrect log_flushed on
> fsync"), fsync sends its own cache flush whenever another task wrote
> out the iclog it waited for. With concurrent fsyncs, these flushes can
> limit throughput.
> 
> Skip the flush when a flush submitted after the data writes completed
> has finished. DAX files keep the old behavior.
> 
> fio 4k append + fsync in a QEMU guest, virtio-blk (cache=none) on a
> Samsung 990 PRO NVMe SSD without power loss protection, before ->
> after:
> 
>   16 jobs: 1111 -> 4498 fsyncs/s, mean fsync latency 14.4 -> 3.6 ms
>    1 job:  399 -> 396 fsyncs/s
> 
> Suggested-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Daejun Park <daejun7.park@samsung.com>
> ---
> Christoph's 2017 mail with the sequence counter idea:
> https://lore.kernel.org/20170830142134.GA650@lst.de
> 
> A dm-log-writes test written for this series (8 fsync writers,
> internal log) lost no fsync-ed block in 528 replays. At least 33319
> of 48048 fsyncs skipped their flush through the new check. A variant
> that samples before the writeback loses 16 of 528. The test needs
> "dm-log-writes: log earlier flushes' writes when a later flush
> completes", sent to dm-devel.
> 
> With an internal log, an fsync that takes the new skip can miss a
> failed flush on bio-based dm. dm drops the error of a preflush sent
> with data, unless the table is linear or stripe on a single device.
> The skip trusts the status of the iclog bio, as the log and the fsync
> that wrote the iclog already do. Before this patch the other fsyncs
> sent their own flush and returned its result. With 8 writers on a
> two-device linear table whose second device fails every flush, 2590
> of 4000 fsyncs returned EIO before and 37 do now. From the code,
> bcache in writeback mode drops that error too. md reports no flush
> error at all. Neither was tested.
> ---

Hmm.. that's kind of interesting. So IIUC unless dm can pass through a
preflush+data bio, it will split out the flushes and ignore any error
status in favor of the packaged data I/O..?

I agree with the reasoning in that we're already subject to that caveat
and the patch looks fine to me. Modulo Dave's comments on perf results:

Reviewed-by: Brian Foster <bfoster@redhat.com>

But if you have to post this again I think it would be useful to
describe this caveat in the commit log description as well. If there
happens to be some currently unknown flush failiure scenario that
materializes in the future and we don't currently hit due to spamming
enough flushes, that would be a useful clue about a subtle behavior
change.

Brian

>  fs/xfs/xfs_buf.c  | 27 +++++++++++++++++++++++++++
>  fs/xfs/xfs_buf.h  | 22 ++++++++++++++++++++++
>  fs/xfs/xfs_file.c | 18 +++++++++++++++---
>  3 files changed, 64 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 9025b177931..2862cbdeda4 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1785,6 +1785,33 @@ xfs_buftarg_flush_done(
>  		;
>  }
>  
> +/*
> + * Return true if a flush has been submitted and completed since @submit_seq
> + * was sampled. If so, all writes that completed before @submit_seq was sampled
> + * are on stable storage.
> + */
> +bool
> +xfs_buftarg_flushed_since(
> +	struct xfs_buftarg	*btp,
> +	s64			submit_seq)
> +{
> +	return atomic64_read(&btp->bt_flush_completed) > submit_seq;
> +}
> +
> +/* Flush the device cache and record it. */
> +int
> +xfs_buftarg_flush(
> +	struct xfs_buftarg	*btp)
> +{
> +	s64			seq = xfs_buftarg_flush_start(btp);
> +	int			error;
> +
> +	error = blkdev_issue_flush(btp->bt_bdev);
> +	if (!error)
> +		xfs_buftarg_flush_done(btp, seq);
> +	return error;
> +}
> +
>  struct xfs_buftarg *
>  xfs_alloc_buftarg(
>  	struct xfs_mount	*mp,
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 6e6f0dbfef6..08282e1b3c8 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -368,6 +368,26 @@ struct xfs_buftarg *xfs_alloc_buftarg(struct xfs_mount *mp,
>  		struct file *bdev_file);
>  extern void xfs_free_buftarg(struct xfs_buftarg *);
>  
> +/*
> + * Sample the flush submit counter of @btp. Call this after the writes that a
> + * flush must cover have completed. Flushes take their number before they are
> + * submitted, so a flush numbered above the sample was submitted after it and
> + * covers those writes once it succeeds. xfs_buftarg_flushed_since() checks
> + * for such a flush.
> + */
> +static inline s64
> +xfs_buftarg_flush_seq(
> +	struct xfs_buftarg	*btp)
> +{
> +	/*
> +	 * Order the read after the write completions the caller waited for.
> +	 * Pairs with the fully ordered atomic64_inc_return() that
> +	 * xfs_buftarg_flush_start() does before the flush is submitted.
> +	 */
> +	smp_mb();
> +	return atomic64_read(&btp->bt_flush_submitted);
> +}
> +
>  /* Call right before submitting a cache flush to @btp. */
>  static inline s64
>  xfs_buftarg_flush_start(
> @@ -377,6 +397,8 @@ xfs_buftarg_flush_start(
>  }
>  
>  void xfs_buftarg_flush_done(struct xfs_buftarg *btp, s64 seq);
> +bool xfs_buftarg_flushed_since(struct xfs_buftarg *btp, s64 submit_seq);
> +int xfs_buftarg_flush(struct xfs_buftarg *btp);
>  extern void xfs_buftarg_wait(struct xfs_buftarg *);
>  extern void xfs_buftarg_drain(struct xfs_buftarg *);
>  int xfs_configure_buftarg(struct xfs_buftarg *btp, unsigned int sectorsize,
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 396aca8c66a..5af00520cbe 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -87,7 +87,9 @@ xfs_file_fsync(
>  	int			datasync)
>  {
>  	struct xfs_inode	*ip = XFS_I(file->f_mapping->host);
> +	struct xfs_buftarg	*btp = xfs_inode_buftarg(ip);
>  	int			log_flushed = 0;
> +	s64			submit_seq;
>  	int			error;
>  
>  	trace_xfs_file_fsync(ip);
> @@ -95,6 +97,7 @@ xfs_file_fsync(
>  	error = file_write_and_wait_range(file, start, end);
>  	if (error)
>  		return error;
> +	submit_seq = xfs_buftarg_flush_seq(btp);
>  
>  	if (xfs_is_shutdown(ip->i_mount))
>  		return -EIO;
> @@ -119,12 +122,21 @@ xfs_file_fsync(
>  	 * file data target cache here. This can happen for fdatasync/O_DSYNC
>  	 * when no metadata needed to be committed.
>  	 *
> +	 * Flush the file data target unless a flush submitted to it after the
> +	 * data writes above completed has already finished. That is usually
> +	 * the flush before an iclog written out by this log force or by
> +	 * another task, or another fsync's flush.
> +	 *
> +	 * DAX writes are not block I/O, so DAX only skips the flush when
> +	 * this log force flushed, as before.
> +	 *
>  	 * Use the inode's actual file data target rather than assuming the
>  	 * main data device.
>  	 */
> -	if (!log_flushed)
> -		return blkdev_issue_flush(xfs_inode_buftarg(ip)->bt_bdev);
> -	return 0;
> +	if (log_flushed ||
> +	    (!IS_DAX(VFS_I(ip)) && xfs_buftarg_flushed_since(btp, submit_seq)))
> +		return 0;
> +	return xfs_buftarg_flush(btp);
>  }
>  
>  static int
> 
> -- 
> 2.43.0
> 
> 
> 

Re: [PATCH v2 0/2] xfs: skip redundant fsync cache flushes

[Dave Chinner]

On Wed, Nov 11, 2026 at 12:17:10PM +0900, Daejun Park via B4 Relay wrote:
> Concurrent fsyncs often wait for an iclog that another task wrote out,
> and then send their own cache flush. Patch 1 numbers the cache flushes
> on each buftarg. Patch 2 uses the numbers to skip that flush when a
> later flush has completed.
> 
> 16 fio jobs of 4k append + fsync in a QEMU guest: 1111 -> 4498
> fsyncs/s.

Ok, the number looks good, but IMO that's a very low number of
concurrent fsyncs in general terms.

Typically the journal doesn't start to bottleneck until there
are hundreds to thousands of concurrent fsyncs and/or synchronous
transaction commits running at once. NFS servers are typical for
this, where there can be thousands of in-flight modifications that
each require require ->commit_metadata to fsync changes before the
server returns the response.

In the past, I have used dbench with increasing number of clients
(into the thousands) to stress fsync/log force behaviour and
scalability. The journal has historically topped out at somewhere
around 20,000 forced log writes a second. It hits that limit when
the built in batching (don't send the current iclog for write when
not full if the previous one is still syncing) is not longer
effective at reducing the number of iclog writes the log forcing
load is driving....

Hence the numbers look promising for low concurrency fsync
workloads, but I'd really like to know waht happens at the high end
when log force count rather than per-file fsync latency is the
performance limiting factor. I suspect that it will improve
performance by further mitigating the number of log writes and/or
cache flushes that need to be done, but it really needs to be
tested....

I'd also like to know how this affects storage that does not require
cache flushes - in those cases flushes are no-ops, so all this
tracking and sequencing would likely be pure overhead....

Cheers,

Dave.
-- 
Dave Chinner
dgc@kernel.org