On Tue, 2023-05-23 at 13:01 +0200, Christian Brauner wrote: > On Tue, May 23, 2023 at 06:56:11AM -0400, Jeff Layton wrote: > > 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@xxxxxxxxxx> > > > > > > > > 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? > > I deliberately put it into a vfs.unstable.* branch. I would leave it > there until you send a new one then drop it. If we get lucky the bots > that run on -next will have time to report potential perf issues while > it's not currently causing conflicts. Sounds good to me. Thanks! -- Jeff Layton <jlayton@xxxxxxxxxx>
