Re: [PATCH v2 08/92] fs: new helper: simple_rename_timestamp

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2023-07-06 at 08:19 +0900, Damien Le Moal wrote:
> On 7/6/23 03:58, Jeff Layton wrote:
> > A rename potentially involves updating 4 different inode timestamps. Add
> > a function that handles the details sanely, and convert the libfs.c
> > callers to use it.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/libfs.c         | 36 +++++++++++++++++++++++++++---------
> >  include/linux/fs.h |  2 ++
> >  2 files changed, 29 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/libfs.c b/fs/libfs.c
> > index a7e56baf8bbd..9ee79668c909 100644
> > --- a/fs/libfs.c
> > +++ b/fs/libfs.c
> > @@ -692,6 +692,31 @@ int simple_rmdir(struct inode *dir, struct dentry *dentry)
> >  }
> >  EXPORT_SYMBOL(simple_rmdir);
> >  
> > +/**
> > + * simple_rename_timestamp - update the various inode timestamps for rename
> > + * @old_dir: old parent directory
> > + * @old_dentry: dentry that is being renamed
> > + * @new_dir: new parent directory
> > + * @new_dentry: target for rename
> > + *
> > + * POSIX mandates that the old and new parent directories have their ctime and
> > + * mtime updated, and that inodes of @old_dentry and @new_dentry (if any), have
> > + * their ctime updated.
> > + */
> > +void simple_rename_timestamp(struct inode *old_dir, struct dentry *old_dentry,
> > +			     struct inode *new_dir, struct dentry *new_dentry)
> > +{
> > +	struct inode *newino = d_inode(new_dentry);
> > +
> > +	old_dir->i_mtime = inode_set_ctime_current(old_dir);
> > +	if (new_dir != old_dir)
> > +		new_dir->i_mtime = inode_set_ctime_current(new_dir);
> > +	inode_set_ctime_current(d_inode(old_dentry));
> > +	if (newino)
> > +		inode_set_ctime_current(newino);
> > +}
> > +EXPORT_SYMBOL_GPL(simple_rename_timestamp);
> > +
> >  int simple_rename_exchange(struct inode *old_dir, struct dentry *old_dentry,
> >  			   struct inode *new_dir, struct dentry *new_dentry)
> >  {
> > @@ -707,11 +732,7 @@ int simple_rename_exchange(struct inode *old_dir, struct dentry *old_dentry,
> >  			inc_nlink(old_dir);
> >  		}
> >  	}
> > -	old_dir->i_ctime = old_dir->i_mtime =
> > -	new_dir->i_ctime = new_dir->i_mtime =
> > -	d_inode(old_dentry)->i_ctime =
> > -	d_inode(new_dentry)->i_ctime = current_time(old_dir);
> > -
> > +	simple_rename_timestamp(old_dir, old_dentry, new_dir, new_dentry);
> 
> This is somewhat changing the current behavior: before the patch, the mtime and
> ctime of old_dir, new_dir and the inodes associated with the dentries are always
> equal. But given that simple_rename_timestamp() calls inode_set_ctime_current()
> 4 times, the times could potentially be different.
> 
> I am not sure if that is an issue, but it seems that calling
> inode_set_ctime_current() once, recording the "now" time it sets and using that
> value to set all times may be more efficient and preserve the existing behavior.
> 

I don't believe it's an issue. I've seen nothing in the POSIX spec that
mandates that timestamp updates to different inodes involved in an
operation be set to the _same_ value. It just says they must be updated.

It's also hard to believe that any software would depend on this either,
given that it's very inconsistent across filesystems today. AFAICT, this
was mostly done in the past just as a matter of convenience.

The other problem with doing it that way is that it assumes that
current_time(inode) should always return the same value when given
different inodes. Is it really correct to do this?

	inode_set_ctime(dir, inode_set_ctime_current(inode));

"dir" and "inode" are different inodes, after all, and you're setting
dir's timestamp to "inode"'s value. It's not a big deal today since
they're always on the same sb, but the ultimate goal of these changes is
to implement multigrain timestamps. That will mean that fetching a fine-
grained timestamp for an update when the existing mtime or ctime value
has been queried via getattr.

With that change, I think it's best that we treat updates to different
inodes individually, as some of them may require updating with a fine-
grained timestamp and some may not.

> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(simple_rename_exchange);
> > @@ -720,7 +741,6 @@ int simple_rename(struct mnt_idmap *idmap, struct inode *old_dir,
> >  		  struct dentry *old_dentry, struct inode *new_dir,
> >  		  struct dentry *new_dentry, unsigned int flags)
> >  {
> > -	struct inode *inode = d_inode(old_dentry);
> >  	int they_are_dirs = d_is_dir(old_dentry);
> >  
> >  	if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE))
> > @@ -743,9 +763,7 @@ int simple_rename(struct mnt_idmap *idmap, struct inode *old_dir,
> >  		inc_nlink(new_dir);
> >  	}
> >  
> > -	old_dir->i_ctime = old_dir->i_mtime = new_dir->i_ctime =
> > -		new_dir->i_mtime = inode->i_ctime = current_time(old_dir);
> > -
> > +	simple_rename_timestamp(old_dir, old_dentry, new_dir, new_dentry);
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL(simple_rename);
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index bdfbd11a5811..14e38bd900f1 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2979,6 +2979,8 @@ extern int simple_open(struct inode *inode, struct file *file);
> >  extern int simple_link(struct dentry *, struct inode *, struct dentry *);
> >  extern int simple_unlink(struct inode *, struct dentry *);
> >  extern int simple_rmdir(struct inode *, struct dentry *);
> > +void simple_rename_timestamp(struct inode *old_dir, struct dentry *old_dentry,
> > +			     struct inode *new_dir, struct dentry *new_dentry);
> >  extern int simple_rename_exchange(struct inode *old_dir, struct dentry *old_dentry,
> >  				  struct inode *new_dir, struct dentry *new_dentry);
> >  extern int simple_rename(struct mnt_idmap *, struct inode *,
> 

-- 
Jeff Layton <jlayton@kernel.org>