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 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? 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). Certainly nfsd might impose extra restrictions, but I think that basic restriction should by in the VFS close to what ATTR_CTIME_SET is honoured. What way if someone else finds another use for it some day they will have to work within the same restriction (or change it explicitly and try to justify that change). Lustre has the equivalent of ATTR_CTIME_SET (MFS_ATTR_CTIME_SET and LA_CTIME) and would want to use it if the server-side code ever landed upstream. It appears to just assume the client sent a valid timestamp. I would rather it were vetted by the VFS. Thanks, NeilBrown > > Signed-off-by: Jeff Layton <[email protected]> > --- > fs/attr.c | 15 +++++++++------ > include/linux/fs.h | 1 + > 2 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/fs/attr.c b/fs/attr.c > index > 9caf63d20d03e86c535e9c8c91d49c2a34d34b7a..f0dabd2985989d283a931536a5fc53eda366b373 > 100644 > --- a/fs/attr.c > +++ b/fs/attr.c > @@ -463,15 +463,18 @@ int notify_change(struct mnt_idmap *idmap, struct > dentry *dentry, > > now = current_time(inode); > > - attr->ia_ctime = now; > - if (!(ia_valid & ATTR_ATIME_SET)) > - attr->ia_atime = now; > - else > + if (ia_valid & ATTR_ATIME_SET) > attr->ia_atime = timestamp_truncate(attr->ia_atime, inode); > - if (!(ia_valid & ATTR_MTIME_SET)) > - attr->ia_mtime = now; > else > + attr->ia_atime = now; > + if (ia_valid & ATTR_CTIME_SET) > + attr->ia_ctime = timestamp_truncate(attr->ia_ctime, inode); > + else > + attr->ia_ctime = now; > + if (ia_valid & ATTR_MTIME_SET) > attr->ia_mtime = timestamp_truncate(attr->ia_mtime, inode); > + else > + attr->ia_mtime = now; > > if (ia_valid & ATTR_KILL_PRIV) { > error = security_inode_need_killpriv(dentry); > diff --git a/include/linux/fs.h b/include/linux/fs.h > index > 040c0036320fdf87a2379d494ab408a7991875bd..f18f45e88545c39716b917b1378fb7248367b41d > 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -237,6 +237,7 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t > offset, > #define ATTR_ATIME_SET (1 << 7) > #define ATTR_MTIME_SET (1 << 8) > #define ATTR_FORCE (1 << 9) /* Not a change, but a change it */ > +#define ATTR_CTIME_SET (1 << 10) > #define ATTR_KILL_SUID (1 << 11) > #define ATTR_KILL_SGID (1 << 12) > #define ATTR_FILE (1 << 13) > > -- > 2.50.1 > >
