Re: [PATCH] xfs: rewrite the fdatasync optimization

[Dave Chinner <david@fromorbit.com>]

On Tue, Feb 06, 2018 at 08:23:56AM +0100, Christoph Hellwig wrote:
> On Tue, Feb 06, 2018 at 09:17:26AM +1100, Dave Chinner wrote:
> > > Currently we need to the ilock over the log force in xfs_fsync so that we
> > > can protect ili_fsync_fields against incorrect manipulation.
> > > 
> > > But if instead we add new XFS_ILOG_VERSION pseudo log area similar to the
> > > timestamp one we can use that to just record the last dirty / fdatasync
> > > dirty lsn as long as the inode is pinned, and clear it when unpinning to
> > > avoid holding the ilock over I/O.
> > 
> > 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.

> and this patch doesn't change that behavior.

I think it does.

> The reason why the
> 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.

That's exactly the behaviour we need to meet the above NFS
requirements - the iversion change is associated with the new data
being written, not the timestamp change we are making as a result of
the data change.

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.

That's not the behaviour NFS requires - we'll stabilise data but we
will not stabilise the iversion change that goes along with that
data change. Hence if we crash after fdatasync, there'll be new data
on disk and an old iversion....

So, AFAICT, this patch does change behviour and it breaks the
iversion semantics that the NFS server requires for data
modification.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com