fix and then optimize cache flushes for the RT device

fix and then optimize cache flushes for the RT device

[Christoph Hellwig]

Hi all,

when tracing workloads, I realized that currently the volatile write
cache on RT devices is only flushed by fsync, but never at all for
workloads that do not use fsync.

This series fixes that, and also optimizes cache flushes for file systems
with multiple devices.

Diffstat:
 xfs_file.c     |   50 +++++++------------------
 xfs_log.c      |  113 +++++++++++++++++++++++++++++++++++++++++++++++++--------
 xfs_log_cil.c  |   16 +++++---
 xfs_log_priv.h |   10 +++--
 4 files changed, 129 insertions(+), 60 deletions(-)

[PATCH 1/6] xfs: also flush the RT device cache in xlog_write_iclog

[Christoph Hellwig]

The cache flush before writing the CIL start record no only needs to
ensure any metadata covered by the overwritten part of the log is on
stable storage, but also that any data pointed to by metadata logged
is on stable storage, as otherwise log recovery could created allocated
blocks that point to stale data.  Fortunately the code already
handles this right for the data device, but it also needs to flush
the RT device for this to work for data on the RT device.

Also update the comments to explicitly mention this case.

This omission goes back to the first days of cache control in XFS.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c     | 45 ++++++++++++++++++++++++++++++--------------
 fs/xfs/xfs_log_cil.c |  7 ++++---
 2 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index f807f8f4f705..8079f2e003db 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1538,6 +1538,35 @@ xlog_bio_end_io(
 		   &iclog->ic_end_io_work);
 }
 
