Message-ID: <09e1f0dbff56189467c29c1e29214e9c46e3f1fe.camel@kernel.org>
Date: Mon, 28 Jul 2025 08:15:47 -0400
From: Jeff Layton <jlayton@...nel.org>
To: NeilBrown <neil@...wn.name>
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, 2025-07-28 at 11:51 +1000, NeilBrown wrote:
> 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?
> 

I do like the idea of "freezing" timestamp updates until the delegation
is returned. Doing a bit of git-archaeology shows that the NOCMTIME
flag was added here:

commit 4d4be482a4d78ca906f45e99fd9fdb91e907f5ad
Author: Christoph Hellwig <hch@...radead.org>
Date:   Tue Dec 9 04:47:33 2008 -0500

    [XFS] add a FMODE flag to make XFS invisible I/O less hacky
    
    XFS has a mode called invisble I/O that doesn't update any of the
    timestamps.  It's used for HSM-style applications and exposed through
    the nasty open by handle ioctl.
    
    Instead of doing directly assignment of file operations that set an
    internal flag for it add a new FMODE_NOCMTIME flag that we can check
    in the normal file operations.
    
    (addition of the generic VFS flag has been ACKed by Al as an interims
     solution)
    
    Signed-off-by: Christoph Hellwig <hch@....de>
    Signed-off-by: Lachlan McIlroy <lachlan@....com>

Delegated timestamps seems like a similar enough use-case that we can
probably make this work.

The main catch here is that there is no guarantee that the client will
ever follow up with a SETATTR, so (like you mentioned), we'd need a
mechanism to ensure that the cmtime can be updated on close, if the
file is ever written while the delegation is in force and the final
SETATTR never comes in.

I'll do a bit of research an get back to you on whether this is
feasible. Thanks for the suggestion!
-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists