Re: [PATCH 1/4] vfs: report change attribute in statx for IS_I_VERSION inodes

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2022-08-16 at 08:51 -0700, Darrick J. Wong wrote:
> On Tue, Aug 16, 2022 at 11:32:24AM -0400, Jeff Layton wrote:
> > On Tue, 2022-08-16 at 16:15 +0100, David Howells wrote:
> > > Jeff Layton <jlayton@kernel.org> wrote:
> > > 
> > > > I think we'll just have to ensure that before we expose this for any
> > > > filesystem that it conforms to some minimum standards. i.e.: it must
> > > > change if there are data or metadata changes to the inode, modulo atime
> > > > changes due to reads on regular files or readdir on dirs.
> > > > 
> > > > The local filesystems, ceph and NFS should all be fine. I guess that
> > > > just leaves AFS. If it can't guarantee that, then we might want to avoid
> > > > exposing the counter for it.
> > > 
> > > AFS monotonically increments the counter on data changes; doesn't make any
> > > change for metadata changes (other than the file size).
> > > 
> > > But you can't assume NFS works as per your suggestion as you don't know what's
> > > backing it (it could be AFS, for example - there's a converter for that).
> > > 
> > 
> > In that case, the NFS server must synthesize a proper change attr. The
> > NFS spec mandates that it change on most metadata changes.
> > 
> > > Further, for ordinary disk filesystems, two data changes may get elided and
> > > only increment the counter once.
> > > 
> > 
> > Not a problem as long as nothing queried the counter in between the
> > changes.
> > 
> > > And then there's mmap...
> > > 
> > 
> > Not sure how that matters here.
> > 
> > > It might be better to reduce the scope of your definition and just say that it
> > > must change if there's a data change and may also be changed if there's a
> > > metadata change.
> > > 
> > 
> > I'd prefer that we mandate that it change on metadata changes as well.
> 
> ...in that case, why not leave the i_version bump in
> xfs_trans_log_inode?  That will capture all changes to file data,
> attribues, and metadata. ;)
> 
> 

Because that includes changes to the atime due to reads which should be
specifically omitted. We could still keep that callsite instead, if you
can see some way to exclude those.

In practice, we are using a change to i_version to mean that "something
changed" in the inode, which usually implies a change to the ctime and
mtime.

Trond pointed out that the NFSv4 spec implies that time_access updates
should be omitted from what we consider to be "metadata" here:

https://mailarchive.ietf.org/arch/msg/nfsv4/yrRBMrVwWWDCrgHPAzq_yAEc7BU/

IMA (which is the only other in-kernel consumer of i_version) also wants
the same behavior.

> > That's what most of the in-kernel users want, and what most of the
> > existing filesystems provide. If AFS can't give that guarantee then we
> > can just omit exposing i_version on it.


-- 
Jeff Layton <jlayton@kernel.org>