Re: [PATCH] xfs: fix i_version handling in xfs

[Dave Chinner <david@fromorbit.com>]

On Wed, Aug 17, 2022 at 08:42:57AM +1000, Dave Chinner wrote:
> On Tue, Aug 16, 2022 at 11:58:06AM -0400, Jeff Layton wrote:
> > On Tue, 2022-08-16 at 08:43 -0700, Darrick J. Wong wrote:
> > > On Tue, Aug 16, 2022 at 09:17:36AM -0400, Jeff Layton wrote:
> > > > @@ -116,20 +118,7 @@ xfs_trans_log_inode(
> > > >  		spin_unlock(&inode->i_lock);
> > > >  	}
> > > >  
> > > > -	/*
> > > > -	 * First time we log the inode in a transaction, bump the inode change
> > > > -	 * counter if it is configured for this to occur. While we have the
> > > > -	 * inode locked exclusively for metadata modification, we can usually
> > > > -	 * avoid setting XFS_ILOG_CORE if no one has queried the value since
> > > > -	 * the last time it was incremented. If we have XFS_ILOG_CORE already
> > > > -	 * set however, then go ahead and bump the i_version counter
> > > > -	 * unconditionally.
> > > > -	 */
> > > > -	if (!test_and_set_bit(XFS_LI_DIRTY, &iip->ili_item.li_flags)) {
> > > > -		if (IS_I_VERSION(inode) &&
> > > > -		    inode_maybe_inc_iversion(inode, flags & XFS_ILOG_CORE))
> > > > -			iversion_flags = XFS_ILOG_CORE;
> > > > -	}
> > > > +	set_bit(XFS_LI_DIRTY, &iip->ili_item.li_flags);
> 
> .... and this removes the sweep that captures in-memory timestamp
> and i_version peeks between any persistent inode metadata
> modifications that have been made, regardless of whether i_version
> has already been bumped for them or not.

Which, BTW, breaks the iversion update for xfs_fs_commit_blocks()
which the pNFS server calls to inform the filesystem that the pNFS
client has finished writing data to a mapped region.

This function runs unwritten extent conversion (making the data
externally visible) and takes timestamps from the pNFS server. It
then persists all these changes, meaning that there will be
externally visible data, metadata and timestamp updates persisted to
disk by the pNFS server without an iversion update occurring.

This iversion stuff is .... complex. It's also really easy to get
wrong, and that's even before we start trying to optimise away stuff
like timestamp updates....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com