Date:   Tue, 23 May 2023 06:56:11 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     Jan Kara <jack@...e.cz>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>,
        "Darrick J. Wong" <djwong@...nel.org>,
        Hugh Dickins <hughd@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dave Chinner <david@...morbit.com>,
        Chuck Lever <chuck.lever@...cle.com>,
        Amir Goldstein <amir73il@...il.com>,
        David Howells <dhowells@...hat.com>,
        Neil Brown <neilb@...e.de>,
        Matthew Wilcox <willy@...radead.org>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        Theodore T'so <tytso@....edu>, Chris Mason <clm@...com>,
        Josef Bacik <josef@...icpanda.com>,
        David Sterba <dsterba@...e.com>,
        Namjae Jeon <linkinjeon@...nel.org>,
        Steve French <sfrench@...ba.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Tom Talpey <tom@...pey.com>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-xfs@...r.kernel.org,
        linux-btrfs@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-mm@...ck.org, linux-nfs@...r.kernel.org,
        linux-cifs@...r.kernel.org
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 <jlayton@...nel.org>
> > 
> > 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 ;)

When we store the new time in the i_ctime field, the flag gets cleared
because at that point we're storing a new (unseen) time.

However, you're correct: if the i_ctime in your above example starts at
the same value that is currently being returned by
ktime_get_coarse_real_ts64, then we'll lose the flag set in statx.

I think the right fix there would be to not update the ctime at all if
it's a coarse grained time, and the value wouldn't have an apparent
change to an observer. That would leave the flag intact.

That does mean we'd need to move to a function that does clock fetch and
assigns it to i_ctime in one go (like you suggest). Something like:

    inode_update_ctime(inode);

How we do that with atomic operations over two values (the tv_sec and
tv_nsec) is a bit tricky. I'll have to think about it.

Christian, given Jan's concerns do you want to drop this series for now
and let me respin it?

Thanks,
-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists