Re: [PATCH v2 3/3] hfs: fix to update ctime after rename

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Mon, 2025-05-19 at 10:52 -0600, Yangtao Li wrote:
> Similar to hfsplus, let's update file ctime after the rename
> operation
> in hfs_rename().
> 

Frankly speaking, I don't quite follow why should we update ctime
during the rename operation. Why do we need to do this? What is the
justification of this?

And we still continue to operate by atime [1-4]. Should we do something
with it?

Thanks,
Slava.

[1]
https://elixir.bootlin.com/linux/v6.15/source/fs/hfsplus/inode.c#L519
[2]
https://elixir.bootlin.com/linux/v6.15/source/fs/hfsplus/inode.c#L562
[3]
https://elixir.bootlin.com/linux/v6.15/source/fs/hfsplus/inode.c#L609
[4]
https://elixir.bootlin.com/linux/v6.15/source/fs/hfsplus/inode.c#L644

> W/O patch(xfstest generic/003):
> 
>  +ERROR: access time has not been updated after accessing file1 first
> time
>  +ERROR: access time has not been updated after accessing file2
>  +ERROR: access time has changed after modifying file1
>  +ERROR: change time has not been updated after changing file1
>  +ERROR: access time has not been updated after accessing file3
> second time
>  +ERROR: access time has not been updated after accessing file3 third
> time
> 
> W/ patch(xfstest generic/003):
> 
>  +ERROR: access time has not been updated after accessing file1 first
> time
>  +ERROR: access time has not been updated after accessing file2
>  +ERROR: access time has changed after modifying file1
>  +ERROR: access time has not been updated after accessing file3
> second time
>  +ERROR: access time has not been updated after accessing file3 third
> time
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>  fs/hfs/dir.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/hfs/dir.c b/fs/hfs/dir.c
> index 86a6b317b474..756ea7b895e2 100644
> --- a/fs/hfs/dir.c
> +++ b/fs/hfs/dir.c
> @@ -284,6 +284,7 @@ static int hfs_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 res;
>  
>  	if (flags & ~RENAME_NOREPLACE)
> @@ -296,14 +297,16 @@ static int hfs_rename(struct mnt_idmap *idmap,
> struct inode *old_dir,
>  			return res;
>  	}
>  
> -	res = hfs_cat_move(d_inode(old_dentry)->i_ino,
> -			   old_dir, &old_dentry->d_name,
> +	res = hfs_cat_move(inode->i_ino, old_dir, &old_dentry-
> >d_name,
>  			   new_dir, &new_dentry->d_name);
> -	if (!res)
> -		hfs_cat_build_key(old_dir->i_sb,
> -				  (btree_key
> *)&HFS_I(d_inode(old_dentry))->cat_key,
> -				  new_dir->i_ino, &new_dentry-
> >d_name);
> -	return res;
> +	if (res)
> +		return res;
> +
> +	hfs_cat_build_key(old_dir->i_sb, (btree_key *)&HFS_I(inode)-
> >cat_key,
> +			  new_dir->i_ino, &new_dentry->d_name);
> +	inode_set_ctime_current(inode);
> +	mark_inode_dirty(inode);
> +	return 0;
>  }
>  
>  const struct file_operations hfs_dir_operations = {