On Wed, 2023-06-21 at 14:19 -0400, Tom Talpey wrote:

On 6/21/2023 2:01 PM, Jeff Layton wrote:

quoted

On Wed, 2023-06-21 at 13:29 -0400, Tom Talpey wrote:

quoted

On 6/21/2023 10:45 AM, Jeff Layton wrote:

quoted

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.

quoted

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!

quoted

quoted

quoted

+ * @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 [off-list ref]