[PATCH v4 1/3] hfsplus: fix to update ctime after rename

[PATCH v4 1/3] hfsplus: fix to update ctime after rename

[Yangtao Li]

[BUG]
$ sudo ./check generic/003
FSTYP         -- hfsplus
PLATFORM      -- Linux/x86_64 graphic 6.8.0-58-generic #60~22.04.1-Ubuntu
MKFS_OPTIONS  -- /dev/loop29
MOUNT_OPTIONS -- /dev/loop29 /mnt/scratch

generic/003       - output mismatch
    --- tests/generic/003.out   2025-04-27 08:49:39.876945323 -0600
    +++ /home/graphic/fs/xfstests-dev/results//generic/003.out.bad

     QA output created by 003
    +ERROR: change time has not been updated after changing file1
     Silence is golden
    ...

Ran: generic/003
Failures: generic/003
Failed 1 of 1 tests

[CAUSE]
change time has not been updated after changing file1

[FIX]
Update file ctime after rename in hfsplus_rename().

Signed-off-by: Yangtao Li <frank.li@vivo.com>
Tested-by: Viacheslav Dubeyko <slava@dubeyko.com>
Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>
---
 fs/hfsplus/dir.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index 876bbb80fb4d..e77942440240 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -534,6 +534,7 @@ static int hfsplus_rename(struct mnt_idmap *idmap,
 			  struct inode *new_dir, struct dentry *new_dentry,
 			  unsigned int flags)
 {
+	struct inode *inode = d_inode(old_dentry);
 	int res;
 
 	if (flags & ~RENAME_NOREPLACE)
@@ -552,9 +553,13 @@ static int hfsplus_rename(struct mnt_idmap *idmap,
 	res = hfsplus_rename_cat((u32)(unsigned long)old_dentry->d_fsdata,
 				 old_dir, &old_dentry->d_name,
 				 new_dir, &new_dentry->d_name);
-	if (!res)
-		new_dentry->d_fsdata = old_dentry->d_fsdata;
-	return res;
+	if (res)
+		return res;
+
+	new_dentry->d_fsdata = old_dentry->d_fsdata;
+	inode_set_ctime_current(inode);
+	mark_inode_dirty(inode);
+	return 0;
 }
 
 const struct inode_operations hfsplus_dir_inode_operations = {
-- 
2.48.1

[PATCH v4 2/3] hfs: correct superblock flags

[Yangtao Li]

We don't support atime updates of any kind,
because hfs actually does not have atime.

   dirCrDat:      LongInt;    {date and time of creation}
   dirMdDat:      LongInt;    {date and time of last modification}
   dirBkDat:      LongInt;    {date and time of last backup}

   filCrDat:      LongInt;    {date and time of creation}
   filMdDat:      LongInt;    {date and time of last modification}
   filBkDat:      LongInt;    {date and time of last backup}

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
v4:
-add both SB_NODIRATIME and SB_NOATIME flags
 fs/hfs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index fe09c2093a93..417950d388b4 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -331,7 +331,7 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
 	sbi->sb = sb;
 	sb->s_op = &hfs_super_operations;
 	sb->s_xattr = hfs_xattr_handlers;
-	sb->s_flags |= SB_NODIRATIME;
+	sb->s_flags |= SB_NODIRATIME | SB_NOATIME;
 	mutex_init(&sbi->bitmap_lock);
 
 	res = hfs_mdb_get(sb);
-- 
2.48.1

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

[Yangtao Li]

Similar to hfsplus, let's update file ctime after the rename operation
in hfs_rename().

W/ patch, the following error in xfstest generic/003 disappears:

 +ERROR: change time has not been updated after changing file1

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
v4:
-update commit msg
 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 = {
-- 
2.48.1

Re: [PATCH v4 1/3] hfsplus: fix to update ctime after rename

[Viacheslav Dubeyko]

On Tue, 2025-07-22 at 01:13 -0600, Yangtao Li wrote:
> [BUG]
> $ sudo ./check generic/003
> FSTYP         -- hfsplus
> PLATFORM      -- Linux/x86_64 graphic 6.8.0-58-generic #60~22.04.1-
> Ubuntu
> MKFS_OPTIONS  -- /dev/loop29
> MOUNT_OPTIONS -- /dev/loop29 /mnt/scratch
> 
> generic/003       - output mismatch
>     --- tests/generic/003.out   2025-04-27 08:49:39.876945323 -0600
>     +++ /home/graphic/fs/xfstests-dev/results//generic/003.out.bad
> 
>      QA output created by 003
>     +ERROR: change time has not been updated after changing file1
>      Silence is golden
>     ...
> 
> Ran: generic/003
> Failures: generic/003
> Failed 1 of 1 tests
> 
> [CAUSE]
> change time has not been updated after changing file1
> 
> [FIX]
> Update file ctime after rename in hfsplus_rename().
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> Tested-by: Viacheslav Dubeyko <slava@dubeyko.com>
> Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>
> ---
>  fs/hfsplus/dir.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 

Probably, it was not very good idea to combine the HFS+ patch with HFS
patches, because I cannot take this one without others. :)

Also, from my point of view, the patchset requires a cover letter.
Otherwise, it looks slightly unusual. :)

Thanks,
Slava.

Re: [PATCH v4 2/3] hfs: correct superblock flags

[Viacheslav Dubeyko]

On Tue, 2025-07-22 at 01:13 -0600, Yangtao Li wrote:
> We don't support atime updates of any kind,
> because hfs actually does not have atime.
> 
>    dirCrDat:      LongInt;    {date and time of creation}
>    dirMdDat:      LongInt;    {date and time of last modification}
>    dirBkDat:      LongInt;    {date and time of last backup}
> 
>    filCrDat:      LongInt;    {date and time of creation}
>    filMdDat:      LongInt;    {date and time of last modification}
>    filBkDat:      LongInt;    {date and time of last backup}
> 

I have troubles with the current state of the comment. If I am trying
to find dirCrDat, dirMdDat, ..., filBkDat in HFS driver source code,
then I cannot find it. So, I prefer to cite the HFS declaration here:

/* The catalog record for a file */
struct hfs_cat_file {
<skipped>
	__be32 CrDat;			/* The creation date */
	__be32 MdDat;			/* The modified date */
	__be32 BkDat;			/* The last backup date */
<skipped>
} __packed;

/* the catalog record for a directory */
struct hfs_cat_dir {
<skipped>
	__be32 CrDat;			/* The creation date */
	__be32 MdDat;			/* The modification date */
	__be32 BkDat;			/* The last backup date */
<skipped>
} __packed;

I assume that you showing information from HFS specification. So, it
makes sense to mention that you are using the HFS specification details
and, maybe, share more detailed explanation of this. Let's combine HFS
driver declarations and citation of HFS specification.

The rest looks pretty good to me.

Thanks,
Slava.

> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
> v4:
> -add both SB_NODIRATIME and SB_NOATIME flags
>  fs/hfs/super.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
> index fe09c2093a93..417950d388b4 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -331,7 +331,7 @@ static int hfs_fill_super(struct super_block *sb,
> struct fs_context *fc)
>  	sbi->sb = sb;
>  	sb->s_op = &hfs_super_operations;
>  	sb->s_xattr = hfs_xattr_handlers;
> -	sb->s_flags |= SB_NODIRATIME;
> +	sb->s_flags |= SB_NODIRATIME | SB_NOATIME;
>  	mutex_init(&sbi->bitmap_lock);
>  
>  	res = hfs_mdb_get(sb);

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

[Viacheslav Dubeyko]

On Tue, 2025-07-22 at 01:13 -0600, Yangtao Li wrote:
> Similar to hfsplus, let's update file ctime after the rename
> operation
> in hfs_rename().
> 

I am not completely happy about mentioning HFS+ in the patch for HFS.
:) We make this fix because HFS should work in correct way but not
because HFS+ does it. Imagine that HFS+ will completely disappear or
HFS+ code will be heavily changed. Nobody will be able to follow this
comment. I prefer to see the explanation something like "The file ctime
should be updated after rename operation and blah, blah, blah". :)  

