Message-id: <175366747582.2234665.13002356331033442863@noble.neil.brown.name>
Date: Mon, 28 Jul 2025 11:51:15 +1000
From: "NeilBrown" <neil@...wn.name>
To: "Jeff Layton" <jlayton@...nel.org>
Cc: "Alexander Viro" <viro@...iv.linux.org.uk>,
 "Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>,
 "Steven Rostedt" <rostedt@...dmis.org>,
 "Masami Hiramatsu" <mhiramat@...nel.org>,
 "Mathieu Desnoyers" <mathieu.desnoyers@...icios.com>,
 "Chuck Lever" <chuck.lever@...cle.com>,
 "Olga Kornievskaia" <okorniev@...hat.com>, "Dai Ngo" <Dai.Ngo@...cle.com>,
 "Tom Talpey" <tom@...pey.com>, "Trond Myklebust" <trondmy@...merspace.com>,
 "Anna Schumaker" <anna@...nel.org>, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
 linux-nfs@...r.kernel.org
Subject: Re: [PATCH v3 3/8] vfs: add ATTR_CTIME_SET flag

On Mon, 28 Jul 2025, Jeff Layton wrote:
> On Mon, 2025-07-28 at 10:04 +1000, NeilBrown wrote:
> > On Mon, 28 Jul 2025, Jeff Layton wrote:
> > > When ATTR_ATIME_SET and ATTR_MTIME_SET are set in the ia_valid mask, the
> > > notify_change() logic takes that to mean that the request should set
> > > those values explicitly, and not override them with "now".
> > > 
> > > With the advent of delegated timestamps, similar functionality is needed
> > > for the ctime. Add a ATTR_CTIME_SET flag, and use that to indicate that
> > > the ctime should be accepted as-is. Also, clean up the if statements to
> > > eliminate the extra negatives.
> > 
> > I don't feel entirely comfortable with this.  ctime is a fallback for
> > "has anything changed" - mtime can be changed but ctime is always
> > reliable, controlled by VFS and FS.
> > 
> > Until now.
> > 
> 
> I know. I have many of the same reservations, but the specification is
> pretty clear (now that I understand it better). I don't see a better
> way to do this.
> 
> > I know you aren't exposing this to user-space, but then not doing so
> > blocks user-space file servers from using this functionality.
> > 
> > I see that you also move vetting of the value out of vfs code and into
> > nfsd code.  I don't really understand why you did that.  Maybe nfsd has
> > more information about previous timestamps than the vfs has?
> > 
> 
> Yes. We need to track the timestamps of the inode at the time that the
> delegation was handed out. nfsd is (arguably) in a better position to
> do this than the VFS is. Patch #5 adds this functionality.
> 
> > Anyway I would much prefer that ATTR_CTIME_SET could only change the
> > ctime value to something between the old ctime value and the current
> > time (inclusive).
> > 
> 
> That will be a problem. What you're suggesting is the current status
> quo with the delegated attrs code, and that behavior was the source of
> the problems that we were seeing in the git regression testsuite.
> 
> 
> When git checks out an object, it opens a file, writes to it and then
> stats it so that it can later see whether it changed. If it gets a
> WRITE_ATTRS_DELEG delegation, the client doesn't wait on writeback
> before returning from that stat().
> 
> Then later, we go to do writeback. The mtime and ctime on the server
> get set to the server's local time (which is later than the time that
> git has recorded). Finally, the client does the SETATTR+DELEGRETURN and
> tries to set the timestamps to the same times that git has recorded,
> but those times are too early vs. the current timestamps on the file
> and they get ignored (in accordance with the spec).
> 
> This was the source of my confusion with the spec. When it says
> "original time", it means the timestamps at the time that the
> delegation was created, but I interpreted it the same way you did.
> 
> Unfortunately, if we want to do this, then we have to allow nfsd to set
> the ctime to a time earlier than the current ctime on the inode. I too
> have some reservations with this. This means that applications on the
> server may see the ctime go backward, which I really do not like. 

An alternate approach would be to allow the writeback through a
delegation to skip the mtime/ctime update, possibly making use of
FMODE_NOCMTIME.

It would be nice to have some mechanism in the VFS to ensure there was
an ATTR_CTIME_SET request on any file which had made use of
FMODE_NOCMTIME before that file was closed, else the times would be set
to the time of the close.  That wouldn't be entirely straight forward,
but should be manageable.  (I would allow some way to avoid the ctime
update on close so XFS_IOC_OPENBY_HANDLE could still be supported, but
it would need to be explicit somewhere).

While FMODE_NOCMTIME also distorts the meaning of ctime, I think it is
better than making it too easy for ctime to go backwards.

How would you feel about that approach?

> 
> Interesting. I don't think they have any immediate plans to upstream
> the server (the priority is the client), but having this functionality
> in the VFS would make it easier to integrate.

I think that splitting the client from the server is a non-trivial task
that brings no benefit to anyone.  Any chance I get I advocate
upstreaming both at once.

Thanks,
NeilBrown

Powered by blists - more mailing lists