[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHk-=wixObEhBXM22JDopRdt7Z=tGGuizq66g4RnUmG9toA2DA@mail.gmail.com>
Date: Wed, 18 Oct 2023 12:18:22 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Jeff Layton <jlayton@...nel.org>
Cc: Alexander Viro <viro@...iv.linux.org.uk>,
Christian Brauner <brauner@...nel.org>,
John Stultz <jstultz@...gle.com>,
Thomas Gleixner <tglx@...utronix.de>,
Stephen Boyd <sboyd@...nel.org>,
Chandan Babu R <chandan.babu@...cle.com>,
"Darrick J. Wong" <djwong@...nel.org>,
Dave Chinner <david@...morbit.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>,
Amir Goldstein <amir73il@...il.com>, 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 Wed, 18 Oct 2023 at 10:41, Jeff Layton <jlayton@...nel.org> wrote:
>
> One way to prevent this is to ensure that when we stamp a file with a
> fine-grained timestamp, that we use that value to establish a floor for
> any later timestamp update.
I'm very leery of this.
I don't like how it's using a global time - and a global fine-grained
offset - when different filesystems will very naturally have different
granularities. I also don't like how it's no using that global lock.
Yes, yes, since the use of this all is then gated by the 'is_mgtime()'
thing, any filesystem with big granularities will presumably never set
FS_MGTIME in the first time, and that hides the worst pointless cases.
But it still feels iffy to me.
Also, the whole current_ctime() logic seems wrong. Later (in 4/9), you do this:
static struct timespec64 current_ctime(struct inode *inode)
{
if (is_mgtime(inode))
return current_mgtime(inode);
and current_mgtime() does
if (nsec & I_CTIME_QUERIED) {
ktime_get_real_ts64(&now);
return timestamp_truncate(now, inode);
}
so once the code has set I_CTIME_QUERIED, it will now use the
expensive fine-grained time - even when it makes no sense.
As far as I can tell, there is *never* a reason to get the
fine-grained time if the old inode ctime is already sufficiently far
away.
IOW, my gut feel is that all this logic should always not only be
guarded by FS_MGTIME (like you do seem to do), *and* by "has anybody
even queried this time" - it should *also* always be guarded by "if I
get the coarse-grained time, is that sufficient?"
So I think the current_ctime() logic should be something along the lines of
static struct timespec64 current_ctime(struct inode *inode)
{
struct timespec64 ts64 = current_time(inode);
unsigned long old_ctime_sec = inode->i_ctime_sec;
unsigned int old_ctime_nsec = inode->i_ctime_nsec;
if (ts64.tv_sec != old_ctime_sec)
return ts64;
/*
* No need to check is_mgtime(inode) - the I_CTIME_QUERIED
* flag is only set for mgtime filesystems
*/
if (!(old_ctime_nsec & I_CTIME_QUERIED))
return ts64;
old_ctime_nsec &= ~I_CTIME_QUERIED;
if (ts64.tv_nsec > old_ctime_nsec + inode->i_sb->s_time_gran)
return ts64;
/* Ok, only *now* do we do a finegrained value */
ktime_get_real_ts64(&ts64);
return timestamp_truncate(ts64);
}
or whatever. Make it *very* clear that the finegrained timestamp is
the absolute last option, after we have checked that the regular one
isn't possible.
I dunno.
Linus
Powered by blists - more mailing lists