> W/ patch, the following error in xfstest generic/003 disappears:
> 
>  +ERROR: change time has not been updated after changing file1
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
> v4:
> -update commit msg
>  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 = {

Looks good.

Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Slava.

Re: [PATCH v4 1/3] hfsplus: fix to update ctime after rename

[Al Viro]

On Tue, Jul 22, 2025 at 01:13:45AM -0600, Yangtao Li wrote:

> @@ -552,9 +553,13 @@ static int hfsplus_rename(struct mnt_idmap *idmap,
>  	res = hfsplus_rename_cat((u32)(unsigned long)old_dentry->d_fsdata,
>  				 old_dir, &old_dentry->d_name,
>  				 new_dir, &new_dentry->d_name);
> -	if (!res)
> -		new_dentry->d_fsdata = old_dentry->d_fsdata;
> -	return res;
> +	if (res)
> +		return res;
> +
> +	new_dentry->d_fsdata = old_dentry->d_fsdata;

	Umm...  Is that assignment (either before or after that patch)
actually correct?

	Note that new_dentry essentially got unlinked here; old_dentry
is about to have its parent/name changed by the caller of ->rename(),
so...  that looks very odd.

	What is that line about?

RE: [PATCH v4 1/3] hfsplus: fix to update ctime after rename

[Viacheslav Dubeyko]

On Wed, 2025-07-23 at 03:32 +0100, Al Viro wrote:
> On Tue, Jul 22, 2025 at 01:13:45AM -0600, Yangtao Li wrote:
> 
> > @@ -552,9 +553,13 @@ static int hfsplus_rename(struct mnt_idmap *idmap,
> >  	res = hfsplus_rename_cat((u32)(unsigned long)old_dentry->d_fsdata,
> >  				 old_dir, &old_dentry->d_name,
> >  				 new_dir, &new_dentry->d_name);
> > -	if (!res)
> > -		new_dentry->d_fsdata = old_dentry->d_fsdata;
> > -	return res;
> > +	if (res)
> > +		return res;
> > +
> > +	new_dentry->d_fsdata = old_dentry->d_fsdata;
> 
> 	Umm...  Is that assignment (either before or after that patch)
> actually correct?
> 
> 	Note that new_dentry essentially got unlinked here; old_dentry
> is about to have its parent/name changed by the caller of ->rename(),
> so...  that looks very odd.
> 
> 	What is that line about?

So, as far as I can see, the dentry structure [1] has:

void *d_fsdata;			/* fs-specific data */

And HFS+ is using this field to store Catalog File's ID (CNID) of
the file or folder, for example [2]:

static inline void hfsplus_instantiate(struct dentry *dentry,
				       struct inode *inode, u32 cnid)
{
	dentry->d_fsdata = (void *)(unsigned long)cnid;
	d_instantiate(dentry, inode);
}

So, this line simply copies CNID from old_dentry->d_fsdata to
new_dentry->d_fsdata during the rename operation. I assume that
->fs_data should be untouched by generic logic of dentries processing.

Thanks,
Slava.

[1]
https://elixir.bootlin.com/linux/v6.16-rc6/source/include/linux/dcache.h#L108
[2] 
https://elixir.bootlin.com/linux/v6.16-rc7/source/fs/hfsplus/dir.c#L25

Re: [PATCH v4 1/3] hfsplus: fix to update ctime after rename

[Al Viro]

On Wed, Jul 23, 2025 at 05:58:01PM +0000, Viacheslav Dubeyko wrote:

> So, this line simply copies CNID from old_dentry->d_fsdata to
> new_dentry->d_fsdata during the rename operation. I assume that
> ->fs_data should be untouched by generic logic of dentries processing.

Yes, I understand that; what I do not understand is why.  Why would
the CNID of renamed object be slapped on dentry of removed target?
I'm trying to understand the logics with link(2) and unlink-of-opened
in that code...

Incidentally, what happens if you
	fd = creat("foo", 0666);
	write(fd, "foo", 3);
	link("foo", "bar");
	unlink("bar");
	close(fd);
The games with S_DEAD in there look odd...

RE: [PATCH v4 1/3] hfsplus: fix to update ctime after rename

[Viacheslav Dubeyko]

On Wed, 2025-07-23 at 22:25 +0100, Al Viro wrote:
> On Wed, Jul 23, 2025 at 05:58:01PM +0000, Viacheslav Dubeyko wrote:
> 
> > So, this line simply copies CNID from old_dentry->d_fsdata to
> > new_dentry->d_fsdata during the rename operation. I assume that
> > ->fs_data should be untouched by generic logic of dentries processing.
> 
> Yes, I understand that; what I do not understand is why.  Why would
> the CNID of renamed object be slapped on dentry of removed target?
> I'm trying to understand the logics with link(2) and unlink-of-opened
> in that code...
> 
> Incidentally, what happens if you
> 	fd = creat("foo", 0666);
> 	write(fd, "foo", 3);
> 	link("foo", "bar");
> 	unlink("bar");
> 	close(fd);
> The games with S_DEAD in there look odd...

Probably, I am missing something in your course of logic. :)

I assume that you are worried about this part:

	/* Unlink destination if it already exists */
	if (d_really_is_positive(new_dentry)) {
		if (d_is_dir(new_dentry))
			res = hfsplus_rmdir(new_dir, new_dentry);
		else
			res = hfsplus_unlink(new_dir, new_dentry);
		if (res)
			return res;
	}

If we have called hfsplus_rmdir() or hfsplus_unlink(), then this action:

> > +	new_dentry->d_fsdata = old_dentry->d_fsdata;

doesn't make sense. Am I correct?

But if we didn't call hfsplus_rmdir() or hfsplus_unlink(), then we still need to
make this assignment. Do I follow your point?

Thanks,
Slava.