[Cluster-devel] [PATCH 01/79] fs: add ctime accessors infrastructure
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
---
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
+ * @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.
--
2.41.0
[Cluster-devel] [PATCH 00/79] fs: new accessors for inode->i_ctime
I've been working on a patchset to change how the inode->i_ctime is accessed in order to give us conditional, high-res timestamps for the ctime and mtime. struct timespec64 has unused bits in it that we can use to implement this. In order to do that however, we need to wrap all accesses of inode->i_ctime to ensure that bits used as flags are appropriately handled. This patchset first adds some new inode_ctime_* accessor functions. It then converts all in-tree accesses of inode->i_ctime to use those new functions and then renames the i_ctime field to __i_ctime to help ensure that use of the accessors. 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. Some of these patches depend on the set I sent recently to add missing ctime updates in various subsystems: https://lore.kernel.org/linux-fsdevel/20230612104524.17058-1-jlay...@kernel.org/T/#m25399f903cc9526e46b2e0f5a35713c80b52fde9 Since this patchset is so large, I'm only going to send individual conversion patches to the appropriate maintainers. Please send Acked-by's or Reviewed-by's if you can. The intent is to merge these as a set (probably in v6.6). Let me know if that causes conflicts though, and we can work it out. This is based on top of linux-next as of yesterday. Jeff Layton (79): fs: add ctime accessors infrastructure spufs: switch to new ctime accessors s390: switch to new ctime accessors binderfs: switch to new ctime accessors qib_fs: switch to new ctime accessors ibm: switch to new ctime accessors usb: switch to new ctime accessors 9p: switch to new ctime accessors adfs: switch to new ctime accessors affs: switch to new ctime accessors afs: switch to new ctime accessors fs: switch to new ctime accessors autofs: switch to new ctime accessors befs: switch to new ctime accessors bfs: switch to new ctime accessors btrfs: switch to new ctime accessors ceph: switch to new ctime accessors coda: switch to new ctime accessors configfs: switch to new ctime accessors cramfs: switch to new ctime accessors debugfs: switch to new ctime accessors devpts: switch to new ctime accessors ecryptfs: switch to new ctime accessors efivarfs: switch to new ctime accessors efs: switch to new ctime accessors erofs: switch to new ctime accessors exfat: switch to new ctime accessors ext2: switch to new ctime accessors ext4: switch to new ctime accessors f2fs: switch to new ctime accessors fat: switch to new ctime accessors freevxfs: switch to new ctime accessors fuse: switch to new ctime accessors gfs2: switch to new ctime accessors hfs: switch to new ctime accessors hfsplus: switch to new ctime accessors hostfs: switch to new ctime accessors hpfs: switch to new ctime accessors hugetlbfs: switch to new ctime accessors isofs: switch to new ctime accessors jffs2: switch to new ctime accessors jfs: switch to new ctime accessors kernfs: switch to new ctime accessors minix: switch to new ctime accessors nfs: switch to new ctime accessors nfsd: switch to new ctime accessors nilfs2: switch to new ctime accessors ntfs: switch to new ctime accessors ntfs3: switch to new ctime accessors ocfs2: switch to new ctime accessors omfs: switch to new ctime accessors openpromfs: switch to new ctime accessors orangefs: switch to new ctime accessors overlayfs: switch to new ctime accessors proc: switch to new ctime accessors pstore: switch to new ctime accessors qnx4: switch to new ctime accessors qnx6: switch to new ctime accessors ramfs: switch to new ctime accessors reiserfs: switch to new ctime accessors romfs: switch to new ctime accessors smb: switch to new ctime accessors squashfs: switch to new ctime accessors sysv: switch to new ctime accessors tracefs: switch to new ctime accessors ubifs: switch to new ctime accessors udf: switch to new ctime accessors ufs: switch to new ctime accessors vboxsf: switch to new ctime accessors xfs: switch to new ctime accessors zonefs: switch to new ctime accessors mqueue: switch to new ctime accessors bpf: switch to new ctime accessors shmem: switch to new ctime accessors rpc_pipefs: switch to new ctime accessors apparmor: switch to new ctime accessors security: switch to new ctime accessors selinux: switch to new ctime accessors fs: rename i_ctime field to __i_ctime arch/powerpc/platforms/cell/spufs/inode.c | 2 +- arch/s390/hypfs/inode.c | 4 +- drivers/android/binderfs.c| 8 +-- drivers/infiniband/hw/qib/qib_fs.c| 4 +- drivers/misc/ibmasm/ibmasmfs.c| 2 +- drivers/misc/ibmvmc.c | 2 +- drivers/usb/core/devio.c | 16 +++--- drivers/usb/gadget/function/f_fs.c| 6 +-- drivers/usb/gad
[Cluster-devel] [PATCH 34/79] gfs2: switch to new ctime accessors
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
---
fs/gfs2/acl.c | 2 +-
fs/gfs2/bmap.c | 11 +--
fs/gfs2/dir.c | 15 ---
fs/gfs2/file.c | 2 +-
fs/gfs2/glops.c | 4 ++--
fs/gfs2/inode.c | 8
fs/gfs2/super.c | 4 ++--
fs/gfs2/xattr.c | 8
8 files changed, 27 insertions(+), 27 deletions(-)
diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
index a392aa0f041d..b267dae0dc63 100644
--- a/fs/gfs2/acl.c
+++ b/fs/gfs2/acl.c
@@ -142,7 +142,7 @@ int gfs2_set_acl(struct mnt_idmap *idmap, struct dentry
*dentry,
ret = __gfs2_set_acl(inode, acl, type);
if (!ret && mode != inode->i_mode) {
- inode->i_ctime = current_time(inode);
+ inode_ctime_set_current(inode);
inode->i_mode = mode;
mark_inode_dirty(inode);
}
diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 8d611fbcf0bd..743b09a0b196 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -1386,7 +1386,7 @@ static int trunc_start(struct inode *inode, u64 newsize)
ip->i_diskflags |= GFS2_DIF_TRUNC_IN_PROG;
i_size_write(inode, newsize);
- ip->i_inode.i_mtime = ip->i_inode.i_ctime = current_time(&ip->i_inode);
+ inode->i_mtime = inode_ctime_set_current(inode);
gfs2_dinode_out(ip, dibh->b_data);
if (journaled)
@@ -1583,8 +1583,7 @@ static int sweep_bh_for_rgrps(struct gfs2_inode *ip,
struct gfs2_holder *rd_gh,
/* Every transaction boundary, we rewrite the dinode
to keep its di_blocks current in case of failure. */
- ip->i_inode.i_mtime = ip->i_inode.i_ctime =
- current_time(&ip->i_inode);
+ ip->i_inode.i_mtime =
inode_ctime_set_current(&ip->i_inode);
gfs2_trans_add_meta(ip->i_gl, dibh);
gfs2_dinode_out(ip, dibh->b_data);
brelse(dibh);
@@ -1950,7 +1949,7 @@ static int punch_hole(struct gfs2_inode *ip, u64 offset,
u64 length)
gfs2_statfs_change(sdp, 0, +btotal, 0);
gfs2_quota_change(ip, -(s64)btotal, ip->i_inode.i_uid,
ip->i_inode.i_gid);
- ip->i_inode.i_mtime = ip->i_inode.i_ctime =
current_time(&ip->i_inode);
+ ip->i_inode.i_mtime = inode_ctime_set_current(&ip->i_inode);
gfs2_trans_add_meta(ip->i_gl, dibh);
gfs2_dinode_out(ip, dibh->b_data);
up_write(&ip->i_rw_mutex);
@@ -1993,7 +1992,7 @@ static int trunc_end(struct gfs2_inode *ip)
gfs2_buffer_clear_tail(dibh, sizeof(struct gfs2_dinode));
gfs2_ordered_del_inode(ip);
}
- ip->i_inode.i_mtime = ip->i_inode.i_ctime = current_time(&ip->i_inode);
+ ip->i_inode.i_mtime = inode_ctime_set_current(&ip->i_inode);
ip->i_diskflags &= ~GFS2_DIF_TRUNC_IN_PROG;
gfs2_trans_add_meta(ip->i_gl, dibh);
@@ -2094,7 +2093,7 @@ static int do_grow(struct inode *inode, u64 size)
goto do_end_trans;
truncate_setsize(inode, size);
- ip->i_inode.i_mtime = ip->i_inode.i_ctime = current_time(&ip->i_inode);
+ ip->i_inode.i_mtime = inode_ctime_set_current(&ip->i_inode);
gfs2_trans_add_meta(ip->i_gl, dibh);
gfs2_dinode_out(ip, dibh->b_data);
brelse(dibh);
diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index 54a6d17b8c25..c07cb9883ea1 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -130,7 +130,7 @@ static int gfs2_dir_write_stuffed(struct gfs2_inode *ip,
const char *buf,
memcpy(dibh->b_data + offset + sizeof(struct gfs2_dinode), buf, size);
if (ip->i_inode.i_size < offset + size)
i_size_write(&ip->i_inode, offset + size);
- ip->i_inode.i_mtime = ip->i_inode.i_ctime = current_time(&ip->i_inode);
+ ip->i_inode.i_mtime = inode_ctime_set_current(&ip->i_inode);
gfs2_dinode_out(ip, dibh->b_data);
brelse(dibh);
@@ -227,7 +227,7 @@ static int gfs2_dir_write_data(struct gfs2_inode *ip, const
char *buf,
if (ip->i_inode.i_size < offset + copied)
i_size_write(&ip->i_inode, offset + copied);
- ip->i_inode.i_mtime = ip->i_inode.i_ctime = current_time(&ip->i_inode);
+ ip->i_inode.i_mtime = inode_ctime_set_current(&ip->i_inode);
gfs2_trans_add_meta(ip->i_gl, dibh);
gfs2_dinode_out(ip, dibh->b_data);
@@ -1814,7 +1814,7 @@ int gfs2_dir_add(struct inode *inode, const struct qstr
*name,
gfs2_inum_out(nip, dent);
dent->de_type = cpu_to_be16(IF2DT(nip->i_inode.i_mode));
dent->de_rahead = cpu_to_be16(gfs2_inode_ra_len(nip));
- tv = current_time(&ip->i_inode);
+ t
[Cluster-devel] [PATCH 79/79] fs: rename i_ctime field to __i_ctime
Now that everything in-tree is converted to use the accessor functions,
rename the i_ctime field in the inode to make its accesses more
self-documenting.
Signed-off-by: Jeff Layton
---
include/linux/fs.h | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9afb30606373..2ca46c532b49 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -642,7 +642,7 @@ struct inode {
loff_t i_size;
struct timespec64 i_atime;
struct timespec64 i_mtime;
- struct timespec64 i_ctime;
+ struct timespec64 __i_ctime; /* use inode_ctime_* accessors! */
spinlock_t i_lock; /* i_blocks, i_bytes, maybe i_size */
unsigned short i_bytes;
u8 i_blkbits;
@@ -1485,7 +1485,7 @@ struct timespec64 inode_ctime_set_current(struct inode
*inode);
*/
static inline struct timespec64 inode_ctime_peek(const struct inode *inode)
{
- return inode->i_ctime;
+ return inode->__i_ctime;
}
/**
@@ -1497,7 +1497,7 @@ static inline struct timespec64 inode_ctime_peek(const
struct inode *inode)
*/
static inline struct timespec64 inode_ctime_set(struct inode *inode, struct
timespec64 ts)
{
- inode->i_ctime = ts;
+ inode->__i_ctime = ts;
return ts;
}
@@ -1510,7 +1510,7 @@ static inline struct timespec64 inode_ctime_set(struct
inode *inode, struct time
*/
static inline time64_t inode_ctime_set_sec(struct inode *inode, time64_t sec)
{
- inode->i_ctime.tv_sec = sec;
+ inode->__i_ctime.tv_sec = sec;
return sec;
}
@@ -1523,7 +1523,7 @@ static inline time64_t inode_ctime_set_sec(struct inode
*inode, time64_t sec)
*/
static inline long inode_ctime_set_nsec(struct inode *inode, long nsec)
{
- inode->i_ctime.tv_nsec = nsec;
+ inode->__i_ctime.tv_nsec = nsec;
return nsec;
}
--
2.41.0
Re: [Cluster-devel] [PATCH 01/79] fs: add ctime accessors infrastructure
On Wed 21-06-23 10:45:06, 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
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara
Honza
> ---
> 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
> + * @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.
> --
> 2.41.0
>
--
Jan Kara
SUSE Labs, CR
Re: [Cluster-devel] [PATCH 01/79] fs: add ctime accessors infrastructure
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
---
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: [Cluster-devel] [PATCH 01/79] fs: add ctime accessors infrastructure
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
> > ---
> > 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
Re: [Cluster-devel] [PATCH 01/79] fs: add ctime accessors infrastructure
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
---
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
+ * @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: [Cluster-devel] [PATCH 01/79] fs: add ctime accessors infrastructure
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
> > > > ---
> > > >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 n
Re: [Cluster-devel] [PATCH 00/79] fs: new accessors for inode->i_ctime
On Wed, 21 Jun 2023 10:45:05 -0400 Jeff Layton 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: [Cluster-devel] [PATCH 00/79] fs: new accessors for inode->i_ctime
On Wed, 2023-06-21 at 15:21 -0400, Steven Rostedt wrote: > On Wed, 21 Jun 2023 10:45:05 -0400 > Jeff Layton 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: [Cluster-devel] [PATCH 01/79] fs: add ctime accessors infrastructure
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 [...] > +/** > + * 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
