[PATCH 0/2] xfs: reduce ILOCK contention on O_DSYNC DIO

[PATCH 0/2] xfs: reduce ILOCK contention on O_DSYNC DIO

[Dave Chinner]

Hi folks,

This is the changes I came up with in the course of the discussion
with Jan about the fallocate+DIO+O_DSYNC performance improvement
thread here:

https://lore.kernel.org/linux-xfs/CAM4Jq_71gxMcnOdgqWhKEa53sr9r57Qpi0hc5bs3NgtnNOGwtg@mail.gmail.com/T/#

The first patch is moves a little bit of code around to ensure
that xfs_inode_item_precommit() always calculates all the changes
before it starts manipulating the dirty and fsync flags on the inode
log item. Whilst technically it could be considered a bug fix, the
bug it fixes requires an inconsistency in the on disk format to
exist first, so it likely won't ever be an issue in normal
production systems. It also requires an application to run a
fdatasync and then have a system crash just after the
inconsistency is addressed to expose it. So the likelihood of it
ever being triggered as a data integrity issue is -extremely- tiny.

The second patch is the one that addresses the performance issue. It
removes the ILOCK from the sync/datasync path completely, and
instead relies on a new datasync commit sequence number being stored
in the inode log item to elide datasync journal flushes when they
are unnecessary. The mechanism is explained at length in the patch
description.

The changes have been perf tested by both Jan and Lukas, and it has
passed multiple fstests runs on a dozen different configs without
regressions here.

-Dave.

[PATCH 1/2] xfs: rearrange code in xfs_inode_item_precommit

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

There are similar extsize checks and updates done inside and outside
the inode item lock, which could all be done under a single top
level logic branch outside the ili_lock. The COW extsize fixup can
potentially miss updating the XFS_ILOG_CORE in ili_fsync_fields, so
moving this code up above the ili_fsync_fields update could also be
considered a fix.

Further, to make the next change a bit cleaner, move where we
calculate the on-disk flag mask to after we attach the cluster
buffer to the the inode log item.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_inode_item.c | 65 ++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 36 deletions(-)

diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index afb6cadf7793..318e7c68ec72 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -131,46 +131,28 @@ xfs_inode_item_precommit(
 	}
 
 	/*
-	 * Inode verifiers do not check that the extent size hint is an integer
-	 * multiple of the rt extent size on a directory with both rtinherit
-	 * and extszinherit flags set.  If we're logging a directory that is
-	 * misconfigured in this way, clear the hint.
+	 * Inode verifiers do not check that the extent size hints are an
+	 * integer multiple of the rt extent size on a directory with
+	 * rtinherit flags set.  If we're logging a directory that is
+	 * misconfigured in this way, clear the bad hints.
 	 */
-	if ((ip->i_diflags & XFS_DIFLAG_RTINHERIT) &&
-	    (ip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) &&
-	    xfs_extlen_to_rtxmod(ip->i_mount, ip->i_extsize) > 0) {
-		ip->i_diflags &= ~(XFS_DIFLAG_EXTSIZE |
-				   XFS_DIFLAG_EXTSZINHERIT);
-		ip->i_extsize = 0;
-		flags |= XFS_ILOG_CORE;
+	if (ip->i_diflags & XFS_DIFLAG_RTINHERIT) {
+		if ((ip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) &&
+		    xfs_extlen_to_rtxmod(ip->i_mount, ip->i_extsize) > 0) {
+			ip->i_diflags &= ~(XFS_DIFLAG_EXTSIZE |
+					   XFS_DIFLAG_EXTSZINHERIT);
+			ip->i_extsize = 0;
+			flags |= XFS_ILOG_CORE;
+		}
+		if ((ip->i_diflags2 & XFS_DIFLAG2_COWEXTSIZE) &&
+		    xfs_extlen_to_rtxmod(ip->i_mount, ip->i_cowextsize) > 0) {
+			ip->i_diflags2 &= ~XFS_DIFLAG2_COWEXTSIZE;
+			ip->i_cowextsize = 0;
+			flags |= XFS_ILOG_CORE;
+		}
 	}
 
-	/*
-	 * Record the specific change for fdatasync optimisation. This allows
-	 * fdatasync to skip log forces for inodes that are only timestamp
-	 * dirty. Once we've processed the XFS_ILOG_IVERSION flag, convert it
-	 * to XFS_ILOG_CORE so that the actual on-disk dirty tracking
-	 * (ili_fields) correctly tracks that the version has changed.
-	 */
 	spin_lock(&iip->ili_lock);
-	iip->ili_fsync_fields |= (flags & ~XFS_ILOG_IVERSION);
-	if (flags & XFS_ILOG_IVERSION)
-		flags = ((flags & ~XFS_ILOG_IVERSION) | XFS_ILOG_CORE);
-
-	/*
-	 * Inode verifiers do not check that the CoW extent size hint is an
-	 * integer multiple of the rt extent size on a directory with both
-	 * rtinherit and cowextsize flags set.  If we're logging a directory
-	 * that is misconfigured in this way, clear the hint.
-	 */
-	if ((ip->i_diflags & XFS_DIFLAG_RTINHERIT) &&
-	    (ip->i_diflags2 & XFS_DIFLAG2_COWEXTSIZE) &&
-	    xfs_extlen_to_rtxmod(ip->i_mount, ip->i_cowextsize) > 0) {
-		ip->i_diflags2 &= ~XFS_DIFLAG2_COWEXTSIZE;
-		ip->i_cowextsize = 0;
-		flags |= XFS_ILOG_CORE;
-	}
-
 	if (!iip->ili_item.li_buf) {
 		struct xfs_buf	*bp;
 		int		error;
@@ -204,6 +186,17 @@ xfs_inode_item_precommit(
 		xfs_trans_brelse(tp, bp);
 	}
 
+	/*
+	 * Record the specific change for fdatasync optimisation. This allows
+	 * fdatasync to skip log forces for inodes that are only timestamp
+	 * dirty. Once we've processed the XFS_ILOG_IVERSION flag, convert it
+	 * to XFS_ILOG_CORE so that the actual on-disk dirty tracking
+	 * (ili_fields) correctly tracks that the version has changed.
+	 */
+	iip->ili_fsync_fields |= (flags & ~XFS_ILOG_IVERSION);
+	if (flags & XFS_ILOG_IVERSION)
+		flags = ((flags & ~XFS_ILOG_IVERSION) | XFS_ILOG_CORE);
+
 	/*
 	 * Always OR in the bits from the ili_last_fields field.  This is to
 	 * coordinate with the xfs_iflush() and xfs_buf_inode_iodone() routines
-- 
2.50.1

[PATCH 2/2] xfs: rework datasync tracking and execution

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Jan Kara reported that the shared ILOCK held across the journal
flush during fdatasync operations slows down O_DSYNC DIO on
unwritten extents significantly. The underlying issue is that
unwritten extent conversion needs the ILOCK exclusive, whilst the
datasync operation after the extent conversion holds it shared.

Hence we cannot be flushing the journal for one IO completion whilst
at the same time doing unwritten extent conversion on another IO
completion on the same inode. This means that IO completions
lock-step, and IO performance is dependent on the journal flush
latency.

Jan demostrated that reducing the ifdatasync lock hold time can
improve O_DSYNC DIO to unwritten extents performance by 2.5x.
Discussion on that patch found issues with the method, and we
came to the conclusion that seperately tracking datasync flush
seqeunces was the best approach to solving the problem.

The fsync code uses the ILOCK to serialise against concurrent
modifications in the transaction commit phase. In a transaction
commit, there are several disjoint updates to inode log item state
that need to be considered atomically by the fsync code. These
operations are allo done under ILOCK_EXCL context:

1. ili_fsync_flags is updated in ->iop_precommit
2. i_pincount is updated in ->iop_pin before it is added to the CIL
3. ili_commit_seq is updated in ->iop_committing, after it has been
   added to the CIL

In fsync, we need to:

1. check that the inode is dirty in the journal (ipincount)
2. check that ili_fsync_flags is set
3. grab the ili_commit_seq if a journal flush is needed
4. clear the ili_fsync_flags to ensure that new modifications that
require fsync are tracked in ->iop_precommit correctly

The serialisation of ipincount/ili_commit_seq is needed
to ensure that we don't try to unnecessarily flush the journal.

The serialisation of ili_fsync_flags being set in
->iop_precommit and cleared in fsync post journal flush is
required for correctness.

Hence holding the ILOCK_SHARED in xfs_file_fsync() performs all this
serialisation for us.  Ideally, we want to remove the need to hold
the ILOCK_SHARED in xfs_file_fsync() for best performance.

We start with the observation that fsync/fdatasync() only need to
wait for operations that have been completed. Hence operations that
are still being committed have not completed and datasync operations
do not need to wait for them.

This means we can use a single point in time in the commit process
to signal "this modification is complete". This is what
->iop_committing is supposed to provide - it is the point at which
the object is unlocked after the modification has been recorded in
the CIL. Hence we could use ili_commit_seq to determine if we should
flush the journal.

In theory, we can already do this. However, in practice this will
expose an internal global CIL lock to the IO path. The ipincount()
checks optimise away the need to take this lock - if the inode is
not pinned, then it is not in the CIL and we don't need to check if
a journal flush at ili_commit_seq needs to be performed.

The reason this is needed is that the ili_commit_seq is never
cleared. Once it is set, it remains set even once the journal has
been committed and the object has been unpinned. Hence we have to
look that journal internal commit sequence state to determine if
ili_commit_seq needs to be acted on or not.

We can solve this by clearing ili_commit_seq when the inode is
unpinned. If we clear it atomically with the last unpin going away,
then we are guaranteed that new modifications will order correctly
as they add a new pin counts and we won't clear a sequence number
for an active modification in the CIL.

Further, we can then allow the per-transaction flag state to
propagate into ->iop_committing (instead of clearing it in
->iop_precommit) and that will allow us to determine if the
modification needs a full fsync or just a datasync, and so we can
record a separate datasync sequence number (Jan's idea!) and then
use that in the fdatasync path instead of the full fsync sequence
number.

With this infrastructure in place, we no longer need the
ILOCK_SHARED in the fsync path. All serialisation is done against
the commit sequence numbers - if the sequence number is set, then we
have to flush the journal. If it is not set, then we have nothing to
do. This greatly simplifies the fsync implementation....

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Tested-by: Jan Kara <jack@suse.cz>
---
 fs/xfs/xfs_file.c       | 75 +++++++++++++++++++----------------------
 fs/xfs/xfs_inode.c      | 25 +++++++++-----
 fs/xfs/xfs_inode_item.c | 72 ++++++++++++++++++++++++++++++---------
 fs/xfs/xfs_inode_item.h | 10 +++++-
 fs/xfs/xfs_iomap.c      | 15 +++++++--
 5 files changed, 128 insertions(+), 69 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index f96fbf5c54c9..2702fef2c90c 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -75,52 +75,47 @@ xfs_dir_fsync(
 	return xfs_log_force_inode(ip);
 }
 
-static xfs_csn_t
-xfs_fsync_seq(
-	struct xfs_inode	*ip,
-	bool			datasync)
-{
-	if (!xfs_ipincount(ip))
-		return 0;
-	if (datasync && !(ip->i_itemp->ili_fsync_fields & ~XFS_ILOG_TIMESTAMP))
-		return 0;
-	return ip->i_itemp->ili_commit_seq;
-}
-
 /*
- * All metadata updates are logged, which means that we just have to flush the
- * log up to the latest LSN that touched the inode.
+ * All metadata updates are logged, which means that we just have to push the
+ * journal to the required sequence number than holds the updates. We track
+ * datasync commits separately to full sync commits, and hence only need to
+ * select the correct sequence number for the log force here.
  *
- * If we have concurrent fsync/fdatasync() calls, we need them to all block on
- * the log force before we clear the ili_fsync_fields field. This ensures that
- * we don't get a racing sync operation that does not wait for the metadata to
- * hit the journal before returning.  If we race with clearing ili_fsync_fields,
- * then all that will happen is the log force will do nothing as the lsn will
- * already be on disk.  We can't race with setting ili_fsync_fields because that
- * is done under XFS_ILOCK_EXCL, and that can't happen because we hold the lock
- * shared until after the ili_fsync_fields is cleared.
+ * We don't have to serialise against concurrent modifications, as we do not
+ * have to wait for modifications that have not yet completed. We define a
+ * transaction commit as completing when the commit sequence number is updated,
+ * hence if the sequence number has not updated, the sync operation has been
+ * run before the commit completed and we don't have to wait for it.
+ *
+ * If we have concurrent fsync/fdatasync() calls, the sequence numbers remain
+ * set on the log item until - at least - the journal flush completes. In
+ * reality, they are only cleared when the inode is fully unpinned (i.e.
+ * persistent in the journal and not dirty in the CIL), and so we rely on
+ * xfs_log_force_seq() either skipping sequences that have been persisted or
+ * waiting on sequences that are still in flight to correctly order concurrent
+ * sync operations.
  */
-static  int
+static int
 xfs_fsync_flush_log(
 	struct xfs_inode	*ip,
 	bool			datasync,
 	int			*log_flushed)
 {
-	int			error = 0;
-	xfs_csn_t		seq;
+	struct xfs_inode_log_item *iip = ip->i_itemp;
+	xfs_csn_t		seq = 0;
 
-	xfs_ilock(ip, XFS_ILOCK_SHARED);
-	seq = xfs_fsync_seq(ip, datasync);
-	if (seq) {
-		error = xfs_log_force_seq(ip->i_mount, seq, XFS_LOG_SYNC,
+	spin_lock(&iip->ili_lock);
+	if (datasync)
+		seq = iip->ili_datasync_seq;
+	else
+		seq = iip->ili_commit_seq;
+	spin_unlock(&iip->ili_lock);
+
+	if (!seq)
+		return 0;
+
+	return xfs_log_force_seq(ip->i_mount, seq, XFS_LOG_SYNC,
 					  log_flushed);
-
-		spin_lock(&ip->i_itemp->ili_lock);
-		ip->i_itemp->ili_fsync_fields = 0;
-		spin_unlock(&ip->i_itemp->ili_lock);
-	}
-	xfs_iunlock(ip, XFS_ILOCK_SHARED);
-	return error;
 }
 
 STATIC int
@@ -158,12 +153,10 @@ xfs_file_fsync(
 		error = blkdev_issue_flush(mp->m_ddev_targp->bt_bdev);
 
 	/*
-	 * Any inode that has dirty modifications in the log is pinned.  The
-	 * racy check here for a pinned inode will not catch modifications
-	 * that happen concurrently to the fsync call, but fsync semantics
-	 * only require to sync previously completed I/O.
+	 * 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 (xfs_ipincount(ip)) {
+	if (ip->i_itemp) {
 		err2 = xfs_fsync_flush_log(ip, datasync, &log_flushed);
 		if (err2 && !error)
 			error = err2;
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 0ddb9ce0f5e3..b5619ed5667b 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1667,7 +1667,6 @@ xfs_ifree_mark_inode_stale(
 	spin_lock(&iip->ili_lock);
 	iip->ili_last_fields = iip->ili_fields;
 	iip->ili_fields = 0;
-	iip->ili_fsync_fields = 0;
 	spin_unlock(&iip->ili_lock);
 	ASSERT(iip->ili_last_fields);
 
@@ -1832,12 +1831,20 @@ static void
 xfs_iunpin(
 	struct xfs_inode	*ip)
 {
-	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED);
+	struct xfs_inode_log_item *iip = ip->i_itemp;
+	xfs_csn_t		seq = 0;
 
 	trace_xfs_inode_unpin_nowait(ip, _RET_IP_);
+	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED);
+
+	spin_lock(&iip->ili_lock);
+	seq = iip->ili_commit_seq;
+	spin_unlock(&iip->ili_lock);
+	if (!seq)
+		return;
 
 	/* Give the log a push to start the unpinning I/O */
-	xfs_log_force_seq(ip->i_mount, ip->i_itemp->ili_commit_seq, 0, NULL);
+	xfs_log_force_seq(ip->i_mount, seq, 0, NULL);
 
 }
 
@@ -2506,7 +2513,6 @@ xfs_iflush(
 	spin_lock(&iip->ili_lock);
 	iip->ili_last_fields = iip->ili_fields;
 	iip->ili_fields = 0;
-	iip->ili_fsync_fields = 0;
 	set_bit(XFS_LI_FLUSHING, &iip->ili_item.li_flags);
 	spin_unlock(&iip->ili_lock);
 
@@ -2665,12 +2671,15 @@ int
 xfs_log_force_inode(
 	struct xfs_inode	*ip)
 {
+	struct xfs_inode_log_item *iip = ip->i_itemp;
 	xfs_csn_t		seq = 0;
 
-	xfs_ilock(ip, XFS_ILOCK_SHARED);
-	if (xfs_ipincount(ip))
-		seq = ip->i_itemp->ili_commit_seq;
-	xfs_iunlock(ip, XFS_ILOCK_SHARED);
+	if (!iip)
+		return 0;
+
+	spin_lock(&iip->ili_lock);
+	seq = iip->ili_commit_seq;
+	spin_unlock(&iip->ili_lock);
 
 	if (!seq)
 		return 0;
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 318e7c68ec72..a35f43b5e768 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -187,13 +187,16 @@ xfs_inode_item_precommit(
 	}
 
 	/*
-	 * Record the specific change for fdatasync optimisation. This allows
-	 * fdatasync to skip log forces for inodes that are only timestamp
-	 * dirty. Once we've processed the XFS_ILOG_IVERSION flag, convert it
-	 * to XFS_ILOG_CORE so that the actual on-disk dirty tracking
-	 * (ili_fields) correctly tracks that the version has changed.
+	 * Store the dirty flags back into the inode item as this state is used
+	 * later on in xfs_inode_item_committing() to determine whether the
+	 * transaction is relevant to fsync state or not.
+	 */
+	iip->ili_dirty_flags = flags;
+
+	/*
+	 * Convert the flags on-disk fields that have been modified in the
+	 * transaction so that ili_fields tracks the changes correctly.
 	 */
-	iip->ili_fsync_fields |= (flags & ~XFS_ILOG_IVERSION);
 	if (flags & XFS_ILOG_IVERSION)
 		flags = ((flags & ~XFS_ILOG_IVERSION) | XFS_ILOG_CORE);
 
@@ -207,12 +210,6 @@ xfs_inode_item_precommit(
 	spin_unlock(&iip->ili_lock);
 
 	xfs_inode_item_precommit_check(ip);
-
-	/*
-	 * We are done with the log item transaction dirty state, so clear it so
-	 * that it doesn't pollute future transactions.
-	 */
-	iip->ili_dirty_flags = 0;
 	return 0;
 }
 
@@ -722,13 +719,24 @@ xfs_inode_item_unpin(
 	struct xfs_log_item	*lip,
 	int			remove)
 {
-	struct xfs_inode	*ip = INODE_ITEM(lip)->ili_inode;
+	struct xfs_inode_log_item *iip = INODE_ITEM(lip);
+	struct xfs_inode	*ip = iip->ili_inode;
 
 	trace_xfs_inode_unpin(ip, _RET_IP_);
 	ASSERT(lip->li_buf || xfs_iflags_test(ip, XFS_ISTALE));
 	ASSERT(atomic_read(&ip->i_pincount) > 0);
-	if (atomic_dec_and_test(&ip->i_pincount))
+
+	/*
+	 * If this is the last unpin, then the inode no longer needs a journal
+	 * flush to persist it. Hence we can clear the commit sequence numbers
+	 * as a fsync/fdatasync operation on the inode at this point is a no-op.
+	 */
+	if (atomic_dec_and_lock(&ip->i_pincount, &iip->ili_lock)) {
+		iip->ili_commit_seq = 0;
+		iip->ili_datasync_seq = 0;
+		spin_unlock(&iip->ili_lock);
 		wake_up_bit(&ip->i_flags, __XFS_IPINNED_BIT);
+	}
 }
 
 STATIC uint
@@ -856,12 +864,45 @@ xfs_inode_item_committed(
 	return lsn;
 }
 
+/*
+ * The modification is now complete, so before we unlock the inode we need to
+ * update the commit sequence numbers for data integrity journal flushes. We
+ * always record the commit sequence number (ili_commit_seq) so that anything
+ * that needs a full journal sync will capture all of this modification.
+ *
+ * We then
+ * check if the changes will impact a datasync (O_DSYNC) journal flush. If the
+ * changes will require a datasync flush, then we also record the sequence in
+ * ili_datasync_seq.
+ *
+ * These commit sequence numbers will get cleared atomically with the inode being
+ * unpinned (i.e. pin count goes to zero), and so it will only be set when the
+ * inode is dirty in the journal. This removes the need for checking if the
+ * inode is pinned to determine if a journal flush is necessary, and hence
+ * removes the need for holding the ILOCK_SHARED in xfs_file_fsync() to
+ * serialise pin counts against commit sequence number updates.
+ *
+ */
 STATIC void
 xfs_inode_item_committing(
 	struct xfs_log_item	*lip,
 	xfs_csn_t		seq)
 {
-	INODE_ITEM(lip)->ili_commit_seq = seq;
+	struct xfs_inode_log_item *iip = INODE_ITEM(lip);
+
+	spin_lock(&iip->ili_lock);
+	iip->ili_commit_seq = seq;
+	if (iip->ili_dirty_flags & ~(XFS_ILOG_IVERSION | XFS_ILOG_TIMESTAMP))
+		iip->ili_datasync_seq = seq;
+	spin_unlock(&iip->ili_lock);
+
+	/*
+	 * Clear the per-transaction dirty flags now that we have finished
+	 * recording the transaction's inode modifications in the CIL and are
+	 * about to release and (maybe) unlock the inode.
+	 */
+	iip->ili_dirty_flags = 0;
+
 	return xfs_inode_item_release(lip);
 }
 
@@ -1053,7 +1094,6 @@ xfs_iflush_abort_clean(
 {
 	iip->ili_last_fields = 0;
 	iip->ili_fields = 0;
-	iip->ili_fsync_fields = 0;
 	iip->ili_flush_lsn = 0;
 	iip->ili_item.li_buf = NULL;
 	list_del_init(&iip->ili_item.li_bio_list);
diff --git a/fs/xfs/xfs_inode_item.h b/fs/xfs/xfs_inode_item.h
index ba92ce11a011..2ddcca41714f 100644
--- a/fs/xfs/xfs_inode_item.h
+++ b/fs/xfs/xfs_inode_item.h
@@ -32,9 +32,17 @@ struct xfs_inode_log_item {
 	spinlock_t		ili_lock;	   /* flush state lock */
 	unsigned int		ili_last_fields;   /* fields when flushed */
 	unsigned int		ili_fields;	   /* fields to be logged */
-	unsigned int		ili_fsync_fields;  /* logged since last fsync */
 	xfs_lsn_t		ili_flush_lsn;	   /* lsn at last flush */
+
+	/*
+	 * We record the sequence number for every inode modification, as
+	 * well as those that only require fdatasync operations for data
+	 * integrity. This allows optimisation of the O_DSYNC/fdatasync path
+	 * without needing to track what modifications the journal is currently
+	 * carrying for the inode. These are protected by the above ili_lock.
+	 */
 	xfs_csn_t		ili_commit_seq;	   /* last transaction commit */
+	xfs_csn_t		ili_datasync_seq;  /* for datasync optimisation */
 };
 
 static inline int xfs_inode_clean(struct xfs_inode *ip)
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 2a74f2957341..f8c925220005 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -149,9 +149,18 @@ xfs_bmbt_to_iomap(
 		iomap->bdev = target->bt_bdev;
 	iomap->flags = iomap_flags;
 
-	if (xfs_ipincount(ip) &&
-	    (ip->i_itemp->ili_fsync_fields & ~XFS_ILOG_TIMESTAMP))
-		iomap->flags |= IOMAP_F_DIRTY;
+	/*
+	 * If the inode is dirty for datasync purposes, let iomap know so it
+	 * doesn't elide the IO completion journal flushes on O_DSYNC IO.
+	 */
+	if (ip->i_itemp) {
+		struct xfs_inode_log_item *iip = ip->i_itemp;
+
+		spin_lock(&iip->ili_lock);
+		if (iip->ili_datasync_seq)
+			iomap->flags |= IOMAP_F_DIRTY;
+		spin_unlock(&iip->ili_lock);
+	}
 
 	iomap->validity_cookie = sequence_cookie;
 	return 0;
-- 
2.50.1

Re: [PATCH 1/2] xfs: rearrange code in xfs_inode_item_precommit

[Darrick J. Wong]

On Thu, Sep 18, 2025 at 08:12:53AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> There are similar extsize checks and updates done inside and outside
> the inode item lock, which could all be done under a single top
> level logic branch outside the ili_lock. The COW extsize fixup can
> potentially miss updating the XFS_ILOG_CORE in ili_fsync_fields, so
> moving this code up above the ili_fsync_fields update could also be
> considered a fix.
> 
> Further, to make the next change a bit cleaner, move where we
> calculate the on-disk flag mask to after we attach the cluster
> buffer to the the inode log item.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_inode_item.c | 65 ++++++++++++++++++-----------------------
>  1 file changed, 29 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index afb6cadf7793..318e7c68ec72 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -131,46 +131,28 @@ xfs_inode_item_precommit(
>  	}
>  
>  	/*
> -	 * Inode verifiers do not check that the extent size hint is an integer
> -	 * multiple of the rt extent size on a directory with both rtinherit
> -	 * and extszinherit flags set.  If we're logging a directory that is
> -	 * misconfigured in this way, clear the hint.
> +	 * Inode verifiers do not check that the extent size hints are an
> +	 * integer multiple of the rt extent size on a directory with
> +	 * rtinherit flags set.  If we're logging a directory that is
> +	 * misconfigured in this way, clear the bad hints.
>  	 */
> -	if ((ip->i_diflags & XFS_DIFLAG_RTINHERIT) &&
> -	    (ip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) &&
> -	    xfs_extlen_to_rtxmod(ip->i_mount, ip->i_extsize) > 0) {
> -		ip->i_diflags &= ~(XFS_DIFLAG_EXTSIZE |
> -				   XFS_DIFLAG_EXTSZINHERIT);
> -		ip->i_extsize = 0;
> -		flags |= XFS_ILOG_CORE;
> +	if (ip->i_diflags & XFS_DIFLAG_RTINHERIT) {
> +		if ((ip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) &&
> +		    xfs_extlen_to_rtxmod(ip->i_mount, ip->i_extsize) > 0) {
> +			ip->i_diflags &= ~(XFS_DIFLAG_EXTSIZE |
> +					   XFS_DIFLAG_EXTSZINHERIT);
> +			ip->i_extsize = 0;
> +			flags |= XFS_ILOG_CORE;
> +		}
> +		if ((ip->i_diflags2 & XFS_DIFLAG2_COWEXTSIZE) &&
> +		    xfs_extlen_to_rtxmod(ip->i_mount, ip->i_cowextsize) > 0) {
> +			ip->i_diflags2 &= ~XFS_DIFLAG2_COWEXTSIZE;
> +			ip->i_cowextsize = 0;
> +			flags |= XFS_ILOG_CORE;
> +		}
>  	}

Hrm, yeah, that cowextsize fixing code looks like it was merged in the
wrong place or something.  In theory it's a bug fix, but since we never
merged support for rtreflink and rtextsize > 1fsb this will never happen
in practice since rtextsize cannot change when reflink is enabled.

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

--D


>  
> -	/*
> -	 * Record the specific change for fdatasync optimisation. This allows
> -	 * fdatasync to skip log forces for inodes that are only timestamp
> -	 * dirty. Once we've processed the XFS_ILOG_IVERSION flag, convert it
> -	 * to XFS_ILOG_CORE so that the actual on-disk dirty tracking
> -	 * (ili_fields) correctly tracks that the version has changed.
> -	 */
>  	spin_lock(&iip->ili_lock);
> -	iip->ili_fsync_fields |= (flags & ~XFS_ILOG_IVERSION);
> -	if (flags & XFS_ILOG_IVERSION)
> -		flags = ((flags & ~XFS_ILOG_IVERSION) | XFS_ILOG_CORE);
> -
> -	/*
> -	 * Inode verifiers do not check that the CoW extent size hint is an
> -	 * integer multiple of the rt extent size on a directory with both
> -	 * rtinherit and cowextsize flags set.  If we're logging a directory
> -	 * that is misconfigured in this way, clear the hint.
> -	 */
> -	if ((ip->i_diflags & XFS_DIFLAG_RTINHERIT) &&
> -	    (ip->i_diflags2 & XFS_DIFLAG2_COWEXTSIZE) &&
> -	    xfs_extlen_to_rtxmod(ip->i_mount, ip->i_cowextsize) > 0) {
> -		ip->i_diflags2 &= ~XFS_DIFLAG2_COWEXTSIZE;
> -		ip->i_cowextsize = 0;
> -		flags |= XFS_ILOG_CORE;
> -	}
> -
>  	if (!iip->ili_item.li_buf) {
>  		struct xfs_buf	*bp;
>  		int		error;
> @@ -204,6 +186,17 @@ xfs_inode_item_precommit(
>  		xfs_trans_brelse(tp, bp);
>  	}
>  
> +	/*
> +	 * Record the specific change for fdatasync optimisation. This allows
> +	 * fdatasync to skip log forces for inodes that are only timestamp
> +	 * dirty. Once we've processed the XFS_ILOG_IVERSION flag, convert it
> +	 * to XFS_ILOG_CORE so that the actual on-disk dirty tracking
> +	 * (ili_fields) correctly tracks that the version has changed.
> +	 */
> +	iip->ili_fsync_fields |= (flags & ~XFS_ILOG_IVERSION);
> +	if (flags & XFS_ILOG_IVERSION)
> +		flags = ((flags & ~XFS_ILOG_IVERSION) | XFS_ILOG_CORE);
> +
>  	/*
>  	 * Always OR in the bits from the ili_last_fields field.  This is to
>  	 * coordinate with the xfs_iflush() and xfs_buf_inode_iodone() routines
> -- 
> 2.50.1
> 
> 

Re: [PATCH 1/2] xfs: rearrange code in xfs_inode_item_precommit

[Christoph Hellwig]

On Thu, Sep 18, 2025 at 08:12:53AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> There are similar extsize checks and updates done inside and outside
> the inode item lock, which could all be done under a single top
> level logic branch outside the ili_lock. The COW extsize fixup can
> potentially miss updating the XFS_ILOG_CORE in ili_fsync_fields, so
> moving this code up above the ili_fsync_fields update could also be
> considered a fix.
> 
> Further, to make the next change a bit cleaner, move where we
> calculate the on-disk flag mask to after we attach the cluster
> buffer to the the inode log item.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_inode_item.c | 65 ++++++++++++++++++-----------------------
>  1 file changed, 29 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index afb6cadf7793..318e7c68ec72 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -131,46 +131,28 @@ xfs_inode_item_precommit(
>  	}
>  
>  	/*
> +	 * Inode verifiers do not check that the extent size hints are an
> +	 * integer multiple of the rt extent size on a directory with
> +	 * rtinherit flags set.  If we're logging a directory that is
> +	 * misconfigured in this way, clear the bad hints.
>  	 */

Not directly related to this patch, but why are we not checking for
that in the inode verifier?  Even if we can't reject that value,
it seems like we should fix that up when reading an inode into memory
instead of in a pre-commit hook?

The patch itself looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] xfs: rework datasync tracking and execution

[Christoph Hellwig]

On Thu, Sep 18, 2025 at 08:12:54AM +1000, Dave Chinner wrote:
> Jan demostrated that reducing the ifdatasync lock hold time can

s/demostrated/demonstrated/

> came to the conclusion that seperately tracking datasync flush

s/seperately/separately/

> seqeunces was the best approach to solving the problem.
 
s/seqeunces/sequences/g

> operations are allo done under ILOCK_EXCL context:

s/allo/all/

>  xfs_fsync_flush_log(
>  	struct xfs_inode	*ip,
>  	bool			datasync,
>  	int			*log_flushed)
>  {
> +	struct xfs_inode_log_item *iip = ip->i_itemp;
> +	xfs_csn_t		seq = 0;
>  
> +	spin_lock(&iip->ili_lock);
> +	if (datasync)
> +		seq = iip->ili_datasync_seq;
> +	else
> +		seq = iip->ili_commit_seq;
> +	spin_unlock(&iip->ili_lock);

If we care about the additional speedup of the READ_ONCE done
in Jan's patch we could make that configurable on CONFIG_64BIT
here.  There's precedence for that in i_size_read for that in the
VFS.  If we have a helper like:

static inline bool
xfs_inode_sync_csn(
	struct xfs_inode	*ip,
 	bool			datasync,
	xfs_csn_t		*seq)
 {
	struct xfs_inode_log_item *iip = ip->i_itemp;

	if (!iip)
		return false;

	spin_lock(&iip->ili_lock);
	if (datasync)
		*seq = iip->ili_datasync_seq;
	else
		*seq = iip->ili_commit_seq;
	spin_unlock(&iip->ili_lock);
	return true;
}

we could isolate that to one single place as well.

But even without that, the patch looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/2] xfs: rearrange code in xfs_inode_item_precommit

[Darrick J. Wong]

On Fri, Sep 19, 2025 at 08:19:52AM -0700, Christoph Hellwig wrote:
> On Thu, Sep 18, 2025 at 08:12:53AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > There are similar extsize checks and updates done inside and outside
> > the inode item lock, which could all be done under a single top
> > level logic branch outside the ili_lock. The COW extsize fixup can
> > potentially miss updating the XFS_ILOG_CORE in ili_fsync_fields, so
> > moving this code up above the ili_fsync_fields update could also be
> > considered a fix.
> > 
> > Further, to make the next change a bit cleaner, move where we
> > calculate the on-disk flag mask to after we attach the cluster
> > buffer to the the inode log item.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_inode_item.c | 65 ++++++++++++++++++-----------------------
> >  1 file changed, 29 insertions(+), 36 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> > index afb6cadf7793..318e7c68ec72 100644
> > --- a/fs/xfs/xfs_inode_item.c
> > +++ b/fs/xfs/xfs_inode_item.c
> > @@ -131,46 +131,28 @@ xfs_inode_item_precommit(
> >  	}
> >  
> >  	/*
> > +	 * Inode verifiers do not check that the extent size hints are an
> > +	 * integer multiple of the rt extent size on a directory with
> > +	 * rtinherit flags set.  If we're logging a directory that is
> > +	 * misconfigured in this way, clear the bad hints.
> >  	 */
> 
> Not directly related to this patch, but why are we not checking for
> that in the inode verifier?  Even if we can't reject that value,
> it seems like we should fix that up when reading an inode into memory
> instead of in a pre-commit hook?

growfs can change rtextsize when adding a rt section to the filesystem,
and we're not allowed to break userspace (even though I'd wager nobody
has actually done this in the past 20 years).

--D

> The patch itself looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

Re: [PATCH 1/2] xfs: rearrange code in xfs_inode_item_precommit

[Christoph Hellwig]

On Fri, Sep 19, 2025 at 09:08:18AM -0700, Darrick J. Wong wrote:
> > Not directly related to this patch, but why are we not checking for
> > that in the inode verifier?  Even if we can't reject that value,
> > it seems like we should fix that up when reading an inode into memory
> > instead of in a pre-commit hook?
> 
> growfs can change rtextsize when adding a rt section to the filesystem,
> and we're not allowed to break userspace (even though I'd wager nobody
> has actually done this in the past 20 years).

Can we document this, please?

Re: [PATCH 2/2] xfs: rework datasync tracking and execution

[Carlos Maiolino]

On Fri, Sep 19, 2025 at 08:30:39AM -0700, Christoph Hellwig wrote:
> On Thu, Sep 18, 2025 at 08:12:54AM +1000, Dave Chinner wrote:
> > Jan demostrated that reducing the ifdatasync lock hold time can
> 
> s/demostrated/demonstrated/
> 
> > came to the conclusion that seperately tracking datasync flush
> 
> s/seperately/separately/
> 
> > seqeunces was the best approach to solving the problem.
> 
> s/seqeunces/sequences/g
> 
> > operations are allo done under ILOCK_EXCL context:
> 
> s/allo/all/

I'll update those at commit time.


> 
> >  xfs_fsync_flush_log(
> >  	struct xfs_inode	*ip,
> >  	bool			datasync,
> >  	int			*log_flushed)
> >  {
> > +	struct xfs_inode_log_item *iip = ip->i_itemp;
> > +	xfs_csn_t		seq = 0;
> >
> > +	spin_lock(&iip->ili_lock);
> > +	if (datasync)
> > +		seq = iip->ili_datasync_seq;
> > +	else
> > +		seq = iip->ili_commit_seq;
> > +	spin_unlock(&iip->ili_lock);
> 
> If we care about the additional speedup of the READ_ONCE done
> in Jan's patch we could make that configurable on CONFIG_64BIT
> here.  There's precedence for that in i_size_read for that in the
> VFS.  If we have a helper like:
> 
> static inline bool
> xfs_inode_sync_csn(
> 	struct xfs_inode	*ip,
>  	bool			datasync,
> 	xfs_csn_t		*seq)
>  {
> 	struct xfs_inode_log_item *iip = ip->i_itemp;
> 
> 	if (!iip)
> 		return false;
> 
> 	spin_lock(&iip->ili_lock);
> 	if (datasync)
> 		*seq = iip->ili_datasync_seq;
> 	else
> 		*seq = iip->ili_commit_seq;
> 	spin_unlock(&iip->ili_lock);
> 	return true;
> }
> 
> we could isolate that to one single place as well.
> 

Yeah, an isolated patch with that would be better.

> But even without that, the patch looks fine:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

Re: [PATCH 0/2] xfs: reduce ILOCK contention on O_DSYNC DIO

[Carlos Maiolino]

On Thu, 18 Sep 2025 08:12:52 +1000, Dave Chinner wrote:
> This is the changes I came up with in the course of the discussion
> with Jan about the fallocate+DIO+O_DSYNC performance improvement
> thread here:
> 
> https://lore.kernel.org/linux-xfs/CAM4Jq_71gxMcnOdgqWhKEa53sr9r57Qpi0hc5bs3NgtnNOGwtg@mail.gmail.com/T/#
> 
> The first patch is moves a little bit of code around to ensure
> that xfs_inode_item_precommit() always calculates all the changes
> before it starts manipulating the dirty and fsync flags on the inode
> log item. Whilst technically it could be considered a bug fix, the
> bug it fixes requires an inconsistency in the on disk format to
> exist first, so it likely won't ever be an issue in normal
> production systems. It also requires an application to run a
> fdatasync and then have a system crash just after the
> inconsistency is addressed to expose it. So the likelihood of it
> ever being triggered as a data integrity issue is -extremely- tiny.
> 
> [...]

Applied to for-next, thanks!

[1/2] xfs: rearrange code in xfs_inode_item_precommit
      commit: bc7d684fea18cc48c3630d2b7f1789000ff2df5b
[2/2] xfs: rework datasync tracking and execution
      commit: c91d38b57f2c4784d885c874b2a1234a01361afd

Best regards,
-- 
Carlos Maiolino <cem@kernel.org>