Re: replacement i_version counter for xfs

Darrick J. Wong
2023-01-25T16:33:02Z

On Wed, Jan 25, 2023 at 06:47:12AM -0500, Jeff Layton wrote:
> On Wed, 2023-01-25 at 11:02 +1100, Dave Chinner wrote:
> > On Tue, Jan 24, 2023 at 07:56:09AM -0500, Jeff Layton wrote:
> > > A few months ago, I posted a patch to make xfs not bump its i_version
> > > counter on atime updates. Dave Chinner NAK'ed that patch, mentioning
> > > that xfs would need to replace it with an entirely new field as the
> > > existing counter is used for other purposes and its semantics are set in
> > > stone.
> > >
> > > Has anything been done toward that end?
> >
> > No, because we don't have official specification of the behaviour
> > the nfsd subsystem requires merged into the kernel yet.
> >
>
> Ok. Hopefully that will be addressed in v6.3.
>
> > > Should I file a bug report or something?
> >
> > There's nothing we can really do until the new specification is set
> > in stone. Filing a bug report won't change anything material.
> >
> > As it is, I'm guessing that you desire the behaviour to be as you
> > described in the iversion patchset you just posted. That is
> > effectively:
> >
> > * 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 larger to observers if there was an explicit change to the inode's
> > + * data or metadata since it was last queried.
> >
> > i.e. the definition is changing from *any* metadata or data change
> > to *explicit* metadata/data changes, right? i.e. it should only
> > change when ctime changes?
> >
>
> Yes.
>
> > IIUC the rest of the justification for i_version is that ctime might
> > lack the timestamp granularity to disambiguate sub-timestamp
> > granularity changes, so i_version is needed to bridge that gap.
> >
> > Given that XFS has nanosecond timestamp resolution in the on-disk
> > format, both i_version and ctime changes are journalled, and
> > ctime/i_version will always change at exactly the same time in the
> > same transactions, there are no inherent sub-timestamp granularity
> > problems with ctime within XFS. Any deficiency in ctime resolution
> > comes solely from the granularity of the VFS inode timestamp
> > functions.
> >
> > And so if current_time() was to provide fine-grained nanosecond
> > timestamp resolution for exported XFS filesystems (i.e. use
> > ktime_get_real_ts64() conditionally), then it seems to me that the
> > nfsd i_version function becomes completely redundant.
> >
> > i.e. we are pretty much guaranteed that ctime on exported
> > filesystems will always be different for explicit modifications to
> > the same inode, and hence we can just use ctime as the version
> > change identifier without needing any on-disk format changes at all.
> >
> > And we can optimise away that overhead when the filesystem is not
> > exported by just using the coarse timestamps because there is no
> > need for sub-timer-tick disambiguation of single file
> > modifications....
> >
>
> Ok, so conditional on (maybe) a per fstype flag, and whether the
> filesystem is exported?
>
> It's not trivial to tell whether something is exported though. We
> typically only do that sort of checking within nfsd. That involves an
> upcall into mountd, at a minimum.
>
> I don't think you want to be plumbing calls to exportfs into xfs for
> this. It may be simpler to just add a new on-disk counter and be done
> with it.

Simpler for you, maybe. Ondisk format changes are a PITA to evaluate
and come with a long support burden. We'd also have to write
xfs-specific testcases to ensure that the counter updates according to
specification.

Poking the kernel to provide sub-jiffies timestamp granularity when
required stays within the existing ondisk format, can be added to any
filesystem with sufficient timestamp granularity, and can be the subject
of a generic/ vfs test.

I also wonder if it's even necessary to use ktime_get_real_ts64 in all
cases -- can we sample the coarse granularity timestamp, and only go for
the higher resolution one if the first matches the ctime?

> > Hence it appears to me that with the new i_version specification
> > that there's an avenue out of this problem entirely that is "nfsd
> > needs to use ctime, not i_version". This solution seems generic
> > enough that filesystems with existing on-disk nanosecond timestamp
> > granularity would no longer need explicit on-disk support for the
> > nfsd i_version functionality, yes?
> >
>
> Pretty much.
>
> My understanding has always been that it's not the on-disk format that's
> the limiting factor, but the resolution of in-kernel timestamp sources.
> If ktime_get_real_ts64 has real ns granularity, then that should be
> sufficient (at least for the moment). I'm unclear on the performance
> implications with such a change though.

I bet you can find some arm board or something with a terrible
clocksource that will take forever to produce high resolution timestamps
and get it wrong.

> You had also mentioned a while back that there was some desire for
> femtosecond resolution on timestamps. Does that change the calculus here
> at all? Note that the i_version is not subject to any timestamp
> granularity issues.

I personally don't care to go enlarge xfs timestamps even further to
support sub-ns resolution, but I see the theoretical argument for
needing them on an 8GHz Intel i9-13900KS(paceheater)...

> If you want nfsd to start using the ctime for i_version with xfs, then
> you can just turn off the SB_I_IVERSION flag. You will need to do some
> work though to keep your "special" i_version that also counts atime
> updates working once you turn that off. You'll probably want to do that
> anyway though since the semantics for xfs's version counter are
> different from everyone else's.
>
> If this is what you choose to do for xfs, then the question becomes: who
> is going to do that timestamp rework?
>
> Note that there are two other lingering issues with i_version. Neither
> of these are xfs-specific, but they may inform the changes you want to
> make there:
>
> 1/ the ctime and i_version can roll backward on a crash.
>
> 2/ the ctime and i_version are both currently updated before write data
> is copied to the pagecache. It would be ideal if that were done
> afterward instead. (FWIW, I have some draft patches for btrfs and ext4
> for this, but they need a lot more testing.)

You might also want some means for xfs to tell the vfs that it already
did the timestamp update (because, say, we had to allocate blocks).
I wonder what people will say when we have to run a transaction before
the write to peel off suid bits and another one after to update ctime.

--D

>
> --
> Jeff Layton <jlayton@kernel.org>