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
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
