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

[Dave Chinner <david@xxxxxxxxxxxxx>]

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@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxxxx