Re: Increase in XFS journal flushes with (direct_write;fdatasync)+

[Dave Chinner <dgc@kernel.org>]

On Thu, May 07, 2026 at 10:34:43PM +0200, Pankaj Raghav (Samsung) wrote:
> On Wed, May 06, 2026 at 09:26:25AM -0400, Andres Freund wrote:
> > Hi,
> > 
> > While looking at performance issues on Samsung client drives due to slow FUA,
> > I tried to reproduce older numbers on a recent kernel.  And couldn't, at first
> > - but not because the problem went away, but because the fdatasync numbers
> > (which shouldn't use FUA) got *much* worse.
> > 
> > These drives have FUA writes that are slower than full flushes, making
> > O_DIRECT|O_DSYNC writes perform poorly and fdatasync() comparatively better.
> > 
> > 
> > What I'm seeing is that with recent kernels the fdatasync() performance is
> > roughly as bad as the O_DSYNC, whereas previously it was > 2x as
> > fasts. blktrace showed that there are ongoing FUA writes during a workload
> > with just overwriting writes and an fdatasync after every write.
> > 
> > 
> > At first I thought it was a regression between 7.0..7.1-rc2, but that turned
> > out to be only because the 7.0 machine did not have lazytime enabled. After
> > fixing that discrepancy, the regression is also visible in 7.0.  I have
> > confirmed it's not visible in 6.18.
> 
> I was able to reproduce this issue. The commit causing the issue is
> indeed from nonblocking timestamps series as you indicated 
> (fs: add support for non-blocking timestamp updates).
> 
> In inode_update_cmtime, we have the following changes as a part of the
> series:
> ...
> mtime_changed = !timespec64_equal(&now, &mtime);
> 	if (mtime_changed || !timespec64_equal(&now, &ctime))
> 		dirty = inode_time_dirty_flag(inode);  // #1
> 
> 	/*
> 	 * Pure timestamp updates can be recorded in the inode without blocking
> 	 * by not dirtying the inode.  But when the file system requires
> 	 * i_version updates, the update of i_version can still block.
> 	 * Error out if we'd actually have to update i_version or don't support
> 	 * lazytime.
> 	 */
> 	if (IS_I_VERSION(inode)) {
> 		if (flags & IOCB_NOWAIT) {
> 			if (!(inode->i_sb->s_flags & SB_LAZYTIME) ||
> 			    inode_iversion_need_inc(inode))
> 				return -EAGAIN;
> 		} else {
> 			if (inode_maybe_inc_iversion(inode, !!dirty)) //#2
> 				dirty |= I_DIRTY_SYNC;
> 		}
> 	}
> 
> ...

Ugh. Given we don't support i_version as an externally visible
change_cookie any more (we have multigrained timestamps for that
now), why do we still set SB_I_VERSION and jump
through all these complex hoops to set maintain something we don't
actually need?

i.e. going back to ip->di_version++ whenever the inode is logged to
maintain the on-disk change version would make things so much
simpler here...

Cheers,

Dave.
-- 
Dave Chinner
dgc@kernel.org