[permalink] [raw]

Subject: Re: [PATCH v4 2/9] fs: add infrastructure for multigrain inode i_m/ctime

On Tue, 2023-05-23 at 12:17 +0200, Jan Kara wrote:
> On Tue 23-05-23 12:02:40, Jan Kara wrote:
> > On Thu 18-05-23 07:47:35, Jeff Layton wrote:
> > > The VFS always uses coarse-grained timestamp updates for filling out the
> > > ctime and mtime after a change. This has the benefit of allowing
> > > filesystems to optimize away a lot metadata updates, down to around 1
> > > per jiffy, even when a file is under heavy writes.
> > >
> > > Unfortunately, this has always been an issue when we're exporting via
> > > NFSv3, which relies on timestamps to validate caches. Even with NFSv4, a
> > > lot of exported filesystems don't properly support a change attribute
> > > and are subject to the same problems with timestamp granularity. Other
> > > applications have similar issues (e.g backup applications).
> > >
> > > Switching to always using fine-grained timestamps would improve the
> > > situation, but that becomes rather expensive, as the underlying
> > > filesystem will have to log a lot more metadata updates.
> > >
> > > What we need is a way to only use fine-grained timestamps when they are
> > > being actively queried.
> > >
> > > The kernel always stores normalized ctime values, so only the first 30
> > > bits of the tv_nsec field are ever used. Whenever the mtime changes, the
> > > ctime must also change.
> > >
> > > Use the 31st bit of the ctime tv_nsec field to indicate that something
> > > has queried the inode for the i_mtime or i_ctime. When this flag is set,
> > > on the next timestamp update, the kernel can fetch a fine-grained
> > > timestamp instead of the usual coarse-grained one.
> > >
> > > This patch adds the infrastructure this scheme. Filesytems can opt
> > > into it by setting the FS_MULTIGRAIN_TS flag in the fstype.
> > >
> > > Later patches will convert individual filesystems over to use it.
> > >
> > > Signed-off-by: Jeff Layton <[email protected]>
> >
> > So there are two things I dislike about this series because I think they
> > are fragile:
> >
> > 1) If we have a filesystem supporting multigrain ts and someone
> > accidentally directly uses the value of inode->i_ctime, he can get bogus
> > value (with QUERIED flag). This mistake is very easy to do. So I think we
> > should rename i_ctime to something like __i_ctime and always use accessor
> > function for it.
> >
> > 2) As I already commented in a previous version of the series, the scheme
> > with just one flag for both ctime and mtime and flag getting cleared in
> > current_time() relies on the fact that filesystems always do an equivalent
> > of:
> >
> > inode->i_mtime = inode->i_ctime = current_time();
> >
> > Otherwise we can do coarse grained update where we should have done a fine
> > grained one. Filesystems often update timestamps like this but not
> > universally. Grepping shows some instances where only inode->i_mtime is set
> > from current_time() e.g. in autofs or bfs. Again a mistake that is rather
> > easy to make and results in subtle issues. I think this would be also
> > nicely solved by renaming i_ctime to __i_ctime and using a function to set
> > ctime. Mtime could then be updated with inode->i_mtime = ctime_peek().
> >
> > I understand this is quite some churn but a very mechanical one that could
> > be just done with Coccinelle and a few manual fixups. So IMHO it is worth
> > the more robust result.
>
> Also as I'm thinking about it your current scheme is slightly racy. Suppose
> the filesystem does:
>
> CPU1 CPU2
>
> statx()
> inode->i_ctime = current_time()
> current_mg_time()
> nsec = atomic_long_fetch_andnot(QUERIED, &inode->i_ctime.tv_nsec)
> nsec = atomic_long_fetch_or(QUERIED, &inode->i_ctime.tv_nsec)
> if (nsec & QUERIED) - not set
> ktime_get_coarse_real_ts64(&now)
> return timestamp_truncate(now, inode);
> - QUERIED flag in the inode->i_ctime gets overwritten by the assignment
> => we need not update ctime due to granularity although it was queried
>
> One more reason to use explicit function to update inode->i_ctime ;)

Thinking about this some more, I think we can fix the race you pointed
out by just not clearing the queried flag when we fetch the
i_ctime.tv_nsec field when we're updating.

So, instead of atomic_long_fetch_andnot, we'd just want to use an
atomic_long_fetch there, and just let the eventual assignment of
inode->__i_ctime.tv_nsec be what clears the flag.

Any task that goes to update the time during the interim window will
fetch a fine-grained time, but that's what we want anyway.

Since you bring up races though, there are a couple of other things we
should be aware of. Note that both problems exist today too:

1) it's possible for two tasks to race in such a way that the ctime goes
backward. There's no synchronization between tasks doing the updating,
so an older time can overwrite a newer one. I think you'd need a pretty
tight race to observe this though.

2) it's possible to fetch a "torn" timestamp out of the inode.
timespec64 is two words, and we don't order its loads or stores. We
could consider adding a seqcount_t and some barriers and fixing it that
way. I'm not sure it's worth it though, given that we haven't worried
about this in the past.

For now, I propose that we ignore both problems, unless and until we can
prove that they are more than theoretical.
--
Jeff Layton <[email protected]>

2023-06-14 06:33:01

by Christian Brauner

[permalink] [raw]

Subject: Re: [PATCH v4 2/9] fs: add infrastructure for multigrain inode i_m/ctime

On Tue, Jun 13, 2023 at 09:09:29AM -0400, Jeff Layton wrote:
> On Tue, 2023-05-23 at 14:46 +0200, Jan Kara wrote:
> > On Tue 23-05-23 06:40:08, Jeff Layton wrote:
> > > On Tue, 2023-05-23 at 12:02 +0200, Jan Kara wrote:
> > > >
> > > > So there are two things I dislike about this series because I think they
> > > > are fragile:
> > > >
> > > > 1) If we have a filesystem supporting multigrain ts and someone
> > > > accidentally directly uses the value of inode->i_ctime, he can get bogus
> > > > value (with QUERIED flag). This mistake is very easy to do. So I think we
> > > > should rename i_ctime to something like __i_ctime and always use accessor
> > > > function for it.
> > > >
> > >
> > > We could do this, but it'll be quite invasive. We'd have to change any
> > > place that touches i_ctime (and there are a lot of them), even on
> > > filesystems that are not being converted.
> >
> > Yes, that's why I suggested Coccinelle to deal with this.
>
>
> I've done the work to convert all of the accesses of i_ctime into
> accessor functions in the kernel. The current state of it is here:
>
>
> https://git.kernel.org/pub/scm/linux/kernel/git/jlayton/linux.git/commit/?h=ctime
>
> As expected, it touches a lot of code, all over the place. So far I have
> most of the conversion in one giant patch, and I need to split it up
> (probably per-subsystem).

Yeah, you have time since it'll be v6.6 material.

>
> What's the best way to feed this change into mainline? Should I try to
> get subsystem maintainers to pick these up, or are we better off feeding
> this in via a separate branch?

I would prefer if we send them all through the vfs tree since trickle
down conversions are otherwise very painful and potentially very slow.