+/*
+ * When using multiple devices, we also need to flush the data and RT device
+ * caches first to ensure that all metadata writeback covered by the LSN in
+ * this iclog is on stable storage. This is slow, but it *must* complete
+ * before we issue the external log IO.
+ *
+ * If the flush fails, we cannot conclude that past metadata writeback from
+ * the log succeeded.  Repeating the flush is not possible, hence we must
+ * shut down with log IO error to avoid shutdown re-entering this path and
+ * erroring out again.
+ */
+static int
+xlog_flush_data_caches(
+	struct xlog		*log)
+{
+	struct xfs_mount	*mp = log->l_mp;
+
+	if (log->l_targ != mp->m_ddev_targp) {
+		if (blkdev_issue_flush(mp->m_ddev_targp->bt_bdev))
+			return -EIO;
+	}
+	if (mp->m_rtdev_targp && mp->m_rtdev_targp != mp->m_ddev_targp) {
+		if (blkdev_issue_flush(mp->m_rtdev_targp->bt_bdev))
+			return -EIO;
+	}
+
+	return 0;
+}
+
 STATIC void
 xlog_write_iclog(
 	struct xlog		*log,
@@ -1582,21 +1611,9 @@ xlog_write_iclog(
 	iclog->ic_bio.bi_private = iclog;
 
 	if (iclog->ic_flags & XLOG_ICL_NEED_FLUSH) {
-		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
-		/*
-		 * For external log devices, we also need to flush the data
-		 * device cache first to ensure all metadata writeback covered
-		 * by the LSN in this iclog is on stable storage. This is slow,
-		 * but it *must* complete before we issue the external log IO.
-		 *
-		 * If the flush fails, we cannot conclude that past metadata
-		 * writeback from the log succeeded.  Repeating the flush is
-		 * not possible, hence we must shut down with log IO error to
-		 * avoid shutdown re-entering this path and erroring out again.
-		 */
-		if (log->l_targ != log->l_mp->m_ddev_targp &&
-		    blkdev_issue_flush(log->l_mp->m_ddev_targp->bt_bdev))
+		if (xlog_flush_data_caches(log))
 			goto shutdown;
+		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
 	}
 	if (iclog->ic_flags & XLOG_ICL_NEED_FUA)
 		iclog->ic_bio.bi_opf |= REQ_FUA;
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 639f875a8fb2..3d159b1350df 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -1055,9 +1055,10 @@ xlog_cil_set_ctx_write_state(
 		spin_unlock(&cil->xc_push_lock);
 
 		/*
-		 * Make sure the metadata we are about to overwrite in the log
-		 * has been flushed to stable storage before this iclog is
-		 * issued.
+		 * Flush the write cache before writing the start record so that
+		 * the metadata we are about to overwrite in the log and the
+		 * data that new allocations in this context refer to are
+		 * persisted to stable storage before this iclog is written.
 		 */
 		spin_lock(&cil->xc_log->l_icloglock);
 		iclog->ic_flags |= XLOG_ICL_NEED_FLUSH;
-- 
2.53.0

[PATCH 2/6] xfs: don't continue on error in xfs_fsync

[Christoph Hellwig]

As soon as we get an error from cache flushing or log forcing, there
is no point in continuing as the data integrity is already impacted.
Return the error instead of continuing to do more work.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_file.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 426a67b813a7..0d31fea67a2c 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -130,8 +130,8 @@ xfs_file_fsync(
 {
 	struct xfs_inode	*ip = XFS_I(file->f_mapping->host);
 	struct xfs_mount	*mp = ip->i_mount;
-	int			error, err2;
 	int			log_flushed = 0;
+	int			error;
 
 	trace_xfs_file_fsync(ip);
 
@@ -154,15 +154,17 @@ xfs_file_fsync(
 		error = blkdev_issue_flush(mp->m_rtdev_targp->bt_bdev);
 	else if (mp->m_logdev_targp != mp->m_ddev_targp)
 		error = blkdev_issue_flush(mp->m_ddev_targp->bt_bdev);
+	if (error)
+		return error;
 
 	/*
 	 * If the inode has a inode log item attached, it may need the journal
 	 * flushed to persist any changes the log item might be tracking.
 	 */
 	if (ip->i_itemp) {
-		err2 = xfs_fsync_flush_log(ip, datasync, &log_flushed);
-		if (err2 && !error)
-			error = err2;
+		error = xfs_fsync_flush_log(ip, datasync, &log_flushed);
+		if (error)
+			return error;
 	}
 
 	/*
@@ -178,14 +180,11 @@ xfs_file_fsync(
 	if (!log_flushed) {
 		struct xfs_buftarg *file_targp = xfs_inode_buftarg(ip);
 
-		if (mp->m_logdev_targp == file_targp) {
-			err2 = blkdev_issue_flush(file_targp->bt_bdev);
-			if (err2 && !error)
-				error = err2;
-		}
+		if (mp->m_logdev_targp == file_targp)
+			return blkdev_issue_flush(file_targp->bt_bdev);
 	}
 
-	return error;
+	return 0;
 }
 
 static int
-- 
2.53.0

[PATCH 3/6] xfs: clean up xfs_fsync_flush_log a bit

[Christoph Hellwig]

The log_flushed parameter still fits on the previous line, so move it up.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_file.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 0d31fea67a2c..cce324321b4c 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -117,8 +117,7 @@ xfs_fsync_flush_log(
 	if (!seq)
 		return 0;
 
-	return xfs_log_force_seq(ip->i_mount, seq, XFS_LOG_SYNC,
-					  log_flushed);
+	return xfs_log_force_seq(ip->i_mount, seq, XFS_LOG_SYNC, log_flushed);
 }
 
 STATIC int
-- 
2.53.0

[PATCH 4/6] xfs: avoid extra cache flushes for multi-device file systems in xfs_fsync

[Christoph Hellwig]

When xlog_force_lsn sets log_flushed, it has just called xlog_force_iclog
through xlog_force_and_check_iclog, which sets XLOG_ICL_NEED_FLUSH before
writing out the head iclog.  This means that we already flushed the log,
data, and (with the recent fix) RT devices before writing out the iclog
start record and no extra cache flushed is required.

The explicit flush of the data that the device resides on when no iclog
was written out is still required.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_file.c | 36 +++++++++---------------------------
 1 file changed, 9 insertions(+), 27 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index cce324321b4c..795e72e1b599 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -128,7 +128,6 @@ xfs_file_fsync(
 	int			datasync)
 {
 	struct xfs_inode	*ip = XFS_I(file->f_mapping->host);
-	struct xfs_mount	*mp = ip->i_mount;
 	int			log_flushed = 0;
 	int			error;
 
@@ -138,27 +137,17 @@ xfs_file_fsync(
 	if (error)
 		return error;
 
-	if (xfs_is_shutdown(mp))
+	if (xfs_is_shutdown(ip->i_mount))
 		return -EIO;
 
 	xfs_iflags_clear(ip, XFS_ITRUNCATED);
 
 	/*
-	 * If we have an RT and/or log subvolume we need to make sure to flush
-	 * the write cache the device used for file data first.  This is to
-	 * ensure newly written file data make it to disk before logging the new
-	 * inode size in case of an extending write.
-	 */
-	if (XFS_IS_REALTIME_INODE(ip) && mp->m_rtdev_targp != mp->m_ddev_targp)
-		error = blkdev_issue_flush(mp->m_rtdev_targp->bt_bdev);
-	else if (mp->m_logdev_targp != mp->m_ddev_targp)
-		error = blkdev_issue_flush(mp->m_ddev_targp->bt_bdev);
-	if (error)
-		return error;
-
-	/*
-	 * If the inode has a inode log item attached, it may need the journal
-	 * flushed to persist any changes the log item might be tracking.
+	 * If the inode has a log item attached, we must force the log up to the
+	 * last LSN in which the inode was modified to ensure all metadata is
+	 * persisted.  The log force will flush the caches for all devices
+	 * before writing the log records unless it is a no-op because there are
+	 * no modifications to this inode that need to be pushed out.
 	 */
 	if (ip->i_itemp) {
 		error = xfs_fsync_flush_log(ip, datasync, &log_flushed);
@@ -172,17 +161,10 @@ xfs_file_fsync(
 	 * when no metadata needed to be committed.
 	 *
 	 * Use the inode's actual file data target rather than assuming the
-	 * main data device. Realtime inodes with a separate realtime device
-	 * are flushed before the log force, so this fallback only applies
-	 * when the file data target is the same as the log target.
+	 * main data device.
 	 */
-	if (!log_flushed) {
-		struct xfs_buftarg *file_targp = xfs_inode_buftarg(ip);
-
-		if (mp->m_logdev_targp == file_targp)
-			return blkdev_issue_flush(file_targp->bt_bdev);
-	}
-
+	if (!log_flushed)
+		return blkdev_issue_flush(xfs_inode_buftarg(ip)->bt_bdev);
 	return 0;
 }
 
-- 
2.53.0

[PATCH 5/6] xfs: optimize cache flushing for CIL commits on multi-device file systems

[Christoph Hellwig]

While the flush before writing a start record needs to cover data on the
data and possibly RT devices, the flush before writing the commit record
iclog only needs to ensure previous iclogs are stable on disk.

Split out a new XLOG_ICL_NEED_FLUSH_LOG flag out of XLOG_ICL_NEED_FLUSH
to signal that only the log device needs to be flushed.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c      |  6 ++++--
 fs/xfs/xfs_log_cil.c  |  9 ++++++---
 fs/xfs/xfs_log_priv.h | 10 ++++++----
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 8079f2e003db..a5870877baed 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1613,12 +1613,14 @@ xlog_write_iclog(
 	if (iclog->ic_flags & XLOG_ICL_NEED_FLUSH) {
 		if (xlog_flush_data_caches(log))
 			goto shutdown;
-		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
 	}
+	if (iclog->ic_flags & (XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FLUSH_LOG))
+		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
 	if (iclog->ic_flags & XLOG_ICL_NEED_FUA)
 		iclog->ic_bio.bi_opf |= REQ_FUA;
 
-	iclog->ic_flags &= ~(XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FUA);
+	iclog->ic_flags &= ~(XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FLUSH_LOG |
+			     XLOG_ICL_NEED_FUA);
 
 	if (is_vmalloc_addr(iclog->ic_header)) {
 		if (!bio_add_vmalloc(&iclog->ic_bio, iclog->ic_header, count))
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 3d159b1350df..e05b77fbfc3d 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -1562,10 +1562,13 @@ xlog_cil_push_work(
 		}
 
 		/*
-		 * We need to issue a pre-flush so that the ordering for this
-		 * checkpoint is correctly preserved down to stable storage.
+		 * We need to issue a pre-flush on the device containing the log
+		 * so that the ordering for this checkpoint is correctly
+		 * preserved down to stable storage.
+		 * There is no need for an extra flush on devices that only
+		 * contain data or non-log metadata.
 		 */
-		ctx->commit_iclog->ic_flags |= XLOG_ICL_NEED_FLUSH;
+		ctx->commit_iclog->ic_flags |= XLOG_ICL_NEED_FLUSH_LOG;
 	}
 
 	/*
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index cf1e4ce61a8c..7c72c3b3ffe1 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -71,12 +71,14 @@ enum xlog_iclog_state {
 /*
  * In core log flags
  */
-#define XLOG_ICL_NEED_FLUSH	(1u << 0)	/* iclog needs REQ_PREFLUSH */
-#define XLOG_ICL_NEED_FUA	(1u << 1)	/* iclog needs REQ_FUA */
+#define XLOG_ICL_NEED_FLUSH	(1u << 0)	/* all devs need REQ_PREFLUSH */
+#define XLOG_ICL_NEED_FLUSH_LOG	(1u << 0)	/* logdev needs REQ_PREFLUSH */
+#define XLOG_ICL_NEED_FUA	(1u << 2)	/* iclog needs REQ_FUA */
 
 #define XLOG_ICL_STRINGS \
-	{ XLOG_ICL_NEED_FLUSH,	"XLOG_ICL_NEED_FLUSH" }, \
-	{ XLOG_ICL_NEED_FUA,	"XLOG_ICL_NEED_FUA" }
+	{ XLOG_ICL_NEED_FLUSH,		"XLOG_ICL_NEED_FLUSH" }, \
+	{ XLOG_ICL_NEED_FLUSH_LOG,	"XLOG_ICL_NEED_FLUSH_LOG" }, \
+	{ XLOG_ICL_NEED_FUA,		"XLOG_ICL_NEED_FUA" }
 
 
 /*
-- 
2.53.0

[PATCH 6/6] xfs: flush multiple device caches in parallel in xlog_write_iclog

[Christoph Hellwig]

When xlog_write_iclog needs to flush the cache for more than one devices,
the current implementations does this sequentially, which adds up the
flush latency for all devices.  Switch to kicking off all cache flushes
in parallel so that only the longest latency bounds the time of the log
I/O.  This removes the REQ_PREFLUSH optimization for the log device,
but as that flag is never passed on to the device and just very slightly
reduce the latency by queueing the following write from a lower-level
context it is trivially shadowed by the latency improvements of the
parallel flush commands.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c | 78 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 71 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index a5870877baed..b392a45d38c6 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1538,6 +1538,47 @@ xlog_bio_end_io(
 		   &iclog->ic_end_io_work);
 }
 
+struct xlog_flush_done {
+	atomic_t		pending;
+	blk_status_t		status;
+	struct completion	done;
+};
+
+static void
+xlog_flush_done(
+	struct xlog_flush_done	*done)
+{
+	if (atomic_dec_and_test(&done->pending))
+		complete(&done->done);
+}
+
+static void
+xlog_flush_end_io(
+	struct bio		*bio)
+{
+	struct xlog_flush_done	*done = bio->bi_private;
+
+	if (bio->bi_status)
+		cmpxchg(&done->status, 0, bio->bi_status);
+	xlog_flush_done(done);
+	bio_put(bio);
+}
+
+static void
+xlog_flush_async(
+	struct xlog_flush_done	*done,
+	struct block_device	*bdev)
+{
+	struct bio		*bio;
+
+	bio = bio_alloc(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);
+}
+
 /*
  * When using multiple devices, we also need to flush the data and RT device
  * caches first to ensure that all metadata writeback covered by the LSN in
@@ -1551,17 +1592,39 @@ xlog_bio_end_io(
  */
 static int
 xlog_flush_data_caches(
-	struct xlog		*log)
+	struct xlog		*log,
+	struct xlog_in_core	*iclog)
 {
 	struct xfs_mount	*mp = log->l_mp;
+	struct xlog_flush_done done = {
+		.pending	= ATOMIC_INIT(1),
+		.done		= COMPLETION_INITIALIZER_ONSTACK(done.done),
+	};
+	bool			did_flush = false;
 
-	if (log->l_targ != mp->m_ddev_targp) {
-		if (blkdev_issue_flush(mp->m_ddev_targp->bt_bdev))
-			return -EIO;
+	if (mp->m_ddev_targp != log->l_targ &&
+	    bdev_write_cache(mp->m_ddev_targp->bt_bdev)) {
+		xlog_flush_async(&done, mp->m_ddev_targp->bt_bdev);
+		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);
+		did_flush = true;
 	}
-	if (mp->m_rtdev_targp && mp->m_rtdev_targp != mp->m_ddev_targp) {
-		if (blkdev_issue_flush(mp->m_rtdev_targp->bt_bdev))
+
+	if (did_flush) {
+		/*
+		 * 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, log->l_targ->bt_bdev);
+
+		xlog_flush_done(&done);
+		wait_for_completion(&done.done);
+		if (done.status)
 			return -EIO;
+		iclog->ic_flags &= ~XLOG_ICL_NEED_FLUSH;
 	}
 
 	return 0;
@@ -1611,8 +1674,9 @@ xlog_write_iclog(
 	iclog->ic_bio.bi_private = iclog;
 
 	if (iclog->ic_flags & XLOG_ICL_NEED_FLUSH) {
-		if (xlog_flush_data_caches(log))
+		if (xlog_flush_data_caches(log, iclog))
 			goto shutdown;
+		/* xlog_flush_data_caches may clear XLOG_ICL_NEED_FLUSH */
 	}
 	if (iclog->ic_flags & (XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FLUSH_LOG))
 		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
-- 
2.53.0

Re: [PATCH 1/6] xfs: also flush the RT device cache in xlog_write_iclog

[Darrick J. Wong]

On Wed, Sep 02, 2026 at 08:49:15AM +0300, Christoph Hellwig wrote:
> The cache flush before writing the CIL start record no only needs to
> ensure any metadata covered by the overwritten part of the log is on
> stable storage, but also that any data pointed to by metadata logged
> is on stable storage, as otherwise log recovery could created allocated
> blocks that point to stale data.  Fortunately the code already
> handles this right for the data device, but it also needs to flush
> the RT device for this to work for data on the RT device.
> 
> Also update the comments to explicitly mention this case.
> 
> This omission goes back to the first days of cache control in XFS.

Yikes.

> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Christoph Hellwig <hch@lst.de>

/me agrees that we should be flushing the rt device after persisting
metadata that might affect it.

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_log.c     | 45 ++++++++++++++++++++++++++++++--------------
>  fs/xfs/xfs_log_cil.c |  7 ++++---
>  2 files changed, 35 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index f807f8f4f705..8079f2e003db 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1538,6 +1538,35 @@ xlog_bio_end_io(
>  		   &iclog->ic_end_io_work);
>  }
>  
> +/*
> + * When using multiple devices, we also need to flush the data and RT device
> + * caches first to ensure that all metadata writeback covered by the LSN in
> + * this iclog is on stable storage. This is slow, but it *must* complete
> + * before we issue the external log IO.
> + *
> + * If the flush fails, we cannot conclude that past metadata writeback from
> + * the log succeeded.  Repeating the flush is not possible, hence we must
> + * shut down with log IO error to avoid shutdown re-entering this path and
> + * erroring out again.
> + */
> +static int
> +xlog_flush_data_caches(
> +	struct xlog		*log)
> +{
> +	struct xfs_mount	*mp = log->l_mp;
> +
> +	if (log->l_targ != mp->m_ddev_targp) {
> +		if (blkdev_issue_flush(mp->m_ddev_targp->bt_bdev))
> +			return -EIO;
> +	}
> +	if (mp->m_rtdev_targp && mp->m_rtdev_targp != mp->m_ddev_targp) {
> +		if (blkdev_issue_flush(mp->m_rtdev_targp->bt_bdev))
> +			return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
>  STATIC void
>  xlog_write_iclog(
>  	struct xlog		*log,
> @@ -1582,21 +1611,9 @@ xlog_write_iclog(
>  	iclog->ic_bio.bi_private = iclog;
>  
>  	if (iclog->ic_flags & XLOG_ICL_NEED_FLUSH) {
> -		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
> -		/*
> -		 * For external log devices, we also need to flush the data
> -		 * device cache first to ensure all metadata writeback covered
> -		 * by the LSN in this iclog is on stable storage. This is slow,
> -		 * but it *must* complete before we issue the external log IO.
> -		 *
> -		 * If the flush fails, we cannot conclude that past metadata
> -		 * writeback from the log succeeded.  Repeating the flush is
> -		 * not possible, hence we must shut down with log IO error to
> -		 * avoid shutdown re-entering this path and erroring out again.
> -		 */
> -		if (log->l_targ != log->l_mp->m_ddev_targp &&
> -		    blkdev_issue_flush(log->l_mp->m_ddev_targp->bt_bdev))
> +		if (xlog_flush_data_caches(log))
>  			goto shutdown;
> +		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
>  	}
>  	if (iclog->ic_flags & XLOG_ICL_NEED_FUA)
>  		iclog->ic_bio.bi_opf |= REQ_FUA;
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 639f875a8fb2..3d159b1350df 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -1055,9 +1055,10 @@ xlog_cil_set_ctx_write_state(
>  		spin_unlock(&cil->xc_push_lock);
>  
>  		/*
> -		 * Make sure the metadata we are about to overwrite in the log
> -		 * has been flushed to stable storage before this iclog is
> -		 * issued.
> +		 * Flush the write cache before writing the start record so that
> +		 * the metadata we are about to overwrite in the log and the
> +		 * data that new allocations in this context refer to are
> +		 * persisted to stable storage before this iclog is written.
>  		 */
>  		spin_lock(&cil->xc_log->l_icloglock);
>  		iclog->ic_flags |= XLOG_ICL_NEED_FLUSH;
> -- 
> 2.53.0
> 
> 

Re: [PATCH 2/6] xfs: don't continue on error in xfs_fsync

[Darrick J. Wong]

On Wed, Sep 02, 2026 at 08:49:16AM +0300, Christoph Hellwig wrote:
> As soon as we get an error from cache flushing or log forcing, there
> is no point in continuing as the data integrity is already impacted.
> Return the error instead of continuing to do more work.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_file.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 426a67b813a7..0d31fea67a2c 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -130,8 +130,8 @@ xfs_file_fsync(
>  {
>  	struct xfs_inode	*ip = XFS_I(file->f_mapping->host);
>  	struct xfs_mount	*mp = ip->i_mount;
> -	int			error, err2;
>  	int			log_flushed = 0;
> +	int			error;
>  
>  	trace_xfs_file_fsync(ip);
>  
> @@ -154,15 +154,17 @@ xfs_file_fsync(
>  		error = blkdev_issue_flush(mp->m_rtdev_targp->bt_bdev);
>  	else if (mp->m_logdev_targp != mp->m_ddev_targp)
>  		error = blkdev_issue_flush(mp->m_ddev_targp->bt_bdev);
> +	if (error)
> +		return error;

Hmm so at this point dirty writeback was pushed out and failed, so now
already we've lost data.  No point in continuing, agreed...

>  
>  	/*
>  	 * If the inode has a inode log item attached, it may need the journal
>  	 * flushed to persist any changes the log item might be tracking.
>  	 */
>  	if (ip->i_itemp) {
> -		err2 = xfs_fsync_flush_log(ip, datasync, &log_flushed);
> -		if (err2 && !error)
> -			error = err2;
> +		error = xfs_fsync_flush_log(ip, datasync, &log_flushed);
> +		if (error)
> +			return error;

...so at this point we failed to push the log to disk and possibly
lost file metadata.  Agreed that we might as well give up...

>  	}
>  
>  	/*
> @@ -178,14 +180,11 @@ xfs_file_fsync(
>  	if (!log_flushed) {
>  		struct xfs_buftarg *file_targp = xfs_inode_buftarg(ip);
>  
> -		if (mp->m_logdev_targp == file_targp) {
> -			err2 = blkdev_issue_flush(file_targp->bt_bdev);
> -			if (err2 && !error)
> -				error = err2;
> -		}
> +		if (mp->m_logdev_targp == file_targp)
> +			return blkdev_issue_flush(file_targp->bt_bdev);

...and same here if the bdev flush fails.

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

>  	}
>  
> -	return error;
> +	return 0;
>  }
>  
>  static int
> -- 
> 2.53.0
> 
> 

Re: [PATCH 3/6] xfs: clean up xfs_fsync_flush_log a bit

[Darrick J. Wong]

On Wed, Sep 02, 2026 at 08:49:17AM +0300, Christoph Hellwig wrote:
> The log_flushed parameter still fits on the previous line, so move it up.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_file.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 0d31fea67a2c..cce324321b4c 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -117,8 +117,7 @@ xfs_fsync_flush_log(
>  	if (!seq)
>  		return 0;
>  
> -	return xfs_log_force_seq(ip->i_mount, seq, XFS_LOG_SYNC,
> -					  log_flushed);
> +	return xfs_log_force_seq(ip->i_mount, seq, XFS_LOG_SYNC, log_flushed);
>  }
>  
>  STATIC int
> -- 
> 2.53.0
> 
> 

Re: [PATCH 4/6] xfs: avoid extra cache flushes for multi-device file systems in xfs_fsync

[Darrick J. Wong]

On Wed, Sep 02, 2026 at 08:49:18AM +0300, Christoph Hellwig wrote:
> When xlog_force_lsn sets log_flushed, it has just called xlog_force_iclog
> through xlog_force_and_check_iclog, which sets XLOG_ICL_NEED_FLUSH before
> writing out the head iclog.  This means that we already flushed the log,
> data, and (with the recent fix) RT devices before writing out the iclog
> start record and no extra cache flushed is required.
> 
> The explicit flush of the data that the device resides on when no iclog
> was written out is still required.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_file.c | 36 +++++++++---------------------------
>  1 file changed, 9 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index cce324321b4c..795e72e1b599 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -128,7 +128,6 @@ xfs_file_fsync(
>  	int			datasync)
>  {
>  	struct xfs_inode	*ip = XFS_I(file->f_mapping->host);
> -	struct xfs_mount	*mp = ip->i_mount;
>  	int			log_flushed = 0;
>  	int			error;
>  
> @@ -138,27 +137,17 @@ xfs_file_fsync(
>  	if (error)
>  		return error;
>  
> -	if (xfs_is_shutdown(mp))
> +	if (xfs_is_shutdown(ip->i_mount))
>  		return -EIO;
>  
>  	xfs_iflags_clear(ip, XFS_ITRUNCATED);
>  
>  	/*
> -	 * If we have an RT and/or log subvolume we need to make sure to flush
> -	 * the write cache the device used for file data first.  This is to
> -	 * ensure newly written file data make it to disk before logging the new
> -	 * inode size in case of an extending write.
> -	 */
> -	if (XFS_IS_REALTIME_INODE(ip) && mp->m_rtdev_targp != mp->m_ddev_targp)
> -		error = blkdev_issue_flush(mp->m_rtdev_targp->bt_bdev);
> -	else if (mp->m_logdev_targp != mp->m_ddev_targp)
> -		error = blkdev_issue_flush(mp->m_ddev_targp->bt_bdev);
> -	if (error)
> -		return error;
> -
> -	/*
> -	 * If the inode has a inode log item attached, it may need the journal
> -	 * flushed to persist any changes the log item might be tracking.
> +	 * If the inode has a log item attached, we must force the log up to the
> +	 * last LSN in which the inode was modified to ensure all metadata is
> +	 * persisted.  The log force will flush the caches for all devices
> +	 * before writing the log records unless it is a no-op because there are
> +	 * no modifications to this inode that need to be pushed out.
>  	 */
>  	if (ip->i_itemp) {
>  		error = xfs_fsync_flush_log(ip, datasync, &log_flushed);
> @@ -172,17 +161,10 @@ xfs_file_fsync(
>  	 * when no metadata needed to be committed.
>  	 *
>  	 * Use the inode's actual file data target rather than assuming the
> -	 * main data device. Realtime inodes with a separate realtime device
> -	 * are flushed before the log force, so this fallback only applies
> -	 * when the file data target is the same as the log target.
> +	 * main data device.
>  	 */
> -	if (!log_flushed) {
> -		struct xfs_buftarg *file_targp = xfs_inode_buftarg(ip);
> -
> -		if (mp->m_logdev_targp == file_targp)
> -			return blkdev_issue_flush(file_targp->bt_bdev);
> -	}
> -
> +	if (!log_flushed)
> +		return blkdev_issue_flush(xfs_inode_buftarg(ip)->bt_bdev);

At first I wondered why this flush call couldn't get moved into
xfs_fsync_flush_log, but then realized that log_flushed==0 is a
possibility if the inode doesn't have a log item attached.  Therefore
this blkdev_issue_flush has to stay in xfs_file_fsync so that we flush
write caches after pushing dirty data to disk in the pure overwrite
case where there's also no timestamp update or anything that would
generate log traffic for this inode.

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

Re: [PATCH 5/6] xfs: optimize cache flushing for CIL commits on multi-device file systems

[Darrick J. Wong]

On Wed, Sep 02, 2026 at 08:49:19AM +0300, Christoph Hellwig wrote:
> While the flush before writing a start record needs to cover data on the
> data and possibly RT devices, the flush before writing the commit record
> iclog only needs to ensure previous iclogs are stable on disk.
> 
> Split out a new XLOG_ICL_NEED_FLUSH_LOG flag out of XLOG_ICL_NEED_FLUSH
> to signal that only the log device needs to be flushed.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c      |  6 ++++--
>  fs/xfs/xfs_log_cil.c  |  9 ++++++---
>  fs/xfs/xfs_log_priv.h | 10 ++++++----
>  3 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 8079f2e003db..a5870877baed 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1613,12 +1613,14 @@ xlog_write_iclog(
>  	if (iclog->ic_flags & XLOG_ICL_NEED_FLUSH) {
>  		if (xlog_flush_data_caches(log))
>  			goto shutdown;
> -		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
>  	}
> +	if (iclog->ic_flags & (XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FLUSH_LOG))
> +		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
>  	if (iclog->ic_flags & XLOG_ICL_NEED_FUA)
>  		iclog->ic_bio.bi_opf |= REQ_FUA;
>  
> -	iclog->ic_flags &= ~(XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FUA);
> +	iclog->ic_flags &= ~(XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FLUSH_LOG |
> +			     XLOG_ICL_NEED_FUA);
>  
>  	if (is_vmalloc_addr(iclog->ic_header)) {
>  		if (!bio_add_vmalloc(&iclog->ic_bio, iclog->ic_header, count))
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 3d159b1350df..e05b77fbfc3d 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -1562,10 +1562,13 @@ xlog_cil_push_work(
>  		}
>  
>  		/*
> -		 * We need to issue a pre-flush so that the ordering for this
> -		 * checkpoint is correctly preserved down to stable storage.
> +		 * We need to issue a pre-flush on the device containing the log
> +		 * so that the ordering for this checkpoint is correctly
> +		 * preserved down to stable storage.
> +		 * There is no need for an extra flush on devices that only
> +		 * contain data or non-log metadata.
>  		 */
> -		ctx->commit_iclog->ic_flags |= XLOG_ICL_NEED_FLUSH;
> +		ctx->commit_iclog->ic_flags |= XLOG_ICL_NEED_FLUSH_LOG;

This is the "wait for all previous iclogs" case, so yes, we only need to
flush the log device.

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

>  	}
>  
>  	/*
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index cf1e4ce61a8c..7c72c3b3ffe1 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -71,12 +71,14 @@ enum xlog_iclog_state {
>  /*
>   * In core log flags
>   */
> -#define XLOG_ICL_NEED_FLUSH	(1u << 0)	/* iclog needs REQ_PREFLUSH */
> -#define XLOG_ICL_NEED_FUA	(1u << 1)	/* iclog needs REQ_FUA */
> +#define XLOG_ICL_NEED_FLUSH	(1u << 0)	/* all devs need REQ_PREFLUSH */
> +#define XLOG_ICL_NEED_FLUSH_LOG	(1u << 0)	/* logdev needs REQ_PREFLUSH */
> +#define XLOG_ICL_NEED_FUA	(1u << 2)	/* iclog needs REQ_FUA */
>  
>  #define XLOG_ICL_STRINGS \
> -	{ XLOG_ICL_NEED_FLUSH,	"XLOG_ICL_NEED_FLUSH" }, \
> -	{ XLOG_ICL_NEED_FUA,	"XLOG_ICL_NEED_FUA" }
> +	{ XLOG_ICL_NEED_FLUSH,		"XLOG_ICL_NEED_FLUSH" }, \
> +	{ XLOG_ICL_NEED_FLUSH_LOG,	"XLOG_ICL_NEED_FLUSH_LOG" }, \
> +	{ XLOG_ICL_NEED_FUA,		"XLOG_ICL_NEED_FUA" }
>  
>  
>  /*
> -- 
> 2.53.0
> 
> 

Re: [PATCH 6/6] xfs: flush multiple device caches in parallel in xlog_write_iclog

[Darrick J. Wong]

On Wed, Sep 02, 2026 at 08:49:20AM +0300, Christoph Hellwig wrote:
> When xlog_write_iclog needs to flush the cache for more than one devices,
> the current implementations does this sequentially, which adds up the
> flush latency for all devices.  Switch to kicking off all cache flushes
> in parallel so that only the longest latency bounds the time of the log
> I/O.  This removes the REQ_PREFLUSH optimization for the log device,
> but as that flag is never passed on to the device and just very slightly
> reduce the latency by queueing the following write from a lower-level
> context it is trivially shadowed by the latency improvements of the
> parallel flush commands.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

That looks like a neat trick.  Do you see any performance speedups from
flushing in parallel?

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_log.c | 78 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 71 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index a5870877baed..b392a45d38c6 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1538,6 +1538,47 @@ xlog_bio_end_io(
>  		   &iclog->ic_end_io_work);
>  }
>  
> +struct xlog_flush_done {
> +	atomic_t		pending;
> +	blk_status_t		status;
> +	struct completion	done;
> +};
> +
> +static void
> +xlog_flush_done(
> +	struct xlog_flush_done	*done)
> +{
> +	if (atomic_dec_and_test(&done->pending))
> +		complete(&done->done);
> +}
> +
> +static void
> +xlog_flush_end_io(
> +	struct bio		*bio)
> +{
> +	struct xlog_flush_done	*done = bio->bi_private;
> +
> +	if (bio->bi_status)
> +		cmpxchg(&done->status, 0, bio->bi_status);
> +	xlog_flush_done(done);
> +	bio_put(bio);
> +}
> +
> +static void
> +xlog_flush_async(
> +	struct xlog_flush_done	*done,
> +	struct block_device	*bdev)
> +{
> +	struct bio		*bio;
> +
> +	bio = bio_alloc(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);
> +}
> +
>  /*
>   * When using multiple devices, we also need to flush the data and RT device
>   * caches first to ensure that all metadata writeback covered by the LSN in
> @@ -1551,17 +1592,39 @@ xlog_bio_end_io(
>   */
>  static int
>  xlog_flush_data_caches(
> -	struct xlog		*log)
> +	struct xlog		*log,
> +	struct xlog_in_core	*iclog)
>  {
>  	struct xfs_mount	*mp = log->l_mp;
> +	struct xlog_flush_done done = {
> +		.pending	= ATOMIC_INIT(1),
> +		.done		= COMPLETION_INITIALIZER_ONSTACK(done.done),
> +	};
> +	bool			did_flush = false;
>  
> -	if (log->l_targ != mp->m_ddev_targp) {
> -		if (blkdev_issue_flush(mp->m_ddev_targp->bt_bdev))
> -			return -EIO;
> +	if (mp->m_ddev_targp != log->l_targ &&
> +	    bdev_write_cache(mp->m_ddev_targp->bt_bdev)) {
> +		xlog_flush_async(&done, mp->m_ddev_targp->bt_bdev);
> +		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);
> +		did_flush = true;
>  	}
> -	if (mp->m_rtdev_targp && mp->m_rtdev_targp != mp->m_ddev_targp) {
> -		if (blkdev_issue_flush(mp->m_rtdev_targp->bt_bdev))
> +
> +	if (did_flush) {
> +		/*
> +		 * 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, log->l_targ->bt_bdev);
> +
> +		xlog_flush_done(&done);
> +		wait_for_completion(&done.done);
> +		if (done.status)
>  			return -EIO;
> +		iclog->ic_flags &= ~XLOG_ICL_NEED_FLUSH;
>  	}
>  
>  	return 0;
> @@ -1611,8 +1674,9 @@ xlog_write_iclog(
>  	iclog->ic_bio.bi_private = iclog;
>  
>  	if (iclog->ic_flags & XLOG_ICL_NEED_FLUSH) {
> -		if (xlog_flush_data_caches(log))
> +		if (xlog_flush_data_caches(log, iclog))
>  			goto shutdown;
> +		/* xlog_flush_data_caches may clear XLOG_ICL_NEED_FLUSH */
>  	}
>  	if (iclog->ic_flags & (XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FLUSH_LOG))
>  		iclog->ic_bio.bi_opf |= REQ_PREFLUSH;
> -- 
> 2.53.0
> 
> 

Re: [PATCH 6/6] xfs: flush multiple device caches in parallel in xlog_write_iclog

[Christoph Hellwig]

On Wed, Sep 02, 2026 at 09:18:58AM -0700, Darrick J. Wong wrote:
> On Wed, Sep 02, 2026 at 08:49:20AM +0300, Christoph Hellwig wrote:
> > When xlog_write_iclog needs to flush the cache for more than one devices,
> > the current implementations does this sequentially, which adds up the
> > flush latency for all devices.  Switch to kicking off all cache flushes
> > in parallel so that only the longest latency bounds the time of the log
> > I/O.  This removes the REQ_PREFLUSH optimization for the log device,
> > but as that flag is never passed on to the device and just very slightly
> > reduce the latency by queueing the following write from a lower-level
> > context it is trivially shadowed by the latency improvements of the
> > parallel flush commands.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> That looks like a neat trick.  Do you see any performance speedups from
> flushing in parallel?

I haven't really done a benchmark, as all the devices I test multi-device
setups don't expose volatile write caches, so I actually had to create
a synthetic setup to fully test this in qemu.  But I can kick off a run
using say consumer SSDs.  I expect it to significantly speed up fsync
bound workloads.