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

[PATCH v2 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 v2 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}

W/O patch(xfstest generic/003):

 +ERROR: access time has changed for file1 after remount
 +ERROR: access time has changed after modifying file1
 +ERROR: change time has not been updated after changing file1
 +ERROR: access time has changed for file in read-only filesystem

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: 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

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 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..9fab84b157b4 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_NOATIME;
 	mutex_init(&sbi->bitmap_lock);
 
 	res = hfs_mdb_get(sb);
-- 
2.48.1

[PATCH v2 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/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 = {
-- 
2.48.1

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

[Viacheslav Dubeyko]

On Mon, 2025-05-19 at 10:52 -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}
> 
> W/O patch(xfstest generic/003):
> 
>  +ERROR: access time has changed for file1 after remount
>  +ERROR: access time has changed after modifying file1
>  +ERROR: change time has not been updated after changing file1
>  +ERROR: access time has changed for file in read-only filesystem
> 
> 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: 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
> 

I am slightly confused by comment. Does it mean that the fix introduces
more errors? It looks like we need to have more clear explanation of
the fix here.

> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>  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..9fab84b157b4 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_NOATIME;

I believe we need to have two flags here:

s->s_flags |= SB_NODIRATIME | SB_NOATIME;


Thanks,
Slava.

>  	mutex_init(&sbi->bitmap_lock);
>  
>  	res = hfs_mdb_get(sb);

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

[Viacheslav Dubeyko]

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 = {

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

[Viacheslav Dubeyko]

On Tue, 2025-05-27 at 15:46 -0700, Viacheslav Dubeyko wrote:
> 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  
> 

Sorry, I mean these links:

[1] https://elixir.bootlin.com/linux/v6.15/source/fs/hfs/sysdep.c#L35
[2] https://elixir.bootlin.com/linux/v6.15/source/fs/hfs/sysdep.c#L36
[3] https://elixir.bootlin.com/linux/v6.15/source/fs/hfs/inode.c#L357
[4] https://elixir.bootlin.com/linux/v6.15/source/fs/hfs/inode.c#L368

Thanks,
Slava.

> > 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 = {

回复: [PATCH v2 2/3] hfs: correct superblock flags

[李扬韬]

Hi Slava,

> I am slightly confused by comment. Does it mean that the fix introduces more errors? It looks like we need to have more clear explanation of the fix here.

I'll update commit msg.

> s->s_flags |= SB_NODIRATIME | SB_NOATIME;

IIUC, SB_NOATIME > SB_NODIRATIME.

So we should correct flags in smb, ceph.

2091 bool atime_needs_update(const struct path *path, struct inode *inode)
2092 {
2093         struct vfsmount *mnt = path->mnt;
2094         struct timespec64 now, atime;
2095
2096         if (inode->i_flags & S_NOATIME)
2097                 return false;
2098
2099         /* Atime updates will likely cause i_uid and i_gid to be written
2100         ¦* back improprely if their true value is unknown to the vfs.
2101         ¦*/
2102         if (HAS_UNMAPPED_ID(mnt_idmap(mnt), inode))
2103                 return false;
2104
2105         if (IS_NOATIME(inode))
2106                 return false;
2107         if ((inode->i_sb->s_flags & SB_NODIRATIME) && S_ISDIR(inode->i_mode))
2108                 return false;

Thx,
Yangtao

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

[李扬韬]

Hi Slava,

> 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?

This is not explicitly stated in the man page or anything like that, but it seems to be a rule generally followed by file systems. 
By the way, I did this experiment on apfs, and after the rename operation, the ctime changed.

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

I looked at other filesystems marked as NOATIME (e.g. jffs2), and similar atime operations were still preserved.

Thx,
Yangtao

回复: [PATCH v2 2/3] hfs: correct superblock flags

[李扬韬]

Hi Slava,

> I am slightly confused by comment. Does it mean that the fix introduces more errors? It looks like we need to have more clear explanation of the fix here.

How about below commit msg.

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}

W/O patch(xfstest generic/003):

 +ERROR: access time has changed for file1 after remount
 +ERROR: access time has changed after modifying file1
 +ERROR: change time has not been updated after changing file1
 +ERROR: access time has changed for file in read-only filesystem

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: 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

With this patch, we do not accept changes to atime under any circumstances.

Re: 回复: [PATCH v2 2/3] hfs: correct superblock flags

[Viacheslav Dubeyko]

On Wed, 2025-05-28 at 16:56 +0000, 李扬韬 wrote:
> Hi Slava,
> 
> > I am slightly confused by comment. Does it mean that the fix introduces more errors? It looks like we need to have more clear explanation of the fix here.
> 
> How about below commit msg.
> 
> 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}
> 
> W/O patch(xfstest generic/003):
> 
>  +ERROR: access time has changed for file1 after remount
>  +ERROR: access time has changed after modifying file1
>  +ERROR: change time has not been updated after changing file1
>  +ERROR: access time has changed for file in read-only filesystem
> 
> W/ patch(xfstest generic/003):
> 
>  +ERROR: access time has not been updated after accessing file1 first time

The +ERROR sounds for me that generic/003 ends with error or failed. So, what
are we trying to say here?

The comment looks like that we had 4 errors before the fix and we have 6 errors
after the fix. It sounds strange. :)

Thanks,
Slava. 

>  +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
> 
> With this patch, we do not accept changes to atime under any circumstances.

Re: 回复: [PATCH v2 2/3] hfs: correct superblock flags

[Viacheslav Dubeyko]

On Wed, 2025-05-28 at 16:37 +0000, 李扬韬 wrote:
> Hi Slava,
> 
> > I am slightly confused by comment. Does it mean that the fix introduces more errors? It looks like we need to have more clear explanation of the fix here.
> 
> I'll update commit msg.
> 
> > s->s_flags |= SB_NODIRATIME | SB_NOATIME;
> 
> IIUC, SB_NOATIME > SB_NODIRATIME.
> 

Semantically, it's two different flags. One is responsible for files and another
one is responsible for folders. So, this is why I believe it's more safe to have
these both flags.

Implementation could change but setting these flags we guarantee that it needs
to take into account not to update atime for files and folders.

> So we should correct flags in smb, ceph.
> 

I am not sure that it makes sense. It's more safe to have both flags set.

Thanks,
Slava.

> 2091 bool atime_needs_update(const struct path *path, struct inode *inode)
> 2092 {
> 2093         struct vfsmount *mnt = path->mnt;
> 2094         struct timespec64 now, atime;
> 2095
> 2096         if (inode->i_flags & S_NOATIME)
> 2097                 return false;
> 2098
> 2099         /* Atime updates will likely cause i_uid and i_gid to be written
> 2100         ¦* back improprely if their true value is unknown to the vfs.
> 2101         ¦*/
> 2102         if (HAS_UNMAPPED_ID(mnt_idmap(mnt), inode))
> 2103                 return false;
> 2104
> 2105         if (IS_NOATIME(inode))
> 2106                 return false;
> 2107         if ((inode->i_sb->s_flags & SB_NODIRATIME) && S_ISDIR(inode->i_mode))
> 2108                 return false;
> 
> Thx,
> Yangtao

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

[Yangtao Li]

+cc Christian Brauner

在 2025/5/29 05:26, Viacheslav Dubeyko 写道:
> On Wed, 2025-05-28 at 16:37 +0000, 李扬韬 wrote:
>> Hi Slava,
>>
>>> I am slightly confused by comment. Does it mean that the fix introduces more errors? It looks like we need to have more clear explanation of the fix here.
>>
>> I'll update commit msg.
>>
>>> s->s_flags |= SB_NODIRATIME | SB_NOATIME;
>>
>> IIUC, SB_NOATIME > SB_NODIRATIME.
>>
> 
> Semantically, it's two different flags. One is responsible for files and another
> one is responsible for folders. So, this is why I believe it's more safe to have
> these both flags.

To be honest, from my point of view, SB_NOATIME is more like disabling 
atime updates for all types of files, not just files. I would like to 
know what vfs people think, whether we need to use both flags at the 
same time.

> 
> Implementation could change but setting these flags we guarantee that it needs
> to take into account not to update atime for files and folders.
> 
>> So we should correct flags in smb, ceph.
>>
> 
> I am not sure that it makes sense. It's more safe to have both flags set.
> 
> Thanks,
> Slava.
> 
>> 2091 bool atime_needs_update(const struct path *path, struct inode *inode)
>> 2092 {
>> 2093         struct vfsmount *mnt = path->mnt;
>> 2094         struct timespec64 now, atime;
>> 2095
>> 2096         if (inode->i_flags & S_NOATIME)
>> 2097                 return false;
>> 2098
>> 2099         /* Atime updates will likely cause i_uid and i_gid to be written
>> 2100         ¦* back improprely if their true value is unknown to the vfs.
>> 2101         ¦*/
>> 2102         if (HAS_UNMAPPED_ID(mnt_idmap(mnt), inode))
>> 2103                 return false;
>> 2104
>> 2105         if (IS_NOATIME(inode))
>> 2106                 return false;
>> 2107         if ((inode->i_sb->s_flags & SB_NODIRATIME) && S_ISDIR(inode->i_mode))
>> 2108                 return false;
>>

Thx,
Yangtao

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

[Christian Brauner]

On Thu, May 29, 2025 at 10:25:02AM +0800, Yangtao Li wrote:
> +cc Christian Brauner
> 
> 在 2025/5/29 05:26, Viacheslav Dubeyko 写道:
> > On Wed, 2025-05-28 at 16:37 +0000, 李扬韬 wrote:
> > > Hi Slava,
> > > 
> > > > I am slightly confused by comment. Does it mean that the fix introduces more errors? It looks like we need to have more clear explanation of the fix here.
> > > 
> > > I'll update commit msg.
> > > 
> > > > s->s_flags |= SB_NODIRATIME | SB_NOATIME;
> > > 
> > > IIUC, SB_NOATIME > SB_NODIRATIME.
> > > 
> > 
> > Semantically, it's two different flags. One is responsible for files and another
> > one is responsible for folders. So, this is why I believe it's more safe to have
> > these both flags.
> 
> To be honest, from my point of view, SB_NOATIME is more like disabling atime
> updates for all types of files, not just files. I would like to know what
> vfs people think, whether we need to use both flags at the same time.

SB_NODIRATIME should be a subset of SB_NOATIME. So all you should need
is SB_NOATIME to disable it for all files.

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

[Yangtao Li]

在 2025/5/30 13:21, Christian Brauner 写道:
> On Thu, May 29, 2025 at 10:25:02AM +0800, Yangtao Li wrote:
>> +cc Christian Brauner
>>
>> 在 2025/5/29 05:26, Viacheslav Dubeyko 写道:
>>> On Wed, 2025-05-28 at 16:37 +0000, 李扬韬 wrote:
>>>> Hi Slava,
>>>>
>>>>> I am slightly confused by comment. Does it mean that the fix introduces more errors? It looks like we need to have more clear explanation of the fix here.
>>>>
>>>> I'll update commit msg.
>>>>
>>>>> s->s_flags |= SB_NODIRATIME | SB_NOATIME;
>>>>
>>>> IIUC, SB_NOATIME > SB_NODIRATIME.
>>>>
>>>
>>> Semantically, it's two different flags. One is responsible for files and another
>>> one is responsible for folders. So, this is why I believe it's more safe to have
>>> these both flags.
>>
>> To be honest, from my point of view, SB_NOATIME is more like disabling atime
>> updates for all types of files, not just files. I would like to know what
>> vfs people think, whether we need to use both flags at the same time.
> 
> SB_NODIRATIME should be a subset of SB_NOATIME. So all you should need
> is SB_NOATIME to disable it for all files.

Thx to point it，I think I'll correct the incorrect usage in other file 
systems later.

MBR，
Yangtao