[PATCH] xfs: fix i_version handling in xfs

[PATCH] xfs: fix i_version handling in xfs

[Jeff Layton]

The i_version in xfs_trans_log_inode is bumped for any inode update,
including atime-only updates due to reads. We don't want to record those
in the i_version, as they don't represent "real" changes. Remove that
callsite.

In xfs_vn_update_time, if S_VERSION is flagged, then attempt to bump the
i_version and turn on XFS_ILOG_CORE if it happens. In
xfs_trans_ichgtime, update the i_version if the mtime or ctime are being
updated.

Cc: Darrick J. Wong <darrick.wong@oracle.com>
Cc: Dave Chinner <david@fromorbit.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/xfs/libxfs/xfs_trans_inode.c | 17 +++--------------
 fs/xfs/xfs_iops.c               |  4 ++++
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
index 8b5547073379..78bf7f491462 100644
--- a/fs/xfs/libxfs/xfs_trans_inode.c
+++ b/fs/xfs/libxfs/xfs_trans_inode.c
@@ -71,6 +71,8 @@ xfs_trans_ichgtime(
 		inode->i_ctime = tv;
 	if (flags & XFS_ICHGTIME_CREATE)
 		ip->i_crtime = tv;
+	if (flags & (XFS_ICHGTIME_MOD|XFS_ICHGTIME_CHG))
+		inode_inc_iversion(inode);
 }
 
 /*
@@ -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);
 
 	/*
 	 * If we're updating the inode core or the timestamps and it's possible
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 45518b8c613c..162e044c7f56 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -718,6 +718,7 @@ xfs_setattr_nonsize(
 	}
 
 	setattr_copy(mnt_userns, inode, iattr);
+	inode_inc_iversion(inode);
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
 	XFS_STATS_INC(mp, xs_ig_attrchg);
@@ -943,6 +944,7 @@ xfs_setattr_size(
 
 	ASSERT(!(iattr->ia_valid & (ATTR_UID | ATTR_GID)));
 	setattr_copy(mnt_userns, inode, iattr);
+	inode_inc_iversion(inode);
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
 	XFS_STATS_INC(mp, xs_ig_attrchg);
@@ -1047,6 +1049,8 @@ xfs_vn_update_time(
 		inode->i_mtime = *now;
 	if (flags & S_ATIME)
 		inode->i_atime = *now;
+	if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false))
+		log_flags |= XFS_ILOG_CORE;
 
 	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
 	xfs_trans_log_inode(tp, ip, log_flags);
-- 
2.37.2

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

[Darrick J. Wong]

On Tue, Aug 16, 2022 at 09:17:36AM -0400, Jeff Layton wrote:
> The i_version in xfs_trans_log_inode is bumped for any inode update,
> including atime-only updates due to reads. We don't want to record those
> in the i_version, as they don't represent "real" changes. Remove that
> callsite.
> 
> In xfs_vn_update_time, if S_VERSION is flagged, then attempt to bump the
> i_version and turn on XFS_ILOG_CORE if it happens. In
> xfs_trans_ichgtime, update the i_version if the mtime or ctime are being
> updated.

What about operations that don't touch the mtime but change the file
metadata anyway?  There are a few of those, like the blockgc garbage
collector, deduperange, and the defrag tool.

Zooming out a bit -- what does i_version signal, concretely?  I thought
it was used by nfs (and maybe ceph?) to signal to clients that the file
on the server has moved on, and the client needs to invalidate its
caches.  I thought afs had a similar generation counter, though it's
only used to cache file data, not metadata?  Does an i_version change
cause all of them to invalidate caches, or is there more behavior I
don't know about?

Does that mean that we should bump i_version for any file data or
attribute that could be queried or observed by userspace?  In which case
I suppose this change is still correct, even if it relaxes i_version
updates from "any change to the inode whatsoever" to "any change that
would bump mtime".  Unless FIEMAP is part of "attributes observed by
userspace".

(The other downside I can see is that now we have to remember to bump
timestamps for every new file operation we add, unlike the current code
which is centrally located in xfs_trans_log_inode.)

--D

> Cc: Darrick J. Wong <darrick.wong@oracle.com>
> Cc: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_trans_inode.c | 17 +++--------------
>  fs/xfs/xfs_iops.c               |  4 ++++
>  2 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> index 8b5547073379..78bf7f491462 100644
> --- a/fs/xfs/libxfs/xfs_trans_inode.c
> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> @@ -71,6 +71,8 @@ xfs_trans_ichgtime(
>  		inode->i_ctime = tv;
>  	if (flags & XFS_ICHGTIME_CREATE)
>  		ip->i_crtime = tv;
> +	if (flags & (XFS_ICHGTIME_MOD|XFS_ICHGTIME_CHG))
> +		inode_inc_iversion(inode);
>  }
>  
>  /*
> @@ -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);
>  
>  	/*
>  	 * If we're updating the inode core or the timestamps and it's possible
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 45518b8c613c..162e044c7f56 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -718,6 +718,7 @@ xfs_setattr_nonsize(
>  	}
>  
>  	setattr_copy(mnt_userns, inode, iattr);
> +	inode_inc_iversion(inode);
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  
>  	XFS_STATS_INC(mp, xs_ig_attrchg);
> @@ -943,6 +944,7 @@ xfs_setattr_size(
>  
>  	ASSERT(!(iattr->ia_valid & (ATTR_UID | ATTR_GID)));
>  	setattr_copy(mnt_userns, inode, iattr);
> +	inode_inc_iversion(inode);
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  
>  	XFS_STATS_INC(mp, xs_ig_attrchg);
> @@ -1047,6 +1049,8 @@ xfs_vn_update_time(
>  		inode->i_mtime = *now;
>  	if (flags & S_ATIME)
>  		inode->i_atime = *now;
> +	if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false))
> +		log_flags |= XFS_ILOG_CORE;
>  
>  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>  	xfs_trans_log_inode(tp, ip, log_flags);
> -- 
> 2.37.2
> 

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

[Jeff Layton]

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:
> > The i_version in xfs_trans_log_inode is bumped for any inode update,
> > including atime-only updates due to reads. We don't want to record those
> > in the i_version, as they don't represent "real" changes. Remove that
> > callsite.
> > 
> > In xfs_vn_update_time, if S_VERSION is flagged, then attempt to bump the
> > i_version and turn on XFS_ILOG_CORE if it happens. In
> > xfs_trans_ichgtime, update the i_version if the mtime or ctime are being
> > updated.
> 
> What about operations that don't touch the mtime but change the file
> metadata anyway?  There are a few of those, like the blockgc garbage
> collector, deduperange, and the defrag tool.
> 

Do those change the c/mtime at all?

It's possible we're missing some places that should change the i_version
as well. We may need some more call sites.

> Zooming out a bit -- what does i_version signal, concretely?  I thought
> it was used by nfs (and maybe ceph?) to signal to clients that the file
> on the server has moved on, and the client needs to invalidate its
> caches.  I thought afs had a similar generation counter, though it's
> only used to cache file data, not metadata?  Does an i_version change
> cause all of them to invalidate caches, or is there more behavior I
> don't know about?
> 

For NFS, it indicates a change to the change attribute indicates that
there has been a change to the data or metadata for the file. atime
changes due to reads are specifically exempted from this, but we do bump
the i_version if someone (e.g.) changes the atime via utimes(). 

The NFS client will generally invalidate its caches for the inode when
it notices a change attribute change.

FWIW, AFS may not meet this standard since it doesn't generally
increment the counter on metadata changes. It may turn out that we don't
want to expose this to the AFS client due to that (or maybe come up with
some way to indicate this difference).

> Does that mean that we should bump i_version for any file data or
> attribute that could be queried or observed by userspace?  In which case
> I suppose this change is still correct, even if it relaxes i_version
> updates from "any change to the inode whatsoever" to "any change that
> would bump mtime".  Unless FIEMAP is part of "attributes observed by
> userspace".
> 
> (The other downside I can see is that now we have to remember to bump
> timestamps for every new file operation we add, unlike the current code
> which is centrally located in xfs_trans_log_inode.)
> 

The main reason for the change attribute in NFS was that NFSv3 is
plagued with cache-coherency problems due to coarse-grained timestamp
granularity. It was conceived as a way to indicate that the inode had
changed without relying on timestamps.

In practice, we want to bump the i_version counter whenever the ctime or
mtime would be changed.

> --D
> 
> > Cc: Darrick J. Wong <darrick.wong@oracle.com>
> > Cc: Dave Chinner <david@fromorbit.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_trans_inode.c | 17 +++--------------
> >  fs/xfs/xfs_iops.c               |  4 ++++
> >  2 files changed, 7 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> > index 8b5547073379..78bf7f491462 100644
> > --- a/fs/xfs/libxfs/xfs_trans_inode.c
> > +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> > @@ -71,6 +71,8 @@ xfs_trans_ichgtime(
> >  		inode->i_ctime = tv;
> >  	if (flags & XFS_ICHGTIME_CREATE)
> >  		ip->i_crtime = tv;
> > +	if (flags & (XFS_ICHGTIME_MOD|XFS_ICHGTIME_CHG))
> > +		inode_inc_iversion(inode);
> >  }
> >  
> >  /*
> > @@ -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);
> >  
> >  	/*
> >  	 * If we're updating the inode core or the timestamps and it's possible
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index 45518b8c613c..162e044c7f56 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -718,6 +718,7 @@ xfs_setattr_nonsize(
> >  	}
> >  
> >  	setattr_copy(mnt_userns, inode, iattr);
> > +	inode_inc_iversion(inode);
> >  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> >  
> >  	XFS_STATS_INC(mp, xs_ig_attrchg);
> > @@ -943,6 +944,7 @@ xfs_setattr_size(
> >  
> >  	ASSERT(!(iattr->ia_valid & (ATTR_UID | ATTR_GID)));
> >  	setattr_copy(mnt_userns, inode, iattr);
> > +	inode_inc_iversion(inode);
> >  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> >  
> >  	XFS_STATS_INC(mp, xs_ig_attrchg);
> > @@ -1047,6 +1049,8 @@ xfs_vn_update_time(
> >  		inode->i_mtime = *now;
> >  	if (flags & S_ATIME)
> >  		inode->i_atime = *now;
> > +	if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false))
> > +		log_flags |= XFS_ILOG_CORE;
> >  
> >  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> >  	xfs_trans_log_inode(tp, ip, log_flags);
> > -- 
> > 2.37.2
> > 

-- 
Jeff Layton <jlayton@kernel.org>

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

[Dave Chinner]

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:
> > > The i_version in xfs_trans_log_inode is bumped for any inode update,
> > > including atime-only updates due to reads. We don't want to record those
> > > in the i_version, as they don't represent "real" changes. Remove that
> > > callsite.
> > > 
> > > In xfs_vn_update_time, if S_VERSION is flagged, then attempt to bump the
> > > i_version and turn on XFS_ILOG_CORE if it happens. In
> > > xfs_trans_ichgtime, update the i_version if the mtime or ctime are being
> > > updated.
> > 
> > What about operations that don't touch the mtime but change the file
> > metadata anyway?  There are a few of those, like the blockgc garbage
> > collector, deduperange, and the defrag tool.
> > 
> 
> Do those change the c/mtime at all?
> 
> It's possible we're missing some places that should change the i_version
> as well. We may need some more call sites.
> 
> > Zooming out a bit -- what does i_version signal, concretely?  I thought
> > it was used by nfs (and maybe ceph?) to signal to clients that the file
> > on the server has moved on, and the client needs to invalidate its
> > caches.  I thought afs had a similar generation counter, though it's
> > only used to cache file data, not metadata?  Does an i_version change
> > cause all of them to invalidate caches, or is there more behavior I
> > don't know about?
> > 
> 
> For NFS, it indicates a change to the change attribute indicates that
> there has been a change to the data or metadata for the file. atime
> changes due to reads are specifically exempted from this, but we do bump
> the i_version if someone (e.g.) changes the atime via utimes(). 

We have relatime behaviour to optimise away unnecessary atime
updates on reads.  Trying to explicitly exclude i_version from atime
updates in one filesystem just because NFS doesn't need that
information seems ....  misguided.  The -on disk- i_version
field behaviour is defined by the filesystem implementation, not the
NFS requirements.

> The NFS client will generally invalidate its caches for the inode when
> it notices a change attribute change.
>
> FWIW, AFS may not meet this standard since it doesn't generally
> increment the counter on metadata changes. It may turn out that we don't
> want to expose this to the AFS client due to that (or maybe come up with
> some way to indicate this difference).

In XFS, we've defined the on-disk i_version field to mean
"increments with any persistent inode data or metadata change",
regardless of what the high level applications that use i_version
might actually require.

That some network filesystem might only need a subset of the
metadata to be covered by i_version is largely irrelevant - if we
don't cover every persistent inode metadata change with i_version,
then applications that *need* stuff like atime change notification
can't be supported.

> > Does that mean that we should bump i_version for any file data or
> > attribute that could be queried or observed by userspace?  In which case
> > I suppose this change is still correct, even if it relaxes i_version
> > updates from "any change to the inode whatsoever" to "any change that
> > would bump mtime".  Unless FIEMAP is part of "attributes observed by
> > userspace".
> > 
> > (The other downside I can see is that now we have to remember to bump
> > timestamps for every new file operation we add, unlike the current code
> > which is centrally located in xfs_trans_log_inode.)
> > 
> 
> The main reason for the change attribute in NFS was that NFSv3 is
> plagued with cache-coherency problems due to coarse-grained timestamp
> granularity. It was conceived as a way to indicate that the inode had
> changed without relying on timestamps.

Yes, and the most important design consideration for a filesystem is
that it -must be persistent-. The constraints on i_version are much
stricter than timestamps, and they are directly related to how the
filesystem persists metadata changes, not how metadata is changed or
accessed in memory.

> In practice, we want to bump the i_version counter whenever the ctime or
> mtime would be changed.

What about O_NOCMTIME modifications? What about lazytime
filesystems? These explicilty avoid or delay persisten c/mtime
updates, and that means bumping i_version only based on c/mtime
updates cannot be relied on. i_version is supposed to track user
visible data and metadata changes, *not timestamp updates*.

> > > Cc: Darrick J. Wong <darrick.wong@oracle.com>
> > > Cc: Dave Chinner <david@fromorbit.com>
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/xfs/libxfs/xfs_trans_inode.c | 17 +++--------------
> > >  fs/xfs/xfs_iops.c               |  4 ++++
> > >  2 files changed, 7 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> > > index 8b5547073379..78bf7f491462 100644
> > > --- a/fs/xfs/libxfs/xfs_trans_inode.c
> > > +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> > > @@ -71,6 +71,8 @@ xfs_trans_ichgtime(
> > >  		inode->i_ctime = tv;
> > >  	if (flags & XFS_ICHGTIME_CREATE)
> > >  		ip->i_crtime = tv;
> > > +	if (flags & (XFS_ICHGTIME_MOD|XFS_ICHGTIME_CHG))
> > > +		inode_inc_iversion(inode);
> > >  }

That looks wrong - this is not the only path through XFS that
modifies timestamps, and I have to ask why this needs to be an
explicit i_version bump given that nobody may have looked at
i_version since the last time it was updated?.

What about xfs_fs_dirty_inode() when we actually persist lazytime
in-memory timestamp updates? We didn't bump i_version when setting
I_DIRTY_TIME, and this patch now removes the mechanism that is used
to bump iversion if it is needed when we persist those lazytime
updates.....

> > >  /*
> > > @@ -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.

IOws, this seems to rely on every future inode modification in XFS
calling xfs_trans_ichgtime() to bump i_version to sweep previous VFS
in-memory timestamp updates that this inode modification captures
and persists to disk.

This seems fragile and error prone - it's relying on the
developers always getting timestamp and iversion updates correct,
rather the code always guaranteeing that it captures timestamp and
iversion updates without any extra effort.

Hence, I don't think that trying to modify how filesystems persist
and maintain i_version coherency because NFS "doesn't need i_version
to cover atime updates" is the wrong approach. On-disk i_version
coherency has to work for more than just one NFS implementation
(especially now i_version will be exported to userspace!). 
Persistent atime updates are already optimised away by relatime, and
so I think that any further atime filtering is largely a NFS
application layer problem and not something that should be solved by
changing the on-disk definition of back end filesystem structure
persistence.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Dave Chinner]

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

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

[Jeff Layton]

On Wed, 2022-08-17 at 08:42 +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:
> > > > The i_version in xfs_trans_log_inode is bumped for any inode update,
> > > > including atime-only updates due to reads. We don't want to record those
> > > > in the i_version, as they don't represent "real" changes. Remove that
> > > > callsite.
> > > > 
> > > > In xfs_vn_update_time, if S_VERSION is flagged, then attempt to bump the
> > > > i_version and turn on XFS_ILOG_CORE if it happens. In
> > > > xfs_trans_ichgtime, update the i_version if the mtime or ctime are being
> > > > updated.
> > > 
> > > What about operations that don't touch the mtime but change the file
> > > metadata anyway?  There are a few of those, like the blockgc garbage
> > > collector, deduperange, and the defrag tool.
> > > 
> > 
> > Do those change the c/mtime at all?
> > 
> > It's possible we're missing some places that should change the i_version
> > as well. We may need some more call sites.
> > 
> > > Zooming out a bit -- what does i_version signal, concretely?  I thought
> > > it was used by nfs (and maybe ceph?) to signal to clients that the file
> > > on the server has moved on, and the client needs to invalidate its
> > > caches.  I thought afs had a similar generation counter, though it's
> > > only used to cache file data, not metadata?  Does an i_version change
> > > cause all of them to invalidate caches, or is there more behavior I
> > > don't know about?
> > > 
> > 
> > For NFS, it indicates a change to the change attribute indicates that
> > there has been a change to the data or metadata for the file. atime
> > changes due to reads are specifically exempted from this, but we do bump
> > the i_version if someone (e.g.) changes the atime via utimes(). 
> 
> We have relatime behaviour to optimise away unnecessary atime
> updates on reads.  Trying to explicitly exclude i_version from atime
> updates in one filesystem just because NFS doesn't need that
> information seems ....  misguided.  The -on disk- i_version
> field behaviour is defined by the filesystem implementation, not the
> NFS requirements.
> 

-o relatime does not fix this.

> > The NFS client will generally invalidate its caches for the inode when
> > it notices a change attribute change.
> > 
> > FWIW, AFS may not meet this standard since it doesn't generally
> > increment the counter on metadata changes. It may turn out that we don't
> > want to expose this to the AFS client due to that (or maybe come up with
> > some way to indicate this difference).
> 
> In XFS, we've defined the on-disk i_version field to mean
> "increments with any persistent inode data or metadata change",
> regardless of what the high level applications that use i_version
> might actually require.
> 
> That some network filesystem might only need a subset of the
> metadata to be covered by i_version is largely irrelevant - if we
> don't cover every persistent inode metadata change with i_version,
> then applications that *need* stuff like atime change notification
> can't be supported.
> 
> > > Does that mean that we should bump i_version for any file data or
> > > attribute that could be queried or observed by userspace?  In which case
> > > I suppose this change is still correct, even if it relaxes i_version
> > > updates from "any change to the inode whatsoever" to "any change that
> > > would bump mtime".  Unless FIEMAP is part of "attributes observed by
> > > userspace".
> > > 
> > > (The other downside I can see is that now we have to remember to bump
> > > timestamps for every new file operation we add, unlike the current code
> > > which is centrally located in xfs_trans_log_inode.)
> > > 
> > 
> > The main reason for the change attribute in NFS was that NFSv3 is
> > plagued with cache-coherency problems due to coarse-grained timestamp
> > granularity. It was conceived as a way to indicate that the inode had
> > changed without relying on timestamps.
> 
> Yes, and the most important design consideration for a filesystem is
> that it -must be persistent-. The constraints on i_version are much
> stricter than timestamps, and they are directly related to how the
> filesystem persists metadata changes, not how metadata is changed or
> accessed in memory.
> 
> > In practice, we want to bump the i_version counter whenever the ctime or
> > mtime would be changed.
> 
> What about O_NOCMTIME modifications? What about lazytime
> filesystems? These explicilty avoid or delay persisten c/mtime
> updates, and that means bumping i_version only based on c/mtime
> updates cannot be relied on. i_version is supposed to track user
> visible data and metadata changes, *not timestamp updates*.
> 

I was speaking more about the sorts of activity that should result in
the i_version being changed, not about tracking timestamp updates. IOW,
if some activity would cause the mtime or ctime to change, then we want
to also bump the i_version.

Specifically, for NOCMTIME, I think we'd still want the i_version to
change since that option is about timestamps and not i_version.

> > > > Cc: Darrick J. Wong <darrick.wong@oracle.com>
> > > > Cc: Dave Chinner <david@fromorbit.com>
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_trans_inode.c | 17 +++--------------
> > > >  fs/xfs/xfs_iops.c               |  4 ++++
> > > >  2 files changed, 7 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> > > > index 8b5547073379..78bf7f491462 100644
> > > > --- a/fs/xfs/libxfs/xfs_trans_inode.c
> > > > +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> > > > @@ -71,6 +71,8 @@ xfs_trans_ichgtime(
> > > >  		inode->i_ctime = tv;
> > > >  	if (flags & XFS_ICHGTIME_CREATE)
> > > >  		ip->i_crtime = tv;
> > > > +	if (flags & (XFS_ICHGTIME_MOD|XFS_ICHGTIME_CHG))
> > > > +		inode_inc_iversion(inode);
> > > >  }
> 
> That looks wrong - this is not the only path through XFS that
> modifies timestamps, and I have to ask why this needs to be an
> explicit i_version bump given that nobody may have looked at
> i_version since the last time it was updated?.
> 
> What about xfs_fs_dirty_inode() when we actually persist lazytime
> in-memory timestamp updates? We didn't bump i_version when setting
> I_DIRTY_TIME, and this patch now removes the mechanism that is used
> to bump iversion if it is needed when we persist those lazytime
> updates.....
> 
> > > >  /*
> > > > @@ -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.
> 
> IOws, this seems to rely on every future inode modification in XFS
> calling xfs_trans_ichgtime() to bump i_version to sweep previous VFS
> in-memory timestamp updates that this inode modification captures
> and persists to disk.
> 
> This seems fragile and error prone - it's relying on the
> developers always getting timestamp and iversion updates correct,
> rather the code always guaranteeing that it captures timestamp and
> iversion updates without any extra effort.
> 
> Hence, I don't think that trying to modify how filesystems persist
> and maintain i_version coherency because NFS "doesn't need i_version
> to cover atime updates" is the wrong approach. On-disk i_version
> coherency has to work for more than just one NFS implementation
> (especially now i_version will be exported to userspace!). 
> Persistent atime updates are already optimised away by relatime, and
> so I think that any further atime filtering is largely a NFS
> application layer problem and not something that should be solved by
> changing the on-disk definition of back end filesystem structure
> persistence.
> 

Fair enough. xfs is not really in my wheelhouse so take this as a patch
that helps illustrate the problem, rather than a serious submission.

There are two consumers of the i_version counter today: the kernel NFS
server and IMA. Having the i_version reflect atime updates due to reads
harms both of those use-cases with unneeded cache invalidations on NFS
and extra measurements on IMA. It would also be problematic for userland
NFS servers such as ganesha if we end up exposing this to userland.

atime updates are really a special case when it comes to metadata (and I
maintain that they are one of the worst ideas in POSIX). The way you're
choosing to define i_version doesn't really work properly for any
current or projected use case. I'd like to see that changed.

If the concern is fragility of the code going forward, then maybe we can
go with a different approach. Would it be possible to just have
xfs_trans_log_inode skip bumping the i_version when the log transaction
is _only_ for an atime update due to reads? Maybe we could add a new
XFS_ILOG_ATIME_UPDATE flag or something and set it the appropriate
codepaths?
-- 
Jeff Layton <jlayton@kernel.org>

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

[Dave Chinner]

On Wed, Aug 17, 2022 at 08:02:23AM -0400, Jeff Layton wrote:
> On Wed, 2022-08-17 at 08:42 +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:
> > > > > The i_version in xfs_trans_log_inode is bumped for any inode update,
> > > > > including atime-only updates due to reads. We don't want to record those
> > > > > in the i_version, as they don't represent "real" changes. Remove that
> > > > > callsite.
> > > > > 
> > > > > In xfs_vn_update_time, if S_VERSION is flagged, then attempt to bump the
> > > > > i_version and turn on XFS_ILOG_CORE if it happens. In
> > > > > xfs_trans_ichgtime, update the i_version if the mtime or ctime are being
> > > > > updated.
> > > > 
> > > > What about operations that don't touch the mtime but change the file
> > > > metadata anyway?  There are a few of those, like the blockgc garbage
> > > > collector, deduperange, and the defrag tool.
> > > > 
> > > 
> > > Do those change the c/mtime at all?
> > > 
> > > It's possible we're missing some places that should change the i_version
> > > as well. We may need some more call sites.
> > > 
> > > > Zooming out a bit -- what does i_version signal, concretely?  I thought
> > > > it was used by nfs (and maybe ceph?) to signal to clients that the file
> > > > on the server has moved on, and the client needs to invalidate its
> > > > caches.  I thought afs had a similar generation counter, though it's
> > > > only used to cache file data, not metadata?  Does an i_version change
> > > > cause all of them to invalidate caches, or is there more behavior I
> > > > don't know about?
> > > > 
> > > 
> > > For NFS, it indicates a change to the change attribute indicates that
> > > there has been a change to the data or metadata for the file. atime
> > > changes due to reads are specifically exempted from this, but we do bump
> > > the i_version if someone (e.g.) changes the atime via utimes(). 
> > 
> > We have relatime behaviour to optimise away unnecessary atime
> > updates on reads.  Trying to explicitly exclude i_version from atime
> > updates in one filesystem just because NFS doesn't need that
> > information seems ....  misguided.  The -on disk- i_version
> > field behaviour is defined by the filesystem implementation, not the
> > NFS requirements.
> 
> -o relatime does not fix this.

So why not fix -o relatime to handle this? That way the fix works
for all filesystems and doesn't require hacking around what the VFS
has told us to do in every filesystem.

i.e. the VFS told us to update atime, we updated atime and that is a
persistent metadata change to the inode. Hence a filesystem with a
persistent change counter has to bump the change counter because
we've been asked by the VFS to make a persistent metadata change.

If you want atime updates to not make persistent changes to on disk
metadata, then change the relatime implementation so that it doesn't
ask the filesystem to update the on-disk atime.

Essentially, what I'm hearing is that NFS wants atime updates to
behave like lazytime, not like relatime. With lazytime, atime always
gets updated in memory, but it is not written back to the filesystem
until a timeout or some other modification is made to the inode or
file data. THe filesystem doesn't bump iversion until the timestamp
gets written back in the lazytime case.

IOWs, we already have a mechanism in the kernel for making atime
updates behave exactly as NFS wants: -o lazytime.

> > > The NFS client will generally invalidate its caches for the inode when
> > > it notices a change attribute change.
> > > 
> > > FWIW, AFS may not meet this standard since it doesn't generally
> > > increment the counter on metadata changes. It may turn out that we don't
> > > want to expose this to the AFS client due to that (or maybe come up with
> > > some way to indicate this difference).
> > 
> > In XFS, we've defined the on-disk i_version field to mean
> > "increments with any persistent inode data or metadata change",
> > regardless of what the high level applications that use i_version
> > might actually require.
> > 
> > That some network filesystem might only need a subset of the
> > metadata to be covered by i_version is largely irrelevant - if we
> > don't cover every persistent inode metadata change with i_version,
> > then applications that *need* stuff like atime change notification
> > can't be supported.
> > 
> > > > Does that mean that we should bump i_version for any file data or
> > > > attribute that could be queried or observed by userspace?  In which case
> > > > I suppose this change is still correct, even if it relaxes i_version
> > > > updates from "any change to the inode whatsoever" to "any change that
> > > > would bump mtime".  Unless FIEMAP is part of "attributes observed by
> > > > userspace".
> > > > 
> > > > (The other downside I can see is that now we have to remember to bump
> > > > timestamps for every new file operation we add, unlike the current code
> > > > which is centrally located in xfs_trans_log_inode.)
> > > > 
> > > 
> > > The main reason for the change attribute in NFS was that NFSv3 is
> > > plagued with cache-coherency problems due to coarse-grained timestamp
> > > granularity. It was conceived as a way to indicate that the inode had
> > > changed without relying on timestamps.
> > 
> > Yes, and the most important design consideration for a filesystem is
> > that it -must be persistent-. The constraints on i_version are much
> > stricter than timestamps, and they are directly related to how the
> > filesystem persists metadata changes, not how metadata is changed or
> > accessed in memory.
> > 
> > > In practice, we want to bump the i_version counter whenever the ctime or
> > > mtime would be changed.
> > 
> > What about O_NOCMTIME modifications? What about lazytime
> > filesystems? These explicilty avoid or delay persisten c/mtime
> > updates, and that means bumping i_version only based on c/mtime
> > updates cannot be relied on. i_version is supposed to track user
> > visible data and metadata changes, *not timestamp updates*.
> 
> I was speaking more about the sorts of activity that should result in
> the i_version being changed, not about tracking timestamp updates. IOW,
> if some activity would cause the mtime or ctime to change, then we want
> to also bump the i_version.
> 
> Specifically, for NOCMTIME, I think we'd still want the i_version to
> change since that option is about timestamps and not i_version.

Exactly my point: this is what XFS currently does. It is also what
your proposed changes break by tying i_version updates to c/mtime
updates.

> > Hence, I don't think that trying to modify how filesystems persist
> > and maintain i_version coherency because NFS "doesn't need i_version
> > to cover atime updates" is the wrong approach. On-disk i_version
> > coherency has to work for more than just one NFS implementation
> > (especially now i_version will be exported to userspace!). 
> > Persistent atime updates are already optimised away by relatime, and
> > so I think that any further atime filtering is largely a NFS
> > application layer problem and not something that should be solved by
> > changing the on-disk definition of back end filesystem structure
> > persistence.
> > 
> 
> Fair enough. xfs is not really in my wheelhouse so take this as a patch
> that helps illustrate the problem, rather than a serious submission.
> 
> There are two consumers of the i_version counter today: the kernel NFS
> server and IMA. Having the i_version reflect atime updates due to reads
> harms both of those use-cases with unneeded cache invalidations on NFS
> and extra measurements on IMA. It would also be problematic for userland
> NFS servers such as ganesha if we end up exposing this to userland.

So you're wanting define an exact behaviour for atime vs I_VERSION
where atime doesn't bump iversion.

However, the definition we baked into the XFS on-disk format is that
the on-disk iversion filed changes for every persistent change made
to the inode. That includes atime updates. We did this for two
reasons - the first is so that we could support any application that
needed change detection, and the second was that knowing how many
changes have occurred to an inode is extremely useful for forensic
purposes (especially given the ability to use O_NOCMTIME to modify
file data).

> atime updates are really a special case when it comes to metadata (and I
> maintain that they are one of the worst ideas in POSIX). The way you're
> choosing to define i_version doesn't really work properly for any
> current or projected use case. I'd like to see that changed.

We chose to do that a decade ago, knowing that it is the
responsibility of the VFS to avoid unnecessary atime updates, not
the responsibility of the filesystem. That was the whole point of
introducing the relatime functionality: fix the problem at the VFS,
not have to work around generic atime behaviour in every filesystem.

> If the concern is fragility of the code going forward, then maybe we can
> go with a different approach. Would it be possible to just have
> xfs_trans_log_inode skip bumping the i_version when the log transaction
> is _only_ for an atime update due to reads? Maybe we could add a new
> XFS_ILOG_ATIME_UPDATE flag or something and set it the appropriate
> codepaths?

No, I don't think this is something we should be hacking around in
individual filesystems. If the VFS tells us we should be updating
atime, we should be updating it and bumping persistent change
counters because *that's what the VFS asked us to do*.

IOWs, if NFS wants atime to be considered "in memory only" as per
the lazytime behaviour, then that behaviour needs to be
supported/changed at the VFS, not at the individual fileystem level.

You could add a subset of SB_LAZYTIME functionality just for atime
updates, handle that entirely within the existing lazytime
infrastructure. THis means the VFS supports non-persistent,
best-effort, non-persistent atime updates directly. The VFS will not
ack filesystems to persist atime updates the moment they are made,
it will tell the filesystem via ->dirty_inode() that it needs to
persist the in-memory timestamps.

This gives all the filesystems the same atime behaviour. If the VFS
is going to expose the persistent change counter to userspace, we
need to have consistent, VFS enforced rules on what is coverred by
the change counter. If atime is not covered by the ipersistent
change counter, then the VFS must not ask the filesystems to persist
atime changes every time atime changes.

Then all filesystems will avoid on-disk atime updates as much as
possible, whilst still gathering them correctly when other
modifications are made.

Until we've got a definition of what this persistent change counter
actually describes and guarantees for userspace, changing filesystem
implementations is premature. And if it is decided that atime is not
going to be considered a "persistent change" by itself, then that
behaviour needs to be encoded at the VFS, not in individual
filesystems....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Jeff Layton]

On Thu, 2022-08-18 at 11:07 +1000, Dave Chinner wrote:
> On Wed, Aug 17, 2022 at 08:02:23AM -0400, Jeff Layton wrote:
> > On Wed, 2022-08-17 at 08:42 +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:
> > > > > > The i_version in xfs_trans_log_inode is bumped for any inode update,
> > > > > > including atime-only updates due to reads. We don't want to record those
> > > > > > in the i_version, as they don't represent "real" changes. Remove that
> > > > > > callsite.
> > > > > > 
> > > > > > In xfs_vn_update_time, if S_VERSION is flagged, then attempt to bump the
> > > > > > i_version and turn on XFS_ILOG_CORE if it happens. In
> > > > > > xfs_trans_ichgtime, update the i_version if the mtime or ctime are being
> > > > > > updated.
> > > > > 
> > > > > What about operations that don't touch the mtime but change the file
> > > > > metadata anyway?  There are a few of those, like the blockgc garbage
> > > > > collector, deduperange, and the defrag tool.
> > > > > 
> > > > 
> > > > Do those change the c/mtime at all?
> > > > 
> > > > It's possible we're missing some places that should change the i_version
> > > > as well. We may need some more call sites.
> > > > 
> > > > > Zooming out a bit -- what does i_version signal, concretely?  I thought
> > > > > it was used by nfs (and maybe ceph?) to signal to clients that the file
> > > > > on the server has moved on, and the client needs to invalidate its
> > > > > caches.  I thought afs had a similar generation counter, though it's
> > > > > only used to cache file data, not metadata?  Does an i_version change
> > > > > cause all of them to invalidate caches, or is there more behavior I
> > > > > don't know about?
> > > > > 
> > > > 
> > > > For NFS, it indicates a change to the change attribute indicates that
> > > > there has been a change to the data or metadata for the file. atime
> > > > changes due to reads are specifically exempted from this, but we do bump
> > > > the i_version if someone (e.g.) changes the atime via utimes(). 
> > > 
> > > We have relatime behaviour to optimise away unnecessary atime
> > > updates on reads.  Trying to explicitly exclude i_version from atime
> > > updates in one filesystem just because NFS doesn't need that
> > > information seems ....  misguided.  The -on disk- i_version
> > > field behaviour is defined by the filesystem implementation, not the
> > > NFS requirements.
> > 
> > -o relatime does not fix this.
> 
> So why not fix -o relatime to handle this? That way the fix works
> for all filesystems and doesn't require hacking around what the VFS
> has told us to do in every filesystem.
> 
> i.e. the VFS told us to update atime, we updated atime and that is a
> persistent metadata change to the inode. Hence a filesystem with a
> persistent change counter has to bump the change counter because
> we've been asked by the VFS to make a persistent metadata change.
> 
> If you want atime updates to not make persistent changes to on disk
> metadata, then change the relatime implementation so that it doesn't
> ask the filesystem to update the on-disk atime.
> 
> Essentially, what I'm hearing is that NFS wants atime updates to
> behave like lazytime, not like relatime. With lazytime, atime always
> gets updated in memory, but it is not written back to the filesystem
> until a timeout or some other modification is made to the inode or
> file data. THe filesystem doesn't bump iversion until the timestamp
> gets written back in the lazytime case.
> 
> IOWs, we already have a mechanism in the kernel for making atime
> updates behave exactly as NFS wants: -o lazytime.
> 

No, that won't help. Both lazytime and relatime don't help anything
since they don't address the fundamental problem, which is that the
i_version changes due to atime updates. They only affect when the atime
gets updated (or when it goes to disk).



> > > > The NFS client will generally invalidate its caches for the inode when
> > > > it notices a change attribute change.
> > > > 
> > > > FWIW, AFS may not meet this standard since it doesn't generally
> > > > increment the counter on metadata changes. It may turn out that we don't
> > > > want to expose this to the AFS client due to that (or maybe come up with
> > > > some way to indicate this difference).
> > > 
> > > In XFS, we've defined the on-disk i_version field to mean
> > > "increments with any persistent inode data or metadata change",
> > > regardless of what the high level applications that use i_version
> > > might actually require.
> > > 
> > > That some network filesystem might only need a subset of the
> > > metadata to be covered by i_version is largely irrelevant - if we
> > > don't cover every persistent inode metadata change with i_version,
> > > then applications that *need* stuff like atime change notification
> > > can't be supported.
> > > 
> > > > > Does that mean that we should bump i_version for any file data or
> > > > > attribute that could be queried or observed by userspace?  In which case
> > > > > I suppose this change is still correct, even if it relaxes i_version
> > > > > updates from "any change to the inode whatsoever" to "any change that
> > > > > would bump mtime".  Unless FIEMAP is part of "attributes observed by
> > > > > userspace".
> > > > > 
> > > > > (The other downside I can see is that now we have to remember to bump
> > > > > timestamps for every new file operation we add, unlike the current code
> > > > > which is centrally located in xfs_trans_log_inode.)
> > > > > 
> > > > 
> > > > The main reason for the change attribute in NFS was that NFSv3 is
> > > > plagued with cache-coherency problems due to coarse-grained timestamp
> > > > granularity. It was conceived as a way to indicate that the inode had
> > > > changed without relying on timestamps.
> > > 
> > > Yes, and the most important design consideration for a filesystem is
> > > that it -must be persistent-. The constraints on i_version are much
> > > stricter than timestamps, and they are directly related to how the
> > > filesystem persists metadata changes, not how metadata is changed or
> > > accessed in memory.
> > > 
> > > > In practice, we want to bump the i_version counter whenever the ctime or
> > > > mtime would be changed.
> > > 
> > > What about O_NOCMTIME modifications? What about lazytime
> > > filesystems? These explicilty avoid or delay persisten c/mtime
> > > updates, and that means bumping i_version only based on c/mtime
> > > updates cannot be relied on. i_version is supposed to track user
> > > visible data and metadata changes, *not timestamp updates*.
> > 
> > I was speaking more about the sorts of activity that should result in
> > the i_version being changed, not about tracking timestamp updates. IOW,
> > if some activity would cause the mtime or ctime to change, then we want
> > to also bump the i_version.
> > 
> > Specifically, for NOCMTIME, I think we'd still want the i_version to
> > change since that option is about timestamps and not i_version.
> 
> Exactly my point: this is what XFS currently does. It is also what
> your proposed changes break by tying i_version updates to c/mtime
> updates.
> 
> > > Hence, I don't think that trying to modify how filesystems persist
> > > and maintain i_version coherency because NFS "doesn't need i_version
> > > to cover atime updates" is the wrong approach. On-disk i_version
> > > coherency has to work for more than just one NFS implementation
> > > (especially now i_version will be exported to userspace!). 
> > > Persistent atime updates are already optimised away by relatime, and
> > > so I think that any further atime filtering is largely a NFS
> > > application layer problem and not something that should be solved by
> > > changing the on-disk definition of back end filesystem structure
> > > persistence.
> > > 
> > 
> > Fair enough. xfs is not really in my wheelhouse so take this as a patch
> > that helps illustrate the problem, rather than a serious submission.
> > 
> > There are two consumers of the i_version counter today: the kernel NFS
> > server and IMA. Having the i_version reflect atime updates due to reads
> > harms both of those use-cases with unneeded cache invalidations on NFS
> > and extra measurements on IMA. It would also be problematic for userland
> > NFS servers such as ganesha if we end up exposing this to userland.
> 
> So you're wanting define an exact behaviour for atime vs I_VERSION
> where atime doesn't bump iversion.
> 
> However, the definition we baked into the XFS on-disk format is that
> the on-disk iversion filed changes for every persistent change made
> to the inode. That includes atime updates. We did this for two
> reasons - the first is so that we could support any application that
> needed change detection, and the second was that knowing how many
> changes have occurred to an inode is extremely useful for forensic
> purposes (especially given the ability to use O_NOCMTIME to modify
> file data).
> 
> > atime updates are really a special case when it comes to metadata (and I
> > maintain that they are one of the worst ideas in POSIX). The way you're
> > choosing to define i_version doesn't really work properly for any
> > current or projected use case. I'd like to see that changed.
> 
> We chose to do that a decade ago, knowing that it is the
> responsibility of the VFS to avoid unnecessary atime updates, not
> the responsibility of the filesystem. That was the whole point of
> introducing the relatime functionality: fix the problem at the VFS,
> not have to work around generic atime behaviour in every filesystem.
> 
> > If the concern is fragility of the code going forward, then maybe we can
> > go with a different approach. Would it be possible to just have
> > xfs_trans_log_inode skip bumping the i_version when the log transaction
> > is _only_ for an atime update due to reads? Maybe we could add a new
> > XFS_ILOG_ATIME_UPDATE flag or something and set it the appropriate
> > codepaths?
> 
> No, I don't think this is something we should be hacking around in
> individual filesystems. If the VFS tells us we should be updating
> atime, we should be updating it and bumping persistent change
> counters because *that's what the VFS asked us to do*.
> 
> IOWs, if NFS wants atime to be considered "in memory only" as per
> the lazytime behaviour, then that behaviour needs to be
> supported/changed at the VFS, not at the individual fileystem level.
> 
> You could add a subset of SB_LAZYTIME functionality just for atime
> updates, handle that entirely within the existing lazytime
> infrastructure. THis means the VFS supports non-persistent,
> best-effort, non-persistent atime updates directly. The VFS will not
> ack filesystems to persist atime updates the moment they are made,
> it will tell the filesystem via ->dirty_inode() that it needs to
> persist the in-memory timestamps.
> 
> This gives all the filesystems the same atime behaviour. If the VFS
> is going to expose the persistent change counter to userspace, we
> need to have consistent, VFS enforced rules on what is coverred by
> the change counter. If atime is not covered by the ipersistent
> change counter, then the VFS must not ask the filesystems to persist
> atime changes every time atime changes.
> 
> Then all filesystems will avoid on-disk atime updates as much as
> possible, whilst still gathering them correctly when other
> modifications are made.
> 
> Until we've got a definition of what this persistent change counter
> actually describes and guarantees for userspace, changing filesystem
> implementations is premature. And if it is decided that atime is not
> going to be considered a "persistent change" by itself, then that
> behaviour needs to be encoded at the VFS, not in individual
> filesystems....
> 
> Cheers,
> 
> Dave.

-- 
Jeff Layton <jlayton@kernel.org>

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

[NeilBrown]

On Wed, 17 Aug 2022, Dave Chinner wrote:
> 
> In XFS, we've defined the on-disk i_version field to mean
> "increments with any persistent inode data or metadata change",
> regardless of what the high level applications that use i_version
> might actually require.
> 
> That some network filesystem might only need a subset of the
> metadata to be covered by i_version is largely irrelevant - if we
> don't cover every persistent inode metadata change with i_version,
> then applications that *need* stuff like atime change notification
> can't be supported.

So what you are saying is that the i_version provided by XFS does not
match the changeid semantics required by NFSv4.  Fair enough.  I guess
we shouldn't use the one to implement the other then.

Maybe we should just go back to using ctime.  ctime is *exactly* what
NFSv4 wants, as long as its granularity is sufficient to catch every
single change.  Presumably XFS doesn't try to ensure this.  How hard
would it be to get any ctime update to add at least one nanosecond?
This would be enabled by a mount option, or possibly be a direct request
from nfsd.

<rant>NFSv4 changeid is really one of the more horrible parts of the
design</rant>

NeilBrown

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

[Dave Chinner]

On Thu, Aug 18, 2022 at 10:34:40AM +1000, NeilBrown wrote:
> On Wed, 17 Aug 2022, Dave Chinner wrote:
> > 
> > In XFS, we've defined the on-disk i_version field to mean
> > "increments with any persistent inode data or metadata change",
> > regardless of what the high level applications that use i_version
> > might actually require.
> > 
> > That some network filesystem might only need a subset of the
> > metadata to be covered by i_version is largely irrelevant - if we
> > don't cover every persistent inode metadata change with i_version,
> > then applications that *need* stuff like atime change notification
> > can't be supported.
> 
> So what you are saying is that the i_version provided by XFS does not
> match the changeid semantics required by NFSv4.  Fair enough.  I guess
> we shouldn't use the one to implement the other then.

True, but there's more nuance to it than that. We can already
provide the NFSv4 requirements without filesystem modification -
just mount the filesytsem with "-o lazytime".

There's *always* a difference between what the filesystem implements
and what an application require. The filesystem implements what the
VFS asks it to do, not what the application needs. The VFS provides
the functionality they applications require, not the filesystems.

atime update filtering has long been VFS functionality - we do it
there so behaviour is common across all filesystems. Therefore, if
the NFS application wants atime to always behave as if lazytime is
enabled, that filtering should be done at the VFS atime filtering
layer.

Why should we duplicate VFS atime update filtering in every
filesystem for the default relatime behaviour when the VFS lazytime
filter already provides this filtering function to everyone?

> Maybe we should just go back to using ctime.  ctime is *exactly* what
> NFSv4 wants, as long as its granularity is sufficient to catch every
> single change.  Presumably XFS doesn't try to ensure this.  How hard
> would it be to get any ctime update to add at least one nanosecond?
> This would be enabled by a mount option, or possibly be a direct request
> from nfsd.

We can't rely on ctime to be changed during a modification because
O_NOCMTIME exists to enable "user invisible" modifications to be
made. On XFS these still bump iversion, so while they are invisible
to the user, they are still tracked by the filesystem and anything
that wants to know if the inode data/metadata changed.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[NeilBrown]

On Thu, 18 Aug 2022, Dave Chinner wrote:
> 
> > Maybe we should just go back to using ctime.  ctime is *exactly* what
> > NFSv4 wants, as long as its granularity is sufficient to catch every
> > single change.  Presumably XFS doesn't try to ensure this.  How hard
> > would it be to get any ctime update to add at least one nanosecond?
> > This would be enabled by a mount option, or possibly be a direct request
> > from nfsd.
> 
> We can't rely on ctime to be changed during a modification because
> O_NOCMTIME exists to enable "user invisible" modifications to be
> made. On XFS these still bump iversion, so while they are invisible
> to the user, they are still tracked by the filesystem and anything
> that wants to know if the inode data/metadata changed.
> 

O_NOCMTIME isn't mentioned in the man page, so it doesn't exist :-(

If they are "user invisible", should they then also be "NFS invisible"?
I think so.
As I understand it, the purpose of O_NOCMTIME is to allow optimisations
- do a lot of writes, then update the mtime, thus reducing latency.  I
think it is perfectly reasonable for all of that to be invisible to NFS.

NeilBrown

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

[Trond Myklebust]

On Thu, 2022-08-18 at 11:52 +1000, NeilBrown wrote:
> On Thu, 18 Aug 2022, Dave Chinner wrote:
> > 
> > > Maybe we should just go back to using ctime.  ctime is *exactly*
> > > what
> > > NFSv4 wants, as long as its granularity is sufficient to catch
> > > every
> > > single change.  Presumably XFS doesn't try to ensure this.  How
> > > hard
> > > would it be to get any ctime update to add at least one
> > > nanosecond?
> > > This would be enabled by a mount option, or possibly be a direct
> > > request
> > > from nfsd.
> > 
> > We can't rely on ctime to be changed during a modification because
> > O_NOCMTIME exists to enable "user invisible" modifications to be
> > made. On XFS these still bump iversion, so while they are invisible
> > to the user, they are still tracked by the filesystem and anything
> > that wants to know if the inode data/metadata changed.
> > 
> 
> O_NOCMTIME isn't mentioned in the man page, so it doesn't exist :-(
> 
> If they are "user invisible", should they then also be "NFS
> invisible"?
> I think so.
> As I understand it, the purpose of O_NOCMTIME is to allow
> optimisations
> - do a lot of writes, then update the mtime, thus reducing latency. 
> I
> think it is perfectly reasonable for all of that to be invisible to
> NFS.

The point is that you can always detect an implicit metadata change by
just reading the attribute that changed. The case of an explicit change
is different, because the application might be changing the value back
to a previous setting. The entire value of ctime is that it allows you
to know not to trust any caches when this might be the case because it
records the fact that there was an explicit data or metadata change
somewhere along the line.

By discarding that information about explicit vs implicit changes, XFS
is making the i_version less useful to applications that need to cache
data and/or metadata. So the real question is: for which real world
applications is this behaviour adding value that could not be derived
through other means?

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

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

[Dave Chinner]

On Thu, Aug 18, 2022 at 11:52:12AM +1000, NeilBrown wrote:
> On Thu, 18 Aug 2022, Dave Chinner wrote:
> > 
> > > Maybe we should just go back to using ctime.  ctime is *exactly* what
> > > NFSv4 wants, as long as its granularity is sufficient to catch every
> > > single change.  Presumably XFS doesn't try to ensure this.  How hard
> > > would it be to get any ctime update to add at least one nanosecond?
> > > This would be enabled by a mount option, or possibly be a direct request
> > > from nfsd.
> > 
> > We can't rely on ctime to be changed during a modification because
> > O_NOCMTIME exists to enable "user invisible" modifications to be
> > made. On XFS these still bump iversion, so while they are invisible
> > to the user, they are still tracked by the filesystem and anything
> > that wants to know if the inode data/metadata changed.
> > 
> 
> O_NOCMTIME isn't mentioned in the man page, so it doesn't exist :-(
> 
> If they are "user invisible", should they then also be "NFS invisible"?
> I think so.

Maybe, but now you're making big assumptions about what is being
done by those operations. Userspace can write whatever it likes,
nothing says that O_NOCMTIME can't change user visible data or
metadata.

> As I understand it, the purpose of O_NOCMTIME is to allow optimisations
> - do a lot of writes, then update the mtime, thus reducing latency.  I
> think it is perfectly reasonable for all of that to be invisible to NFS.

O_NOCMTIME is used by things like HSMs, file defragmenters,
deduplication tools, backup programs, etc to be able to read/write
data and manipulate file layout without modifying user visible
timestamps. i.e. users shouldn't notice that the online defragmenter
defragmented their file. Backup programs shouldn't notice the
defragmenter defragmented the file. 

But having uses of it that don't change user visible data does not
mean it can't be used for changing user visible data. Hence we made
the defensive assumption that O_NOCMTIME was a mechanism that could
be used to hide data changes from forensic analysis. With that in
mind, it was important that the change counter captured changes made
even when O_NOCMTIME was specified to leave behind a breadcrumb to
indicate unexpected changes may had been made to the file.

Yeah, we had lots of different requirements for the XFS on-disk
change counter when we were considering adding it. NFSv4 was one of
the least demanding and least defined requirements; it's taken a
*decade* for this atime issue to be noticed, so I really don't think
there's anything wrong with how XFs has implemented persistent
change counters.

What it tells me is that the VFS needs more appropriate atime
filtering for NFSv4's change attribute requirements....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[NeilBrown]

On Thu, 18 Aug 2022, Dave Chinner wrote:
> On Thu, Aug 18, 2022 at 11:52:12AM +1000, NeilBrown wrote:
> > On Thu, 18 Aug 2022, Dave Chinner wrote:
> > > 
> > > > Maybe we should just go back to using ctime.  ctime is *exactly* what
> > > > NFSv4 wants, as long as its granularity is sufficient to catch every
> > > > single change.  Presumably XFS doesn't try to ensure this.  How hard
> > > > would it be to get any ctime update to add at least one nanosecond?
> > > > This would be enabled by a mount option, or possibly be a direct request
> > > > from nfsd.
> > > 
> > > We can't rely on ctime to be changed during a modification because
> > > O_NOCMTIME exists to enable "user invisible" modifications to be
> > > made. On XFS these still bump iversion, so while they are invisible
> > > to the user, they are still tracked by the filesystem and anything
> > > that wants to know if the inode data/metadata changed.
> > > 
> > 
> > O_NOCMTIME isn't mentioned in the man page, so it doesn't exist :-(
> > 
> > If they are "user invisible", should they then also be "NFS invisible"?
> > I think so.
> 
> Maybe, but now you're making big assumptions about what is being
> done by those operations. Userspace can write whatever it likes,
> nothing says that O_NOCMTIME can't change user visible data or
> metadata.

Nope.  The only assumption I'm making is that if the ctime/mtime don't
change, then it is safe to trust any cached content.  I think that is
broadly assumed in the Posix world.  Anyone who uses O_NOCMTIME must
understand the risks (not currently documented ....) and it must be
assumed they will handled them properly.  We cannot allow the addition
of O_NOCMTIME to make us think "ctime and mtime don't mean what they
used to, we cannot trust them any more".

> But having uses of it that don't change user visible data does not
> mean it can't be used for changing user visible data. Hence we made
> the defensive assumption that O_NOCMTIME was a mechanism that could
> be used to hide data changes from forensic analysis. With that in
> mind, it was important that the change counter captured changes made
> even when O_NOCMTIME was specified to leave behind a breadcrumb to
> indicate unexpected changes may had been made to the file.

Having a breadcrumb seems reasonable.  Calling that breadcrumb "i_version"
might be questionable - though specifications seem to be vague so this
decision is probably defensible.

> 
> Yeah, we had lots of different requirements for the XFS on-disk
> change counter when we were considering adding it. NFSv4 was one of
> the least demanding and least defined requirements; it's taken a
> *decade* for this atime issue to be noticed, so I really don't think
> there's anything wrong with how XFs has implemented persistent
> change counters.
> 
> What it tells me is that the VFS needs more appropriate atime
> filtering for NFSv4's change attribute requirements....

I don't agree with that last point.  I think "atime == mtime" and 
"atime > mtime" are distinctly different states which should recorded.

I think Trond's' observation about implicit updates is on-point.
There is no need to include implicit atime updates in i_version.  If
anyone cares about those they can combine i_version and atime into a
single value.  If that value changes, then something changed, possibly
an implicit atime update.  

Excluding implicit atime updates makes i_version strictly more useful.
It doesn't lose any value and does gain some.

NeilBrown

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

[Jeff Layton]

On Thu, 2022-08-18 at 10:34 +1000, NeilBrown wrote:
> On Wed, 17 Aug 2022, Dave Chinner wrote:
> > 
> > In XFS, we've defined the on-disk i_version field to mean
> > "increments with any persistent inode data or metadata change",
> > regardless of what the high level applications that use i_version
> > might actually require.
> > 
> > That some network filesystem might only need a subset of the
> > metadata to be covered by i_version is largely irrelevant - if we
> > don't cover every persistent inode metadata change with i_version,
> > then applications that *need* stuff like atime change notification
> > can't be supported.
> 
> So what you are saying is that the i_version provided by XFS does not
> match the changeid semantics required by NFSv4.  Fair enough.  I guess
> we shouldn't use the one to implement the other then.
> 
> Maybe we should just go back to using ctime.  ctime is *exactly* what
> NFSv4 wants, as long as its granularity is sufficient to catch every
> single change.  Presumably XFS doesn't try to ensure this.  How hard
> would it be to get any ctime update to add at least one nanosecond?
> This would be enabled by a mount option, or possibly be a direct request
> from nfsd.
> 

I think that would be an unfortunate outcome, but if we can't stop xfs
from bumping the i_version on atime updates, then we may have no choice
but to do so. I suppose we could add a fetch_iversion for xfs that takes
it back to using the ctime.

> <rant>NFSv4 changeid is really one of the more horrible parts of the
> design</rant>
> 

Hah! I was telling Tom Talpey yesterday that I thought that the change
counter was one of the best ideas in NFSv4 and that we should be trying
to get all filesystems to implement it correctly.

The part that does suck about the design is that the original specs
weren't specific enough about its behavior. I think that's been somewhat
remedied in more recent RFCs though.
-- 
Jeff Layton <jlayton@kernel.org>

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

[NeilBrown]

On Thu, 18 Aug 2022, Jeff Layton wrote:
> On Thu, 2022-08-18 at 10:34 +1000, NeilBrown wrote:
> > On Wed, 17 Aug 2022, Dave Chinner wrote:
> > > 
> > > In XFS, we've defined the on-disk i_version field to mean
> > > "increments with any persistent inode data or metadata change",
> > > regardless of what the high level applications that use i_version
> > > might actually require.
> > > 
> > > That some network filesystem might only need a subset of the
> > > metadata to be covered by i_version is largely irrelevant - if we
> > > don't cover every persistent inode metadata change with i_version,
> > > then applications that *need* stuff like atime change notification
> > > can't be supported.
> > 
> > So what you are saying is that the i_version provided by XFS does not
> > match the changeid semantics required by NFSv4.  Fair enough.  I guess
> > we shouldn't use the one to implement the other then.
> > 
> > Maybe we should just go back to using ctime.  ctime is *exactly* what
> > NFSv4 wants, as long as its granularity is sufficient to catch every
> > single change.  Presumably XFS doesn't try to ensure this.  How hard
> > would it be to get any ctime update to add at least one nanosecond?
> > This would be enabled by a mount option, or possibly be a direct request
> > from nfsd.
> > 
> 
> I think that would be an unfortunate outcome, but if we can't stop xfs
> from bumping the i_version on atime updates, then we may have no choice
> but to do so. I suppose we could add a fetch_iversion for xfs that takes
> it back to using the ctime.

"unfortunate" for who I wonder..

I think Trond's argument about not needing implicit updates to be
reflected i_version is sound - as the effective i_version can be
constructed from stored i_version plus all attributes, if you ever want
an effective i_version that covers all implicit changes to attributes.
However I doubt Dave will be convinced.

> 
> > <rant>NFSv4 changeid is really one of the more horrible parts of the
> > design</rant>
> > 
> 
> Hah! I was telling Tom Talpey yesterday that I thought that the change
> counter was one of the best ideas in NFSv4 and that we should be trying
> to get all filesystems to implement it correctly.
> 
> The part that does suck about the design is that the original specs
> weren't specific enough about its behavior. I think that's been somewhat
> remedied in more recent RFCs though.

That's enough bate for me to expand my rant...
The problem with changeid is that it imposes on the filesystem.  You
CANNOT provide a compliant NFSv4 services on a filesystem which has 1
second resolution of time stamps and no internal i_version.  When you
are designing a protocol - particularly one where interoperability is a
focus - making it impossible to export some common (at the time)
filesystems correctly is crazy.

And not at all necessary.  There is a much better way.

When reporting the ctime of a file, the server could add a Bool which is
true if that time is "now" to within the resolution of the timestamp.
If you see a "now" ctime, then you cannot use timestamps to validate any
cached data.  i.e.  a "now" ctime is different to every other ctime,
even another "now" ctime with the same timestamp.
If the filesystem has high resolution timestamps, it will never say
"now" and you get the same semantics as changeid.  If timestamps are low
resolution, then you cannot cache a file while it is changing (unless
you get a read delegation), but you can accurately cache a file as long
as it doesn't change - which is all that changeid gives you anyway.

NeilBrown


> -- 
> Jeff Layton <jlayton@kernel.org>
> 

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

[Trond Myklebust]

On Wed, 2022-08-17 at 08:42 +1000, Dave Chinner wrote:
> 
> In XFS, we've defined the on-disk i_version field to mean
> "increments with any persistent inode data or metadata change",
> regardless of what the high level applications that use i_version
> might actually require.
> 
> That some network filesystem might only need a subset of the
> metadata to be covered by i_version is largely irrelevant - if we
> don't cover every persistent inode metadata change with i_version,
> then applications that *need* stuff like atime change notification
> can't be supported.

OK, I'll bite...

What real world application are we talking about here, and why can't it
just read both the atime + i_version if it cares?

The value of the change attribute lies in the fact that it gives you
ctime semantics without the time resolution limitation.
i.e. if the change attribute has changed, then you know that someone
has explicitly modified either the file data or the file metadata (with
the emphasis being on the word "explicitly").
Implicit changes such as the mtime change due to a write are reflected
only because they are necessarily also accompanied by an explicit
change to the data contents of the file.
Implicit changes, such as the atime changes due to a read are not
reflected in the change attribute because there is no explicit change
being made by an application.

Any implicit change in the metadata can be derived by just reading the
attribute in question: there is only 1 (atime) and it is supposed to be
monotonically increasing, hence is dead simple to detect.



-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

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

[Dave Chinner]

On Thu, Aug 18, 2022 at 01:11:09AM +0000, Trond Myklebust wrote:
> On Wed, 2022-08-17 at 08:42 +1000, Dave Chinner wrote:
> > 
> > In XFS, we've defined the on-disk i_version field to mean
> > "increments with any persistent inode data or metadata change",
> > regardless of what the high level applications that use i_version
> > might actually require.
> > 
> > That some network filesystem might only need a subset of the
> > metadata to be covered by i_version is largely irrelevant - if we
> > don't cover every persistent inode metadata change with i_version,
> > then applications that *need* stuff like atime change notification
> > can't be supported.
> 
> OK, I'll bite...
> 
> What real world application are we talking about here, and why can't it
> just read both the atime + i_version if it cares?

The whole point of i_version is that the aplication can skip the
storage and comparison of individual metadata fields to determine if
anythign changed. If you're going to store multiple fields and
compare them all in addition to the change attribute, then what is
the change attribute actually gaining you?

> The value of the change attribute lies in the fact that it gives you
> ctime semantics without the time resolution limitation.
> i.e. if the change attribute has changed, then you know that someone
> has explicitly modified either the file data or the file metadata (with
> the emphasis being on the word "explicitly").
> Implicit changes such as the mtime change due to a write are reflected
> only because they are necessarily also accompanied by an explicit
> change to the data contents of the file.
> Implicit changes, such as the atime changes due to a read are not
> reflected in the change attribute because there is no explicit change
> being made by an application.

That's the *NFSv4 requirements*, not what people were asking XFS to
support in a persistent change attribute 10-15 years ago. What NFS
required was just one of the inputs at the time, and what we
implemented has kept NFSv4 happy for the past decade. I've mentioned
other requirements elsewhere in the thread.

The problem we're talking about here is essentially a relatime
filtering issue; it's triggering an filesystem update because the
first access after a modification triggers an on-disk atime update
rahter than just storing it in memory.

This is not a filesystem issue - the VFS controls when the on-disk
updates occur, and that what NFSv4 appears to need changed.
If NFS doesn't want the filesystem to bump change counters for
on-disk atime updates, then it should be asking the VFS to keep the
atime updates in memory. e.g. use lazytime semantics.

This way, every filesystem will have the same behaviour, regardless
of how they track/store persistent change count metadata.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Trond Myklebust]

On Thu, 2022-08-18 at 13:37 +1000, Dave Chinner wrote:
> On Thu, Aug 18, 2022 at 01:11:09AM +0000, Trond Myklebust wrote:
> > On Wed, 2022-08-17 at 08:42 +1000, Dave Chinner wrote:
> > > 
> > > In XFS, we've defined the on-disk i_version field to mean
> > > "increments with any persistent inode data or metadata change",
> > > regardless of what the high level applications that use i_version
> > > might actually require.
> > > 
> > > That some network filesystem might only need a subset of the
> > > metadata to be covered by i_version is largely irrelevant - if we
> > > don't cover every persistent inode metadata change with
> > > i_version,
> > > then applications that *need* stuff like atime change
> > > notification
> > > can't be supported.
> > 
> > OK, I'll bite...
> > 
> > What real world application are we talking about here, and why
> > can't it
> > just read both the atime + i_version if it cares?
> 
> The whole point of i_version is that the aplication can skip the
> storage and comparison of individual metadata fields to determine if
> anythign changed. If you're going to store multiple fields and
> compare them all in addition to the change attribute, then what is
> the change attribute actually gaining you?

Information that is not contained in the fields themselves. Such as
metadata about the fact that they were explicitly changed by an
application.

> 
> > The value of the change attribute lies in the fact that it gives
> > you
> > ctime semantics without the time resolution limitation.
> > i.e. if the change attribute has changed, then you know that
> > someone
> > has explicitly modified either the file data or the file metadata
> > (with
> > the emphasis being on the word "explicitly").
> > Implicit changes such as the mtime change due to a write are
> > reflected
> > only because they are necessarily also accompanied by an explicit
> > change to the data contents of the file.
> > Implicit changes, such as the atime changes due to a read are not
> > reflected in the change attribute because there is no explicit
> > change
> > being made by an application.
> 
> That's the *NFSv4 requirements*, not what people were asking XFS to
> support in a persistent change attribute 10-15 years ago. What NFS
> required was just one of the inputs at the time, and what we
> implemented has kept NFSv4 happy for the past decade. I've mentioned
> other requirements elsewhere in the thread

NFS can work with a change attribute that tells it to invalidate its
cache on every read. The only side effect will be that the performance
graph will act as if you were filtering it through a cow's digestive
system...

> The problem we're talking about here is essentially a relatime
> filtering issue; it's triggering an filesystem update because the
> first access after a modification triggers an on-disk atime update
> rahter than just storing it in memory.

No. It's not... You appear to be discarding valuable information about
why the attributes changed. I've been asking you for reasons why, and
you're avoiding the question.

> This is not a filesystem issue - the VFS controls when the on-disk
> updates occur, and that what NFSv4 appears to need changed.
> If NFS doesn't want the filesystem to bump change counters for
> on-disk atime updates, then it should be asking the VFS to keep the
> atime updates in memory. e.g. use lazytime semantics.
> 
> This way, every filesystem will have the same behaviour, regardless
> of how they track/store persistent change count metadata.

Right now, the i_version updates are not exported via any common API,
so any piss poor performance side-effects of the implementation are
pretty much limited to the kernel users (NFS and... ???)

Who do you expect to use this attribute if it were to be exported via
statx() as Jeff is proposing, and why is the XFS behaviour appropriate?
It already differs from the behaviour of both btrfs and NFS, so the
argument that this will magically consolidate behaviour can be ignored.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

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

[Jeff Layton]

On Thu, 2022-08-18 at 04:15 +0000, Trond Myklebust wrote:
> On Thu, 2022-08-18 at 13:37 +1000, Dave Chinner wrote:
> > On Thu, Aug 18, 2022 at 01:11:09AM +0000, Trond Myklebust wrote:
> > > On Wed, 2022-08-17 at 08:42 +1000, Dave Chinner wrote:
> > > > 
> > > > In XFS, we've defined the on-disk i_version field to mean
> > > > "increments with any persistent inode data or metadata change",
> > > > regardless of what the high level applications that use i_version
> > > > might actually require.
> > > > 
> > > > That some network filesystem might only need a subset of the
> > > > metadata to be covered by i_version is largely irrelevant - if we
> > > > don't cover every persistent inode metadata change with
> > > > i_version,
> > > > then applications that *need* stuff like atime change
> > > > notification
> > > > can't be supported.
> > > 
> > > OK, I'll bite...
> > > 
> > > What real world application are we talking about here, and why
> > > can't it
> > > just read both the atime + i_version if it cares?
> > 
> > The whole point of i_version is that the aplication can skip the
> > storage and comparison of individual metadata fields to determine if
> > anythign changed. If you're going to store multiple fields and
> > compare them all in addition to the change attribute, then what is
> > the change attribute actually gaining you?
> 
> Information that is not contained in the fields themselves. Such as
> metadata about the fact that they were explicitly changed by an
> application.
> 
> > 
> > > The value of the change attribute lies in the fact that it gives
> > > you
> > > ctime semantics without the time resolution limitation.
> > > i.e. if the change attribute has changed, then you know that
> > > someone
> > > has explicitly modified either the file data or the file metadata
> > > (with
> > > the emphasis being on the word "explicitly").
> > > Implicit changes such as the mtime change due to a write are
> > > reflected
> > > only because they are necessarily also accompanied by an explicit
> > > change to the data contents of the file.
> > > Implicit changes, such as the atime changes due to a read are not
> > > reflected in the change attribute because there is no explicit
> > > change
> > > being made by an application.
> > 
> > That's the *NFSv4 requirements*, not what people were asking XFS to
> > support in a persistent change attribute 10-15 years ago. What NFS
> > required was just one of the inputs at the time, and what we
> > implemented has kept NFSv4 happy for the past decade. I've mentioned
> > other requirements elsewhere in the thread
> 
> NFS can work with a change attribute that tells it to invalidate its
> cache on every read. The only side effect will be that the performance
> graph will act as if you were filtering it through a cow's digestive
> system...
> 
> > The problem we're talking about here is essentially a relatime
> > filtering issue; it's triggering an filesystem update because the
> > first access after a modification triggers an on-disk atime update
> > rahter than just storing it in memory.
> 
> No. It's not... You appear to be discarding valuable information about
> why the attributes changed. I've been asking you for reasons why, and
> you're avoiding the question.
> 
> > This is not a filesystem issue - the VFS controls when the on-disk
> > updates occur, and that what NFSv4 appears to need changed.
> > If NFS doesn't want the filesystem to bump change counters for
> > on-disk atime updates, then it should be asking the VFS to keep the
> > atime updates in memory. e.g. use lazytime semantics.
> > 
> > This way, every filesystem will have the same behaviour, regardless
> > of how they track/store persistent change count metadata.
> 
> Right now, the i_version updates are not exported via any common API,
> so any piss poor performance side-effects of the implementation are
> pretty much limited to the kernel users (NFS and... ???)
> 
> Who do you expect to use this attribute if it were to be exported via
> statx() as Jeff is proposing, and why is the XFS behaviour appropriate?
> It already differs from the behaviour of both btrfs and NFS, so the
> argument that this will magically consolidate behaviour can be ignored.
> 

Thanks Trond,

That's been exactly the point I've been trying to make. The _only_
consumers of i_version at this time are the kernel's NFS server and IMA.
Both of them will still work with the i_version being updated due to
atime updates, but their performance suffers.

The change I'm proposing should bring xfs in line with other providers
of i_version as well. btrfs already behaves correctly, and I have a
proposed patch for ext4 which should fix it. The ext4 devs seem amenable
to it so far.

Dave, you keep talking about the xfs i_version counter as if there are
other applications already relying on its behavior, but I don't see how
that can be. There is no way for userland applications to fetch the
counter currently.
-- 
Jeff Layton <jlayton@kernel.org>

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

[Dave Chinner]

On Thu, Aug 18, 2022 at 07:03:42AM -0400, Jeff Layton wrote:
> Dave, you keep talking about the xfs i_version counter as if there are
> other applications already relying on its behavior, but I don't see how
> that can be. There is no way for userland applications to fetch the
> counter currently.

You miss the point entirely: the behaviour is defined by the on-disk
format (the di_changecount filed), not the applications that are
using the kernel internal iversion API.

Just because there are no in-kernel users of the di_changecount
field in the XFS inode, it does not mean that it doesn't get used by
applications. Forensic analysis tools that walk filesystem images.

Did you not notice that xfs_trans_log_inode() forces an iversion
update if the inode core is marked for update:

	inode_maybe_inc_iversion(inode, flags & XFS_ILOG_CORE))
					^^^^^^^^^^^^^^^^^^^^^^
					this?

So every modification to the inode core (which almost all inode
modifications will do, except for pure timestamp updates) will bump
the iversion regardless of whether it was queried or not.

I use this information all the time for forensic analysis of broken
filesystem images. There are forensic tools that use expose this
information from filesystem images (e.g. xfs_db) so that we can use
it for forensic analysis.

See the problem? On-disk format di_changecount != NFS change
attribute. We can implement the NFS change attribute with the
existing di_changecount field, but if you want to constrain the
definition of how the NFS change attribute is calculated, we can't
necessarily implement that in di_changecount without changing the
on-disk format definition.

And, yes, this has implications for iversion being exposed to
userspace via statx(), as I've mentioned in reply to the v2 patch
you've posted. iversion is persistent information - you can't just
redefine it's behaviour without some fairly serious knock-on
effects for the subsystems that provide the persistent storage...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Trond Myklebust]

On Tue, 2022-08-23 at 10:05 +1000, Dave Chinner wrote:
> On Thu, Aug 18, 2022 at 07:03:42AM -0400, Jeff Layton wrote:
> > Dave, you keep talking about the xfs i_version counter as if there
> > are
> > other applications already relying on its behavior, but I don't see
> > how
> > that can be. There is no way for userland applications to fetch the
> > counter currently.
> 
> You miss the point entirely: the behaviour is defined by the on-disk
> format (the di_changecount filed), not the applications that are
> using the kernel internal iversion API.
> 
> Just because there are no in-kernel users of the di_changecount
> field in the XFS inode, it does not mean that it doesn't get used by
> applications. Forensic analysis tools that walk filesystem images.
> 
> Did you not notice that xfs_trans_log_inode() forces an iversion
> update if the inode core is marked for update:
> 
>         inode_maybe_inc_iversion(inode, flags & XFS_ILOG_CORE))
>                                         ^^^^^^^^^^^^^^^^^^^^^^
>                                         this?
> 
> So every modification to the inode core (which almost all inode
> modifications will do, except for pure timestamp updates) will bump
> the iversion regardless of whether it was queried or not.
> 
> I use this information all the time for forensic analysis of broken
> filesystem images. There are forensic tools that use expose this
> information from filesystem images (e.g. xfs_db) so that we can use
> it for forensic analysis.
> 
> See the problem? On-disk format di_changecount != NFS change
> attribute. We can implement the NFS change attribute with the
> existing di_changecount field, but if you want to constrain the
> definition of how the NFS change attribute is calculated, we can't
> necessarily implement that in di_changecount without changing the
> on-disk format definition.
> 
> And, yes, this has implications for iversion being exposed to
> userspace via statx(), as I've mentioned in reply to the v2 patch
> you've posted. iversion is persistent information - you can't just
> redefine it's behaviour without some fairly serious knock-on
> effects for the subsystems that provide the persistent storage...
> 

That still doesn't explain why a regular application would want to use
that definition of a version counter.
Your use case of a forensic tool is not a generic use case. It is very
limited to something which needs access to detailed knowledge of the
internals of your transactional filesystem. This isn't even something
that needs to be exposed to the VFS layer through the i_version if the
users are all going to be internal to XFS.

What is being requested was initially driven (yes, over 20 years ago
now, as you've pointed out) by the use case of caching applications, of
which NFS is only one. There are other applications that can benefit
from it. Pretty much anything that is currently using the ctime and/or
mtime, wants a standard that overcomes the time resolution issues that
increasingly affect storage as performance improves. That means
applications such as build utilities, backup/restore utilities, search
utilities, etc... Even 'git' could make use of it.

However none of the above applications need the 'all metadata
transactions' definition that you appear to need for your forensics use
case. All will suffer a performance loss with such a definition. Worst
of all, the application behaviour would vary wildly depending on the
combination of mount options being used (noatime vs nodiratime vs...).

So the point isn't about 'redefining the behaviour of i_version'. It is
about delivering a statx() API that meets the requirements of a number
of applications. If XFS is going to opt in to that API, then it needs
to be able to deliver an attribute that meets the same requirements.
Otherwise, it could choose to opt out.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

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

[David Wysochanski]

On Tue, Aug 16, 2022 at 9:19 AM Jeff Layton <jlayton@kernel.org> wrote:
>
> The i_version in xfs_trans_log_inode is bumped for any inode update,
> including atime-only updates due to reads. We don't want to record those
> in the i_version, as they don't represent "real" changes. Remove that
> callsite.
>
> In xfs_vn_update_time, if S_VERSION is flagged, then attempt to bump the
> i_version and turn on XFS_ILOG_CORE if it happens. In
> xfs_trans_ichgtime, update the i_version if the mtime or ctime are being
> updated.
>
> Cc: Darrick J. Wong <darrick.wong@oracle.com>
> Cc: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_trans_inode.c | 17 +++--------------
>  fs/xfs/xfs_iops.c               |  4 ++++
>  2 files changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> index 8b5547073379..78bf7f491462 100644
> --- a/fs/xfs/libxfs/xfs_trans_inode.c
> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> @@ -71,6 +71,8 @@ xfs_trans_ichgtime(
>                 inode->i_ctime = tv;
>         if (flags & XFS_ICHGTIME_CREATE)
>                 ip->i_crtime = tv;
> +       if (flags & (XFS_ICHGTIME_MOD|XFS_ICHGTIME_CHG))
> +               inode_inc_iversion(inode);
>  }
>
>  /*
> @@ -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);
>
>         /*
>          * If we're updating the inode core or the timestamps and it's possible
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 45518b8c613c..162e044c7f56 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -718,6 +718,7 @@ xfs_setattr_nonsize(
>         }
>
>         setattr_copy(mnt_userns, inode, iattr);
> +       inode_inc_iversion(inode);
>         xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>
>         XFS_STATS_INC(mp, xs_ig_attrchg);
> @@ -943,6 +944,7 @@ xfs_setattr_size(
>
>         ASSERT(!(iattr->ia_valid & (ATTR_UID | ATTR_GID)));
>         setattr_copy(mnt_userns, inode, iattr);
> +       inode_inc_iversion(inode);
>         xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>
>         XFS_STATS_INC(mp, xs_ig_attrchg);
> @@ -1047,6 +1049,8 @@ xfs_vn_update_time(
>                 inode->i_mtime = *now;
>         if (flags & S_ATIME)
>                 inode->i_atime = *now;
> +       if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false))
> +               log_flags |= XFS_ILOG_CORE;
>
>         xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>         xfs_trans_log_inode(tp, ip, log_flags);
> --
> 2.37.2
>

I have a test (details below) that shows an open issue with NFSv4.x +
fscache where an xfs exported filesystem would trigger unnecessary
over the wire READs after a umount/mount cycle of the NFS mount.  I
previously tracked this down to atime updates, but never followed
through on any patch.  Now that Jeff worked it out and this patch is
under review, I built 5.19 vanilla, retested, then built 5.19 + this
patch and verified the problem is fixed.
You can add:
Tested-by: Dave Wysochanski <dwysocha@redhat.com>



# ./t0_bz1913591.sh 4.1 xfs relatime
Setting NFS vers=4.1 filesystem to xfs and mount options relatime,rw
 0. On NFS server, setup export with xfs filesystem on loop device
/dev/loop0 /export/dir1 xfs
rw,seclabel,relatime,attr2,inode64,logbufs=8,logbsize=32k,noquota 0 0
 1. On NFS client, install and enable cachefilesd
 2. On NFS client, mount -o vers=4.1,fsc 127.0.0.1:/export/dir1 /mnt
 3. On NFS client, dd if=/dev/zero of=/mnt/file1.bin bs=4096 count=1
 4. On NFS client, echo 3 > /proc/sys/vm/drop_caches
 5. On NFS client, dd if=/mnt/file1.bin of=/dev/null (read into fscache)
 6. On NFS client, umount /mnt
 7. On NFS client, mount -o vers=4.1,fsc 127.0.0.1:/export/dir1 /mnt
 8. On NFS client, repeat steps 4-5 (read from fscache)
 9. On NFS client, check for READ ops (1st number) > 0 in /proc/self/mountstats
Found 4200 NFS READ and READ_PLUS ops in /proc/self/mountstats > 0
                READ: 1 1 0 220 4200 0 0 1 0
           READ_PLUS: 0 0 0 0 0 0 0 0 0
FAILED TEST ./t0_bz1913591.sh on kernel 5.19.0 with NFS vers=4.1
exported filesystem xfs options relatime,rw


# ./t0_bz1913591.sh 4.1 xfs relatime
Setting NFS vers=4.1 filesystem to xfs and mount options relatime,rw
 0. On NFS server, setup export with xfs filesystem on loop device
/dev/loop0 /export/dir1 xfs
rw,seclabel,relatime,attr2,inode64,logbufs=8,logbsize=32k,noquota 0 0
 1. On NFS client, install and enable cachefilesd
 2. On NFS client, mount -o vers=4.1,fsc 127.0.0.1:/export/dir1 /mnt
 3. On NFS client, dd if=/dev/zero of=/mnt/file1.bin bs=4096 count=1
 4. On NFS client, echo 3 > /proc/sys/vm/drop_caches
 5. On NFS client, dd if=/mnt/file1.bin of=/dev/null (read into fscache)
 6. On NFS client, umount /mnt
 7. On NFS client, mount -o vers=4.1,fsc 127.0.0.1:/export/dir1 /mnt
 8. On NFS client, repeat steps 4-5 (read from fscache)
 9. On NFS client, check for READ ops (1st number) > 0 in /proc/self/mountstats
10. On NFS client, check /proc/fs/fscache/stats fscache reads incrementing
PASSED TEST ./t0_bz1913591.sh on kernel 5.19.0i_version+ with NFS
vers=4.1 exported filesystem xfs options relatime,rw

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

[Dave Chinner]

On Tue, Aug 16, 2022 at 01:14:55PM -0400, David Wysochanski wrote:
> On Tue, Aug 16, 2022 at 9:19 AM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > The i_version in xfs_trans_log_inode is bumped for any inode update,
> > including atime-only updates due to reads. We don't want to record those
> > in the i_version, as they don't represent "real" changes. Remove that
> > callsite.
> >
> > In xfs_vn_update_time, if S_VERSION is flagged, then attempt to bump the
> > i_version and turn on XFS_ILOG_CORE if it happens. In
> > xfs_trans_ichgtime, update the i_version if the mtime or ctime are being
> > updated.
> >
> > Cc: Darrick J. Wong <darrick.wong@oracle.com>
> > Cc: Dave Chinner <david@fromorbit.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_trans_inode.c | 17 +++--------------
> >  fs/xfs/xfs_iops.c               |  4 ++++
> >  2 files changed, 7 insertions(+), 14 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> > index 8b5547073379..78bf7f491462 100644
> > --- a/fs/xfs/libxfs/xfs_trans_inode.c
> > +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> > @@ -71,6 +71,8 @@ xfs_trans_ichgtime(
> >                 inode->i_ctime = tv;
> >         if (flags & XFS_ICHGTIME_CREATE)
> >                 ip->i_crtime = tv;
> > +       if (flags & (XFS_ICHGTIME_MOD|XFS_ICHGTIME_CHG))
> > +               inode_inc_iversion(inode);
> >  }
> >
> >  /*
> > @@ -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);
> >
> >         /*
> >          * If we're updating the inode core or the timestamps and it's possible
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index 45518b8c613c..162e044c7f56 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -718,6 +718,7 @@ xfs_setattr_nonsize(
> >         }
> >
> >         setattr_copy(mnt_userns, inode, iattr);
> > +       inode_inc_iversion(inode);
> >         xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> >
> >         XFS_STATS_INC(mp, xs_ig_attrchg);
> > @@ -943,6 +944,7 @@ xfs_setattr_size(
> >
> >         ASSERT(!(iattr->ia_valid & (ATTR_UID | ATTR_GID)));
> >         setattr_copy(mnt_userns, inode, iattr);
> > +       inode_inc_iversion(inode);
> >         xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> >
> >         XFS_STATS_INC(mp, xs_ig_attrchg);
> > @@ -1047,6 +1049,8 @@ xfs_vn_update_time(
> >                 inode->i_mtime = *now;
> >         if (flags & S_ATIME)
> >                 inode->i_atime = *now;
> > +       if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false))
> > +               log_flags |= XFS_ILOG_CORE;
> >
> >         xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> >         xfs_trans_log_inode(tp, ip, log_flags);
> > --
> > 2.37.2
> >
> 
> I have a test (details below) that shows an open issue with NFSv4.x +
> fscache where an xfs exported filesystem would trigger unnecessary
> over the wire READs after a umount/mount cycle of the NFS mount.  I
> previously tracked this down to atime updates, but never followed
> through on any patch.  Now that Jeff worked it out and this patch is
> under review, I built 5.19 vanilla, retested, then built 5.19 + this
> patch and verified the problem is fixed.

And so the question that needs to be answered is "why isn't relatime
working for this workload to avoid unnecessary atime updates"?

Which then makes me ask "what's changing atime on the server between
client side reads"?

Which then makes me wonder "what's actually changing iversion on the
server?" because I don't think atime is the issue here.

I suspect that Jeff's patch is affecting this test case by removing
iversion updates when the data is written back on the server. i.e.
delayed allocation and unwritten extent conversion will no longer
bump iversion when they log the inode metadata changes associated
with extent allocation to store the data being written.  There may
be other places where Jeff's patch removes implicit iversion
updates, too, so it may not be writeback that is the issue here.

How that impacts on the observed behaviour is dependent on things I
don't know, like what cachefiles is doing in the background,
especially across NFS client unmount/mount cycles. However, this all
makes me think the "atime is updated" behaviour is an observed
symptom of something else changing iversion and/or cmtime between
reads from the server...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Jeff Layton]

On Wed, 2022-08-17 at 09:37 +1000, Dave Chinner wrote:
> On Tue, Aug 16, 2022 at 01:14:55PM -0400, David Wysochanski wrote:
> > On Tue, Aug 16, 2022 at 9:19 AM Jeff Layton <jlayton@kernel.org> wrote:
> > > 
> > > The i_version in xfs_trans_log_inode is bumped for any inode update,
> > > including atime-only updates due to reads. We don't want to record those
> > > in the i_version, as they don't represent "real" changes. Remove that
> > > callsite.
> > > 
> > > In xfs_vn_update_time, if S_VERSION is flagged, then attempt to bump the
> > > i_version and turn on XFS_ILOG_CORE if it happens. In
> > > xfs_trans_ichgtime, update the i_version if the mtime or ctime are being
> > > updated.
> > > 
> > > Cc: Darrick J. Wong <darrick.wong@oracle.com>
> > > Cc: Dave Chinner <david@fromorbit.com>
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/xfs/libxfs/xfs_trans_inode.c | 17 +++--------------
> > >  fs/xfs/xfs_iops.c               |  4 ++++
> > >  2 files changed, 7 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> > > index 8b5547073379..78bf7f491462 100644
> > > --- a/fs/xfs/libxfs/xfs_trans_inode.c
> > > +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> > > @@ -71,6 +71,8 @@ xfs_trans_ichgtime(
> > >                 inode->i_ctime = tv;
> > >         if (flags & XFS_ICHGTIME_CREATE)
> > >                 ip->i_crtime = tv;
> > > +       if (flags & (XFS_ICHGTIME_MOD|XFS_ICHGTIME_CHG))
> > > +               inode_inc_iversion(inode);
> > >  }
> > > 
> > >  /*
> > > @@ -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);
> > > 
> > >         /*
> > >          * If we're updating the inode core or the timestamps and it's possible
> > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > index 45518b8c613c..162e044c7f56 100644
> > > --- a/fs/xfs/xfs_iops.c
> > > +++ b/fs/xfs/xfs_iops.c
> > > @@ -718,6 +718,7 @@ xfs_setattr_nonsize(
> > >         }
> > > 
> > >         setattr_copy(mnt_userns, inode, iattr);
> > > +       inode_inc_iversion(inode);
> > >         xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > > 
> > >         XFS_STATS_INC(mp, xs_ig_attrchg);
> > > @@ -943,6 +944,7 @@ xfs_setattr_size(
> > > 
> > >         ASSERT(!(iattr->ia_valid & (ATTR_UID | ATTR_GID)));
> > >         setattr_copy(mnt_userns, inode, iattr);
> > > +       inode_inc_iversion(inode);
> > >         xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > > 
> > >         XFS_STATS_INC(mp, xs_ig_attrchg);
> > > @@ -1047,6 +1049,8 @@ xfs_vn_update_time(
> > >                 inode->i_mtime = *now;
> > >         if (flags & S_ATIME)
> > >                 inode->i_atime = *now;
> > > +       if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false))
> > > +               log_flags |= XFS_ILOG_CORE;
> > > 
> > >         xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> > >         xfs_trans_log_inode(tp, ip, log_flags);
> > > --
> > > 2.37.2
> > > 
> > 
> > I have a test (details below) that shows an open issue with NFSv4.x +
> > fscache where an xfs exported filesystem would trigger unnecessary
> > over the wire READs after a umount/mount cycle of the NFS mount.  I
> > previously tracked this down to atime updates, but never followed
> > through on any patch.  Now that Jeff worked it out and this patch is
> > under review, I built 5.19 vanilla, retested, then built 5.19 + this
> > patch and verified the problem is fixed.
> 
> And so the question that needs to be answered is "why isn't relatime
> working for this workload to avoid unnecessary atime updates"?
> 
> Which then makes me ask "what's changing atime on the server between
> client side reads"?
> 
> Which then makes me wonder "what's actually changing iversion on the
> server?" because I don't think atime is the issue here.
> 
> I suspect that Jeff's patch is affecting this test case by removing
> iversion updates when the data is written back on the server. i.e.
> delayed allocation and unwritten extent conversion will no longer
> bump iversion when they log the inode metadata changes associated
> with extent allocation to store the data being written.  There may
> be other places where Jeff's patch removes implicit iversion
> updates, too, so it may not be writeback that is the issue here.
> 
> How that impacts on the observed behaviour is dependent on things I
> don't know, like what cachefiles is doing in the background,
> especially across NFS client unmount/mount cycles. However, this all
> makes me think the "atime is updated" behaviour is an observed
> symptom of something else changing iversion and/or cmtime between
> reads from the server...
> 

You may be right here.

What I see with both noatime and relatime is that the first read after a
write to a file results in the i_version being incremented, but then it
doesn't change on subsequent reads. Write to the file again, and the
i_version will get incremented again on the next read and then not again
until there is a write.

-- 
Jeff Layton <jlayton@kernel.org>

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

[Dave Chinner]

On Wed, Aug 17, 2022 at 08:10:22AM -0400, Jeff Layton wrote:
> On Wed, 2022-08-17 at 09:37 +1000, Dave Chinner wrote:
> > On Tue, Aug 16, 2022 at 01:14:55PM -0400, David Wysochanski wrote:
> > > On Tue, Aug 16, 2022 at 9:19 AM Jeff Layton <jlayton@kernel.org> wrote:
> > > I have a test (details below) that shows an open issue with NFSv4.x +
> > > fscache where an xfs exported filesystem would trigger unnecessary
> > > over the wire READs after a umount/mount cycle of the NFS mount.  I
> > > previously tracked this down to atime updates, but never followed
> > > through on any patch.  Now that Jeff worked it out and this patch is
> > > under review, I built 5.19 vanilla, retested, then built 5.19 + this
> > > patch and verified the problem is fixed.
> > 
> > And so the question that needs to be answered is "why isn't relatime
> > working for this workload to avoid unnecessary atime updates"?
> > 
> > Which then makes me ask "what's changing atime on the server between
> > client side reads"?
> > 
> > Which then makes me wonder "what's actually changing iversion on the
> > server?" because I don't think atime is the issue here.
> > 
> > I suspect that Jeff's patch is affecting this test case by removing
> > iversion updates when the data is written back on the server. i.e.
> > delayed allocation and unwritten extent conversion will no longer
> > bump iversion when they log the inode metadata changes associated
> > with extent allocation to store the data being written.  There may
> > be other places where Jeff's patch removes implicit iversion
> > updates, too, so it may not be writeback that is the issue here.
> > 
> > How that impacts on the observed behaviour is dependent on things I
> > don't know, like what cachefiles is doing in the background,
> > especially across NFS client unmount/mount cycles. However, this all
> > makes me think the "atime is updated" behaviour is an observed
> > symptom of something else changing iversion and/or cmtime between
> > reads from the server...
> > 
> 
> You may be right here.
> 
> What I see with both noatime and relatime is that the first read after a
> write to a file results in the i_version being incremented, but then it
> doesn't change on subsequent reads. Write to the file again, and the
> i_version will get incremented again on the next read and then not again
> until there is a write.

Yup, that is exactly the behaviour relatime encodes at the VFS. THe
data write updates mtime (or is it ctime? doesn't matter, though)
and the very next atime access check done by the VFS sees that the
mtime is more recent than the atime, so it asks the filesystem to do
an atime update.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com