Message-ID: <ZUGSB4DhY4mNazz6@dread.disaster.area>
Date:   Wed, 1 Nov 2023 10:47:19 +1100
From:   Dave Chinner <david@...morbit.com>
To:     "Darrick J. Wong" <djwong@...nel.org>
Cc:     Jeff Layton <jlayton@...nel.org>,
        Amir Goldstein <amir73il@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Kent Overstreet <kent.overstreet@...ux.dev>,
        Christian Brauner <brauner@...nel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        John Stultz <jstultz@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Stephen Boyd <sboyd@...nel.org>,
        Chandan Babu R <chandan.babu@...cle.com>,
        Theodore Ts'o <tytso@....edu>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>,
        David Sterba <dsterba@...e.com>,
        Hugh Dickins <hughd@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jan Kara <jack@...e.de>, David Howells <dhowells@...hat.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-xfs@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-btrfs@...r.kernel.org, linux-mm@...ck.org,
        linux-nfs@...r.kernel.org
Subject: Re: [PATCH RFC 2/9] timekeeping: new interfaces for multigrain
 timestamp handing

On Tue, Oct 31, 2023 at 04:02:42PM -0700, Darrick J. Wong wrote:
> On Wed, Nov 01, 2023 at 08:57:09AM +1100, Dave Chinner wrote:
> > On Tue, Oct 31, 2023 at 07:29:18AM -0400, Jeff Layton wrote:
> > > On Tue, 2023-10-31 at 09:03 +0200, Amir Goldstein wrote:
> > > > On Tue, Oct 31, 2023 at 3:42 AM Dave Chinner <david@...morbit.com> wrote:
> > e.g. the current code flow for an atime update is:
> > 
> > touch_atime()
> >   atime_needs_update()
> >   <freeze/write protection>
> >   inode_update_time(S_ATIME)
> >     ->update_time(S_ATIME)
> >       <filesystem atime update>
> > 
> > I'd much prefer this to be:
> > 
> > touch_atime()
> >   if (->update_time(S_ATIME)) {
> >     ->update_time(S_ATIME)
> >       xfs_inode_update_time(S_ATIME)
> >         if (atime_needs_update())
> > 	  <filesystem atime update>
> >   } else {
> >     /* run the existing code */
> >   }
> > 
> > Similarly we'd turn file_modified()/file_update_time() inside out,
> > and this then allows the filesystem to add custom timestamp update
> > checks alongside the VFS timestamp update checks.
> > 
> > It would also enable us to untangle the mess that is lazytime, where
> > we have to implement ->update_time to catch lazytime updates and
> > punt them back to generic_update_time(), which then has to check for
> > lazytime again to determine how to dirty and queue the inode.
> > Of course, generic_update_time() also does timespec_equal checks on
> > timestamps to determine if times should be updated, and so we'd
> > probably need overrides on that, too.
> 
> Hmm.  So would the VFS update the incore timestamps of struct inode in
> whatever manner it wants?

That's kind of what I want to avoid - i want the filesystem to
direct the VFS as to the type of checks and modifications it can
make.

e.g. the timestamp comparisons and actions taken need to be
different for a timestamp-with-integrated-change-counter setup. It
doesn't fold neatly into inode_needs_update_time() - it becomes a
branchy, unreadable mess trying to handle all the different
situations.

Hence the VFS could provide two helpers - one for the existing
timestamp format and one for the new integrated change counter
timestamp. The filesystem can then select the right one to call.

And, further, filesystems that have lazytime enabled should be
checking that early to determine what to do. Lazytime specific
helpers would be useful here.

> Could that include incrementing the lower
> bits of i_ctime.tv_nsec for filesystems that advertise a non-1nsec
> granularity but also set a flag that effectively means "but you can use
> the lower tv_nsec bits if you want"?

Certainly. Similar to multi-grain timestamps, I don't see anything
filesystem specific about this mechanism. I think that anyone saying
"it's ok if it's internal to XFS" is still missing the point that
i_version as a VFS construct needs to die.

At most, i_version is only needed for filesystems that don't have
nanosecond timestamp resolution in their on-disk format and so need
some kind of external ctime change counter to provide fine-grained,
sub-timestamp granularity change recording.

> And perhaps after all that, the VFS should decide if a timestamp update
> needs to be persisted (e.g. lazytime/nodiratime/poniesatime) and if so,
> call ->update_time or __mark_inode_dirty?  Then XFS doesn't have to know
> about all the timestamp persistence rules, it just has to follow
> whatever the VFS tells it.

Sure. I'm not suggesting that the filesystem duplicate and encode
all these rules itself.

I'm just saying that it seems completely backwards that the VFS
encode all this generic logic to handle all these separate cases in
a single code path and then provides a callout that allows the
filesystem to override it's decisions (e.g. lazytime) and do
something else.

The filesystem already knows exactly what specific subset of checks
and updates need to be done so call ou tinto the filesystem first
and then run the VFS helpers that do exactly what is needed for
relatime, lazytime, using timestamps with integrated change
counters, etc.

> > Sorting the lazytime mess for internal change counters really needs
> > for all the timestamp updates to be handled in the filesystem, not
> > bounced back and forward between the filesystem and VFS helpers as
> > it currently is, hence I think we need to rework ->update_time to
> > make this all work cleanly.
> 
> (Oh, I guess I proposed sort of the opposite of what you just said.)

Not really, just seems you're thinking about how to code all the
VFS helpers we'd need a bit differently...

Cheers,

Dav.e
-- 
Dave Chinner
david@...morbit.com

Powered by blists - more mailing lists