[PATCH 12/79] fs: switch to new ctime accessors

[PATCH 12/79] fs: switch to new ctime accessors

[Jeff Layton]

In later patches, we're going to change how the ctime.tv_nsec field is
utilized. Switch to using accessor functions instead of raw accesses of
inode->i_ctime.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/attr.c                |  2 +-
 fs/bad_inode.c           |  3 +--
 fs/binfmt_misc.c         |  3 +--
 fs/inode.c               | 12 ++++++++----
 fs/libfs.c               | 32 +++++++++++++++++---------------
 fs/nsfs.c                |  2 +-
 fs/pipe.c                |  2 +-
 fs/posix_acl.c           |  2 +-
 fs/stack.c               |  2 +-
 fs/stat.c                |  2 +-
 include/linux/fs_stack.h |  2 +-
 11 files changed, 34 insertions(+), 30 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index d60dc1edb526..2750e5f98dfb 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -312,7 +312,7 @@ void setattr_copy(struct mnt_idmap *idmap, struct inode *inode,
 	if (ia_valid & ATTR_MTIME)
 		inode->i_mtime = attr->ia_mtime;
 	if (ia_valid & ATTR_CTIME)
-		inode->i_ctime = attr->ia_ctime;
+		inode_ctime_set(inode, attr->ia_ctime);
 	if (ia_valid & ATTR_MODE) {
 		umode_t mode = attr->ia_mode;
 		if (!in_group_or_capable(idmap, inode,
diff --git a/fs/bad_inode.c b/fs/bad_inode.c
index db649487d58c..bd3762e1b670 100644
--- a/fs/bad_inode.c
+++ b/fs/bad_inode.c
@@ -209,8 +209,7 @@ void make_bad_inode(struct inode *inode)
 	remove_inode_hash(inode);
 
 	inode->i_mode = S_IFREG;
-	inode->i_atime = inode->i_mtime = inode->i_ctime =
-		current_time(inode);
+	inode->i_atime = inode->i_mtime = inode_ctime_set_current(inode);
 	inode->i_op = &bad_inode_ops;	
 	inode->i_opflags &= ~IOP_XATTR;
 	inode->i_fop = &bad_file_ops;	
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index bb202ad369d5..6af92eb1b871 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -547,8 +547,7 @@ static struct inode *bm_get_inode(struct super_block *sb, int mode)
 	if (inode) {
 		inode->i_ino = get_next_ino();
 		inode->i_mode = mode;
-		inode->i_atime = inode->i_mtime = inode->i_ctime =
-			current_time(inode);
+		inode->i_atime = inode->i_mtime = inode_ctime_set_current(inode);
 	}
 	return inode;
 }
diff --git a/fs/inode.c b/fs/inode.c
index c005e7328fbb..a7f484e9e7c1 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1851,6 +1851,7 @@ EXPORT_SYMBOL(bmap);
 static int relatime_need_update(struct vfsmount *mnt, struct inode *inode,
 			     struct timespec64 now)
 {
+	struct timespec64 ctime;
 
 	if (!(mnt->mnt_flags & MNT_RELATIME))
 		return 1;
@@ -1862,7 +1863,8 @@ static int relatime_need_update(struct vfsmount *mnt, struct inode *inode,
 	/*
 	 * Is ctime younger than or equal to atime? If yes, update atime:
 	 */
-	if (timespec64_compare(&inode->i_ctime, &inode->i_atime) >= 0)
+	ctime = inode_ctime_peek(inode);
+	if (timespec64_compare(&ctime, &inode->i_atime) >= 0)
 		return 1;
 
 	/*
@@ -1885,7 +1887,7 @@ int generic_update_time(struct inode *inode, struct timespec64 *time, int flags)
 		if (flags & S_ATIME)
 			inode->i_atime = *time;
 		if (flags & S_CTIME)
-			inode->i_ctime = *time;
+			inode_ctime_set(inode, *time);
 		if (flags & S_MTIME)
 			inode->i_mtime = *time;
 
@@ -2071,6 +2073,7 @@ EXPORT_SYMBOL(file_remove_privs);
 static int inode_needs_update_time(struct inode *inode, struct timespec64 *now)
 {
 	int sync_it = 0;
+	struct timespec64 ctime;
 
 	/* First try to exhaust all avenues to not sync */
 	if (IS_NOCMTIME(inode))
@@ -2079,7 +2082,8 @@ static int inode_needs_update_time(struct inode *inode, struct timespec64 *now)
 	if (!timespec64_equal(&inode->i_mtime, now))
 		sync_it = S_MTIME;
 
-	if (!timespec64_equal(&inode->i_ctime, now))
+	ctime = inode_ctime_peek(inode);
+	if (!timespec64_equal(&ctime, now))
 		sync_it |= S_CTIME;
 
 	if (IS_I_VERSION(inode) && inode_iversion_need_inc(inode))
@@ -2510,7 +2514,7 @@ struct timespec64 inode_ctime_set_current(struct inode *inode)
 {
 	struct timespec64 now = current_time(inode);
 
-	inode_set_ctime(inode, now);
+	inode_ctime_set(inode, now);
 	return now;
 }
 EXPORT_SYMBOL(inode_ctime_set_current);
diff --git a/fs/libfs.c b/fs/libfs.c
index 5b851315eeed..4a914f09fa87 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -275,7 +275,7 @@ void simple_recursive_removal(struct dentry *dentry,
 		while ((child = find_next_child(this, victim)) == NULL) {
 			// kill and ascend
 			// update metadata while it's still locked
-			inode->i_ctime = current_time(inode);
+			inode_ctime_set_current(inode);
 			clear_nlink(inode);
 			inode_unlock(inode);
 			victim = this;
@@ -293,8 +293,7 @@ void simple_recursive_removal(struct dentry *dentry,
 				dput(victim);		// unpin it
 			}
 			if (victim == dentry) {
-				inode->i_ctime = inode->i_mtime =
-					current_time(inode);
+				inode->i_mtime = inode_ctime_set_current(inode);
 				if (d_is_dir(dentry))
 					drop_nlink(inode);
 				inode_unlock(inode);
@@ -335,7 +334,7 @@ static int pseudo_fs_fill_super(struct super_block *s, struct fs_context *fc)
 	 */
 	root->i_ino = 1;
 	root->i_mode = S_IFDIR | S_IRUSR | S_IWUSR;
-	root->i_atime = root->i_mtime = root->i_ctime = current_time(root);
+	root->i_atime = root->i_mtime = inode_ctime_set_current(root);
 	s->s_root = d_make_root(root);
 	if (!s->s_root)
 		return -ENOMEM;
@@ -391,7 +390,8 @@ int simple_link(struct dentry *old_dentry, struct inode *dir, struct dentry *den
 {
 	struct inode *inode = d_inode(old_dentry);
 
-	inode->i_ctime = dir->i_ctime = dir->i_mtime = current_time(inode);
+	inode_ctime_set_current(inode);
+	inode->i_mtime = inode_ctime_set_current(dir);
 	inc_nlink(inode);
 	ihold(inode);
 	dget(dentry);
@@ -425,7 +425,8 @@ int simple_unlink(struct inode *dir, struct dentry *dentry)
 {
 	struct inode *inode = d_inode(dentry);
 
-	inode->i_ctime = dir->i_ctime = dir->i_mtime = current_time(inode);
+	inode_ctime_set_current(inode);
+	dir->i_mtime = inode_ctime_set_current(dir);
 	drop_nlink(inode);
 	dput(dentry);
 	return 0;
@@ -459,10 +460,10 @@ 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);
+	old_dir->i_mtime = inode_ctime_set_current(old_dir);
+	new_dir->i_mtime = inode_ctime_set_current(new_dir);
+	inode_ctime_set_current(d_inode(old_dentry));
+	inode_ctime_set_current(d_inode(new_dentry));
 
 	return 0;
 }
@@ -495,8 +496,9 @@ 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);
+	old_dir->i_mtime = inode_ctime_set_current(old_dir);
+	new_dir->i_mtime = inode_ctime_set_current(new_dir);
+	inode_ctime_set_current(inode);
 
 	return 0;
 }
@@ -659,7 +661,7 @@ int simple_fill_super(struct super_block *s, unsigned long magic,
 	 */
 	inode->i_ino = 1;
 	inode->i_mode = S_IFDIR | 0755;
-	inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
+	inode->i_atime = inode->i_mtime = inode_ctime_set_current(inode);
 	inode->i_op = &simple_dir_inode_operations;
 	inode->i_fop = &simple_dir_operations;
 	set_nlink(inode, 2);
@@ -685,7 +687,7 @@ int simple_fill_super(struct super_block *s, unsigned long magic,
 			goto out;
 		}
 		inode->i_mode = S_IFREG | files->mode;
-		inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
+		inode->i_atime = inode->i_mtime = inode_ctime_set_current(inode);
 		inode->i_fop = files->ops;
 		inode->i_ino = i;
 		d_add(dentry, inode);
@@ -1253,7 +1255,7 @@ struct inode *alloc_anon_inode(struct super_block *s)
 	inode->i_uid = current_fsuid();
 	inode->i_gid = current_fsgid();
 	inode->i_flags |= S_PRIVATE;
-	inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
+	inode->i_atime = inode->i_mtime = inode_ctime_set_current(inode);
 	return inode;
 }
 EXPORT_SYMBOL(alloc_anon_inode);
diff --git a/fs/nsfs.c b/fs/nsfs.c
index f602a96a1afe..c052cc55eacd 100644
--- a/fs/nsfs.c
+++ b/fs/nsfs.c
@@ -84,7 +84,7 @@ static int __ns_get_path(struct path *path, struct ns_common *ns)
 		return -ENOMEM;
 	}
 	inode->i_ino = ns->inum;
-	inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode);
+	inode->i_mtime = inode->i_atime = inode_ctime_set_current(inode);
 	inode->i_flags |= S_IMMUTABLE;
 	inode->i_mode = S_IFREG | S_IRUGO;
 	inode->i_fop = &ns_file_operations;
diff --git a/fs/pipe.c b/fs/pipe.c
index 2d88f73f585a..bb90b6fc4a96 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -899,7 +899,7 @@ static struct inode * get_pipe_inode(void)
 	inode->i_mode = S_IFIFO | S_IRUSR | S_IWUSR;
 	inode->i_uid = current_fsuid();
 	inode->i_gid = current_fsgid();
-	inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
+	inode->i_atime = inode->i_mtime = inode_ctime_set_current(inode);
 
 	return inode;
 
diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 7fa1b738bbab..cc9c390fd2af 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -1027,7 +1027,7 @@ int simple_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
 			return error;
 	}
 
-	inode->i_ctime = current_time(inode);
+	inode_ctime_set_current(inode);
 	if (IS_I_VERSION(inode))
 		inode_inc_iversion(inode);
 	set_cached_acl(inode, type, acl);
diff --git a/fs/stack.c b/fs/stack.c
index c9830924eb12..efd0de85bace 100644
--- a/fs/stack.c
+++ b/fs/stack.c
@@ -68,7 +68,7 @@ void fsstack_copy_attr_all(struct inode *dest, const struct inode *src)
 	dest->i_rdev = src->i_rdev;
 	dest->i_atime = src->i_atime;
 	dest->i_mtime = src->i_mtime;
-	dest->i_ctime = src->i_ctime;
+	inode_ctime_set(dest, inode_ctime_peek(src));
 	dest->i_blkbits = src->i_blkbits;
 	dest->i_flags = src->i_flags;
 	set_nlink(dest, src->i_nlink);
diff --git a/fs/stat.c b/fs/stat.c
index 7c238da22ef0..5d87e34d6dd5 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -58,7 +58,7 @@ void generic_fillattr(struct mnt_idmap *idmap, struct inode *inode,
 	stat->size = i_size_read(inode);
 	stat->atime = inode->i_atime;
 	stat->mtime = inode->i_mtime;
-	stat->ctime = inode->i_ctime;
+	stat->ctime = inode_ctime_peek(inode);
 	stat->blksize = i_blocksize(inode);
 	stat->blocks = inode->i_blocks;
 }
diff --git a/include/linux/fs_stack.h b/include/linux/fs_stack.h
index 54210a42c30d..1488a118fe91 100644
--- a/include/linux/fs_stack.h
+++ b/include/linux/fs_stack.h
@@ -24,7 +24,7 @@ static inline void fsstack_copy_attr_times(struct inode *dest,
 {
 	dest->i_atime = src->i_atime;
 	dest->i_mtime = src->i_mtime;
-	dest->i_ctime = src->i_ctime;
+	inode_ctime_set(dest, inode_ctime_peek(src));
 }
 
 #endif /* _LINUX_FS_STACK_H */
-- 
2.41.0

Re: [PATCH 12/79] fs: switch to new ctime accessors

[Jan Kara]

On Wed 21-06-23 10:45:25, Jeff Layton wrote:
> In later patches, we're going to change how the ctime.tv_nsec field is
> utilized. Switch to using accessor functions instead of raw accesses of
> inode->i_ctime.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/attr.c                |  2 +-
>  fs/bad_inode.c           |  3 +--
>  fs/binfmt_misc.c         |  3 +--
>  fs/inode.c               | 12 ++++++++----
>  fs/libfs.c               | 32 +++++++++++++++++---------------
>  fs/nsfs.c                |  2 +-
>  fs/pipe.c                |  2 +-
>  fs/posix_acl.c           |  2 +-
>  fs/stack.c               |  2 +-
>  fs/stat.c                |  2 +-
>  include/linux/fs_stack.h |  2 +-
>  11 files changed, 34 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/attr.c b/fs/attr.c
> index d60dc1edb526..2750e5f98dfb 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -312,7 +312,7 @@ void setattr_copy(struct mnt_idmap *idmap, struct inode *inode,
>  	if (ia_valid & ATTR_MTIME)
>  		inode->i_mtime = attr->ia_mtime;
>  	if (ia_valid & ATTR_CTIME)
> -		inode->i_ctime = attr->ia_ctime;
> +		inode_ctime_set(inode, attr->ia_ctime);
>  	if (ia_valid & ATTR_MODE) {
>  		umode_t mode = attr->ia_mode;
>  		if (!in_group_or_capable(idmap, inode,
> diff --git a/fs/bad_inode.c b/fs/bad_inode.c
> index db649487d58c..bd3762e1b670 100644
> --- a/fs/bad_inode.c
> +++ b/fs/bad_inode.c
> @@ -209,8 +209,7 @@ void make_bad_inode(struct inode *inode)
>  	remove_inode_hash(inode);
>  
>  	inode->i_mode = S_IFREG;
> -	inode->i_atime = inode->i_mtime = inode->i_ctime =
> -		current_time(inode);
> +	inode->i_atime = inode->i_mtime = inode_ctime_set_current(inode);
>  	inode->i_op = &bad_inode_ops;	
>  	inode->i_opflags &= ~IOP_XATTR;
>  	inode->i_fop = &bad_file_ops;	
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index bb202ad369d5..6af92eb1b871 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -547,8 +547,7 @@ static struct inode *bm_get_inode(struct super_block *sb, int mode)
>  	if (inode) {
>  		inode->i_ino = get_next_ino();
>  		inode->i_mode = mode;
> -		inode->i_atime = inode->i_mtime = inode->i_ctime =
> -			current_time(inode);
> +		inode->i_atime = inode->i_mtime = inode_ctime_set_current(inode);
>  	}
>  	return inode;
>  }
> diff --git a/fs/inode.c b/fs/inode.c
> index c005e7328fbb..a7f484e9e7c1 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1851,6 +1851,7 @@ EXPORT_SYMBOL(bmap);
>  static int relatime_need_update(struct vfsmount *mnt, struct inode *inode,
>  			     struct timespec64 now)
>  {
> +	struct timespec64 ctime;
>  
>  	if (!(mnt->mnt_flags & MNT_RELATIME))
>  		return 1;
> @@ -1862,7 +1863,8 @@ static int relatime_need_update(struct vfsmount *mnt, struct inode *inode,
>  	/*
>  	 * Is ctime younger than or equal to atime? If yes, update atime:
>  	 */
> -	if (timespec64_compare(&inode->i_ctime, &inode->i_atime) >= 0)
> +	ctime = inode_ctime_peek(inode);
> +	if (timespec64_compare(&ctime, &inode->i_atime) >= 0)
>  		return 1;
>  
>  	/*
> @@ -1885,7 +1887,7 @@ int generic_update_time(struct inode *inode, struct timespec64 *time, int flags)
>  		if (flags & S_ATIME)
>  			inode->i_atime = *time;
>  		if (flags & S_CTIME)
> -			inode->i_ctime = *time;
> +			inode_ctime_set(inode, *time);
>  		if (flags & S_MTIME)
>  			inode->i_mtime = *time;
>  
> @@ -2071,6 +2073,7 @@ EXPORT_SYMBOL(file_remove_privs);
>  static int inode_needs_update_time(struct inode *inode, struct timespec64 *now)
>  {
>  	int sync_it = 0;
> +	struct timespec64 ctime;
>  
>  	/* First try to exhaust all avenues to not sync */
>  	if (IS_NOCMTIME(inode))
> @@ -2079,7 +2082,8 @@ static int inode_needs_update_time(struct inode *inode, struct timespec64 *now)
>  	if (!timespec64_equal(&inode->i_mtime, now))
>  		sync_it = S_MTIME;
>  
> -	if (!timespec64_equal(&inode->i_ctime, now))
> +	ctime = inode_ctime_peek(inode);
> +	if (!timespec64_equal(&ctime, now))
>  		sync_it |= S_CTIME;
>  
>  	if (IS_I_VERSION(inode) && inode_iversion_need_inc(inode))
> @@ -2510,7 +2514,7 @@ struct timespec64 inode_ctime_set_current(struct inode *inode)
>  {
>  	struct timespec64 now = current_time(inode);
>  
> -	inode_set_ctime(inode, now);
> +	inode_ctime_set(inode, now);
>  	return now;
>  }
>  EXPORT_SYMBOL(inode_ctime_set_current);
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 5b851315eeed..4a914f09fa87 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -275,7 +275,7 @@ void simple_recursive_removal(struct dentry *dentry,
>  		while ((child = find_next_child(this, victim)) == NULL) {
>  			// kill and ascend
>  			// update metadata while it's still locked
> -			inode->i_ctime = current_time(inode);
> +			inode_ctime_set_current(inode);
>  			clear_nlink(inode);
>  			inode_unlock(inode);
>  			victim = this;
> @@ -293,8 +293,7 @@ void simple_recursive_removal(struct dentry *dentry,
>  				dput(victim);		// unpin it
>  			}
>  			if (victim == dentry) {
> -				inode->i_ctime = inode->i_mtime =
> -					current_time(inode);
> +				inode->i_mtime = inode_ctime_set_current(inode);
>  				if (d_is_dir(dentry))
>  					drop_nlink(inode);
>  				inode_unlock(inode);
> @@ -335,7 +334,7 @@ static int pseudo_fs_fill_super(struct super_block *s, struct fs_context *fc)
>  	 */
>  	root->i_ino = 1;
>  	root->i_mode = S_IFDIR | S_IRUSR | S_IWUSR;
> -	root->i_atime = root->i_mtime = root->i_ctime = current_time(root);
> +	root->i_atime = root->i_mtime = inode_ctime_set_current(root);
>  	s->s_root = d_make_root(root);
>  	if (!s->s_root)
>  		return -ENOMEM;
> @@ -391,7 +390,8 @@ int simple_link(struct dentry *old_dentry, struct inode *dir, struct dentry *den
>  {
>  	struct inode *inode = d_inode(old_dentry);
>  
> -	inode->i_ctime = dir->i_ctime = dir->i_mtime = current_time(inode);
> +	inode_ctime_set_current(inode);
> +	inode->i_mtime = inode_ctime_set_current(dir);
>  	inc_nlink(inode);
>  	ihold(inode);
>  	dget(dentry);
> @@ -425,7 +425,8 @@ int simple_unlink(struct inode *dir, struct dentry *dentry)
>  {
>  	struct inode *inode = d_inode(dentry);
>  
> -	inode->i_ctime = dir->i_ctime = dir->i_mtime = current_time(inode);
> +	inode_ctime_set_current(inode);
> +	dir->i_mtime = inode_ctime_set_current(dir);
>  	drop_nlink(inode);
>  	dput(dentry);
>  	return 0;
> @@ -459,10 +460,10 @@ 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);
> +	old_dir->i_mtime = inode_ctime_set_current(old_dir);
> +	new_dir->i_mtime = inode_ctime_set_current(new_dir);
> +	inode_ctime_set_current(d_inode(old_dentry));
> +	inode_ctime_set_current(d_inode(new_dentry));
>  
>  	return 0;
>  }
> @@ -495,8 +496,9 @@ 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);
> +	old_dir->i_mtime = inode_ctime_set_current(old_dir);
> +	new_dir->i_mtime = inode_ctime_set_current(new_dir);
> +	inode_ctime_set_current(inode);
>  
>  	return 0;
>  }
> @@ -659,7 +661,7 @@ int simple_fill_super(struct super_block *s, unsigned long magic,
>  	 */
>  	inode->i_ino = 1;
>  	inode->i_mode = S_IFDIR | 0755;
> -	inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
> +	inode->i_atime = inode->i_mtime = inode_ctime_set_current(inode);
>  	inode->i_op = &simple_dir_inode_operations;
>  	inode->i_fop = &simple_dir_operations;
>  	set_nlink(inode, 2);
> @@ -685,7 +687,7 @@ int simple_fill_super(struct super_block *s, unsigned long magic,
>  			goto out;
>  		}
>  		inode->i_mode = S_IFREG | files->mode;
> -		inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
> +		inode->i_atime = inode->i_mtime = inode_ctime_set_current(inode);
>  		inode->i_fop = files->ops;
>  		inode->i_ino = i;
>  		d_add(dentry, inode);
> @@ -1253,7 +1255,7 @@ struct inode *alloc_anon_inode(struct super_block *s)
>  	inode->i_uid = current_fsuid();
>  	inode->i_gid = current_fsgid();
>  	inode->i_flags |= S_PRIVATE;
> -	inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
> +	inode->i_atime = inode->i_mtime = inode_ctime_set_current(inode);
>  	return inode;
>  }
>  EXPORT_SYMBOL(alloc_anon_inode);
> diff --git a/fs/nsfs.c b/fs/nsfs.c
> index f602a96a1afe..c052cc55eacd 100644
> --- a/fs/nsfs.c
> +++ b/fs/nsfs.c
> @@ -84,7 +84,7 @@ static int __ns_get_path(struct path *path, struct ns_common *ns)
>  		return -ENOMEM;
>  	}
>  	inode->i_ino = ns->inum;
> -	inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode);
> +	inode->i_mtime = inode->i_atime = inode_ctime_set_current(inode);
>  	inode->i_flags |= S_IMMUTABLE;
>  	inode->i_mode = S_IFREG | S_IRUGO;
>  	inode->i_fop = &ns_file_operations;
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 2d88f73f585a..bb90b6fc4a96 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -899,7 +899,7 @@ static struct inode * get_pipe_inode(void)
>  	inode->i_mode = S_IFIFO | S_IRUSR | S_IWUSR;
>  	inode->i_uid = current_fsuid();
>  	inode->i_gid = current_fsgid();
> -	inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
> +	inode->i_atime = inode->i_mtime = inode_ctime_set_current(inode);
>  
>  	return inode;
>  
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 7fa1b738bbab..cc9c390fd2af 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -1027,7 +1027,7 @@ int simple_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
>  			return error;
>  	}
>  
> -	inode->i_ctime = current_time(inode);
> +	inode_ctime_set_current(inode);
>  	if (IS_I_VERSION(inode))
>  		inode_inc_iversion(inode);
>  	set_cached_acl(inode, type, acl);
> diff --git a/fs/stack.c b/fs/stack.c
> index c9830924eb12..efd0de85bace 100644
> --- a/fs/stack.c
> +++ b/fs/stack.c
> @@ -68,7 +68,7 @@ void fsstack_copy_attr_all(struct inode *dest, const struct inode *src)
>  	dest->i_rdev = src->i_rdev;
>  	dest->i_atime = src->i_atime;
>  	dest->i_mtime = src->i_mtime;
> -	dest->i_ctime = src->i_ctime;
> +	inode_ctime_set(dest, inode_ctime_peek(src));
>  	dest->i_blkbits = src->i_blkbits;
>  	dest->i_flags = src->i_flags;
>  	set_nlink(dest, src->i_nlink);
> diff --git a/fs/stat.c b/fs/stat.c
> index 7c238da22ef0..5d87e34d6dd5 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -58,7 +58,7 @@ void generic_fillattr(struct mnt_idmap *idmap, struct inode *inode,
>  	stat->size = i_size_read(inode);
>  	stat->atime = inode->i_atime;
>  	stat->mtime = inode->i_mtime;
> -	stat->ctime = inode->i_ctime;
> +	stat->ctime = inode_ctime_peek(inode);
>  	stat->blksize = i_blocksize(inode);
>  	stat->blocks = inode->i_blocks;
>  }
> diff --git a/include/linux/fs_stack.h b/include/linux/fs_stack.h
> index 54210a42c30d..1488a118fe91 100644
> --- a/include/linux/fs_stack.h
> +++ b/include/linux/fs_stack.h
> @@ -24,7 +24,7 @@ static inline void fsstack_copy_attr_times(struct inode *dest,
>  {
>  	dest->i_atime = src->i_atime;
>  	dest->i_mtime = src->i_mtime;
> -	dest->i_ctime = src->i_ctime;
> +	inode_ctime_set(dest, inode_ctime_peek(src));
>  }
>  
>  #endif /* _LINUX_FS_STACK_H */
> -- 
> 2.41.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 39/79] hugetlbfs: switch to new ctime accessors

[Jeff Layton]

In later patches, we're going to change how the ctime.tv_nsec field is
utilized. Switch to using accessor functions instead of raw accesses of
inode->i_ctime.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/hugetlbfs/inode.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 90361a922cec..7be5a8f5927f 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -889,7 +889,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
 
 	if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + len > inode->i_size)
 		i_size_write(inode, offset + len);
-	inode->i_ctime = current_time(inode);
+	inode_ctime_set_current(inode);
 out:
 	inode_unlock(inode);
 	return error;
@@ -937,7 +937,7 @@ static struct inode *hugetlbfs_get_root(struct super_block *sb,
 		inode->i_mode = S_IFDIR | ctx->mode;
 		inode->i_uid = ctx->uid;
 		inode->i_gid = ctx->gid;
-		inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
+		inode->i_atime = inode->i_mtime = inode_ctime_set_current(inode);
 		inode->i_op = &hugetlbfs_dir_inode_operations;
 		inode->i_fop = &simple_dir_operations;
 		/* directory inodes start off with i_nlink == 2 (for "." entry) */
@@ -981,7 +981,7 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb,
 		lockdep_set_class(&inode->i_mapping->i_mmap_rwsem,
 				&hugetlbfs_i_mmap_rwsem_key);
 		inode->i_mapping->a_ops = &hugetlbfs_aops;
-		inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
+		inode->i_atime = inode->i_mtime = inode_ctime_set_current(inode);
 		inode->i_mapping->private_data = resv_map;
 		info->seals = F_SEAL_SEAL;
 		switch (mode & S_IFMT) {
@@ -1024,7 +1024,7 @@ static int hugetlbfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
 	inode = hugetlbfs_get_inode(dir->i_sb, dir, mode, dev);
 	if (!inode)
 		return -ENOSPC;
-	dir->i_ctime = dir->i_mtime = current_time(dir);
+	dir->i_mtime = inode_ctime_set_current(dir);
 	d_instantiate(dentry, inode);
 	dget(dentry);/* Extra count - pin the dentry in core */
 	return 0;
@@ -1056,7 +1056,7 @@ static int hugetlbfs_tmpfile(struct mnt_idmap *idmap,
 	inode = hugetlbfs_get_inode(dir->i_sb, dir, mode | S_IFREG, 0);
 	if (!inode)
 		return -ENOSPC;
-	dir->i_ctime = dir->i_mtime = current_time(dir);
+	dir->i_mtime = inode_ctime_set_current(dir);
 	d_tmpfile(file, inode);
 	return finish_open_simple(file, 0);
 }
@@ -1078,7 +1078,7 @@ static int hugetlbfs_symlink(struct mnt_idmap *idmap,
 		} else
 			iput(inode);
 	}
-	dir->i_ctime = dir->i_mtime = current_time(dir);
+	dir->i_mtime = inode_ctime_set_current(dir);
 
 	return error;
 }
-- 
2.41.0

[PATCH 74/79] shmem: switch to new ctime accessors

[Jeff Layton]

In later patches, we're going to change how the ctime.tv_nsec field is
utilized. Switch to using accessor functions instead of raw accesses of
inode->i_ctime.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 mm/shmem.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 4752084720b2..4979cb3e37e5 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1064,7 +1064,7 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
 void shmem_truncate_range(struct inode *inode, loff_t lstart, loff_t lend)
 {
 	shmem_undo_range(inode, lstart, lend, false);
-	inode->i_ctime = inode->i_mtime = current_time(inode);
+	inode->i_mtime = inode_ctime_set_current(inode);
 	inode_inc_iversion(inode);
 }
 EXPORT_SYMBOL_GPL(shmem_truncate_range);
@@ -1161,9 +1161,9 @@ static int shmem_setattr(struct mnt_idmap *idmap,
 	if (attr->ia_valid & ATTR_MODE)
 		error = posix_acl_chmod(idmap, dentry, inode->i_mode);
 	if (!error && update_ctime) {
-		inode->i_ctime = current_time(inode);
+		inode_ctime_set_current(inode);
 		if (update_mtime)
-			inode->i_mtime = inode->i_ctime;
+			inode->i_mtime = inode_ctime_peek(inode);
 		inode_inc_iversion(inode);
 	}
 	return error;
@@ -2389,7 +2389,7 @@ static struct inode *shmem_get_inode(struct mnt_idmap *idmap, struct super_block
 		inode->i_ino = ino;
 		inode_init_owner(idmap, inode, dir, mode);
 		inode->i_blocks = 0;
-		inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
+		inode->i_atime = inode->i_mtime = inode_ctime_set_current(inode);
 		inode->i_generation = get_random_u32();
 		info = SHMEM_I(inode);
 		memset(info, 0, (char *)inode - (char *)info);
@@ -3101,7 +3101,7 @@ shmem_mknod(struct mnt_idmap *idmap, struct inode *dir,
 
 		error = 0;
 		dir->i_size += BOGO_DIRENT_SIZE;
-		dir->i_ctime = dir->i_mtime = current_time(dir);
+		dir->i_mtime = inode_ctime_set_current(dir);
 		inode_inc_iversion(dir);
 		d_instantiate(dentry, inode);
 		dget(dentry); /* Extra count - pin the dentry in core */
@@ -3177,7 +3177,8 @@ static int shmem_link(struct dentry *old_dentry, struct inode *dir, struct dentr
 	}
 
 	dir->i_size += BOGO_DIRENT_SIZE;
-	inode->i_ctime = dir->i_ctime = dir->i_mtime = current_time(inode);
+	dir->i_mtime = inode_ctime_set_current(inode);
+	inode_ctime_set(dir, dir->i_mtime);
 	inode_inc_iversion(dir);
 	inc_nlink(inode);
 	ihold(inode);	/* New dentry reference */
@@ -3195,7 +3196,8 @@ static int shmem_unlink(struct inode *dir, struct dentry *dentry)
 		shmem_free_inode(inode->i_sb);
 
 	dir->i_size -= BOGO_DIRENT_SIZE;
-	inode->i_ctime = dir->i_ctime = dir->i_mtime = current_time(inode);
+	dir->i_mtime = inode_ctime_set_current(inode);
+	inode_ctime_set(dir, dir->i_mtime);
 	inode_inc_iversion(dir);
 	drop_nlink(inode);
 	dput(dentry);	/* Undo the count from "create" - this does all the work */
@@ -3283,9 +3285,9 @@ static int shmem_rename2(struct mnt_idmap *idmap,
 
 	old_dir->i_size -= BOGO_DIRENT_SIZE;
 	new_dir->i_size += BOGO_DIRENT_SIZE;
-	old_dir->i_ctime = old_dir->i_mtime =
-	new_dir->i_ctime = new_dir->i_mtime =
-	inode->i_ctime = current_time(old_dir);
+	old_dir->i_mtime = inode_ctime_set_current(old_dir);
+	new_dir->i_mtime = inode_ctime_set_current(new_dir);
+	inode_ctime_set_current(inode);
 	inode_inc_iversion(old_dir);
 	inode_inc_iversion(new_dir);
 	return 0;
@@ -3339,7 +3341,7 @@ static int shmem_symlink(struct mnt_idmap *idmap, struct inode *dir,
 		folio_put(folio);
 	}
 	dir->i_size += BOGO_DIRENT_SIZE;
-	dir->i_ctime = dir->i_mtime = current_time(dir);
+	dir->i_mtime = inode_ctime_set_current(dir);
 	inode_inc_iversion(dir);
 	d_instantiate(dentry, inode);
 	dget(dentry);
@@ -3411,7 +3413,7 @@ static int shmem_fileattr_set(struct mnt_idmap *idmap,
 		(fa->flags & SHMEM_FL_USER_MODIFIABLE);
 
 	shmem_set_inode_flags(inode, info->fsflags);
-	inode->i_ctime = current_time(inode);
+	inode_ctime_set_current(inode);
 	inode_inc_iversion(inode);
 	return 0;
 }
@@ -3481,7 +3483,7 @@ static int shmem_xattr_handler_set(const struct xattr_handler *handler,
 	name = xattr_full_name(handler, name);
 	err = simple_xattr_set(&info->xattrs, name, value, size, flags, NULL);
 	if (!err) {
-		inode->i_ctime = current_time(inode);
+		inode_ctime_set_current(inode);
 		inode_inc_iversion(inode);
 	}
 	return err;
-- 
2.41.0

Re: [PATCH 01/79] fs: add ctime accessors infrastructure

[Tom Talpey]

On 6/21/2023 10:45 AM, Jeff Layton wrote:
> struct timespec64 has unused bits in the tv_nsec field that can be used
> for other purposes. In future patches, we're going to change how the
> inode->i_ctime is accessed in certain inodes in order to make use of
> them. In order to do that safely though, we'll need to eradicate raw
> accesses of the inode->i_ctime field from the kernel.
> 
> Add new accessor functions for the ctime that we can use to replace them.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>   fs/inode.c         | 16 ++++++++++++++
>   include/linux/fs.h | 53 +++++++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index d37fad91c8da..c005e7328fbb 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2499,6 +2499,22 @@ struct timespec64 current_time(struct inode *inode)
>   }
>   EXPORT_SYMBOL(current_time);
>   
> +/**
> + * inode_ctime_set_current - set the ctime to current_time
> + * @inode: inode
> + *
> + * Set the inode->i_ctime to the current value for the inode. Returns
> + * the current value that was assigned to i_ctime.
> + */
> +struct timespec64 inode_ctime_set_current(struct inode *inode)
> +{
> +	struct timespec64 now = current_time(inode);
> +
> +	inode_set_ctime(inode, now);
> +	return now;
> +}
> +EXPORT_SYMBOL(inode_ctime_set_current);
> +
>   /**
>    * in_group_or_capable - check whether caller is CAP_FSETID privileged
>    * @idmap:	idmap of the mount @inode was found from
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 6867512907d6..9afb30606373 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1474,7 +1474,58 @@ static inline bool fsuidgid_has_mapping(struct super_block *sb,
>   	       kgid_has_mapping(fs_userns, kgid);
>   }
>   
> -extern struct timespec64 current_time(struct inode *inode);
> +struct timespec64 current_time(struct inode *inode);
> +struct timespec64 inode_ctime_set_current(struct inode *inode);
> +
> +/**
> + * inode_ctime_peek - fetch the current ctime from the inode
> + * @inode: inode from which to fetch ctime
> + *
> + * Grab the current ctime from the inode and return it.
> + */
> +static inline struct timespec64 inode_ctime_peek(const struct inode *inode)
> +{
> +	return inode->i_ctime;
> +}
> +
> +/**
> + * inode_ctime_set - set the ctime in the inode to the given value
> + * @inode: inode in which to set the ctime
> + * @ts: timespec value to set the ctime
> + *
> + * Set the ctime in @inode to @ts.
> + */
> +static inline struct timespec64 inode_ctime_set(struct inode *inode, struct timespec64 ts)
> +{
> +	inode->i_ctime = ts;
> +	return ts;
> +}
> +
> +/**
> + * inode_ctime_set_sec - set only the tv_sec field in the inode ctime

I'm curious about why you choose to split the tv_sec and tv_nsec
set_ functions. Do any callers not set them both? Wouldn't a
single call enable a more atomic behavior someday?

   inode_ctime_set_sec_nsec(struct inode *, time64_t, time64_t)

(or simply initialize a timespec64 and use inode_ctime_spec() )

Tom.

> + * @inode: inode in which to set the ctime
> + * @sec:  value to set the tv_sec field
> + *
> + * Set the sec field in the ctime. Returns @sec.
> + */
> +static inline time64_t inode_ctime_set_sec(struct inode *inode, time64_t sec)
> +{
> +	inode->i_ctime.tv_sec = sec;
> +	return sec;
> +}
> +
> +/**
> + * inode_ctime_set_nsec - set only the tv_nsec field in the inode ctime
> + * @inode: inode in which to set the ctime
> + * @nsec:  value to set the tv_nsec field
> + *
> + * Set the nsec field in the ctime. Returns @nsec.
> + */
> +static inline long inode_ctime_set_nsec(struct inode *inode, long nsec)
> +{
> +	inode->i_ctime.tv_nsec = nsec;
> +	return nsec;
> +}
>   
>   /*
>    * Snapshotting support.

Re: [PATCH 01/79] fs: add ctime accessors infrastructure

[Jeff Layton]

On Wed, 2023-06-21 at 13:29 -0400, Tom Talpey wrote:
> On 6/21/2023 10:45 AM, Jeff Layton wrote:
> > struct timespec64 has unused bits in the tv_nsec field that can be used
> > for other purposes. In future patches, we're going to change how the
> > inode->i_ctime is accessed in certain inodes in order to make use of
> > them. In order to do that safely though, we'll need to eradicate raw
> > accesses of the inode->i_ctime field from the kernel.
> > 
> > Add new accessor functions for the ctime that we can use to replace them.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >   fs/inode.c         | 16 ++++++++++++++
> >   include/linux/fs.h | 53 +++++++++++++++++++++++++++++++++++++++++++++-
> >   2 files changed, 68 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/inode.c b/fs/inode.c
> > index d37fad91c8da..c005e7328fbb 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -2499,6 +2499,22 @@ struct timespec64 current_time(struct inode *inode)
> >   }
> >   EXPORT_SYMBOL(current_time);
> >   
> > +/**
> > + * inode_ctime_set_current - set the ctime to current_time
> > + * @inode: inode
> > + *
> > + * Set the inode->i_ctime to the current value for the inode. Returns
> > + * the current value that was assigned to i_ctime.
> > + */
> > +struct timespec64 inode_ctime_set_current(struct inode *inode)
> > +{
> > +	struct timespec64 now = current_time(inode);
> > +
> > +	inode_set_ctime(inode, now);
> > +	return now;
> > +}
> > +EXPORT_SYMBOL(inode_ctime_set_current);
> > +
> >   /**
> >    * in_group_or_capable - check whether caller is CAP_FSETID privileged
> >    * @idmap:	idmap of the mount @inode was found from
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 6867512907d6..9afb30606373 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1474,7 +1474,58 @@ static inline bool fsuidgid_has_mapping(struct super_block *sb,
> >   	       kgid_has_mapping(fs_userns, kgid);
> >   }
> >   
> > -extern struct timespec64 current_time(struct inode *inode);
> > +struct timespec64 current_time(struct inode *inode);
> > +struct timespec64 inode_ctime_set_current(struct inode *inode);
> > +
> > +/**
> > + * inode_ctime_peek - fetch the current ctime from the inode
> > + * @inode: inode from which to fetch ctime
> > + *
> > + * Grab the current ctime from the inode and return it.
> > + */
> > +static inline struct timespec64 inode_ctime_peek(const struct inode *inode)
> > +{
> > +	return inode->i_ctime;
> > +}
> > +
> > +/**
> > + * inode_ctime_set - set the ctime in the inode to the given value
> > + * @inode: inode in which to set the ctime
> > + * @ts: timespec value to set the ctime
> > + *
> > + * Set the ctime in @inode to @ts.
> > + */
> > +static inline struct timespec64 inode_ctime_set(struct inode *inode, struct timespec64 ts)
> > +{
> > +	inode->i_ctime = ts;
> > +	return ts;
> > +}
> > +
> > +/**
> > + * inode_ctime_set_sec - set only the tv_sec field in the inode ctime
> 
> I'm curious about why you choose to split the tv_sec and tv_nsec
> set_ functions. Do any callers not set them both? Wouldn't a
> single call enable a more atomic behavior someday?
> 
>    inode_ctime_set_sec_nsec(struct inode *, time64_t, time64_t)
> 
> (or simply initialize a timespec64 and use inode_ctime_spec() )
> 

Yes, quite a few places set the fields individually. For example, when
loading a value from disk that doesn't have sufficient granularity to
set the nsecs field to anything but 0.

Could I have done it by declaring a local timespec64 variable and just
use the inode_ctime_set function in these places? Absolutely.

That's a bit more difficult to handle with coccinelle though. If someone
wants to suggest a way to do that without having to change all of these
call sites manually, then I'm open to redoing the set.

That might be better left for a later cleanup though.

> > + * @inode: inode in which to set the ctime
> > + * @sec:  value to set the tv_sec field
> > + *
> > + * Set the sec field in the ctime. Returns @sec.
> > + */
> > +static inline time64_t inode_ctime_set_sec(struct inode *inode, time64_t sec)
> > +{
> > +	inode->i_ctime.tv_sec = sec;
> > +	return sec;
> > +}
> > +
> > +/**
> > + * inode_ctime_set_nsec - set only the tv_nsec field in the inode ctime
> > + * @inode: inode in which to set the ctime
> > + * @nsec:  value to set the tv_nsec field
> > + *
> > + * Set the nsec field in the ctime. Returns @nsec.
> > + */
> > +static inline long inode_ctime_set_nsec(struct inode *inode, long nsec)
> > +{
> > +	inode->i_ctime.tv_nsec = nsec;
> > +	return nsec;
> > +}
> >   
> >   /*
> >    * Snapshotting support.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 01/79] fs: add ctime accessors infrastructure

[Tom Talpey]

On 6/21/2023 2:01 PM, Jeff Layton wrote:
> On Wed, 2023-06-21 at 13:29 -0400, Tom Talpey wrote:
>> On 6/21/2023 10:45 AM, Jeff Layton wrote:
>>> struct timespec64 has unused bits in the tv_nsec field that can be used
>>> for other purposes. In future patches, we're going to change how the
>>> inode->i_ctime is accessed in certain inodes in order to make use of
>>> them. In order to do that safely though, we'll need to eradicate raw
>>> accesses of the inode->i_ctime field from the kernel.
>>>
>>> Add new accessor functions for the ctime that we can use to replace them.
>>>
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>>    fs/inode.c         | 16 ++++++++++++++
>>>    include/linux/fs.h | 53 +++++++++++++++++++++++++++++++++++++++++++++-
>>>    2 files changed, 68 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/inode.c b/fs/inode.c
>>> index d37fad91c8da..c005e7328fbb 100644
>>> --- a/fs/inode.c
>>> +++ b/fs/inode.c
>>> @@ -2499,6 +2499,22 @@ struct timespec64 current_time(struct inode *inode)
>>>    }
>>>    EXPORT_SYMBOL(current_time);
>>>    
>>> +/**
>>> + * inode_ctime_set_current - set the ctime to current_time
>>> + * @inode: inode
>>> + *
>>> + * Set the inode->i_ctime to the current value for the inode. Returns
>>> + * the current value that was assigned to i_ctime.
>>> + */
>>> +struct timespec64 inode_ctime_set_current(struct inode *inode)
>>> +{
>>> +	struct timespec64 now = current_time(inode);
>>> +
>>> +	inode_set_ctime(inode, now);
>>> +	return now;
>>> +}
>>> +EXPORT_SYMBOL(inode_ctime_set_current);
>>> +
>>>    /**
>>>     * in_group_or_capable - check whether caller is CAP_FSETID privileged
>>>     * @idmap:	idmap of the mount @inode was found from
>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>> index 6867512907d6..9afb30606373 100644
>>> --- a/include/linux/fs.h
>>> +++ b/include/linux/fs.h
>>> @@ -1474,7 +1474,58 @@ static inline bool fsuidgid_has_mapping(struct super_block *sb,
>>>    	       kgid_has_mapping(fs_userns, kgid);
>>>    }
>>>    
>>> -extern struct timespec64 current_time(struct inode *inode);
>>> +struct timespec64 current_time(struct inode *inode);
>>> +struct timespec64 inode_ctime_set_current(struct inode *inode);
>>> +
>>> +/**
>>> + * inode_ctime_peek - fetch the current ctime from the inode
>>> + * @inode: inode from which to fetch ctime
>>> + *
>>> + * Grab the current ctime from the inode and return it.
>>> + */
>>> +static inline struct timespec64 inode_ctime_peek(const struct inode *inode)
>>> +{
>>> +	return inode->i_ctime;
>>> +}
>>> +
>>> +/**
>>> + * inode_ctime_set - set the ctime in the inode to the given value
>>> + * @inode: inode in which to set the ctime
>>> + * @ts: timespec value to set the ctime
>>> + *
>>> + * Set the ctime in @inode to @ts.
>>> + */
>>> +static inline struct timespec64 inode_ctime_set(struct inode *inode, struct timespec64 ts)
>>> +{
>>> +	inode->i_ctime = ts;
>>> +	return ts;
>>> +}
>>> +
>>> +/**
>>> + * inode_ctime_set_sec - set only the tv_sec field in the inode ctime
>>
>> I'm curious about why you choose to split the tv_sec and tv_nsec
>> set_ functions. Do any callers not set them both? Wouldn't a
>> single call enable a more atomic behavior someday?
>>
>>     inode_ctime_set_sec_nsec(struct inode *, time64_t, time64_t)
>>
>> (or simply initialize a timespec64 and use inode_ctime_spec() )
>>
> 
> Yes, quite a few places set the fields individually. For example, when
> loading a value from disk that doesn't have sufficient granularity to
> set the nsecs field to anything but 0.

Well, they still need to set the tv_nsec so they could just pass 0.
But ok.

> Could I have done it by declaring a local timespec64 variable and just
> use the inode_ctime_set function in these places? Absolutely.
> 
> That's a bit more difficult to handle with coccinelle though. If someone
> wants to suggest a way to do that without having to change all of these
> call sites manually, then I'm open to redoing the set.
> 
> That might be better left for a later cleanup though.

Acked-by: Tom Talpey <tom@talpey.com>

>>> + * @inode: inode in which to set the ctime
>>> + * @sec:  value to set the tv_sec field
>>> + *
>>> + * Set the sec field in the ctime. Returns @sec.
>>> + */
>>> +static inline time64_t inode_ctime_set_sec(struct inode *inode, time64_t sec)
>>> +{
>>> +	inode->i_ctime.tv_sec = sec;
>>> +	return sec;
>>> +}
>>> +
>>> +/**
>>> + * inode_ctime_set_nsec - set only the tv_nsec field in the inode ctime
>>> + * @inode: inode in which to set the ctime
>>> + * @nsec:  value to set the tv_nsec field
>>> + *
>>> + * Set the nsec field in the ctime. Returns @nsec.
>>> + */
>>> +static inline long inode_ctime_set_nsec(struct inode *inode, long nsec)
>>> +{
>>> +	inode->i_ctime.tv_nsec = nsec;
>>> +	return nsec;
>>> +}
>>>    
>>>    /*
>>>     * Snapshotting support.
> 

Re: [PATCH 01/79] fs: add ctime accessors infrastructure

[Jeff Layton]

On Wed, 2023-06-21 at 14:19 -0400, Tom Talpey wrote:
> On 6/21/2023 2:01 PM, Jeff Layton wrote:
> > On Wed, 2023-06-21 at 13:29 -0400, Tom Talpey wrote:
> > > On 6/21/2023 10:45 AM, Jeff Layton wrote:
> > > > struct timespec64 has unused bits in the tv_nsec field that can be used
> > > > for other purposes. In future patches, we're going to change how the
> > > > inode->i_ctime is accessed in certain inodes in order to make use of
> > > > them. In order to do that safely though, we'll need to eradicate raw
> > > > accesses of the inode->i_ctime field from the kernel.
> > > > 
> > > > Add new accessor functions for the ctime that we can use to replace them.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >    fs/inode.c         | 16 ++++++++++++++
> > > >    include/linux/fs.h | 53 +++++++++++++++++++++++++++++++++++++++++++++-
> > > >    2 files changed, 68 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/inode.c b/fs/inode.c
> > > > index d37fad91c8da..c005e7328fbb 100644
> > > > --- a/fs/inode.c
> > > > +++ b/fs/inode.c
> > > > @@ -2499,6 +2499,22 @@ struct timespec64 current_time(struct inode *inode)
> > > >    }
> > > >    EXPORT_SYMBOL(current_time);
> > > >    
> > > > +/**
> > > > + * inode_ctime_set_current - set the ctime to current_time
> > > > + * @inode: inode
> > > > + *
> > > > + * Set the inode->i_ctime to the current value for the inode. Returns
> > > > + * the current value that was assigned to i_ctime.
> > > > + */
> > > > +struct timespec64 inode_ctime_set_current(struct inode *inode)
> > > > +{
> > > > +	struct timespec64 now = current_time(inode);
> > > > +
> > > > +	inode_set_ctime(inode, now);
> > > > +	return now;
> > > > +}
> > > > +EXPORT_SYMBOL(inode_ctime_set_current);
> > > > +
> > > >    /**
> > > >     * in_group_or_capable - check whether caller is CAP_FSETID privileged
> > > >     * @idmap:	idmap of the mount @inode was found from
> > > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > > index 6867512907d6..9afb30606373 100644
> > > > --- a/include/linux/fs.h
> > > > +++ b/include/linux/fs.h
> > > > @@ -1474,7 +1474,58 @@ static inline bool fsuidgid_has_mapping(struct super_block *sb,
> > > >    	       kgid_has_mapping(fs_userns, kgid);
> > > >    }
> > > >    
> > > > -extern struct timespec64 current_time(struct inode *inode);
> > > > +struct timespec64 current_time(struct inode *inode);
> > > > +struct timespec64 inode_ctime_set_current(struct inode *inode);
> > > > +
> > > > +/**
> > > > + * inode_ctime_peek - fetch the current ctime from the inode
> > > > + * @inode: inode from which to fetch ctime
> > > > + *
> > > > + * Grab the current ctime from the inode and return it.
> > > > + */
> > > > +static inline struct timespec64 inode_ctime_peek(const struct inode *inode)
> > > > +{
> > > > +	return inode->i_ctime;
> > > > +}
> > > > +
> > > > +/**
> > > > + * inode_ctime_set - set the ctime in the inode to the given value
> > > > + * @inode: inode in which to set the ctime
> > > > + * @ts: timespec value to set the ctime
> > > > + *
> > > > + * Set the ctime in @inode to @ts.
> > > > + */
> > > > +static inline struct timespec64 inode_ctime_set(struct inode *inode, struct timespec64 ts)
> > > > +{
> > > > +	inode->i_ctime = ts;
> > > > +	return ts;
> > > > +}
> > > > +
> > > > +/**
> > > > + * inode_ctime_set_sec - set only the tv_sec field in the inode ctime
> > > 
> > > I'm curious about why you choose to split the tv_sec and tv_nsec
> > > set_ functions. Do any callers not set them both? Wouldn't a
> > > single call enable a more atomic behavior someday?
> > > 
> > >     inode_ctime_set_sec_nsec(struct inode *, time64_t, time64_t)
> > > 
> > > (or simply initialize a timespec64 and use inode_ctime_spec() )
> > > 
> > 
> > Yes, quite a few places set the fields individually. For example, when
> > loading a value from disk that doesn't have sufficient granularity to
> > set the nsecs field to anything but 0.
> 
> Well, they still need to set the tv_nsec so they could just pass 0.
> But ok.
> 

Sure. The difficulty is in trying to do this in an automated way. For
instance, look at the hfsplus patch; it has separate assignments in
place already:

-       result->i_ctime.tv_sec = result->i_mtime.tv_sec = result->i_atime.tv_sec = local_to_gmt(dir->i_sb, le32_to_cpu(dee.creation_date));
-       result->i_ctime.tv_nsec = 0;
+       inode_ctime_set_sec(result,
+                           result->i_mtime.tv_sec = result->i_atime.tv_sec = local_to_gmt(dir->i_sb, le32_to_cpu(dee.creation_date)));
+       inode_ctime_set_nsec(result, 0);

Granted the new code is pretty ugly, but it compiles!

Transforming that into what you're suggesting is a tougher proposition
to do with coccinelle. I didn't see a way to conditionally catch cases
like this, declare a new variable in the appropriate spot and then
transform two assignments (that may not be next to one another!) into a
single one.

Maybe it's possible, but my grasp of SMPL is not that great. The docs
and examples (including Kees' vey helpful ones!) cover fairly simple
changes well, but I didn't quite grasp how to do that complex an
evolution.

> > Could I have done it by declaring a local timespec64 variable and just
> > use the inode_ctime_set function in these places? Absolutely.
> > 
> > That's a bit more difficult to handle with coccinelle though. If someone
> > wants to suggest a way to do that without having to change all of these
> > call sites manually, then I'm open to redoing the set.
> > 
> > That might be better left for a later cleanup though.
> 
> Acked-by: Tom Talpey <tom@talpey.com>
> 

Many thanks!

> > > > + * @inode: inode in which to set the ctime
> > > > + * @sec:  value to set the tv_sec field
> > > > + *
> > > > + * Set the sec field in the ctime. Returns @sec.
> > > > + */
> > > > +static inline time64_t inode_ctime_set_sec(struct inode *inode, time64_t sec)
> > > > +{
> > > > +	inode->i_ctime.tv_sec = sec;
> > > > +	return sec;
> > > > +}
> > > > +
> > > > +/**
> > > > + * inode_ctime_set_nsec - set only the tv_nsec field in the inode ctime
> > > > + * @inode: inode in which to set the ctime
> > > > + * @nsec:  value to set the tv_nsec field
> > > > + *
> > > > + * Set the nsec field in the ctime. Returns @nsec.
> > > > + */
> > > > +static inline long inode_ctime_set_nsec(struct inode *inode, long nsec)
> > > > +{
> > > > +	inode->i_ctime.tv_nsec = nsec;
> > > > +	return nsec;
> > > > +}
> > > >    
> > > >    /*
> > > >     * Snapshotting support.
> > 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 01/79] fs: add ctime accessors infrastructure

[Damien Le Moal]

On 6/21/23 23:45, Jeff Layton wrote:
> struct timespec64 has unused bits in the tv_nsec field that can be used
> for other purposes. In future patches, we're going to change how the
> inode->i_ctime is accessed in certain inodes in order to make use of
> them. In order to do that safely though, we'll need to eradicate raw
> accesses of the inode->i_ctime field from the kernel.
> 
> Add new accessor functions for the ctime that we can use to replace them.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

[...]

> +/**
> + * inode_ctime_peek - fetch the current ctime from the inode
> + * @inode: inode from which to fetch ctime
> + *
> + * Grab the current ctime from the inode and return it.
> + */
> +static inline struct timespec64 inode_ctime_peek(const struct inode *inode)

To be consistent with inode_ctime_set(), why not call this one inode_ctime_get()
? Also, inode_set_ctime() & inode_get_ctime() may be a little more natural. But
no strong opinion about that though.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 01/79] fs: add ctime accessors infrastructure

[Jeff Layton]

On Thu, 2023-06-22 at 09:46 +0900, Damien Le Moal wrote:
> On 6/21/23 23:45, Jeff Layton wrote:
> > struct timespec64 has unused bits in the tv_nsec field that can be used
> > for other purposes. In future patches, we're going to change how the
> > inode->i_ctime is accessed in certain inodes in order to make use of
> > them. In order to do that safely though, we'll need to eradicate raw
> > accesses of the inode->i_ctime field from the kernel.
> > 
> > Add new accessor functions for the ctime that we can use to replace them.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> 
> [...]
> 
> > +/**
> > + * inode_ctime_peek - fetch the current ctime from the inode
> > + * @inode: inode from which to fetch ctime
> > + *
> > + * Grab the current ctime from the inode and return it.
> > + */
> > +static inline struct timespec64 inode_ctime_peek(const struct inode *inode)
> 
> To be consistent with inode_ctime_set(), why not call this one inode_ctime_get()

In later patches fetching the ctime for presentation may have side
effects on certain filesystems. Using "peek" here is a hint that we want
to avoid those side effects in these calls.

> ? Also, inode_set_ctime() & inode_get_ctime() may be a little more natural. But
> no strong opinion about that though.
> 

I like the consistency of the inode_ctime_* prefix. It makes it simpler
to find these calls when grepping, etc.

That said, my opinions on naming are pretty loosely-held, so if the
consensus is that the names should as you suggest, I'll go along with
it.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 01/79] fs: add ctime accessors infrastructure

[Luis Chamberlain]

On Wed, Jun 21, 2023 at 10:45:06AM -0400, Jeff Layton wrote:
> struct timespec64 has unused bits in the tv_nsec field that can be used
> for other purposes. In future patches, we're going to change how the
> inode->i_ctime is accessed in certain inodes in order to make use of
> them. In order to do that safely though, we'll need to eradicate raw
> accesses of the inode->i_ctime field from the kernel.
> 
> Add new accessor functions for the ctime that we can use to replace them.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH 01/79] fs: add ctime accessors infrastructure

[Randy Dunlap]

Hi Jeff,

On arch/um/, (subarch i386 or x86_64), hostfs build fails with:

../fs/hostfs/hostfs_kern.c:520:36: error: incompatible type for arg
ument 2 of 'inode_set_ctime_to_ts'
../include/linux/fs.h:1499:73: note: expected 'struct timespec64' b
ut argument is of type 'const struct hostfs_timespec *'


-- 
~Randy

Re: [PATCH 00/79] fs: new accessors for inode->i_ctime

[Steven Rostedt]

On Wed, 21 Jun 2023 10:45:05 -0400
Jeff Layton <jlayton@kernel.org> wrote:

> Most of this conversion was done via coccinelle, with a few of the more
> non-standard accesses done by hand. There should be no behavioral
> changes with this set. That will come later, as we convert individual
> filesystems to use multigrain timestamps.

BTW, Linus has suggested to me that whenever a conccinelle script is used,
it should be included in the change log.

-- Steve

Re: [PATCH 00/79] fs: new accessors for inode->i_ctime

[Jeff Layton]

On Wed, 2023-06-21 at 15:21 -0400, Steven Rostedt wrote:
> On Wed, 21 Jun 2023 10:45:05 -0400
> Jeff Layton <jlayton@kernel.org> wrote:
> 
> > Most of this conversion was done via coccinelle, with a few of the more
> > non-standard accesses done by hand. There should be no behavioral
> > changes with this set. That will come later, as we convert individual
> > filesystems to use multigrain timestamps.
> 
> BTW, Linus has suggested to me that whenever a conccinelle script is used,
> it should be included in the change log.
> 

Ok, here's what I have. I note again that my usage of coccinelle is
pretty primitive, so I ended up doing a fair bit of by-hand fixing after
applying these.

Given the way that this change is broken up into 77 patches by
subsystem, to which changelogs should I add it? I could add it to the
"infrastructure" patch, but that's the one where I _didn't_ use it. 

Maybe to patch #79 (the one that renames i_ctime)?


------------------------8<------------------------------
@@
expression inode;
@@

- inode->i_ctime = current_time(inode)
+ inode_set_current_ctime(inode)

@@
expression inode;
@@

- inode->i_ctime = inode->i_mtime = current_time(inode)
+ inode->i_mtime = inode_set_current_ctime(inode)

@@
struct inode *inode;
expression value;
@@

- inode->i_ctime = value;
+ inode_set_ctime(inode, value);

@@
struct inode *inode;
expression val;
@@
- inode->i_ctime.tv_sec = val
+ inode_set_ctime_sec(inode, val)

@@
struct inode *inode;
expression val;
@@
- inode->i_ctime.tv_nsec = val
+ inode_set_ctime_nsec(inode, val)

@@
struct inode *inode;
@@
- inode->i_ctime
+ inode_ctime_peek(inode)

Re: [PATCH 00/79] fs: new accessors for inode->i_ctime

[Christian Brauner]

On Wed, Jun 21, 2023 at 03:52:27PM -0400, Jeff Layton wrote:
> On Wed, 2023-06-21 at 15:21 -0400, Steven Rostedt wrote:
> > On Wed, 21 Jun 2023 10:45:05 -0400
> > Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > > Most of this conversion was done via coccinelle, with a few of the more
> > > non-standard accesses done by hand. There should be no behavioral
> > > changes with this set. That will come later, as we convert individual
> > > filesystems to use multigrain timestamps.
> > 
> > BTW, Linus has suggested to me that whenever a conccinelle script is used,
> > it should be included in the change log.
> > 
> 
> Ok, here's what I have. I note again that my usage of coccinelle is
> pretty primitive, so I ended up doing a fair bit of by-hand fixing after
> applying these.
> 
> Given the way that this change is broken up into 77 patches by
> subsystem, to which changelogs should I add it? I could add it to the
> "infrastructure" patch, but that's the one where I _didn't_ use it. 
> 
> Maybe to patch #79 (the one that renames i_ctime)?

That works. I can also put this into a merge commit or pr message.

Re: [PATCH 00/79] fs: new accessors for inode->i_ctime

[Luis Chamberlain]

On Wed, Jun 21, 2023 at 03:21:41PM -0400, Steven Rostedt wrote:
> On Wed, 21 Jun 2023 10:45:05 -0400
> Jeff Layton <jlayton@kernel.org> wrote:
> 
> > Most of this conversion was done via coccinelle, with a few of the more
> > non-standard accesses done by hand. There should be no behavioral
> > changes with this set. That will come later, as we convert individual
> > filesystems to use multigrain timestamps.
> 
> BTW, Linus has suggested to me that whenever a conccinelle script is used,
> it should be included in the change log.

Sometimes people like the coccinelle included in the commit, sometimes
people don't [0], it really ends up being up to a subjective maintainer
preference. A compromise could be to use git notes as these are
optional, however if we want to go down that path we should try to make
a general consensus on it so we can send a consistent message.

[0] https://lore.kernel.org/all/20230512073100.GC32559@twin.jikos.cz/

  Luis