Re: [PATCH] xfs: rewrite the fdatasync optimization

[Dave Chinner <david@fromorbit.com>]

On Tue, Feb 13, 2018 at 04:04:02PM +0100, Christoph Hellwig wrote:
> On Fri, Feb 09, 2018 at 10:17:25AM +1100, Dave Chinner wrote:
> > > > I thought that NFS requires the iversion changes to be made stable
> > > > at the same time as the data changes they correspond to is made
> > > > stable? i.e. NFS requires us to stabilise the on disk iversion field
> > > > during fdatasync.
> > > 
> > > Yes,
> > 
> > So you're agreeing with me that if iversion is changed, then
> > fdatasync needs to force the log.
> 
> No, not at all.  fdatasync does simply not matter for NFS changetimes.
> NFS comes in through commit_inode_metadata, for which the fdatasync
> optimization is irrelevant.

Yes, for NFS server based IO.i

No, for local user IO to the local fs that is also exported via NFS.

> But even discounting NFS - anyone caring
> about timestamps persisting must use fsync and not fdatasync, in fact
> that is the whole point for fdatasyncs existence!

Yes, but timestamps are not the issue here - we know the behaviour
is correct.  The issue here is the iversion change counter
semantics be w.r.t. fdatasync, and you haven't established a case
fdatasync being able to avoid it.

> > > new XFS_ILOG_VERSION flag is needed is to be able to check if
> > > an fdatasync needs to flush out an inode log item.
> > 
> > If we don't change iversion because it's not been queried, then in
> > the current code then XFS_ILOG_CORE will not be set and so the inode
> > remains only timestamp dirty and does not get flushed by fdatasync.
> > If we change iversion, then XFS_ILOG_CORE gets set, and the next
> > fdatasync will correctly flush the inode log item.
> 
> Yes, if we talk about ili_fields.  Once we are deep down in the logging
> core XFS_ILOG_CORE of course gets set, see xfs_inode_item_format.
> 
> > If we look at your code now, when we change iversion we'll set
> > XFS_ILOG_VERSION and that will now behave identically to the
> > timestamp code w.r.t. fdatasync. i.e. we will *never* flush pure
> > iversion/timestamp changes on fdatasync.
> 
> And that is the correct behavior for fdatasync.

For *timestamps*, yes.

But iversion is a *data change notifier*, and hence it's directly
related to the data being stabilised during fdatasync.

e.g. In the case of a local write to an NFS exported filesystem, the
iversion needs to be made stable at the same time as the data via
fdatasync. That way if the server crashes after fdatasync, when it
comes back up all the NFS clients see that the data in that file
has changed via the recovered iversion change from the fdatasync.
If the iversion change is not written to the log during the
fdatasync, then none of the NFS clients will detect that the data
has actually changed after the server has recovered from the crash.

AIUI, this violates the semantics NFS requires from the iversion
field, and that means fdatasync needs to flush iversion changes.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com