Re: [PATCH] iversion: update comments with info about atime updates

[Trond Myklebust]

On Tue, 2022-08-23 at 08:42 +1000, NeilBrown wrote:
> On Mon, 22 Aug 2022, Jeff Layton wrote:
> > Add an explicit paragraph codifying that atime updates due to reads
> > should not be counted against the i_version counter. None of the
> > existing subsystems that use the i_version want those counted, and
> > there is an easy workaround for those that do.
> > 
> > Cc: NeilBrown <neilb@xxxxxxx>
> > Cc: Trond Myklebust <trondmy@xxxxxxxxxxxxxxx>
> > Cc: Dave Chinner <david@xxxxxxxxxxxxx>
> > Link:
> > https://lore.kernel.org/linux-xfs/166086932784.5425.17134712694961326033@xxxxxxxxxxxxxxxxxxxxx/#t
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> >  include/linux/iversion.h | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/iversion.h b/include/linux/iversion.h
> > index 3bfebde5a1a6..da6cc1cc520a 100644
> > --- a/include/linux/iversion.h
> > +++ b/include/linux/iversion.h
> > @@ -9,8 +9,8 @@
> >   * ---------------------------
> >   * The change attribute (i_version) is mandated by NFSv4 and is
> > mostly for
> >   * knfsd, but is also used for other purposes (e.g. IMA). The
> > i_version must
> > - * appear different to observers if there was a change to the
> > inode's data or
> > - * metadata since it was last queried.
> > + * appear different to observers if there was an explicit change
> > to the inode's
> > + * data or metadata since it was last queried.
> 
> Should rename change the i_version?
> It does not explicitly change data or metadata, though it seems to
> implicitly change the ctime.

Actually, POSIX only requires that the mtime and ctime change on the
source and target directory. There is no requirement that the ctime
change on the file itself, although such a change is permitted by the
spec in order to allow for existing filesystem implementations.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html

I'd prefer not changing the i_version on the file on rename, but could
live with an implementation that copies the ctime behaviour.

> 
> >   *
> >   * Observers see the i_version as a 64-bit number that never
> > decreases. If it
> >   * remains the same since it was last checked, then nothing has
> > changed in the
> > @@ -18,6 +18,12 @@
> >   * anything about the nature or magnitude of the changes from the
> > value, only
> >   * that the inode has changed in some fashion.
> >   *
> > + * Note that atime updates due to reads or similar activity do
> > _not_ represent
> > + * an explicit change to the inode. If the only change is to the
> > atime and it
> > + * wasn't set via utimes() or a similar mechanism, then i_version
> > should not be
> > + * incremented. If an observer cares about atime updates, it
> > should plan to
> > + * fetch and store them in conjunction with the i_version.
> > + *
> 
> If an implicit atime update happened to make the atime go backwards
> (possible, but not common), the updating i_version should be
> permitted,
> and possibly should be preferred.
> 

Maybe.

> NeilBrown
> 
> 
> >   * Not all filesystems properly implement the i_version counter.
> > Subsystems that
> >   * want to use i_version field on an inode should first check
> > whether the
> >   * filesystem sets the SB_I_VERSION flag (usually via the
> > IS_I_VERSION macro).
> > -- 
> > 2.37.2
> > 
> > 

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