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

[Trond Myklebust <trondmy@hammerspace.com>]

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