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

[Jeff Layton <jlayton@kernel.org>]

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>