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

["NeilBrown" <neilb@suse.de>]

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