Re: [PATCH] xfs: optimise away log forces on timestamp updates for fdatasync

[Dave Chinner <david@fromorbit.com>]

On Thu, Oct 22, 2015 at 01:36:19PM -0400, Brian Foster wrote:
> On Wed, Oct 21, 2015 at 01:59:03PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > xfs: timestamp updates cause excessive fdatasync log traffic
....
> > +++ b/fs/xfs/xfs_file.c
> > @@ -248,8 +248,10 @@ xfs_file_fsync(
> >  	xfs_ilock(ip, XFS_ILOCK_SHARED);
> >  	if (xfs_ipincount(ip)) {
> >  		if (!datasync ||
> > -		    (ip->i_itemp->ili_fields & ~XFS_ILOG_TIMESTAMP))
> > +		    (ip->i_itemp->ili_fsync_fields & ~XFS_ILOG_TIMESTAMP)) {
> >  			lsn = ip->i_itemp->ili_last_lsn;
> > +			ip->i_itemp->ili_fsync_fields = 0;
> > +		}
> 
> Ok, so we check what's been logged since the last fsync that forced the
> log. If anything other than the timestamp has been logged, we force the
> log and clear the fields. Seems like a reasonable optimization to me.
> 
> One question... is it safe to clear the ili_fsync fields here if we have
> parallel fsync()/fdatasync() calls coming in? This is under the shared
> ilock, so assume that one fsync() comes in and finds non-timestamp
> changes to flush. It grabs the lsn, clears the flags and calls the log
> force. If an fdatasync() comes in before the log force completes,
> shouldn't it wait?

Probably, but the only way to do that is to run a log force on that
same lsn. Actually, it is safe to do that log force while holding
the XFS_ILOCK (xfs_trans_commit() does that for synchronous
transactions), so we should simply be able to do:

	xfs_ilock(ip, XFS_ILOCK_SHARED);
	if (xfs_ipincount(ip)) {
		if (!datasync ||
		    (ip->i_itemp->ili_fsync_fields & ~XFS_ILOG_TIMESTAMP))
			lsn = ip->i_itemp->ili_last_lsn;
	}

	if (lsn) {
		error = _xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, &log_flushed);
		ip->i_itemp->ili_fsync_fields = 0;
	}
	xfs_iunlock(ip, XFS_ILOCK_SHARED);


> Also, is it me or are we sending an unconditional flush in the hunk
> following the log force call in xfs_file_fsync() (even if we've skipped
> the log force)?

The flush is needed - fdatasync needs to guarantee the data is
on stable storage even if no metadata needs to be written to the
journal.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs