[PATCH v5 0/9] fs: multigrain timestamp redux

[PATCH v5 0/9] fs: multigrain timestamp redux

[Jeff Layton]

tl;dr for those who have been following along:

There are several changes in this version. The conversion of ctime to
be a ktime_t value has been dropped, and we now use an unused bit in
the nsec field as the QUERIED flag (like the earlier patchset did).

The floor value is now tracked as a monotonic clock value, and is
converted to a realtime value on an as-needed basis. This eliminates the
problem of trying to detect when the realtime clock jumps backward.

Longer patch description for those just joining in:

At LSF/MM this year, we had a discussion about the inode change
attribute. At the time I mentioned that I thought I could salvage the
multigrain timestamp work that had to be reverted last year [1].

That version had to be reverted because it was possible for a file to
get a coarse grained timestamp that appeared to be earlier than another
file that had recently gotten a fine-grained stamp.

This version corrects the problem by establishing a per-time_namespace
ctime_floor value that should prevent this from occurring. In the above
situation, the two files might end up with the same timestamp value, but
they won't appear to have been modified in the wrong order.

That problem was discovered by the test-stat-time gnulib test. Note that
that test still fails on multigrain timestamps, but that's because its
method of determining the minimum delay that will show a timestamp
change will no longer work with multigrain timestamps. I have a patch to
change the testcase to use a different method that is in the process of
being merged.

The testing I've done seems to show performance parity with multigrain
timestamps enabled vs. disabled, but it's hard to rule this out
regressing some workload.

This set is based on top of Christian's vfs.misc branch (which has the
earlier change to track inode timestamps as discrete integers). If there
are no major objections, I'd like to have this considered for v6.12,
after a nice long full-cycle soak in linux-next.

PS: I took a stab at a conversion for bcachefs too, but it's not
trivial. bcachefs handles timestamps backward from the way most
block-based filesystems do. Instead of updating them in struct inode and
eventually copying them to a disk-based representation, it does the
reverse and updates the timestamps in its in-core image of the on-disk
inode, and then copies that into struct inode. Either that will need to
be changed, or we'll need to come up with a different way to do this for
bcachefs.

[1]: https://lore.kernel.org/linux-fsdevel/20230807-mgctime-v7-0-d1dec143a704@kernel.org/

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v5:
- refetch coarse time in coarse_ctime if not returning floor
- timestamp_truncate before swapping new ctime value into place
- track floor value as atomic64_t
- cleanups to Documentation file
- Link to v4: https://lore.kernel.org/r/20240708-mgtime-v4-0-a0f3c6fb57f3@kernel.org

Changes in v4:
- reordered tracepoint fields for better packing
- rework percpu counters again to also count fine grained timestamps
- switch to try_cmpxchg for better efficiency
- Link to v3: https://lore.kernel.org/r/20240705-mgtime-v3-0-85b2daa9b335@kernel.org

Changes in v3:
- Drop the conversion of i_ctime fields to ktime_t, and use an unused bit
  of the i_ctime_nsec field as QUERIED flag.
- Better tracepoints for tracking floor and ctime updates
- Reworked percpu counters to be more useful
- Track floor as monotonic value, which eliminates clock-jump problem

Changes in v2:
- Added Documentation file
- Link to v1: https://lore.kernel.org/r/20240626-mgtime-v1-0-a189352d0f8f@kernel.org

---
Jeff Layton (9):
      fs: add infrastructure for multigrain timestamps
      fs: tracepoints around multigrain timestamp events
      fs: add percpu counters for significant multigrain timestamp events
      fs: have setattr_copy handle multigrain timestamps appropriately
      Documentation: add a new file documenting multigrain timestamps
      xfs: switch to multigrain timestamps
      ext4: switch to multigrain timestamps
      btrfs: convert to multigrain timestamps
      tmpfs: add support for multigrain timestamps

 Documentation/filesystems/multigrain-ts.rst | 120 ++++++++++++++
 fs/attr.c                                   |  52 ++++++-
 fs/btrfs/file.c                             |  25 +--
 fs/btrfs/super.c                            |   3 +-
 fs/ext4/super.c                             |   2 +-
 fs/inode.c                                  | 234 ++++++++++++++++++++++++----
 fs/stat.c                                   |  39 ++++-
 fs/xfs/libxfs/xfs_trans_inode.c             |   6 +-
 fs/xfs/xfs_iops.c                           |  10 +-
 fs/xfs/xfs_super.c                          |   2 +-
 include/linux/fs.h                          |  34 +++-
 include/trace/events/timestamp.h            | 109 +++++++++++++
 mm/shmem.c                                  |   2 +-
 13 files changed, 560 insertions(+), 78 deletions(-)
---
base-commit: 7507ae6c41bb8990d3ae98ad0f5b0c15ca4156fe
change-id: 20240626-mgtime-5cd80b18d810

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH v5 1/9] fs: add infrastructure for multigrain timestamps

[Jeff Layton]

The VFS has always used coarse-grained timestamps when updating the
ctime and mtime after a change. This has the benefit of allowing
filesystems to optimize away a lot metadata updates, down to around 1
per jiffy, even when a file is under heavy writes.

Unfortunately, this has always been an issue when we're exporting via
NFSv3, which relies on timestamps to validate caches. A lot of changes
can happen in a jiffy, so timestamps aren't sufficient to help the
client decide when to invalidate the cache. Even with NFSv4, a lot of
exported filesystems don't properly support a change attribute and are
subject to the same problems with timestamp granularity. Other
applications have similar issues with timestamps (e.g backup
applications).

If we were to always use fine-grained timestamps, that would improve the
situation, but that becomes rather expensive, as the underlying
filesystem would have to log a lot more metadata updates.

What we need is a way to only use fine-grained timestamps when they are
being actively queried. Use the (unused) top bit in inode->i_ctime_nsec
as a flag that indicates whether the current timestamps have been
queried via stat() or the like. When it's set, we allow the kernel to
use a fine-grained timestamp iff it's necessary to make the ctime show
a different value.

This solves the problem of being able to distinguish the timestamp
between updates, but introduces a new problem: it's now possible for a
file being changed to get a fine-grained timestamp. A file that is
altered just a bit later can then get a coarse-grained one that appears
older than the earlier fine-grained time. This violates timestamp
ordering guarantees.

To remedy this, keep a global monotonic ktime_t value that acts as a
timestamp floor.  When we go to stamp a file, we first get the latter of
the current floor value and the current coarse-grained time. If the
inode ctime hasn't been queried then we just attempt to stamp it with
that value.

If it has been queried, then first see whether the current coarse time
is later than the existing ctime. If it is, then we accept that value.
If it isn't, then we get a fine-grained time and try to swap that into
the global floor. Whether that succeeds or fails, we take the resulting
floor time, convert it to realtime and try to swap that into the ctime.

We take the result of the ctime swap whether it succeeds or fails, since
either is just as valid.

Filesystems can opt into this by setting the FS_MGTIME fstype flag.
Others should be unaffected (other than being subject to the same floor
value as multigrain filesystems).

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/inode.c         | 171 ++++++++++++++++++++++++++++++++++++++++++++---------
 fs/stat.c          |  36 ++++++++++-
 include/linux/fs.h |  34 ++++++++---
 3 files changed, 204 insertions(+), 37 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index f356fe2ec2b6..2b5889ff7b36 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -60,6 +60,13 @@ static unsigned int i_hash_shift __ro_after_init;
 static struct hlist_head *inode_hashtable __ro_after_init;
 static __cacheline_aligned_in_smp DEFINE_SPINLOCK(inode_hash_lock);
 
+/*
+ * This represents the latest fine-grained time that we have handed out as a
+ * timestamp on the system. Tracked as a monotonic value, and converted to the
+ * realtime clock on an as-needed basis.
+ */
+static __cacheline_aligned_in_smp atomic64_t ctime_floor;
+
 /*
  * Empty aops. Can be used for the cases where the user does not
  * define any of the address_space operations.
@@ -2127,19 +2134,72 @@ int file_remove_privs(struct file *file)
 }
 EXPORT_SYMBOL(file_remove_privs);
 
+/**
+ * coarse_ctime - return the current coarse-grained time
+ * @floor: current (monotonic) ctime_floor value
+ *
+ * Get the coarse-grained time, and then determine whether to
+ * return it or the current floor value. Returns the later of the
+ * floor and coarse grained timestamps, converted to realtime
+ * clock value.
+ */
+static ktime_t coarse_ctime(ktime_t floor)
+{
+	ktime_t coarse = ktime_get_coarse();
+
+	/* If coarse time is already newer, return that */
+	if (!ktime_after(floor, coarse))
+		return ktime_get_coarse_real();
+	return ktime_mono_to_real(floor);
+}
+
+/**
+ * current_time - Return FS time (possibly fine-grained)
+ * @inode: inode.
+ *
+ * Return the current time truncated to the time granularity supported by
+ * the fs, as suitable for a ctime/mtime change. If the ctime is flagged
+ * as having been QUERIED, get a fine-grained timestamp.
+ */
+struct timespec64 current_time(struct inode *inode)
+{
+	ktime_t floor = atomic64_read(&ctime_floor);
+	ktime_t now = coarse_ctime(floor);
+	struct timespec64 now_ts = ktime_to_timespec64(now);
+	u32 cns;
+
+	if (!is_mgtime(inode))
+		goto out;
+
+	/* If nothing has queried it, then coarse time is fine */
+	cns = smp_load_acquire(&inode->i_ctime_nsec);
+	if (cns & I_CTIME_QUERIED) {
+		/*
+		 * If there is no apparent change, then
+		 * get a fine-grained timestamp.
+		 */
+		if (now_ts.tv_nsec == (cns & ~I_CTIME_QUERIED))
+			ktime_get_real_ts64(&now_ts);
+	}
+out:
+	return timestamp_truncate(now_ts, inode);
+}
+EXPORT_SYMBOL(current_time);
+
 static int inode_needs_update_time(struct inode *inode)
 {
+	struct timespec64 now, ts;
 	int sync_it = 0;
-	struct timespec64 now = current_time(inode);
-	struct timespec64 ts;
 
 	/* First try to exhaust all avenues to not sync */
 	if (IS_NOCMTIME(inode))
 		return 0;
 
+	now = current_time(inode);
+
 	ts = inode_get_mtime(inode);
 	if (!timespec64_equal(&ts, &now))
-		sync_it = S_MTIME;
+		sync_it |= S_MTIME;
 
 	ts = inode_get_ctime(inode);
 	if (!timespec64_equal(&ts, &now))
@@ -2507,6 +2567,14 @@ void inode_nohighmem(struct inode *inode)
 }
 EXPORT_SYMBOL(inode_nohighmem);
 
+struct timespec64 inode_set_ctime_to_ts(struct inode *inode, struct timespec64 ts)
+{
+	inode->i_ctime_sec = ts.tv_sec;
+	inode->i_ctime_nsec = ts.tv_nsec & ~I_CTIME_QUERIED;
+	return ts;
+}
+EXPORT_SYMBOL(inode_set_ctime_to_ts);
+
 /**
  * timestamp_truncate - Truncate timespec to a granularity
  * @t: Timespec
@@ -2538,38 +2606,87 @@ struct timespec64 timestamp_truncate(struct timespec64 t, struct inode *inode)
 }
 EXPORT_SYMBOL(timestamp_truncate);
 
-/**
- * current_time - Return FS time
- * @inode: inode.
- *
- * Return the current time truncated to the time granularity supported by
- * the fs.
- *
- * Note that inode and inode->sb cannot be NULL.
- * Otherwise, the function warns and returns time without truncation.
- */
-struct timespec64 current_time(struct inode *inode)
-{
-	struct timespec64 now;
-
-	ktime_get_coarse_real_ts64(&now);
-	return timestamp_truncate(now, inode);
-}
-EXPORT_SYMBOL(current_time);
-
 /**
  * inode_set_ctime_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.
+ * Set the inode's ctime to the current value for the inode. Returns the
+ * current value that was assigned. If this is not a multigrain inode, then we
+ * just set it to whatever the coarse_ctime is.
+ *
+ * If it is multigrain, then we first see if the coarse-grained timestamp is
+ * distinct from what we have. If so, then we'll just use that. If we have to
+ * get a fine-grained timestamp, then do so, and try to swap it into the floor.
+ * We accept the new floor value regardless of the outcome of the cmpxchg.
+ * After that, we try to swap the new value into i_ctime_nsec. Again, we take
+ * the resulting ctime, regardless of the outcome of the swap.
  */
 struct timespec64 inode_set_ctime_current(struct inode *inode)
 {
-	struct timespec64 now = current_time(inode);
+	ktime_t now, floor = atomic64_read(&ctime_floor);
+	struct timespec64 now_ts;
+	u32 cns, cur;
+
+	now = coarse_ctime(floor);
 
-	inode_set_ctime_to_ts(inode, now);
-	return now;
+	/* Just return that if this is not a multigrain fs */
+	if (!is_mgtime(inode)) {
+		now_ts = timestamp_truncate(ktime_to_timespec64(now), inode);
+		inode_set_ctime_to_ts(inode, now_ts);
+		goto out;
+	}
+
+	/*
+	 * We only need a fine-grained time if someone has queried it,
+	 * and the current coarse grained time isn't later than what's
+	 * already there.
+	 */
+	cns = smp_load_acquire(&inode->i_ctime_nsec);
+	if (cns & I_CTIME_QUERIED) {
+		ktime_t ctime = ktime_set(inode->i_ctime_sec, cns & ~I_CTIME_QUERIED);
+
+		if (!ktime_after(now, ctime)) {
+			ktime_t old, fine;
+
+			/* Get a fine-grained time */
+			fine = ktime_get();
+
+			/*
+			 * If the cmpxchg works, we take the new floor value. If
+			 * not, then that means that someone else changed it after we
+			 * fetched it but before we got here. That value is just
+			 * as good, so keep it.
+			 */
+			old = floor;
+			if (!atomic64_try_cmpxchg(&ctime_floor, &old, fine))
+				fine = old;
+			now = ktime_mono_to_real(fine);
+		}
+	}
+	now_ts = timestamp_truncate(ktime_to_timespec64(now), inode);
+	cur = cns;
+retry:
+	/* Try to swap the nsec value into place. */
+	if (try_cmpxchg(&inode->i_ctime_nsec, &cur, now_ts.tv_nsec)) {
+		/* If swap occurred, then we're (mostly) done */
+		inode->i_ctime_sec = now_ts.tv_sec;
+	} else {
+		/*
+		 * Was the change due to someone marking the old ctime QUERIED?
+		 * If so then retry the swap. This can only happen once since
+		 * the only way to clear I_CTIME_QUERIED is to stamp the inode
+		 * with a new ctime.
+		 */
+		if (!(cns & I_CTIME_QUERIED) && (cns | I_CTIME_QUERIED) == cur) {
+			cns = cur;
+			goto retry;
+		}
+		/* Otherwise, keep the existing ctime */
+		now_ts.tv_sec = inode->i_ctime_sec;
+		now_ts.tv_nsec = cur & ~I_CTIME_QUERIED;
+	}
+out:
+	return now_ts;
 }
 EXPORT_SYMBOL(inode_set_ctime_current);
 
diff --git a/fs/stat.c b/fs/stat.c
index 6f65b3456cad..df7fdd3afed9 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -26,6 +26,32 @@
 #include "internal.h"
 #include "mount.h"
 
+/**
+ * fill_mg_cmtime - Fill in the mtime and ctime and flag ctime as QUERIED
+ * @stat: where to store the resulting values
+ * @request_mask: STATX_* values requested
+ * @inode: inode from which to grab the c/mtime
+ *
+ * Given @inode, grab the ctime and mtime out if it and store the result
+ * in @stat. When fetching the value, flag it as queried so the next write
+ * will ensure a distinct timestamp.
+ */
+void fill_mg_cmtime(struct kstat *stat, u32 request_mask, struct inode *inode)
+{
+	atomic_t *pcn = (atomic_t *)&inode->i_ctime_nsec;
+
+	/* If neither time was requested, then don't report them */
+	if (!(request_mask & (STATX_CTIME|STATX_MTIME))) {
+		stat->result_mask &= ~(STATX_CTIME|STATX_MTIME);
+		return;
+	}
+
+	stat->mtime = inode_get_mtime(inode);
+	stat->ctime.tv_sec = inode->i_ctime_sec;
+	stat->ctime.tv_nsec = ((u32)atomic_fetch_or(I_CTIME_QUERIED, pcn)) & ~I_CTIME_QUERIED;
+}
+EXPORT_SYMBOL(fill_mg_cmtime);
+
 /**
  * generic_fillattr - Fill in the basic attributes from the inode struct
  * @idmap:		idmap of the mount the inode was found from
@@ -58,8 +84,14 @@ void generic_fillattr(struct mnt_idmap *idmap, u32 request_mask,
 	stat->rdev = inode->i_rdev;
 	stat->size = i_size_read(inode);
 	stat->atime = inode_get_atime(inode);
-	stat->mtime = inode_get_mtime(inode);
-	stat->ctime = inode_get_ctime(inode);
+
+	if (is_mgtime(inode)) {
+		fill_mg_cmtime(stat, request_mask, inode);
+	} else {
+		stat->ctime = inode_get_ctime(inode);
+		stat->mtime = inode_get_mtime(inode);
+	}
+
 	stat->blksize = i_blocksize(inode);
 	stat->blocks = inode->i_blocks;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index dc9f9c4b2572..f873f6c58669 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1608,6 +1608,17 @@ static inline struct timespec64 inode_set_mtime(struct inode *inode,
 	return inode_set_mtime_to_ts(inode, ts);
 }
 
+/*
+ * Multigrain timestamps
+ *
+ * Conditionally use fine-grained ctime and mtime timestamps when there
+ * are users actively observing them via getattr. The primary use-case
+ * for this is NFS clients that use the ctime to distinguish between
+ * different states of the file, and that are often fooled by multiple
+ * operations that occur in the same coarse-grained timer tick.
+ */
+#define I_CTIME_QUERIED		((u32)BIT(31))
+
 static inline time64_t inode_get_ctime_sec(const struct inode *inode)
 {
 	return inode->i_ctime_sec;
@@ -1615,7 +1626,7 @@ static inline time64_t inode_get_ctime_sec(const struct inode *inode)
 
 static inline long inode_get_ctime_nsec(const struct inode *inode)
 {
-	return inode->i_ctime_nsec;
+	return inode->i_ctime_nsec & ~I_CTIME_QUERIED;
 }
 
 static inline struct timespec64 inode_get_ctime(const struct inode *inode)
@@ -1626,13 +1637,7 @@ static inline struct timespec64 inode_get_ctime(const struct inode *inode)
 	return ts;
 }
 
-static inline struct timespec64 inode_set_ctime_to_ts(struct inode *inode,
-						      struct timespec64 ts)
-{
-	inode->i_ctime_sec = ts.tv_sec;
-	inode->i_ctime_nsec = ts.tv_nsec;
-	return ts;
-}
+struct timespec64 inode_set_ctime_to_ts(struct inode *inode, struct timespec64 ts);
 
 /**
  * inode_set_ctime - set the ctime in the inode
@@ -2490,6 +2495,7 @@ struct file_system_type {
 #define FS_USERNS_MOUNT		8	/* Can be mounted by userns root */
 #define FS_DISALLOW_NOTIFY_PERM	16	/* Disable fanotify permission events */
 #define FS_ALLOW_IDMAP         32      /* FS has been updated to handle vfs idmappings. */
+#define FS_MGTIME		64	/* FS uses multigrain timestamps */
 #define FS_RENAME_DOES_D_MOVE	32768	/* FS will handle d_move() during rename() internally. */
 	int (*init_fs_context)(struct fs_context *);
 	const struct fs_parameter_spec *parameters;
@@ -2513,6 +2519,17 @@ struct file_system_type {
 
 #define MODULE_ALIAS_FS(NAME) MODULE_ALIAS("fs-" NAME)
 
+/**
+ * is_mgtime: is this inode using multigrain timestamps
+ * @inode: inode to test for multigrain timestamps
+ *
+ * Return true if the inode uses multigrain timestamps, false otherwise.
+ */
+static inline bool is_mgtime(const struct inode *inode)
+{
+	return inode->i_sb->s_type->fs_flags & FS_MGTIME;
+}
+
 extern struct dentry *mount_bdev(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data,
 	int (*fill_super)(struct super_block *, void *, int));
@@ -3252,6 +3269,7 @@ extern void page_put_link(void *);
 extern int page_symlink(struct inode *inode, const char *symname, int len);
 extern const struct inode_operations page_symlink_inode_operations;
 extern void kfree_link(void *);
+void fill_mg_cmtime(struct kstat *stat, u32 request_mask, struct inode *inode);
 void generic_fillattr(struct mnt_idmap *, u32, struct inode *, struct kstat *);
 void generic_fill_statx_attr(struct inode *inode, struct kstat *stat);
 extern int vfs_getattr_nosec(const struct path *, struct kstat *, u32, unsigned int);

-- 
2.45.2

[PATCH v5 2/9] fs: tracepoints around multigrain timestamp events

[Jeff Layton]

Add some tracepoints around various multigrain timestamp events.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/inode.c                       |   5 ++
 fs/stat.c                        |   3 ++
 include/trace/events/timestamp.h | 109 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 117 insertions(+)

diff --git a/fs/inode.c b/fs/inode.c
index 2b5889ff7b36..81b45e0a95a6 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -22,6 +22,9 @@
 #include <linux/iversion.h>
 #include <linux/rw_hint.h>
 #include <trace/events/writeback.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/timestamp.h>
+
 #include "internal.h"
 
 /*
@@ -2571,6 +2574,7 @@ struct timespec64 inode_set_ctime_to_ts(struct inode *inode, struct timespec64 t
 {
 	inode->i_ctime_sec = ts.tv_sec;
 	inode->i_ctime_nsec = ts.tv_nsec & ~I_CTIME_QUERIED;
+	trace_inode_set_ctime_to_ts(inode, &ts);
 	return ts;
 }
 EXPORT_SYMBOL(inode_set_ctime_to_ts);
@@ -2670,6 +2674,7 @@ struct timespec64 inode_set_ctime_current(struct inode *inode)
 	if (try_cmpxchg(&inode->i_ctime_nsec, &cur, now_ts.tv_nsec)) {
 		/* If swap occurred, then we're (mostly) done */
 		inode->i_ctime_sec = now_ts.tv_sec;
+		trace_ctime_ns_xchg(inode, cns, now_ts.tv_nsec, cur);
 	} else {
 		/*
 		 * Was the change due to someone marking the old ctime QUERIED?
diff --git a/fs/stat.c b/fs/stat.c
index df7fdd3afed9..552dfd67688b 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -23,6 +23,8 @@
 #include <linux/uaccess.h>
 #include <asm/unistd.h>
 
+#include <trace/events/timestamp.h>
+
 #include "internal.h"
 #include "mount.h"
 
@@ -49,6 +51,7 @@ void fill_mg_cmtime(struct kstat *stat, u32 request_mask, struct inode *inode)
 	stat->mtime = inode_get_mtime(inode);
 	stat->ctime.tv_sec = inode->i_ctime_sec;
 	stat->ctime.tv_nsec = ((u32)atomic_fetch_or(I_CTIME_QUERIED, pcn)) & ~I_CTIME_QUERIED;
+	trace_fill_mg_cmtime(inode, &stat->ctime, &stat->mtime);
 }
 EXPORT_SYMBOL(fill_mg_cmtime);
 
diff --git a/include/trace/events/timestamp.h b/include/trace/events/timestamp.h
new file mode 100644
index 000000000000..3a603190b46c
--- /dev/null
+++ b/include/trace/events/timestamp.h
@@ -0,0 +1,109 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM timestamp
+
+#if !defined(_TRACE_TIMESTAMP_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_TIMESTAMP_H
+
+#include <linux/tracepoint.h>
+#include <linux/fs.h>
+
+TRACE_EVENT(inode_set_ctime_to_ts,
+	TP_PROTO(struct inode *inode,
+		 struct timespec64 *ctime),
+
+	TP_ARGS(inode, ctime),
+
+	TP_STRUCT__entry(
+		__field(dev_t,			dev)
+		__field(ino_t,			ino)
+		__field(time64_t,		ctime_s)
+		__field(u32,			ctime_ns)
+		__field(u32,			gen)
+	),
+
+	TP_fast_assign(
+		__entry->dev		= inode->i_sb->s_dev;
+		__entry->ino		= inode->i_ino;
+		__entry->gen		= inode->i_generation;
+		__entry->ctime_s	= ctime->tv_sec;
+		__entry->ctime_ns	= ctime->tv_nsec;
+	),
+
+	TP_printk("ino=%d:%d:%ld:%u ctime=%lld.%u",
+		MAJOR(__entry->dev), MINOR(__entry->dev), __entry->ino, __entry->gen,
+		__entry->ctime_s, __entry->ctime_ns
+	)
+);
+
+TRACE_EVENT(ctime_ns_xchg,
+	TP_PROTO(struct inode *inode,
+		 u32 old,
+		 u32 new,
+		 u32 cur),
+
+	TP_ARGS(inode, old, new, cur),
+
+	TP_STRUCT__entry(
+		__field(dev_t,				dev)
+		__field(ino_t,				ino)
+		__field(u32,				gen)
+		__field(u32,				old)
+		__field(u32,				new)
+		__field(u32,				cur)
+	),
+
+	TP_fast_assign(
+		__entry->dev		= inode->i_sb->s_dev;
+		__entry->ino		= inode->i_ino;
+		__entry->gen		= inode->i_generation;
+		__entry->old		= old;
+		__entry->new		= new;
+		__entry->cur		= cur;
+	),
+
+	TP_printk("ino=%d:%d:%ld:%u old=%u:%c new=%u cur=%u:%c",
+		MAJOR(__entry->dev), MINOR(__entry->dev), __entry->ino, __entry->gen,
+		__entry->old & ~I_CTIME_QUERIED, __entry->old & I_CTIME_QUERIED ? 'Q' : '-',
+		__entry->new,
+		__entry->cur & ~I_CTIME_QUERIED, __entry->cur & I_CTIME_QUERIED ? 'Q' : '-'
+	)
+);
+
+TRACE_EVENT(fill_mg_cmtime,
+	TP_PROTO(struct inode *inode,
+		 struct timespec64 *ctime,
+		 struct timespec64 *mtime),
+
+	TP_ARGS(inode, ctime, mtime),
+
+	TP_STRUCT__entry(
+		__field(dev_t,			dev)
+		__field(ino_t,			ino)
+		__field(time64_t,		ctime_s)
+		__field(time64_t,		mtime_s)
+		__field(u32,			ctime_ns)
+		__field(u32,			mtime_ns)
+		__field(u32,			gen)
+	),
+
+	TP_fast_assign(
+		__entry->dev		= inode->i_sb->s_dev;
+		__entry->ino		= inode->i_ino;
+		__entry->gen		= inode->i_generation;
+		__entry->ctime_s	= ctime->tv_sec;
+		__entry->mtime_s	= mtime->tv_sec;
+		__entry->ctime_ns	= ctime->tv_nsec;
+		__entry->mtime_ns	= mtime->tv_nsec;
+	),
+
+	TP_printk("ino=%d:%d:%ld:%u ctime=%lld.%u mtime=%lld.%u",
+		MAJOR(__entry->dev), MINOR(__entry->dev), __entry->ino, __entry->gen,
+		__entry->ctime_s, __entry->ctime_ns,
+		__entry->mtime_s, __entry->mtime_ns
+	)
+);
+#endif /* _TRACE_TIMESTAMP_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>

-- 
2.45.2

[PATCH v5 3/9] fs: add percpu counters for significant multigrain timestamp events

[Jeff Layton]

Four percpu counters for counting various stats around mgtimes, and a
new debugfs file for displaying them:

- number of attempted ctime updates
- number of successful i_ctime_nsec swaps
- number of fine-grained timestamp fetches
- number of floor value swaps

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/inode.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/fs/inode.c b/fs/inode.c
index 81b45e0a95a6..011148c82901 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -21,6 +21,8 @@
 #include <linux/list_lru.h>
 #include <linux/iversion.h>
 #include <linux/rw_hint.h>
+#include <linux/seq_file.h>
+#include <linux/debugfs.h>
 #include <trace/events/writeback.h>
 #define CREATE_TRACE_POINTS
 #include <trace/events/timestamp.h>
@@ -70,6 +72,11 @@ static __cacheline_aligned_in_smp DEFINE_SPINLOCK(inode_hash_lock);
  */
 static __cacheline_aligned_in_smp atomic64_t ctime_floor;
 
+static struct percpu_counter mg_ctime_updates;
+static struct percpu_counter mg_floor_swaps;
+static struct percpu_counter mg_ctime_swaps;
+static struct percpu_counter mg_fine_stamps;
+
 /*
  * Empty aops. Can be used for the cases where the user does not
  * define any of the address_space operations.
@@ -2654,6 +2661,7 @@ struct timespec64 inode_set_ctime_current(struct inode *inode)
 
 			/* Get a fine-grained time */
 			fine = ktime_get();
+			percpu_counter_inc(&mg_fine_stamps);
 
 			/*
 			 * If the cmpxchg works, we take the new floor value. If
@@ -2662,11 +2670,14 @@ struct timespec64 inode_set_ctime_current(struct inode *inode)
 			 * as good, so keep it.
 			 */
 			old = floor;
-			if (!atomic64_try_cmpxchg(&ctime_floor, &old, fine))
+			if (atomic64_try_cmpxchg(&ctime_floor, &old, fine))
+				percpu_counter_inc(&mg_floor_swaps);
+			else
 				fine = old;
 			now = ktime_mono_to_real(fine);
 		}
 	}
+	percpu_counter_inc(&mg_ctime_updates);
 	now_ts = timestamp_truncate(ktime_to_timespec64(now), inode);
 	cur = cns;
 retry:
@@ -2675,6 +2686,7 @@ struct timespec64 inode_set_ctime_current(struct inode *inode)
 		/* If swap occurred, then we're (mostly) done */
 		inode->i_ctime_sec = now_ts.tv_sec;
 		trace_ctime_ns_xchg(inode, cns, now_ts.tv_nsec, cur);
+		percpu_counter_inc(&mg_ctime_swaps);
 	} else {
 		/*
 		 * Was the change due to someone marking the old ctime QUERIED?
@@ -2744,3 +2756,49 @@ umode_t mode_strip_sgid(struct mnt_idmap *idmap,
 	return mode & ~S_ISGID;
 }
 EXPORT_SYMBOL(mode_strip_sgid);
+
+static int mgts_show(struct seq_file *s, void *p)
+{
+	u64 ctime_updates = percpu_counter_sum(&mg_ctime_updates);
+	u64 ctime_swaps = percpu_counter_sum(&mg_ctime_swaps);
+	u64 fine_stamps = percpu_counter_sum(&mg_fine_stamps);
+	u64 floor_swaps = percpu_counter_sum(&mg_floor_swaps);
+
+	seq_printf(s, "%llu %llu %llu %llu\n",
+		   ctime_updates, ctime_swaps, fine_stamps, floor_swaps);
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(mgts);
+
+static int __init mg_debugfs_init(void)
+{
+	int ret = percpu_counter_init(&mg_ctime_updates, 0, GFP_KERNEL);
+
+	if (ret)
+		return ret;
+
+	ret = percpu_counter_init(&mg_floor_swaps, 0, GFP_KERNEL);
+	if (ret) {
+		percpu_counter_destroy(&mg_ctime_updates);
+		return ret;
+	}
+
+	ret = percpu_counter_init(&mg_ctime_swaps, 0, GFP_KERNEL);
+	if (ret) {
+		percpu_counter_destroy(&mg_floor_swaps);
+		percpu_counter_destroy(&mg_ctime_updates);
+		return ret;
+	}
+
+	ret = percpu_counter_init(&mg_fine_stamps, 0, GFP_KERNEL);
+	if (ret) {
+		percpu_counter_destroy(&mg_floor_swaps);
+		percpu_counter_destroy(&mg_ctime_updates);
+		percpu_counter_destroy(&mg_ctime_swaps);
+		return ret;
+	}
+	debugfs_create_file("multigrain_timestamps", S_IFREG | S_IRUGO, NULL, NULL, &mgts_fops);
+	return 0;
+}
+late_initcall(mg_debugfs_init);

-- 
2.45.2

[PATCH v5 4/9] fs: have setattr_copy handle multigrain timestamps appropriately

[Jeff Layton]

The setattr codepath is still using coarse-grained timestamps, even on
multigrain filesystems. To fix this, we need to fetch the timestamp for
ctime updates later, at the point where the assignment occurs in
setattr_copy.

On a multigrain inode, ignore the ia_ctime in the attrs, and always
update the ctime to the current clock value. Update the atime and mtime
with the same value (if needed) unless they are being set to other
specific values, a'la utimes().

Note that we don't want to do this universally however, as some
filesystems (e.g. most networked fs) want to do an explicit update
elsewhere before updating the local inode.

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

diff --git a/fs/attr.c b/fs/attr.c
index 825007d5cda4..e03ea6951864 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -271,6 +271,42 @@ int inode_newsize_ok(const struct inode *inode, loff_t offset)
 }
 EXPORT_SYMBOL(inode_newsize_ok);
 
+/**
+ * setattr_copy_mgtime - update timestamps for mgtime inodes
+ * @inode: inode timestamps to be updated
+ * @attr: attrs for the update
+ *
+ * With multigrain timestamps, we need to take more care to prevent races
+ * when updating the ctime. Always update the ctime to the very latest
+ * using the standard mechanism, and use that to populate the atime and
+ * mtime appropriately (unless we're setting those to specific values).
+ */
+static void setattr_copy_mgtime(struct inode *inode, const struct iattr *attr)
+{
+	unsigned int ia_valid = attr->ia_valid;
+	struct timespec64 now;
+
+	/*
+	 * If the ctime isn't being updated then nothing else should be
+	 * either.
+	 */
+	if (!(ia_valid & ATTR_CTIME)) {
+		WARN_ON_ONCE(ia_valid & (ATTR_ATIME|ATTR_MTIME));
+		return;
+	}
+
+	now = inode_set_ctime_current(inode);
+	if (ia_valid & ATTR_ATIME_SET)
+		inode_set_atime_to_ts(inode, attr->ia_atime);
+	else if (ia_valid & ATTR_ATIME)
+		inode_set_atime_to_ts(inode, now);
+
+	if (ia_valid & ATTR_MTIME_SET)
+		inode_set_mtime_to_ts(inode, attr->ia_mtime);
+	else if (ia_valid & ATTR_MTIME)
+		inode_set_mtime_to_ts(inode, now);
+}
+
 /**
  * setattr_copy - copy simple metadata updates into the generic inode
  * @idmap:	idmap of the mount the inode was found from
@@ -303,12 +339,6 @@ void setattr_copy(struct mnt_idmap *idmap, struct inode *inode,
 
 	i_uid_update(idmap, attr, inode);
 	i_gid_update(idmap, attr, inode);
-	if (ia_valid & ATTR_ATIME)
-		inode_set_atime_to_ts(inode, attr->ia_atime);
-	if (ia_valid & ATTR_MTIME)
-		inode_set_mtime_to_ts(inode, attr->ia_mtime);
-	if (ia_valid & ATTR_CTIME)
-		inode_set_ctime_to_ts(inode, attr->ia_ctime);
 	if (ia_valid & ATTR_MODE) {
 		umode_t mode = attr->ia_mode;
 		if (!in_group_or_capable(idmap, inode,
@@ -316,6 +346,16 @@ void setattr_copy(struct mnt_idmap *idmap, struct inode *inode,
 			mode &= ~S_ISGID;
 		inode->i_mode = mode;
 	}
+
+	if (is_mgtime(inode))
+		return setattr_copy_mgtime(inode, attr);
+
+	if (ia_valid & ATTR_ATIME)
+		inode_set_atime_to_ts(inode, attr->ia_atime);
+	if (ia_valid & ATTR_MTIME)
+		inode_set_mtime_to_ts(inode, attr->ia_mtime);
+	if (ia_valid & ATTR_CTIME)
+		inode_set_ctime_to_ts(inode, attr->ia_ctime);
 }
 EXPORT_SYMBOL(setattr_copy);
 

-- 
2.45.2

[PATCH v5 5/9] Documentation: add a new file documenting multigrain timestamps

[Jeff Layton]

Add a high-level document that describes how multigrain timestamps work,
rationale for them, and some info about implementation and tradeoffs.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 Documentation/filesystems/multigrain-ts.rst | 120 ++++++++++++++++++++++++++++
 1 file changed, 120 insertions(+)

diff --git a/Documentation/filesystems/multigrain-ts.rst b/Documentation/filesystems/multigrain-ts.rst
new file mode 100644
index 000000000000..5cefc204ecec
--- /dev/null
+++ b/Documentation/filesystems/multigrain-ts.rst
@@ -0,0 +1,120 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=====================
+Multigrain Timestamps
+=====================
+
+Introduction
+============
+Historically, the kernel has always used coarse time values to stamp
+inodes. This value is updated on every jiffy, so any change that happens
+within that jiffy will end up with the same timestamp.
+
+When the kernel goes to stamp an inode (due to a read or write), it first gets
+the current time and then compares it to the existing timestamp(s) to see
+whether anything will change. If nothing changed, then it can avoid updating
+the inode's metadata.
+
+Coarse timestamps are therefore good from a performance standpoint, since they
+reduce the need for metadata updates, but bad from the standpoint of
+determining whether anything has changed, since a lot of things can happen in a
+jiffy.
+
+They are particularly troublesome with NFSv3, where unchanging timestamps can
+make it difficult to tell whether to invalidate caches. NFSv4 provides a
+dedicated change attribute that should always show a visible change, but not
+all filesystems implement this properly, causing the NFS server to substitute
+the ctime in many cases.
+
+Multigrain timestamps aim to remedy this by selectively using fine-grained
+timestamps when a file has had its timestamps queried recently, and the current
+coarse-grained time does not cause a change.
+
+Inode Timestamps
+================
+There are currently 3 timestamps in the inode that are updated to the current
+wallclock time on different activity:
+
+ctime:
+  The inode change time. This is stamped with the current time whenever
+  the inode's metadata is changed. Note that this value is not settable
+  from userland.
+
+mtime:
+  The inode modification time. This is stamped with the current time
+  any time a file's contents change.
+
+atime:
+  The inode access time. This is stamped whenever an inode's contents are
+  read. Widely considered to be a terrible mistake. Usually avoided with
+  options like noatime or relatime.
+
+Updating the mtime always implies a change to the ctime, but updating the
+atime due to a read request does not.
+
+Multigrain timestamps are only tracked for the ctime and the mtime. atimes are
+not affected and always use the coarse-grained value (subject to the floor).
+
+Inode Timestamp Ordering
+========================
+
+In addition to just providing info about changes to individual files, file
+timestamps also serve an important purpose in applications like "make". These
+programs measure timestamps in order to determine whether source files might be
+newer than cached objects.
+
+Userland applications like make can only determine ordering based on
+operational boundaries. For a syscall those are the syscall entry and exit
+points. For io_uring or nfsd operations, that's the request submission and
+response. In the case of concurrent operations, userland can make no
+determination about the order in which things will occur.
+
+For instance, if a single thread modifies one file, and then another file in
+sequence, the second file must show an equal or later mtime than the first. The
+same is true if two threads are issuing similar operations that do not overlap
+in time.
+
+If however, two threads have racing syscalls that overlap in time, then there
+is no such guarantee, and the second file may appear to have been modified
+before, after or at the same time as the first, regardless of which one was
+submitted first.
+
+Multigrain Timestamps
+=====================
+Multigrain timestamps are aimed at ensuring that changes to a single file are
+always recognizable, without violating the ordering guarantees when multiple
+different files are modified. This affects the mtime and the ctime, but the
+atime will always use coarse-grained timestamps.
+
+It uses an unused bit in the i_ctime_nsec field to indicate whether the mtime
+or ctime has been queried. If either or both have, then the kernel takes
+special care to ensure the next timestamp update will display a visible change.
+This ensures tight cache coherency for use-cases like NFS, without sacrificing
+the benefits of reduced metadata updates when files aren't being watched.
+
+The Ctime Floor Value
+=====================
+It's not sufficient to simply use fine or coarse-grained timestamps based on
+whether the mtime or ctime has been queried. A file could get a fine grained
+timestamp, and then a second file modified later could get a coarse-grained one
+that appears earlier than the first, which would break the kernel's timestamp
+ordering guarantees.
+
+To mitigate this problem, we maintain a global floor value that ensures that
+this can't happen. The two files in the above example may appear to have been
+modified at the same time in such a case, but they will never show the reverse
+order. To avoid problems with realtime clock jumps, the floor is managed as a
+monotonic ktime_t, and the values are converted to realtime clock values as
+needed.
+
+Implementation Notes
+====================
+Multigrain timestamps are intended for use by local filesystems that get
+ctime values from the local clock. This is in contrast to network filesystems
+and the like that just mirror timestamp values from a server.
+
+For most filesystems, it's sufficient to just set the FS_MGTIME flag in the
+fstype->fs_flags in order to opt-in, providing the ctime is only ever set via
+inode_set_ctime_current(). If the filesystem has a ->getattr routine that
+doesn't call generic_fillattr, then you should have it call fill_mg_cmtime to
+fill those values.

-- 
2.45.2

[PATCH v5 6/9] xfs: switch to multigrain timestamps

[Jeff Layton]

Enable multigrain timestamps, which should ensure that there is an
apparent change to the timestamp whenever it has been written after
being actively observed via getattr.

Also, anytime the mtime changes, the ctime must also change, and those
are now the only two options for xfs_trans_ichgtime. Have that function
unconditionally bump the ctime, and ASSERT that XFS_ICHGTIME_CHG is
always set.

Finally, stop setting STATX_CHANGE_COOKIE in getattr, since the ctime
should give us better semantics now.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/xfs/libxfs/xfs_trans_inode.c |  6 +++---
 fs/xfs/xfs_iops.c               | 10 +++-------
 fs/xfs/xfs_super.c              |  2 +-
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
index 69fc5b981352..1f3639bbf5f0 100644
--- a/fs/xfs/libxfs/xfs_trans_inode.c
+++ b/fs/xfs/libxfs/xfs_trans_inode.c
@@ -62,12 +62,12 @@ xfs_trans_ichgtime(
 	ASSERT(tp);
 	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
 
-	tv = current_time(inode);
+	/* If the mtime changes, then ctime must also change */
+	ASSERT(flags & XFS_ICHGTIME_CHG);
 
+	tv = inode_set_ctime_current(inode);
 	if (flags & XFS_ICHGTIME_MOD)
 		inode_set_mtime_to_ts(inode, tv);
-	if (flags & XFS_ICHGTIME_CHG)
-		inode_set_ctime_to_ts(inode, tv);
 	if (flags & XFS_ICHGTIME_CREATE)
 		ip->i_crtime = tv;
 }
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index a00dcbc77e12..d25872f818fa 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -592,8 +592,9 @@ xfs_vn_getattr(
 	stat->gid = vfsgid_into_kgid(vfsgid);
 	stat->ino = ip->i_ino;
 	stat->atime = inode_get_atime(inode);
-	stat->mtime = inode_get_mtime(inode);
-	stat->ctime = inode_get_ctime(inode);
+
+	fill_mg_cmtime(stat, request_mask, inode);
+
 	stat->blocks = XFS_FSB_TO_BB(mp, ip->i_nblocks + ip->i_delayed_blks);
 
 	if (xfs_has_v3inodes(mp)) {
@@ -603,11 +604,6 @@ xfs_vn_getattr(
 		}
 	}
 
-	if ((request_mask & STATX_CHANGE_COOKIE) && IS_I_VERSION(inode)) {
-		stat->change_cookie = inode_query_iversion(inode);
-		stat->result_mask |= STATX_CHANGE_COOKIE;
-	}
-
 	/*
 	 * Note: If you add another clause to set an attribute flag, please
 	 * update attributes_mask below.
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 27e9f749c4c7..210481b03fdb 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -2052,7 +2052,7 @@ static struct file_system_type xfs_fs_type = {
 	.init_fs_context	= xfs_init_fs_context,
 	.parameters		= xfs_fs_parameters,
 	.kill_sb		= xfs_kill_sb,
-	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
+	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP | FS_MGTIME,
 };
 MODULE_ALIAS_FS("xfs");
 

-- 
2.45.2

[PATCH v5 7/9] ext4: switch to multigrain timestamps

[Jeff Layton]

Enable multigrain timestamps, which should ensure that there is an
apparent change to the timestamp whenever it has been written after
being actively observed via getattr.

For ext4, we only need to enable the FS_MGTIME flag.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ext4/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index eb899628e121..95d4d7c0957a 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -7294,7 +7294,7 @@ static struct file_system_type ext4_fs_type = {
 	.init_fs_context	= ext4_init_fs_context,
 	.parameters		= ext4_param_specs,
 	.kill_sb		= ext4_kill_sb,
-	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
+	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP | FS_MGTIME,
 };
 MODULE_ALIAS_FS("ext4");
 

-- 
2.45.2

[PATCH v5 8/9] btrfs: convert to multigrain timestamps

[Jeff Layton]

Enable multigrain timestamps, which should ensure that there is an
apparent change to the timestamp whenever it has been written after
being actively observed via getattr.

Beyond enabling the FS_MGTIME flag, this patch eliminates
update_time_for_write, which goes to great pains to avoid in-memory
stores. Just have it overwrite the timestamps unconditionally.

Note that this also drops the IS_I_VERSION check and unconditionally
bumps the change attribute, since SB_I_VERSION is always set on btrfs.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/btrfs/file.c  | 25 ++++---------------------
 fs/btrfs/super.c |  3 ++-
 2 files changed, 6 insertions(+), 22 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index d90138683a0a..409628c0c3cc 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1120,26 +1120,6 @@ void btrfs_check_nocow_unlock(struct btrfs_inode *inode)
 	btrfs_drew_write_unlock(&inode->root->snapshot_lock);
 }
 
-static void update_time_for_write(struct inode *inode)
-{
-	struct timespec64 now, ts;
-
-	if (IS_NOCMTIME(inode))
-		return;
-
-	now = current_time(inode);
-	ts = inode_get_mtime(inode);
-	if (!timespec64_equal(&ts, &now))
-		inode_set_mtime_to_ts(inode, now);
-
-	ts = inode_get_ctime(inode);
-	if (!timespec64_equal(&ts, &now))
-		inode_set_ctime_to_ts(inode, now);
-
-	if (IS_I_VERSION(inode))
-		inode_inc_iversion(inode);
-}
-
 static int btrfs_write_check(struct kiocb *iocb, struct iov_iter *from,
 			     size_t count)
 {
@@ -1171,7 +1151,10 @@ static int btrfs_write_check(struct kiocb *iocb, struct iov_iter *from,
 	 * need to start yet another transaction to update the inode as we will
 	 * update the inode when we finish writing whatever data we write.
 	 */
-	update_time_for_write(inode);
+	if (!IS_NOCMTIME(inode)) {
+		inode_set_mtime_to_ts(inode, inode_set_ctime_current(inode));
+		inode_inc_iversion(inode);
+	}
 
 	start_pos = round_down(pos, fs_info->sectorsize);
 	oldsize = i_size_read(inode);
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index f05cce7c8b8d..1cd50293b98d 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2173,7 +2173,8 @@ static struct file_system_type btrfs_fs_type = {
 	.init_fs_context	= btrfs_init_fs_context,
 	.parameters		= btrfs_fs_parameters,
 	.kill_sb		= btrfs_kill_super,
-	.fs_flags		= FS_REQUIRES_DEV | FS_BINARY_MOUNTDATA | FS_ALLOW_IDMAP,
+	.fs_flags		= FS_REQUIRES_DEV | FS_BINARY_MOUNTDATA |
+				  FS_ALLOW_IDMAP | FS_MGTIME,
  };
 
 MODULE_ALIAS_FS("btrfs");

-- 
2.45.2

[PATCH v5 9/9] tmpfs: add support for multigrain timestamps

[Jeff Layton]

Enable multigrain timestamps, which should ensure that there is an
apparent change to the timestamp whenever it has been written after
being actively observed via getattr.

tmpfs only requires the FS_MGTIME flag.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 mm/shmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 7f2b609945a5..75a9a73a769f 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -4660,7 +4660,7 @@ static struct file_system_type shmem_fs_type = {
 	.parameters	= shmem_fs_parameters,
 #endif
 	.kill_sb	= kill_litter_super,
-	.fs_flags	= FS_USERNS_MOUNT | FS_ALLOW_IDMAP,
+	.fs_flags	= FS_USERNS_MOUNT | FS_ALLOW_IDMAP | FS_MGTIME,
 };
 
 void __init shmem_init(void)

-- 
2.45.2

Re: [PATCH v5 0/9] fs: multigrain timestamp redux

[Josef Bacik]

On Thu, Jul 11, 2024 at 07:08:04AM -0400, Jeff Layton wrote:
> tl;dr for those who have been following along:
> 
> There are several changes in this version. The conversion of ctime to
> be a ktime_t value has been dropped, and we now use an unused bit in
> the nsec field as the QUERIED flag (like the earlier patchset did).
> 
> The floor value is now tracked as a monotonic clock value, and is
> converted to a realtime value on an as-needed basis. This eliminates the
> problem of trying to detect when the realtime clock jumps backward.
> 
> Longer patch description for those just joining in:
> 
> At LSF/MM this year, we had a discussion about the inode change
> attribute. At the time I mentioned that I thought I could salvage the
> multigrain timestamp work that had to be reverted last year [1].
> 
> That version had to be reverted because it was possible for a file to
> get a coarse grained timestamp that appeared to be earlier than another
> file that had recently gotten a fine-grained stamp.
> 
> This version corrects the problem by establishing a per-time_namespace
> ctime_floor value that should prevent this from occurring. In the above
> situation, the two files might end up with the same timestamp value, but
> they won't appear to have been modified in the wrong order.
> 
> That problem was discovered by the test-stat-time gnulib test. Note that
> that test still fails on multigrain timestamps, but that's because its
> method of determining the minimum delay that will show a timestamp
> change will no longer work with multigrain timestamps. I have a patch to
> change the testcase to use a different method that is in the process of
> being merged.
> 
> The testing I've done seems to show performance parity with multigrain
> timestamps enabled vs. disabled, but it's hard to rule this out
> regressing some workload.
> 
> This set is based on top of Christian's vfs.misc branch (which has the
> earlier change to track inode timestamps as discrete integers). If there
> are no major objections, I'd like to have this considered for v6.12,
> after a nice long full-cycle soak in linux-next.
> 
> PS: I took a stab at a conversion for bcachefs too, but it's not
> trivial. bcachefs handles timestamps backward from the way most
> block-based filesystems do. Instead of updating them in struct inode and
> eventually copying them to a disk-based representation, it does the
> reverse and updates the timestamps in its in-core image of the on-disk
> inode, and then copies that into struct inode. Either that will need to
> be changed, or we'll need to come up with a different way to do this for
> bcachefs.
> 
> [1]: https://lore.kernel.org/linux-fsdevel/20230807-mgctime-v7-0-d1dec143a704@kernel.org/
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH v5 6/9] xfs: switch to multigrain timestamps

[Darrick J. Wong]

On Thu, Jul 11, 2024 at 07:08:10AM -0400, Jeff Layton wrote:
> Enable multigrain timestamps, which should ensure that there is an
> apparent change to the timestamp whenever it has been written after
> being actively observed via getattr.
> 
> Also, anytime the mtime changes, the ctime must also change, and those
> are now the only two options for xfs_trans_ichgtime. Have that function
> unconditionally bump the ctime, and ASSERT that XFS_ICHGTIME_CHG is
> always set.
> 
> Finally, stop setting STATX_CHANGE_COOKIE in getattr, since the ctime
> should give us better semantics now.

Following up on "As long as the fs isn't touching i_ctime_nsec directly,
you shouldn't need to worry about this" from:
https://lore.kernel.org/linux-xfs/cae5c28f172ac57b7eaaa98a00b23f342f01ba64.camel@kernel.org/

xfs /does/ touch i_ctime_nsec directly when it's writing inodes to disk.
From xfs_inode_to_disk, see:

	to->di_ctime = xfs_inode_to_disk_ts(ip, inode_get_ctime(inode));

AFAICT, inode_get_ctime itself remains unchanged, and still returns
inode->__i_ctime, right?  In which case it's returning a raw timespec64,
which can include the QUERIED flag in tv_nsec, right?

Now let's look at the consumer:

static inline xfs_timestamp_t
xfs_inode_to_disk_ts(
	struct xfs_inode		*ip,
	const struct timespec64		tv)
{
	struct xfs_legacy_timestamp	*lts;
	xfs_timestamp_t			ts;

	if (xfs_inode_has_bigtime(ip))
		return cpu_to_be64(xfs_inode_encode_bigtime(tv));

	lts = (struct xfs_legacy_timestamp *)&ts;
	lts->t_sec = cpu_to_be32(tv.tv_sec);
	lts->t_nsec = cpu_to_be32(tv.tv_nsec);

	return ts;
}

For the !bigtime case (aka before we added y2038 support) the queried
flag gets encoded into the tv_nsec field since xfs doesn't filter the
queried flag.

For the bigtime case, the timespec is turned into an absolute nsec count
since the xfs epoch (which is the minimum timestamp possible under the
old encoding scheme):

static inline uint64_t xfs_inode_encode_bigtime(struct timespec64 tv)
{
	return xfs_unix_to_bigtime(tv.tv_sec) * NSEC_PER_SEC + tv.tv_nsec;
}

Here we'd also be mixing in the QUERIED flag, only now we've encoded a
time that's a second in the future.  I think the solution is to add a:

static inline struct timespec64
inode_peek_ctime(const struct inode *inode)
{
	return (struct timespec64){
		.tv_sec = inode->__i_ctime.tv_sec,
		.tv_nsec = inode->__i_ctime.tv_nsec & ~I_CTIME_QUERIED,
	};
}

similar to what inode_peek_iversion does for iversion; and then
xfs_inode_to_disk can do:

	to->di_ctime = xfs_inode_to_disk_ts(ip, inode_peek_ctime(inode));

which would prevent I_CTIME_QUERIED from going out to disk.

At load time, xfs_inode_from_disk uses inode_set_ctime_to_ts so I think
xfs won't accidentally introduce QUERIED when it's loading an inode from
disk.

--D

> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_trans_inode.c |  6 +++---
>  fs/xfs/xfs_iops.c               | 10 +++-------
>  fs/xfs/xfs_super.c              |  2 +-
>  3 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> index 69fc5b981352..1f3639bbf5f0 100644
> --- a/fs/xfs/libxfs/xfs_trans_inode.c
> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> @@ -62,12 +62,12 @@ xfs_trans_ichgtime(
>  	ASSERT(tp);
>  	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
>  
> -	tv = current_time(inode);
> +	/* If the mtime changes, then ctime must also change */
> +	ASSERT(flags & XFS_ICHGTIME_CHG);
>  
> +	tv = inode_set_ctime_current(inode);
>  	if (flags & XFS_ICHGTIME_MOD)
>  		inode_set_mtime_to_ts(inode, tv);
> -	if (flags & XFS_ICHGTIME_CHG)
> -		inode_set_ctime_to_ts(inode, tv);
>  	if (flags & XFS_ICHGTIME_CREATE)
>  		ip->i_crtime = tv;
>  }
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index a00dcbc77e12..d25872f818fa 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -592,8 +592,9 @@ xfs_vn_getattr(
>  	stat->gid = vfsgid_into_kgid(vfsgid);
>  	stat->ino = ip->i_ino;
>  	stat->atime = inode_get_atime(inode);
> -	stat->mtime = inode_get_mtime(inode);
> -	stat->ctime = inode_get_ctime(inode);
> +
> +	fill_mg_cmtime(stat, request_mask, inode);
> +
>  	stat->blocks = XFS_FSB_TO_BB(mp, ip->i_nblocks + ip->i_delayed_blks);
>  
>  	if (xfs_has_v3inodes(mp)) {
> @@ -603,11 +604,6 @@ xfs_vn_getattr(
>  		}
>  	}
>  
> -	if ((request_mask & STATX_CHANGE_COOKIE) && IS_I_VERSION(inode)) {
> -		stat->change_cookie = inode_query_iversion(inode);
> -		stat->result_mask |= STATX_CHANGE_COOKIE;
> -	}
> -
>  	/*
>  	 * Note: If you add another clause to set an attribute flag, please
>  	 * update attributes_mask below.
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 27e9f749c4c7..210481b03fdb 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -2052,7 +2052,7 @@ static struct file_system_type xfs_fs_type = {
>  	.init_fs_context	= xfs_init_fs_context,
>  	.parameters		= xfs_fs_parameters,
>  	.kill_sb		= xfs_kill_sb,
> -	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
> +	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP | FS_MGTIME,
>  };
>  MODULE_ALIAS_FS("xfs");
>  
> 
> -- 
> 2.45.2
> 

Re: [PATCH v5 6/9] xfs: switch to multigrain timestamps

[Jeff Layton]

On Thu, 2024-07-11 at 08:09 -0700, Darrick J. Wong wrote:
> On Thu, Jul 11, 2024 at 07:08:10AM -0400, Jeff Layton wrote:
> > Enable multigrain timestamps, which should ensure that there is an
> > apparent change to the timestamp whenever it has been written after
> > being actively observed via getattr.
> > 
> > Also, anytime the mtime changes, the ctime must also change, and those
> > are now the only two options for xfs_trans_ichgtime. Have that function
> > unconditionally bump the ctime, and ASSERT that XFS_ICHGTIME_CHG is
> > always set.
> > 
> > Finally, stop setting STATX_CHANGE_COOKIE in getattr, since the ctime
> > should give us better semantics now.
> 
> Following up on "As long as the fs isn't touching i_ctime_nsec directly,
> you shouldn't need to worry about this" from:
> https://lore.kernel.org/linux-xfs/cae5c28f172ac57b7eaaa98a00b23f342f01ba64.camel@kernel.org/
> 
> xfs /does/ touch i_ctime_nsec directly when it's writing inodes to disk.
> From xfs_inode_to_disk, see:
> 
> 	to->di_ctime = xfs_inode_to_disk_ts(ip, inode_get_ctime(inode));
> 
> AFAICT, inode_get_ctime itself remains unchanged, and still returns
> inode->__i_ctime, right?  In which case it's returning a raw timespec64,
> which can include the QUERIED flag in tv_nsec, right?
> 

No, in the first patch in the series, inode_get_ctime becomes this:

#define I_CTIME_QUERIED         ((u32)BIT(31))

static inline time64_t inode_get_ctime_sec(const struct inode *inode)
{
        return inode->i_ctime_sec;
}

static inline long inode_get_ctime_nsec(const struct inode *inode)
{
        return inode->i_ctime_nsec & ~I_CTIME_QUERIED;
}

static inline struct timespec64 inode_get_ctime(const struct inode *inode)
{
        struct timespec64 ts = { .tv_sec  = inode_get_ctime_sec(inode),
                                 .tv_nsec = inode_get_ctime_nsec(inode) };

        return ts;
}

...which should ensure that you never store the QUERIED bit.

> Now let's look at the consumer:
> 
> static inline xfs_timestamp_t
> xfs_inode_to_disk_ts(
> 	struct xfs_inode		*ip,
> 	const struct timespec64		tv)
> {
> 	struct xfs_legacy_timestamp	*lts;
> 	xfs_timestamp_t			ts;
> 
> 	if (xfs_inode_has_bigtime(ip))
> 		return cpu_to_be64(xfs_inode_encode_bigtime(tv));
> 
> 	lts = (struct xfs_legacy_timestamp *)&ts;
> 	lts->t_sec = cpu_to_be32(tv.tv_sec);
> 	lts->t_nsec = cpu_to_be32(tv.tv_nsec);
> 
> 	return ts;
> }
> 
> For the !bigtime case (aka before we added y2038 support) the queried
> flag gets encoded into the tv_nsec field since xfs doesn't filter the
> queried flag.
> 
> For the bigtime case, the timespec is turned into an absolute nsec count
> since the xfs epoch (which is the minimum timestamp possible under the
> old encoding scheme):
> 
> static inline uint64_t xfs_inode_encode_bigtime(struct timespec64 tv)
> {
> 	return xfs_unix_to_bigtime(tv.tv_sec) * NSEC_PER_SEC + tv.tv_nsec;
> }
> 
> Here we'd also be mixing in the QUERIED flag, only now we've encoded a
> time that's a second in the future.  I think the solution is to add a:
> 
> static inline struct timespec64
> inode_peek_ctime(const struct inode *inode)
> {
> 	return (struct timespec64){
> 		.tv_sec = inode->__i_ctime.tv_sec,
> 		.tv_nsec = inode->__i_ctime.tv_nsec & ~I_CTIME_QUERIED,
> 	};
> }
> 
> similar to what inode_peek_iversion does for iversion; and then
> xfs_inode_to_disk can do:
> 
> 	to->di_ctime = xfs_inode_to_disk_ts(ip, inode_peek_ctime(inode));
> 
> which would prevent I_CTIME_QUERIED from going out to disk.
> 
> At load time, xfs_inode_from_disk uses inode_set_ctime_to_ts so I think
> xfs won't accidentally introduce QUERIED when it's loading an inode from
> disk.
> 
> 

Also already done in this patchset:

struct timespec64 inode_set_ctime_to_ts(struct inode *inode, struct timespec64 ts)
{
        inode->i_ctime_sec = ts.tv_sec;
        inode->i_ctime_nsec = ts.tv_nsec & ~I_CTIME_QUERIED;
        trace_inode_set_ctime_to_ts(inode, &ts);
        return ts;
}
EXPORT_SYMBOL(inode_set_ctime_to_ts);

Basically, we never want to store or fetch the QUERIED flag from disk,
and since it's in an unused bit, we can just universally mask it off
when dealing with "external" users of it.

One caveat -- I am using the sign bit for the QUERIED flag, so I'm
assuming that no one should ever pass inode_set_ctime_to_ts a negative
tv_nsec value.

Maybe I should add a WARN_ON_ONCE here to check for that? It seems
nonsensical, but you never know...

> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_trans_inode.c |  6 +++---
> >  fs/xfs/xfs_iops.c               | 10 +++-------
> >  fs/xfs/xfs_super.c              |  2 +-
> >  3 files changed, 7 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> > index 69fc5b981352..1f3639bbf5f0 100644
> > --- a/fs/xfs/libxfs/xfs_trans_inode.c
> > +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> > @@ -62,12 +62,12 @@ xfs_trans_ichgtime(
> >  	ASSERT(tp);
> >  	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> >  
> > -	tv = current_time(inode);
> > +	/* If the mtime changes, then ctime must also change */
> > +	ASSERT(flags & XFS_ICHGTIME_CHG);
> >  
> > +	tv = inode_set_ctime_current(inode);
> >  	if (flags & XFS_ICHGTIME_MOD)
> >  		inode_set_mtime_to_ts(inode, tv);
> > -	if (flags & XFS_ICHGTIME_CHG)
> > -		inode_set_ctime_to_ts(inode, tv);
> >  	if (flags & XFS_ICHGTIME_CREATE)
> >  		ip->i_crtime = tv;
> >  }
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index a00dcbc77e12..d25872f818fa 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -592,8 +592,9 @@ xfs_vn_getattr(
> >  	stat->gid = vfsgid_into_kgid(vfsgid);
> >  	stat->ino = ip->i_ino;
> >  	stat->atime = inode_get_atime(inode);
> > -	stat->mtime = inode_get_mtime(inode);
> > -	stat->ctime = inode_get_ctime(inode);
> > +
> > +	fill_mg_cmtime(stat, request_mask, inode);
> > +
> >  	stat->blocks = XFS_FSB_TO_BB(mp, ip->i_nblocks + ip->i_delayed_blks);
> >  
> >  	if (xfs_has_v3inodes(mp)) {
> > @@ -603,11 +604,6 @@ xfs_vn_getattr(
> >  		}
> >  	}
> >  
> > -	if ((request_mask & STATX_CHANGE_COOKIE) && IS_I_VERSION(inode)) {
> > -		stat->change_cookie = inode_query_iversion(inode);
> > -		stat->result_mask |= STATX_CHANGE_COOKIE;
> > -	}
> > -
> >  	/*
> >  	 * Note: If you add another clause to set an attribute flag, please
> >  	 * update attributes_mask below.
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 27e9f749c4c7..210481b03fdb 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -2052,7 +2052,7 @@ static struct file_system_type xfs_fs_type = {
> >  	.init_fs_context	= xfs_init_fs_context,
> >  	.parameters		= xfs_fs_parameters,
> >  	.kill_sb		= xfs_kill_sb,
> > -	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
> > +	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP | FS_MGTIME,
> >  };
> >  MODULE_ALIAS_FS("xfs");
> >  
> > 
> > -- 
> > 2.45.2
> > 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v5 2/9] fs: tracepoints around multigrain timestamp events

[Darrick J. Wong]

On Thu, Jul 11, 2024 at 07:08:06AM -0400, Jeff Layton wrote:
> Add some tracepoints around various multigrain timestamp events.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/inode.c                       |   5 ++
>  fs/stat.c                        |   3 ++
>  include/trace/events/timestamp.h | 109 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 117 insertions(+)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 2b5889ff7b36..81b45e0a95a6 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -22,6 +22,9 @@
>  #include <linux/iversion.h>
>  #include <linux/rw_hint.h>
>  #include <trace/events/writeback.h>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/timestamp.h>
> +
>  #include "internal.h"
>  
>  /*
> @@ -2571,6 +2574,7 @@ struct timespec64 inode_set_ctime_to_ts(struct inode *inode, struct timespec64 t
>  {
>  	inode->i_ctime_sec = ts.tv_sec;
>  	inode->i_ctime_nsec = ts.tv_nsec & ~I_CTIME_QUERIED;
> +	trace_inode_set_ctime_to_ts(inode, &ts);
>  	return ts;
>  }
>  EXPORT_SYMBOL(inode_set_ctime_to_ts);
> @@ -2670,6 +2674,7 @@ struct timespec64 inode_set_ctime_current(struct inode *inode)
>  	if (try_cmpxchg(&inode->i_ctime_nsec, &cur, now_ts.tv_nsec)) {
>  		/* If swap occurred, then we're (mostly) done */
>  		inode->i_ctime_sec = now_ts.tv_sec;
> +		trace_ctime_ns_xchg(inode, cns, now_ts.tv_nsec, cur);
>  	} else {
>  		/*
>  		 * Was the change due to someone marking the old ctime QUERIED?
> diff --git a/fs/stat.c b/fs/stat.c
> index df7fdd3afed9..552dfd67688b 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -23,6 +23,8 @@
>  #include <linux/uaccess.h>
>  #include <asm/unistd.h>
>  
> +#include <trace/events/timestamp.h>
> +
>  #include "internal.h"
>  #include "mount.h"
>  
> @@ -49,6 +51,7 @@ void fill_mg_cmtime(struct kstat *stat, u32 request_mask, struct inode *inode)
>  	stat->mtime = inode_get_mtime(inode);
>  	stat->ctime.tv_sec = inode->i_ctime_sec;
>  	stat->ctime.tv_nsec = ((u32)atomic_fetch_or(I_CTIME_QUERIED, pcn)) & ~I_CTIME_QUERIED;
> +	trace_fill_mg_cmtime(inode, &stat->ctime, &stat->mtime);
>  }
>  EXPORT_SYMBOL(fill_mg_cmtime);
>  
> diff --git a/include/trace/events/timestamp.h b/include/trace/events/timestamp.h
> new file mode 100644
> index 000000000000..3a603190b46c
> --- /dev/null
> +++ b/include/trace/events/timestamp.h
> @@ -0,0 +1,109 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM timestamp
> +
> +#if !defined(_TRACE_TIMESTAMP_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_TIMESTAMP_H
> +
> +#include <linux/tracepoint.h>
> +#include <linux/fs.h>
> +
> +TRACE_EVENT(inode_set_ctime_to_ts,
> +	TP_PROTO(struct inode *inode,
> +		 struct timespec64 *ctime),
> +
> +	TP_ARGS(inode, ctime),
> +
> +	TP_STRUCT__entry(
> +		__field(dev_t,			dev)
> +		__field(ino_t,			ino)
> +		__field(time64_t,		ctime_s)
> +		__field(u32,			ctime_ns)
> +		__field(u32,			gen)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->dev		= inode->i_sb->s_dev;

Odd indenting of the second columns between the struct definition above
and the assignment code here.

> +		__entry->ino		= inode->i_ino;
> +		__entry->gen		= inode->i_generation;
> +		__entry->ctime_s	= ctime->tv_sec;
> +		__entry->ctime_ns	= ctime->tv_nsec;
> +	),
> +
> +	TP_printk("ino=%d:%d:%ld:%u ctime=%lld.%u",
> +		MAJOR(__entry->dev), MINOR(__entry->dev), __entry->ino, __entry->gen,
> +		__entry->ctime_s, __entry->ctime_ns
> +	)
> +);
> +
> +TRACE_EVENT(ctime_ns_xchg,
> +	TP_PROTO(struct inode *inode,
> +		 u32 old,
> +		 u32 new,
> +		 u32 cur),
> +
> +	TP_ARGS(inode, old, new, cur),
> +
> +	TP_STRUCT__entry(
> +		__field(dev_t,				dev)
> +		__field(ino_t,				ino)
> +		__field(u32,				gen)
> +		__field(u32,				old)
> +		__field(u32,				new)
> +		__field(u32,				cur)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->dev		= inode->i_sb->s_dev;
> +		__entry->ino		= inode->i_ino;
> +		__entry->gen		= inode->i_generation;
> +		__entry->old		= old;
> +		__entry->new		= new;
> +		__entry->cur		= cur;
> +	),
> +
> +	TP_printk("ino=%d:%d:%ld:%u old=%u:%c new=%u cur=%u:%c",
> +		MAJOR(__entry->dev), MINOR(__entry->dev), __entry->ino, __entry->gen,
> +		__entry->old & ~I_CTIME_QUERIED, __entry->old & I_CTIME_QUERIED ? 'Q' : '-',
> +		__entry->new,
> +		__entry->cur & ~I_CTIME_QUERIED, __entry->cur & I_CTIME_QUERIED ? 'Q' : '-'

This /might/ be overkill for a single flag, but anything you put in the
TP_printk seems to end up in the format file:

# cat /sys/kernel/debug/tracing/events/xfs/xfbtree_create_root_buf/format
name: xfbtree_create_root_buf
ID: 1644
format:
        field:unsigned short common_type;       offset:0;       size:2; signed:0;
        field:unsigned char common_flags;       offset:2;       size:1; signed:0;
        field:unsigned char common_preempt_count;       offset:3;       size:1; signed:0;
        field:int common_pid;   offset:4;       size:4; signed:1;

        field:unsigned long xfino;      offset:8;       size:8; signed:0;
        field:xfs_daddr_t bno;  offset:16;      size:8; signed:1;
        field:int nblks;        offset:24;      size:4; signed:1;
        field:int hold; offset:28;      size:4; signed:1;
        field:int pincount;     offset:32;      size:4; signed:1;
        field:unsigned int lockval;     offset:36;      size:4; signed:0;
        field:unsigned int flags;       offset:40;      size:4; signed:0;

print fmt: "xfino 0x%lx daddr 0x%llx bbcount 0x%x hold %d pincount %d lock %d flags %s", REC->xfino, (unsigned long long)REC->bno, REC->nblks, REC->hold, REC->pincount, REC->lockval, __print_flags(REC->flags, "|", { (1u << 0), "READ" }, { (1u << 1), "WRITE" }, { (1u << 2), "READ_AHEAD" }, { (1u << 3), "NO_IOACCT" }, { (1u << 4), "ASYNC" }, { (1u << 5), "DONE" }, { (1u << 6), "STALE" }, { (1u << 7), "WRITE_FAIL" }, { (1u << 16), "INODES" }, { (1u << 17), "DQUOTS" }, { (1u << 18), "LOG_RECOVERY" }, { (1u << 20), "PAGES" }, { (1u << 21), "KMEM" }, { (1u << 22), "DELWRI_Q" }, { (1u << 28), "LIVESCAN" }, { (1u << 29), "INCORE" }, { (1u << 30), "TRYLOCK" }, { (1u << 31), "UNMAPPED" })

I /think/ all that code gets compiled (interpreted?) as if it were C
code, but a more compact format might be:

#define CTIME_QUERIED_FLAGS \
	{ I_CTIME_QUERIED, "queried" }

	TP_printk("ino=%d:%d:%ld:%u old=%u:%s new=%u cur=%u:%c",
		MAJOR(__entry->dev), MINOR(__entry->dev), __entry->ino, __entry->gen,
		__entry->old & ~I_CTIME_QUERIED,
		__print_flags(__entry->old & I_CTIME_QUERIED, "|",
			      CTIME_QUERIED_FLAGS),
		...

But, again, that could be overkill for a single flag.  Aside from my
minor bikeshedding, this all looks good, and I like that we can now
monitor what's going on wrt ctime. :)

--D

> +	)
> +);
> +
> +TRACE_EVENT(fill_mg_cmtime,
> +	TP_PROTO(struct inode *inode,
> +		 struct timespec64 *ctime,
> +		 struct timespec64 *mtime),
> +
> +	TP_ARGS(inode, ctime, mtime),
> +
> +	TP_STRUCT__entry(
> +		__field(dev_t,			dev)
> +		__field(ino_t,			ino)
> +		__field(time64_t,		ctime_s)
> +		__field(time64_t,		mtime_s)
> +		__field(u32,			ctime_ns)
> +		__field(u32,			mtime_ns)
> +		__field(u32,			gen)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->dev		= inode->i_sb->s_dev;
> +		__entry->ino		= inode->i_ino;
> +		__entry->gen		= inode->i_generation;
> +		__entry->ctime_s	= ctime->tv_sec;
> +		__entry->mtime_s	= mtime->tv_sec;
> +		__entry->ctime_ns	= ctime->tv_nsec;
> +		__entry->mtime_ns	= mtime->tv_nsec;
> +	),
> +
> +	TP_printk("ino=%d:%d:%ld:%u ctime=%lld.%u mtime=%lld.%u",
> +		MAJOR(__entry->dev), MINOR(__entry->dev), __entry->ino, __entry->gen,
> +		__entry->ctime_s, __entry->ctime_ns,
> +		__entry->mtime_s, __entry->mtime_ns
> +	)
> +);
> +#endif /* _TRACE_TIMESTAMP_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> 
> -- 
> 2.45.2
> 
> 

Re: [PATCH v5 4/9] fs: have setattr_copy handle multigrain timestamps appropriately

[Darrick J. Wong]

On Thu, Jul 11, 2024 at 07:08:08AM -0400, Jeff Layton wrote:
> The setattr codepath is still using coarse-grained timestamps, even on
> multigrain filesystems. To fix this, we need to fetch the timestamp for
> ctime updates later, at the point where the assignment occurs in
> setattr_copy.
> 
> On a multigrain inode, ignore the ia_ctime in the attrs, and always
> update the ctime to the current clock value. Update the atime and mtime
> with the same value (if needed) unless they are being set to other
> specific values, a'la utimes().
> 
> Note that we don't want to do this universally however, as some
> filesystems (e.g. most networked fs) want to do an explicit update
> elsewhere before updating the local inode.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

Makes sense to me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/attr.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 46 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/attr.c b/fs/attr.c
> index 825007d5cda4..e03ea6951864 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -271,6 +271,42 @@ int inode_newsize_ok(const struct inode *inode, loff_t offset)
>  }
>  EXPORT_SYMBOL(inode_newsize_ok);
>  
> +/**
> + * setattr_copy_mgtime - update timestamps for mgtime inodes
> + * @inode: inode timestamps to be updated
> + * @attr: attrs for the update
> + *
> + * With multigrain timestamps, we need to take more care to prevent races
> + * when updating the ctime. Always update the ctime to the very latest
> + * using the standard mechanism, and use that to populate the atime and
> + * mtime appropriately (unless we're setting those to specific values).
> + */
> +static void setattr_copy_mgtime(struct inode *inode, const struct iattr *attr)
> +{
> +	unsigned int ia_valid = attr->ia_valid;
> +	struct timespec64 now;
> +
> +	/*
> +	 * If the ctime isn't being updated then nothing else should be
> +	 * either.
> +	 */
> +	if (!(ia_valid & ATTR_CTIME)) {
> +		WARN_ON_ONCE(ia_valid & (ATTR_ATIME|ATTR_MTIME));
> +		return;
> +	}
> +
> +	now = inode_set_ctime_current(inode);
> +	if (ia_valid & ATTR_ATIME_SET)
> +		inode_set_atime_to_ts(inode, attr->ia_atime);
> +	else if (ia_valid & ATTR_ATIME)
> +		inode_set_atime_to_ts(inode, now);
> +
> +	if (ia_valid & ATTR_MTIME_SET)
> +		inode_set_mtime_to_ts(inode, attr->ia_mtime);
> +	else if (ia_valid & ATTR_MTIME)
> +		inode_set_mtime_to_ts(inode, now);
> +}
> +
>  /**
>   * setattr_copy - copy simple metadata updates into the generic inode
>   * @idmap:	idmap of the mount the inode was found from
> @@ -303,12 +339,6 @@ void setattr_copy(struct mnt_idmap *idmap, struct inode *inode,
>  
>  	i_uid_update(idmap, attr, inode);
>  	i_gid_update(idmap, attr, inode);
> -	if (ia_valid & ATTR_ATIME)
> -		inode_set_atime_to_ts(inode, attr->ia_atime);
> -	if (ia_valid & ATTR_MTIME)
> -		inode_set_mtime_to_ts(inode, attr->ia_mtime);
> -	if (ia_valid & ATTR_CTIME)
> -		inode_set_ctime_to_ts(inode, attr->ia_ctime);
>  	if (ia_valid & ATTR_MODE) {
>  		umode_t mode = attr->ia_mode;
>  		if (!in_group_or_capable(idmap, inode,
> @@ -316,6 +346,16 @@ void setattr_copy(struct mnt_idmap *idmap, struct inode *inode,
>  			mode &= ~S_ISGID;
>  		inode->i_mode = mode;
>  	}
> +
> +	if (is_mgtime(inode))
> +		return setattr_copy_mgtime(inode, attr);
> +
> +	if (ia_valid & ATTR_ATIME)
> +		inode_set_atime_to_ts(inode, attr->ia_atime);
> +	if (ia_valid & ATTR_MTIME)
> +		inode_set_mtime_to_ts(inode, attr->ia_mtime);
> +	if (ia_valid & ATTR_CTIME)
> +		inode_set_ctime_to_ts(inode, attr->ia_ctime);
>  }
>  EXPORT_SYMBOL(setattr_copy);
>  
> 
> -- 
> 2.45.2
> 
> 

Re: [PATCH v5 1/9] fs: add infrastructure for multigrain timestamps

[Darrick J. Wong]

On Thu, Jul 11, 2024 at 07:08:05AM -0400, Jeff Layton wrote:
> The VFS has always used coarse-grained timestamps when updating the
> ctime and mtime after a change. This has the benefit of allowing
> filesystems to optimize away a lot metadata updates, down to around 1
> per jiffy, even when a file is under heavy writes.
> 
> Unfortunately, this has always been an issue when we're exporting via
> NFSv3, which relies on timestamps to validate caches. A lot of changes
> can happen in a jiffy, so timestamps aren't sufficient to help the
> client decide when to invalidate the cache. Even with NFSv4, a lot of
> exported filesystems don't properly support a change attribute and are
> subject to the same problems with timestamp granularity. Other
> applications have similar issues with timestamps (e.g backup
> applications).
> 
> If we were to always use fine-grained timestamps, that would improve the
> situation, but that becomes rather expensive, as the underlying
> filesystem would have to log a lot more metadata updates.
> 
> What we need is a way to only use fine-grained timestamps when they are
> being actively queried. Use the (unused) top bit in inode->i_ctime_nsec
> as a flag that indicates whether the current timestamps have been
> queried via stat() or the like. When it's set, we allow the kernel to
> use a fine-grained timestamp iff it's necessary to make the ctime show
> a different value.
> 
> This solves the problem of being able to distinguish the timestamp
> between updates, but introduces a new problem: it's now possible for a
> file being changed to get a fine-grained timestamp. A file that is
> altered just a bit later can then get a coarse-grained one that appears
> older than the earlier fine-grained time. This violates timestamp
> ordering guarantees.
> 
> To remedy this, keep a global monotonic ktime_t value that acts as a

It's an atomic64_t now, right?

> timestamp floor.  When we go to stamp a file, we first get the latter of
> the current floor value and the current coarse-grained time. If the
> inode ctime hasn't been queried then we just attempt to stamp it with
> that value.
> 
> If it has been queried, then first see whether the current coarse time
> is later than the existing ctime. If it is, then we accept that value.
> If it isn't, then we get a fine-grained time and try to swap that into
> the global floor. Whether that succeeds or fails, we take the resulting
> floor time, convert it to realtime and try to swap that into the ctime.
> 
> We take the result of the ctime swap whether it succeeds or fails, since
> either is just as valid.
> 
> Filesystems can opt into this by setting the FS_MGTIME fstype flag.
> Others should be unaffected (other than being subject to the same floor
> value as multigrain filesystems).
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

With that corrected,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/inode.c         | 171 ++++++++++++++++++++++++++++++++++++++++++++---------
>  fs/stat.c          |  36 ++++++++++-
>  include/linux/fs.h |  34 ++++++++---
>  3 files changed, 204 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index f356fe2ec2b6..2b5889ff7b36 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -60,6 +60,13 @@ static unsigned int i_hash_shift __ro_after_init;
>  static struct hlist_head *inode_hashtable __ro_after_init;
>  static __cacheline_aligned_in_smp DEFINE_SPINLOCK(inode_hash_lock);
>  
> +/*
> + * This represents the latest fine-grained time that we have handed out as a
> + * timestamp on the system. Tracked as a monotonic value, and converted to the
> + * realtime clock on an as-needed basis.
> + */
> +static __cacheline_aligned_in_smp atomic64_t ctime_floor;
> +
>  /*
>   * Empty aops. Can be used for the cases where the user does not
>   * define any of the address_space operations.
> @@ -2127,19 +2134,72 @@ int file_remove_privs(struct file *file)
>  }
>  EXPORT_SYMBOL(file_remove_privs);
>  
> +/**
> + * coarse_ctime - return the current coarse-grained time
> + * @floor: current (monotonic) ctime_floor value
> + *
> + * Get the coarse-grained time, and then determine whether to
> + * return it or the current floor value. Returns the later of the
> + * floor and coarse grained timestamps, converted to realtime
> + * clock value.
> + */
> +static ktime_t coarse_ctime(ktime_t floor)
> +{
> +	ktime_t coarse = ktime_get_coarse();
> +
> +	/* If coarse time is already newer, return that */
> +	if (!ktime_after(floor, coarse))
> +		return ktime_get_coarse_real();
> +	return ktime_mono_to_real(floor);
> +}
> +
> +/**
> + * current_time - Return FS time (possibly fine-grained)
> + * @inode: inode.
> + *
> + * Return the current time truncated to the time granularity supported by
> + * the fs, as suitable for a ctime/mtime change. If the ctime is flagged
> + * as having been QUERIED, get a fine-grained timestamp.
> + */
> +struct timespec64 current_time(struct inode *inode)
> +{
> +	ktime_t floor = atomic64_read(&ctime_floor);
> +	ktime_t now = coarse_ctime(floor);
> +	struct timespec64 now_ts = ktime_to_timespec64(now);
> +	u32 cns;
> +
> +	if (!is_mgtime(inode))
> +		goto out;
> +
> +	/* If nothing has queried it, then coarse time is fine */
> +	cns = smp_load_acquire(&inode->i_ctime_nsec);
> +	if (cns & I_CTIME_QUERIED) {
> +		/*
> +		 * If there is no apparent change, then
> +		 * get a fine-grained timestamp.
> +		 */
> +		if (now_ts.tv_nsec == (cns & ~I_CTIME_QUERIED))
> +			ktime_get_real_ts64(&now_ts);
> +	}
> +out:
> +	return timestamp_truncate(now_ts, inode);
> +}
> +EXPORT_SYMBOL(current_time);
> +
>  static int inode_needs_update_time(struct inode *inode)
>  {
> +	struct timespec64 now, ts;
>  	int sync_it = 0;
> -	struct timespec64 now = current_time(inode);
> -	struct timespec64 ts;
>  
>  	/* First try to exhaust all avenues to not sync */
>  	if (IS_NOCMTIME(inode))
>  		return 0;
>  
> +	now = current_time(inode);
> +
>  	ts = inode_get_mtime(inode);
>  	if (!timespec64_equal(&ts, &now))
> -		sync_it = S_MTIME;
> +		sync_it |= S_MTIME;
>  
>  	ts = inode_get_ctime(inode);
>  	if (!timespec64_equal(&ts, &now))
> @@ -2507,6 +2567,14 @@ void inode_nohighmem(struct inode *inode)
>  }
>  EXPORT_SYMBOL(inode_nohighmem);
>  
> +struct timespec64 inode_set_ctime_to_ts(struct inode *inode, struct timespec64 ts)
> +{
> +	inode->i_ctime_sec = ts.tv_sec;
> +	inode->i_ctime_nsec = ts.tv_nsec & ~I_CTIME_QUERIED;
> +	return ts;
> +}
> +EXPORT_SYMBOL(inode_set_ctime_to_ts);
> +
>  /**
>   * timestamp_truncate - Truncate timespec to a granularity
>   * @t: Timespec
> @@ -2538,38 +2606,87 @@ struct timespec64 timestamp_truncate(struct timespec64 t, struct inode *inode)
>  }
>  EXPORT_SYMBOL(timestamp_truncate);
>  
> -/**
> - * current_time - Return FS time
> - * @inode: inode.
> - *
> - * Return the current time truncated to the time granularity supported by
> - * the fs.
> - *
> - * Note that inode and inode->sb cannot be NULL.
> - * Otherwise, the function warns and returns time without truncation.
> - */
> -struct timespec64 current_time(struct inode *inode)
> -{
> -	struct timespec64 now;
> -
> -	ktime_get_coarse_real_ts64(&now);
> -	return timestamp_truncate(now, inode);
> -}
> -EXPORT_SYMBOL(current_time);
> -
>  /**
>   * inode_set_ctime_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.
> + * Set the inode's ctime to the current value for the inode. Returns the
> + * current value that was assigned. If this is not a multigrain inode, then we
> + * just set it to whatever the coarse_ctime is.
> + *
> + * If it is multigrain, then we first see if the coarse-grained timestamp is
> + * distinct from what we have. If so, then we'll just use that. If we have to
> + * get a fine-grained timestamp, then do so, and try to swap it into the floor.
> + * We accept the new floor value regardless of the outcome of the cmpxchg.
> + * After that, we try to swap the new value into i_ctime_nsec. Again, we take
> + * the resulting ctime, regardless of the outcome of the swap.
>   */
>  struct timespec64 inode_set_ctime_current(struct inode *inode)
>  {
> -	struct timespec64 now = current_time(inode);
> +	ktime_t now, floor = atomic64_read(&ctime_floor);
> +	struct timespec64 now_ts;
> +	u32 cns, cur;
> +
> +	now = coarse_ctime(floor);
>  
> -	inode_set_ctime_to_ts(inode, now);
> -	return now;
> +	/* Just return that if this is not a multigrain fs */
> +	if (!is_mgtime(inode)) {
> +		now_ts = timestamp_truncate(ktime_to_timespec64(now), inode);
> +		inode_set_ctime_to_ts(inode, now_ts);
> +		goto out;
> +	}
> +
> +	/*
> +	 * We only need a fine-grained time if someone has queried it,
> +	 * and the current coarse grained time isn't later than what's
> +	 * already there.
> +	 */
> +	cns = smp_load_acquire(&inode->i_ctime_nsec);
> +	if (cns & I_CTIME_QUERIED) {
> +		ktime_t ctime = ktime_set(inode->i_ctime_sec, cns & ~I_CTIME_QUERIED);
> +
> +		if (!ktime_after(now, ctime)) {
> +			ktime_t old, fine;
> +
> +			/* Get a fine-grained time */
> +			fine = ktime_get();
> +
> +			/*
> +			 * If the cmpxchg works, we take the new floor value. If
> +			 * not, then that means that someone else changed it after we
> +			 * fetched it but before we got here. That value is just
> +			 * as good, so keep it.
> +			 */
> +			old = floor;
> +			if (!atomic64_try_cmpxchg(&ctime_floor, &old, fine))
> +				fine = old;
> +			now = ktime_mono_to_real(fine);
> +		}
> +	}
> +	now_ts = timestamp_truncate(ktime_to_timespec64(now), inode);
> +	cur = cns;
> +retry:
> +	/* Try to swap the nsec value into place. */
> +	if (try_cmpxchg(&inode->i_ctime_nsec, &cur, now_ts.tv_nsec)) {
> +		/* If swap occurred, then we're (mostly) done */
> +		inode->i_ctime_sec = now_ts.tv_sec;
> +	} else {
> +		/*
> +		 * Was the change due to someone marking the old ctime QUERIED?
> +		 * If so then retry the swap. This can only happen once since
> +		 * the only way to clear I_CTIME_QUERIED is to stamp the inode
> +		 * with a new ctime.
> +		 */
> +		if (!(cns & I_CTIME_QUERIED) && (cns | I_CTIME_QUERIED) == cur) {
> +			cns = cur;
> +			goto retry;
> +		}
> +		/* Otherwise, keep the existing ctime */
> +		now_ts.tv_sec = inode->i_ctime_sec;
> +		now_ts.tv_nsec = cur & ~I_CTIME_QUERIED;
> +	}
> +out:
> +	return now_ts;
>  }
>  EXPORT_SYMBOL(inode_set_ctime_current);
>  
> diff --git a/fs/stat.c b/fs/stat.c
> index 6f65b3456cad..df7fdd3afed9 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -26,6 +26,32 @@
>  #include "internal.h"
>  #include "mount.h"
>  
> +/**
> + * fill_mg_cmtime - Fill in the mtime and ctime and flag ctime as QUERIED
> + * @stat: where to store the resulting values
> + * @request_mask: STATX_* values requested
> + * @inode: inode from which to grab the c/mtime
> + *
> + * Given @inode, grab the ctime and mtime out if it and store the result
> + * in @stat. When fetching the value, flag it as queried so the next write
> + * will ensure a distinct timestamp.
> + */
> +void fill_mg_cmtime(struct kstat *stat, u32 request_mask, struct inode *inode)
> +{
> +	atomic_t *pcn = (atomic_t *)&inode->i_ctime_nsec;
> +
> +	/* If neither time was requested, then don't report them */
> +	if (!(request_mask & (STATX_CTIME|STATX_MTIME))) {
> +		stat->result_mask &= ~(STATX_CTIME|STATX_MTIME);
> +		return;
> +	}
> +
> +	stat->mtime = inode_get_mtime(inode);
> +	stat->ctime.tv_sec = inode->i_ctime_sec;
> +	stat->ctime.tv_nsec = ((u32)atomic_fetch_or(I_CTIME_QUERIED, pcn)) & ~I_CTIME_QUERIED;
> +}
> +EXPORT_SYMBOL(fill_mg_cmtime);
> +
>  /**
>   * generic_fillattr - Fill in the basic attributes from the inode struct
>   * @idmap:		idmap of the mount the inode was found from
> @@ -58,8 +84,14 @@ void generic_fillattr(struct mnt_idmap *idmap, u32 request_mask,
>  	stat->rdev = inode->i_rdev;
>  	stat->size = i_size_read(inode);
>  	stat->atime = inode_get_atime(inode);
> -	stat->mtime = inode_get_mtime(inode);
> -	stat->ctime = inode_get_ctime(inode);
> +
> +	if (is_mgtime(inode)) {
> +		fill_mg_cmtime(stat, request_mask, inode);
> +	} else {
> +		stat->ctime = inode_get_ctime(inode);
> +		stat->mtime = inode_get_mtime(inode);
> +	}
> +
>  	stat->blksize = i_blocksize(inode);
>  	stat->blocks = inode->i_blocks;
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index dc9f9c4b2572..f873f6c58669 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1608,6 +1608,17 @@ static inline struct timespec64 inode_set_mtime(struct inode *inode,
>  	return inode_set_mtime_to_ts(inode, ts);
>  }
>  
> +/*
> + * Multigrain timestamps
> + *
> + * Conditionally use fine-grained ctime and mtime timestamps when there
> + * are users actively observing them via getattr. The primary use-case
> + * for this is NFS clients that use the ctime to distinguish between
> + * different states of the file, and that are often fooled by multiple
> + * operations that occur in the same coarse-grained timer tick.
> + */
> +#define I_CTIME_QUERIED		((u32)BIT(31))
> +
>  static inline time64_t inode_get_ctime_sec(const struct inode *inode)
>  {
>  	return inode->i_ctime_sec;
> @@ -1615,7 +1626,7 @@ static inline time64_t inode_get_ctime_sec(const struct inode *inode)
>  
>  static inline long inode_get_ctime_nsec(const struct inode *inode)
>  {
> -	return inode->i_ctime_nsec;
> +	return inode->i_ctime_nsec & ~I_CTIME_QUERIED;
>  }
>  
>  static inline struct timespec64 inode_get_ctime(const struct inode *inode)
> @@ -1626,13 +1637,7 @@ static inline struct timespec64 inode_get_ctime(const struct inode *inode)
>  	return ts;
>  }
>  
> -static inline struct timespec64 inode_set_ctime_to_ts(struct inode *inode,
> -						      struct timespec64 ts)
> -{
> -	inode->i_ctime_sec = ts.tv_sec;
> -	inode->i_ctime_nsec = ts.tv_nsec;
> -	return ts;
> -}
> +struct timespec64 inode_set_ctime_to_ts(struct inode *inode, struct timespec64 ts);
>  
>  /**
>   * inode_set_ctime - set the ctime in the inode
> @@ -2490,6 +2495,7 @@ struct file_system_type {
>  #define FS_USERNS_MOUNT		8	/* Can be mounted by userns root */
>  #define FS_DISALLOW_NOTIFY_PERM	16	/* Disable fanotify permission events */
>  #define FS_ALLOW_IDMAP         32      /* FS has been updated to handle vfs idmappings. */
> +#define FS_MGTIME		64	/* FS uses multigrain timestamps */
>  #define FS_RENAME_DOES_D_MOVE	32768	/* FS will handle d_move() during rename() internally. */
>  	int (*init_fs_context)(struct fs_context *);
>  	const struct fs_parameter_spec *parameters;
> @@ -2513,6 +2519,17 @@ struct file_system_type {
>  
>  #define MODULE_ALIAS_FS(NAME) MODULE_ALIAS("fs-" NAME)
>  
> +/**
> + * is_mgtime: is this inode using multigrain timestamps
> + * @inode: inode to test for multigrain timestamps
> + *
> + * Return true if the inode uses multigrain timestamps, false otherwise.
> + */
> +static inline bool is_mgtime(const struct inode *inode)
> +{
> +	return inode->i_sb->s_type->fs_flags & FS_MGTIME;
> +}
> +
>  extern struct dentry *mount_bdev(struct file_system_type *fs_type,
>  	int flags, const char *dev_name, void *data,
>  	int (*fill_super)(struct super_block *, void *, int));
> @@ -3252,6 +3269,7 @@ extern void page_put_link(void *);
>  extern int page_symlink(struct inode *inode, const char *symname, int len);
>  extern const struct inode_operations page_symlink_inode_operations;
>  extern void kfree_link(void *);
> +void fill_mg_cmtime(struct kstat *stat, u32 request_mask, struct inode *inode);
>  void generic_fillattr(struct mnt_idmap *, u32, struct inode *, struct kstat *);
>  void generic_fill_statx_attr(struct inode *inode, struct kstat *stat);
>  extern int vfs_getattr_nosec(const struct path *, struct kstat *, u32, unsigned int);
> 
> -- 
> 2.45.2
> 
> 

Re: [PATCH v5 2/9] fs: tracepoints around multigrain timestamp events

[Jeff Layton]

On Thu, 2024-07-11 at 09:49 -0700, Darrick J. Wong wrote:
> On Thu, Jul 11, 2024 at 07:08:06AM -0400, Jeff Layton wrote:
> > Add some tracepoints around various multigrain timestamp events.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/inode.c                       |   5 ++
> >  fs/stat.c                        |   3 ++
> >  include/trace/events/timestamp.h | 109
> > +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 117 insertions(+)
> > 
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 2b5889ff7b36..81b45e0a95a6 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -22,6 +22,9 @@
> >  #include <linux/iversion.h>
> >  #include <linux/rw_hint.h>
> >  #include <trace/events/writeback.h>
> > +#define CREATE_TRACE_POINTS
> > +#include <trace/events/timestamp.h>
> > +
> >  #include "internal.h"
> >  
> >  /*
> > @@ -2571,6 +2574,7 @@ struct timespec64
> > inode_set_ctime_to_ts(struct inode *inode, struct timespec64 t
> >  {
> >  	inode->i_ctime_sec = ts.tv_sec;
> >  	inode->i_ctime_nsec = ts.tv_nsec & ~I_CTIME_QUERIED;
> > +	trace_inode_set_ctime_to_ts(inode, &ts);
> >  	return ts;
> >  }
> >  EXPORT_SYMBOL(inode_set_ctime_to_ts);
> > @@ -2670,6 +2674,7 @@ struct timespec64
> > inode_set_ctime_current(struct inode *inode)
> >  	if (try_cmpxchg(&inode->i_ctime_nsec, &cur,
> > now_ts.tv_nsec)) {
> >  		/* If swap occurred, then we're (mostly) done */
> >  		inode->i_ctime_sec = now_ts.tv_sec;
> > +		trace_ctime_ns_xchg(inode, cns, now_ts.tv_nsec,
> > cur);
> >  	} else {
> >  		/*
> >  		 * Was the change due to someone marking the old
> > ctime QUERIED?
> > diff --git a/fs/stat.c b/fs/stat.c
> > index df7fdd3afed9..552dfd67688b 100644
> > --- a/fs/stat.c
> > +++ b/fs/stat.c
> > @@ -23,6 +23,8 @@
> >  #include <linux/uaccess.h>
> >  #include <asm/unistd.h>
> >  
> > +#include <trace/events/timestamp.h>
> > +
> >  #include "internal.h"
> >  #include "mount.h"
> >  
> > @@ -49,6 +51,7 @@ void fill_mg_cmtime(struct kstat *stat, u32
> > request_mask, struct inode *inode)
> >  	stat->mtime = inode_get_mtime(inode);
> >  	stat->ctime.tv_sec = inode->i_ctime_sec;
> >  	stat->ctime.tv_nsec =
> > ((u32)atomic_fetch_or(I_CTIME_QUERIED, pcn)) & ~I_CTIME_QUERIED;
> > +	trace_fill_mg_cmtime(inode, &stat->ctime, &stat->mtime);
> >  }
> >  EXPORT_SYMBOL(fill_mg_cmtime);
> >  
> > diff --git a/include/trace/events/timestamp.h
> > b/include/trace/events/timestamp.h
> > new file mode 100644
> > index 000000000000..3a603190b46c
> > --- /dev/null
> > +++ b/include/trace/events/timestamp.h
> > @@ -0,0 +1,109 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#undef TRACE_SYSTEM
> > +#define TRACE_SYSTEM timestamp
> > +
> > +#if !defined(_TRACE_TIMESTAMP_H) ||
> > defined(TRACE_HEADER_MULTI_READ)
> > +#define _TRACE_TIMESTAMP_H
> > +
> > +#include <linux/tracepoint.h>
> > +#include <linux/fs.h>
> > +
> > +TRACE_EVENT(inode_set_ctime_to_ts,
> > +	TP_PROTO(struct inode *inode,
> > +		 struct timespec64 *ctime),
> > +
> > +	TP_ARGS(inode, ctime),
> > +
> > +	TP_STRUCT__entry(
> > +		__field(dev_t,			dev)
> > +		__field(ino_t,			ino)
> > +		__field(time64_t,		ctime_s)
> > +		__field(u32,			ctime_ns)
> > +		__field(u32,			gen)
> > +	),
> > +
> > +	TP_fast_assign(
> > +		__entry->dev		= inode->i_sb->s_dev;
> 
> Odd indenting of the second columns between the struct definition
> above
> and the assignment code here.
> 
> > +		__entry->ino		= inode->i_ino;
> > +		__entry->gen		= inode->i_generation;
> > +		__entry->ctime_s	= ctime->tv_sec;
> > +		__entry->ctime_ns	= ctime->tv_nsec;
> > +	),
> > +
> > +	TP_printk("ino=%d:%d:%ld:%u ctime=%lld.%u",
> > +		MAJOR(__entry->dev), MINOR(__entry->dev), __entry-
> > >ino, __entry->gen,
> > +		__entry->ctime_s, __entry->ctime_ns
> > +	)
> > +);
> > +
> > +TRACE_EVENT(ctime_ns_xchg,
> > +	TP_PROTO(struct inode *inode,
> > +		 u32 old,
> > +		 u32 new,
> > +		 u32 cur),
> > +
> > +	TP_ARGS(inode, old, new, cur),
> > +
> > +	TP_STRUCT__entry(
> > +		__field(dev_t,				dev)
> > +		__field(ino_t,				ino)
> > +		__field(u32,				gen)
> > +		__field(u32,				old)
> > +		__field(u32,				new)
> > +		__field(u32,				cur)
> > +	),
> > +
> > +	TP_fast_assign(
> > +		__entry->dev		= inode->i_sb->s_dev;
> > +		__entry->ino		= inode->i_ino;
> > +		__entry->gen		= inode->i_generation;
> > +		__entry->old		= old;
> > +		__entry->new		= new;
> > +		__entry->cur		= cur;
> > +	),
> > +
> > +	TP_printk("ino=%d:%d:%ld:%u old=%u:%c new=%u cur=%u:%c",
> > +		MAJOR(__entry->dev), MINOR(__entry->dev), __entry-
> > >ino, __entry->gen,
> > +		__entry->old & ~I_CTIME_QUERIED, __entry->old &
> > I_CTIME_QUERIED ? 'Q' : '-',
> > +		__entry->new,
> > +		__entry->cur & ~I_CTIME_QUERIED, __entry->cur &
> > I_CTIME_QUERIED ? 'Q' : '-'
> 
> This /might/ be overkill for a single flag, but anything you put in
> the
> TP_printk seems to end up in the format file:
> 
> # cat
> /sys/kernel/debug/tracing/events/xfs/xfbtree_create_root_buf/format
> name: xfbtree_create_root_buf
> ID: 1644
> format:
>         field:unsigned short common_type;       offset:0;      
> size:2; signed:0;
>         field:unsigned char common_flags;       offset:2;      
> size:1; signed:0;
>         field:unsigned char common_preempt_count;      
> offset:3;       size:1; signed:0;
>         field:int common_pid;   offset:4;       size:4; signed:1;
> 
>         field:unsigned long xfino;      offset:8;       size:8;
> signed:0;
>         field:xfs_daddr_t bno;  offset:16;      size:8; signed:1;
>         field:int nblks;        offset:24;      size:4; signed:1;
>         field:int hold; offset:28;      size:4; signed:1;
>         field:int pincount;     offset:32;      size:4; signed:1;
>         field:unsigned int lockval;     offset:36;      size:4;
> signed:0;
>         field:unsigned int flags;       offset:40;      size:4;
> signed:0;
> 
> print fmt: "xfino 0x%lx daddr 0x%llx bbcount 0x%x hold %d pincount %d
> lock %d flags %s", REC->xfino, (unsigned long long)REC->bno, REC-
> >nblks, REC->hold, REC->pincount, REC->lockval, __print_flags(REC-
> >flags, "|", { (1u << 0), "READ" }, { (1u << 1), "WRITE" }, { (1u <<
> 2), "READ_AHEAD" }, { (1u << 3), "NO_IOACCT" }, { (1u << 4), "ASYNC"
> }, { (1u << 5), "DONE" }, { (1u << 6), "STALE" }, { (1u << 7),
> "WRITE_FAIL" }, { (1u << 16), "INODES" }, { (1u << 17), "DQUOTS" }, {
> (1u << 18), "LOG_RECOVERY" }, { (1u << 20), "PAGES" }, { (1u << 21),
> "KMEM" }, { (1u << 22), "DELWRI_Q" }, { (1u << 28), "LIVESCAN" }, {
> (1u << 29), "INCORE" }, { (1u << 30), "TRYLOCK" }, { (1u << 31),
> "UNMAPPED" })
> 
> I /think/ all that code gets compiled (interpreted?) as if it were C
> code, but a more compact format might be:
> 
> #define CTIME_QUERIED_FLAGS \
> 	{ I_CTIME_QUERIED, "queried" }
> 
> 	TP_printk("ino=%d:%d:%ld:%u old=%u:%s new=%u cur=%u:%c",
> 		MAJOR(__entry->dev), MINOR(__entry->dev), __entry-
> >ino, __entry->gen,
> 		__entry->old & ~I_CTIME_QUERIED,
> 		__print_flags(__entry->old & I_CTIME_QUERIED, "|",
> 			      CTIME_QUERIED_FLAGS),
> 		...
> 
> But, again, that could be overkill for a single flag.  Aside from my
> minor bikeshedding, this all looks good, and I like that we can now
> monitor what's going on wrt ctime. :)
> 

That seems reasonable. I hadn't looked at the unrolled format
monstrosity, but making it more compact is good. I'll make that change
and see how it looks.

Thanks for the reviews so far!

> > +	)
> > +);
> > +
> > +TRACE_EVENT(fill_mg_cmtime,
> > +	TP_PROTO(struct inode *inode,
> > +		 struct timespec64 *ctime,
> > +		 struct timespec64 *mtime),
> > +
> > +	TP_ARGS(inode, ctime, mtime),
> > +
> > +	TP_STRUCT__entry(
> > +		__field(dev_t,			dev)
> > +		__field(ino_t,			ino)
> > +		__field(time64_t,		ctime_s)
> > +		__field(time64_t,		mtime_s)
> > +		__field(u32,			ctime_ns)
> > +		__field(u32,			mtime_ns)
> > +		__field(u32,			gen)
> > +	),
> > +
> > +	TP_fast_assign(
> > +		__entry->dev		= inode->i_sb->s_dev;
> > +		__entry->ino		= inode->i_ino;
> > +		__entry->gen		= inode->i_generation;
> > +		__entry->ctime_s	= ctime->tv_sec;
> > +		__entry->mtime_s	= mtime->tv_sec;
> > +		__entry->ctime_ns	= ctime->tv_nsec;
> > +		__entry->mtime_ns	= mtime->tv_nsec;
> > +	),
> > +
> > +	TP_printk("ino=%d:%d:%ld:%u ctime=%lld.%u mtime=%lld.%u",
> > +		MAJOR(__entry->dev), MINOR(__entry->dev), __entry-
> > >ino, __entry->gen,
> > +		__entry->ctime_s, __entry->ctime_ns,
> > +		__entry->mtime_s, __entry->mtime_ns
> > +	)
> > +);
> > +#endif /* _TRACE_TIMESTAMP_H */
> > +
> > +/* This part must be outside protection */
> > +#include <trace/define_trace.h>
> > 
> > -- 
> > 2.45.2
> > 
> > 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v5 5/9] Documentation: add a new file documenting multigrain timestamps

[Darrick J. Wong]

On Thu, Jul 11, 2024 at 07:08:09AM -0400, Jeff Layton wrote:
> Add a high-level document that describes how multigrain timestamps work,
> rationale for them, and some info about implementation and tradeoffs.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  Documentation/filesystems/multigrain-ts.rst | 120 ++++++++++++++++++++++++++++
>  1 file changed, 120 insertions(+)
> 
> diff --git a/Documentation/filesystems/multigrain-ts.rst b/Documentation/filesystems/multigrain-ts.rst
> new file mode 100644
> index 000000000000..5cefc204ecec
> --- /dev/null
> +++ b/Documentation/filesystems/multigrain-ts.rst
> @@ -0,0 +1,120 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=====================
> +Multigrain Timestamps
> +=====================
> +
> +Introduction
> +============
> +Historically, the kernel has always used coarse time values to stamp
> +inodes. This value is updated on every jiffy, so any change that happens
> +within that jiffy will end up with the same timestamp.
> +
> +When the kernel goes to stamp an inode (due to a read or write), it first gets
> +the current time and then compares it to the existing timestamp(s) to see
> +whether anything will change. If nothing changed, then it can avoid updating
> +the inode's metadata.
> +
> +Coarse timestamps are therefore good from a performance standpoint, since they
> +reduce the need for metadata updates, but bad from the standpoint of
> +determining whether anything has changed, since a lot of things can happen in a
> +jiffy.
> +
> +They are particularly troublesome with NFSv3, where unchanging timestamps can
> +make it difficult to tell whether to invalidate caches. NFSv4 provides a
> +dedicated change attribute that should always show a visible change, but not
> +all filesystems implement this properly, causing the NFS server to substitute
> +the ctime in many cases.
> +
> +Multigrain timestamps aim to remedy this by selectively using fine-grained
> +timestamps when a file has had its timestamps queried recently, and the current
> +coarse-grained time does not cause a change.
> +
> +Inode Timestamps
> +================
> +There are currently 3 timestamps in the inode that are updated to the current
> +wallclock time on different activity:
> +
> +ctime:
> +  The inode change time. This is stamped with the current time whenever
> +  the inode's metadata is changed. Note that this value is not settable
> +  from userland.
> +
> +mtime:
> +  The inode modification time. This is stamped with the current time
> +  any time a file's contents change.
> +
> +atime:
> +  The inode access time. This is stamped whenever an inode's contents are
> +  read. Widely considered to be a terrible mistake. Usually avoided with
> +  options like noatime or relatime.

And for btime/crtime (aka creation time) a filesystem can take the
coarse timestamp, right?  It's not settable by userspace, and I think
statx is the only way those are ever exposed.  QUERIED is never set when
the file is being created.

> +Updating the mtime always implies a change to the ctime, but updating the
> +atime due to a read request does not.
> +
> +Multigrain timestamps are only tracked for the ctime and the mtime. atimes are
> +not affected and always use the coarse-grained value (subject to the floor).

Is it ok if an atime update uses the same timespec as was used for a
ctime update?  There's a pending update for 6.11 that changes
xfs_trans_ichgtime to do:

	tv = current_time(inode);

	if (flags & XFS_ICHGTIME_MOD)
		inode_set_mtime_to_ts(inode, tv);
	if (flags & XFS_ICHGTIME_CHG)
		inode_set_ctime_to_ts(inode, tv);
	if (flags & XFS_ICHGTIME_ACCESS)
		inode_set_atime_to_ts(inode, tv);
	if (flags & XFS_ICHGTIME_CREATE)
		ip->i_crtime = tv;

So I guess xfs could do something like this to set @tv:

	if (flags & XFS_ICHGTIME_CHG)
		tv = inode_set_ctime_current(inode);
	else
		tv = current_time();
...
	if (flags & XFS_ICHGTIME_ACCESS)
		inode_set_atime_to_ts(inode, tv);

Thoughts?

> +Inode Timestamp Ordering
> +========================
> +
> +In addition to just providing info about changes to individual files, file
> +timestamps also serve an important purpose in applications like "make". These
> +programs measure timestamps in order to determine whether source files might be
> +newer than cached objects.
> +
> +Userland applications like make can only determine ordering based on
> +operational boundaries. For a syscall those are the syscall entry and exit
> +points. For io_uring or nfsd operations, that's the request submission and
> +response. In the case of concurrent operations, userland can make no
> +determination about the order in which things will occur.
> +
> +For instance, if a single thread modifies one file, and then another file in
> +sequence, the second file must show an equal or later mtime than the first. The
> +same is true if two threads are issuing similar operations that do not overlap
> +in time.
> +
> +If however, two threads have racing syscalls that overlap in time, then there
> +is no such guarantee, and the second file may appear to have been modified
> +before, after or at the same time as the first, regardless of which one was
> +submitted first.
> +
> +Multigrain Timestamps
> +=====================
> +Multigrain timestamps are aimed at ensuring that changes to a single file are
> +always recognizable, without violating the ordering guarantees when multiple
> +different files are modified. This affects the mtime and the ctime, but the
> +atime will always use coarse-grained timestamps.
> +
> +It uses an unused bit in the i_ctime_nsec field to indicate whether the mtime
> +or ctime has been queried. If either or both have, then the kernel takes
> +special care to ensure the next timestamp update will display a visible change.
> +This ensures tight cache coherency for use-cases like NFS, without sacrificing
> +the benefits of reduced metadata updates when files aren't being watched.
> +
> +The Ctime Floor Value
> +=====================
> +It's not sufficient to simply use fine or coarse-grained timestamps based on
> +whether the mtime or ctime has been queried. A file could get a fine grained
> +timestamp, and then a second file modified later could get a coarse-grained one
> +that appears earlier than the first, which would break the kernel's timestamp
> +ordering guarantees.
> +
> +To mitigate this problem, we maintain a global floor value that ensures that
> +this can't happen. The two files in the above example may appear to have been
> +modified at the same time in such a case, but they will never show the reverse
> +order. To avoid problems with realtime clock jumps, the floor is managed as a
> +monotonic ktime_t, and the values are converted to realtime clock values as
> +needed.

monotonic atomic64_t?

--D

> +
> +Implementation Notes
> +====================
> +Multigrain timestamps are intended for use by local filesystems that get
> +ctime values from the local clock. This is in contrast to network filesystems
> +and the like that just mirror timestamp values from a server.
> +
> +For most filesystems, it's sufficient to just set the FS_MGTIME flag in the
> +fstype->fs_flags in order to opt-in, providing the ctime is only ever set via
> +inode_set_ctime_current(). If the filesystem has a ->getattr routine that
> +doesn't call generic_fillattr, then you should have it call fill_mg_cmtime to
> +fill those values.
> 
> -- 
> 2.45.2
> 
> 

Re: [PATCH v5 6/9] xfs: switch to multigrain timestamps

[Darrick J. Wong]

On Thu, Jul 11, 2024 at 11:58:59AM -0400, Jeff Layton wrote:
> On Thu, 2024-07-11 at 08:09 -0700, Darrick J. Wong wrote:
> > On Thu, Jul 11, 2024 at 07:08:10AM -0400, Jeff Layton wrote:
> > > Enable multigrain timestamps, which should ensure that there is an
> > > apparent change to the timestamp whenever it has been written after
> > > being actively observed via getattr.
> > > 
> > > Also, anytime the mtime changes, the ctime must also change, and those
> > > are now the only two options for xfs_trans_ichgtime. Have that function
> > > unconditionally bump the ctime, and ASSERT that XFS_ICHGTIME_CHG is
> > > always set.
> > > 
> > > Finally, stop setting STATX_CHANGE_COOKIE in getattr, since the ctime
> > > should give us better semantics now.
> > 
> > Following up on "As long as the fs isn't touching i_ctime_nsec directly,
> > you shouldn't need to worry about this" from:
> > https://lore.kernel.org/linux-xfs/cae5c28f172ac57b7eaaa98a00b23f342f01ba64.camel@kernel.org/
> > 
> > xfs /does/ touch i_ctime_nsec directly when it's writing inodes to disk.
> > From xfs_inode_to_disk, see:
> > 
> > 	to->di_ctime = xfs_inode_to_disk_ts(ip, inode_get_ctime(inode));
> > 
> > AFAICT, inode_get_ctime itself remains unchanged, and still returns
> > inode->__i_ctime, right?  In which case it's returning a raw timespec64,
> > which can include the QUERIED flag in tv_nsec, right?
> > 
> 
> No, in the first patch in the series, inode_get_ctime becomes this:
> 
> #define I_CTIME_QUERIED         ((u32)BIT(31))
> 
> static inline time64_t inode_get_ctime_sec(const struct inode *inode)
> {
>         return inode->i_ctime_sec;
> }
> 
> static inline long inode_get_ctime_nsec(const struct inode *inode)
> {
>         return inode->i_ctime_nsec & ~I_CTIME_QUERIED;
> }
> 
> static inline struct timespec64 inode_get_ctime(const struct inode *inode)
> {
>         struct timespec64 ts = { .tv_sec  = inode_get_ctime_sec(inode),
>                                  .tv_nsec = inode_get_ctime_nsec(inode) };
> 
>         return ts;
> }

Doh!  I forgot that this has already been soaking in the vfs tree:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/include/linux/fs.h?h=next-20240711&id=3aa63a569c64e708df547a8913c84e64a06e7853

> ...which should ensure that you never store the QUERIED bit.

So yep, we're fine here.  Sorry about the noise; this was the very
subtle clue in the diff that the change had already been applied:

 static inline struct timespec64 inode_get_ctime(const struct inode *inode)
@@ -1626,13 +1637,7 @@ static inline struct timespec64 inode_get_ctime(const struct inode *inode)
 	return ts;
 }

(Doh doh doh doh doh...)

> > Now let's look at the consumer:
> > 
> > static inline xfs_timestamp_t
> > xfs_inode_to_disk_ts(
> > 	struct xfs_inode		*ip,
> > 	const struct timespec64		tv)
> > {
> > 	struct xfs_legacy_timestamp	*lts;
> > 	xfs_timestamp_t			ts;
> > 
> > 	if (xfs_inode_has_bigtime(ip))
> > 		return cpu_to_be64(xfs_inode_encode_bigtime(tv));
> > 
> > 	lts = (struct xfs_legacy_timestamp *)&ts;
> > 	lts->t_sec = cpu_to_be32(tv.tv_sec);
> > 	lts->t_nsec = cpu_to_be32(tv.tv_nsec);
> > 
> > 	return ts;
> > }
> > 
> > For the !bigtime case (aka before we added y2038 support) the queried
> > flag gets encoded into the tv_nsec field since xfs doesn't filter the
> > queried flag.
> > 
> > For the bigtime case, the timespec is turned into an absolute nsec count
> > since the xfs epoch (which is the minimum timestamp possible under the
> > old encoding scheme):
> > 
> > static inline uint64_t xfs_inode_encode_bigtime(struct timespec64 tv)
> > {
> > 	return xfs_unix_to_bigtime(tv.tv_sec) * NSEC_PER_SEC + tv.tv_nsec;
> > }
> > 
> > Here we'd also be mixing in the QUERIED flag, only now we've encoded a
> > time that's a second in the future.  I think the solution is to add a:
> > 
> > static inline struct timespec64
> > inode_peek_ctime(const struct inode *inode)
> > {
> > 	return (struct timespec64){
> > 		.tv_sec = inode->__i_ctime.tv_sec,
> > 		.tv_nsec = inode->__i_ctime.tv_nsec & ~I_CTIME_QUERIED,
> > 	};
> > }
> > 
> > similar to what inode_peek_iversion does for iversion; and then
> > xfs_inode_to_disk can do:
> > 
> > 	to->di_ctime = xfs_inode_to_disk_ts(ip, inode_peek_ctime(inode));
> > 
> > which would prevent I_CTIME_QUERIED from going out to disk.
> > 
> > At load time, xfs_inode_from_disk uses inode_set_ctime_to_ts so I think
> > xfs won't accidentally introduce QUERIED when it's loading an inode from
> > disk.
> > 
> > 
> 
> Also already done in this patchset:
> 
> struct timespec64 inode_set_ctime_to_ts(struct inode *inode, struct timespec64 ts)
> {
>         inode->i_ctime_sec = ts.tv_sec;
>         inode->i_ctime_nsec = ts.tv_nsec & ~I_CTIME_QUERIED;
>         trace_inode_set_ctime_to_ts(inode, &ts);
>         return ts;
> }
> EXPORT_SYMBOL(inode_set_ctime_to_ts);
> 
> Basically, we never want to store or fetch the QUERIED flag from disk,
> and since it's in an unused bit, we can just universally mask it off
> when dealing with "external" users of it.
> 
> One caveat -- I am using the sign bit for the QUERIED flag, so I'm
> assuming that no one should ever pass inode_set_ctime_to_ts a negative
> tv_nsec value.
> 
> Maybe I should add a WARN_ON_ONCE here to check for that? It seems
> nonsensical, but you never know...

Well in theory filesystems should validate incoming timestamps and
reject tv_nsec with the high bit set, but I'd bet there's a filesystem
out there that allows negative nanoseconds, even if the kernel will
never pass it such a thing. ;)

> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/xfs/libxfs/xfs_trans_inode.c |  6 +++---
> > >  fs/xfs/xfs_iops.c               | 10 +++-------
> > >  fs/xfs/xfs_super.c              |  2 +-
> > >  3 files changed, 7 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> > > index 69fc5b981352..1f3639bbf5f0 100644
> > > --- a/fs/xfs/libxfs/xfs_trans_inode.c
> > > +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> > > @@ -62,12 +62,12 @@ xfs_trans_ichgtime(
> > >  	ASSERT(tp);
> > >  	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> > >  
> > > -	tv = current_time(inode);
> > > +	/* If the mtime changes, then ctime must also change */
> > > +	ASSERT(flags & XFS_ICHGTIME_CHG);
> > >  
> > > +	tv = inode_set_ctime_current(inode);
> > >  	if (flags & XFS_ICHGTIME_MOD)
> > >  		inode_set_mtime_to_ts(inode, tv);
> > > -	if (flags & XFS_ICHGTIME_CHG)
> > > -		inode_set_ctime_to_ts(inode, tv);
> > >  	if (flags & XFS_ICHGTIME_CREATE)
> > >  		ip->i_crtime = tv;

And as I mentioned elsewhere in this thread, 6.11 contains a change to
make it so that xfs_trans_ichgtime can set the access time.  That breaks
the old assertion that XFS_ICHGTIME_CHG is always set, but I think we
can work around that easily.

	if (flags & XFS_ICHGTIME_CHG)
		tv = inode_set_ctime_current(inode);
	else
		tv = current_time(inode);

--D

> > >  }
> > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > index a00dcbc77e12..d25872f818fa 100644
> > > --- a/fs/xfs/xfs_iops.c
> > > +++ b/fs/xfs/xfs_iops.c
> > > @@ -592,8 +592,9 @@ xfs_vn_getattr(
> > >  	stat->gid = vfsgid_into_kgid(vfsgid);
> > >  	stat->ino = ip->i_ino;
> > >  	stat->atime = inode_get_atime(inode);
> > > -	stat->mtime = inode_get_mtime(inode);
> > > -	stat->ctime = inode_get_ctime(inode);
> > > +
> > > +	fill_mg_cmtime(stat, request_mask, inode);
> > > +
> > >  	stat->blocks = XFS_FSB_TO_BB(mp, ip->i_nblocks + ip->i_delayed_blks);
> > >  
> > >  	if (xfs_has_v3inodes(mp)) {
> > > @@ -603,11 +604,6 @@ xfs_vn_getattr(
> > >  		}
> > >  	}
> > >  
> > > -	if ((request_mask & STATX_CHANGE_COOKIE) && IS_I_VERSION(inode)) {
> > > -		stat->change_cookie = inode_query_iversion(inode);
> > > -		stat->result_mask |= STATX_CHANGE_COOKIE;
> > > -	}
> > > -
> > >  	/*
> > >  	 * Note: If you add another clause to set an attribute flag, please
> > >  	 * update attributes_mask below.
> > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > index 27e9f749c4c7..210481b03fdb 100644
> > > --- a/fs/xfs/xfs_super.c
> > > +++ b/fs/xfs/xfs_super.c
> > > @@ -2052,7 +2052,7 @@ static struct file_system_type xfs_fs_type = {
> > >  	.init_fs_context	= xfs_init_fs_context,
> > >  	.parameters		= xfs_fs_parameters,
> > >  	.kill_sb		= xfs_kill_sb,
> > > -	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
> > > +	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP | FS_MGTIME,
> > >  };
> > >  MODULE_ALIAS_FS("xfs");
> > >  
> > > 
> > > -- 
> > > 2.45.2
> > > 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

Re: [PATCH v5 5/9] Documentation: add a new file documenting multigrain timestamps

[Jeff Layton]

On Thu, 2024-07-11 at 12:12 -0700, Darrick J. Wong wrote:
> On Thu, Jul 11, 2024 at 07:08:09AM -0400, Jeff Layton wrote:
> > Add a high-level document that describes how multigrain timestamps work,
> > rationale for them, and some info about implementation and tradeoffs.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  Documentation/filesystems/multigrain-ts.rst | 120 ++++++++++++++++++++++++++++
> >  1 file changed, 120 insertions(+)
> > 
> > diff --git a/Documentation/filesystems/multigrain-ts.rst b/Documentation/filesystems/multigrain-ts.rst
> > new file mode 100644
> > index 000000000000..5cefc204ecec
> > --- /dev/null
> > +++ b/Documentation/filesystems/multigrain-ts.rst
> > @@ -0,0 +1,120 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +=====================
> > +Multigrain Timestamps
> > +=====================
> > +
> > +Introduction
> > +============
> > +Historically, the kernel has always used coarse time values to stamp
> > +inodes. This value is updated on every jiffy, so any change that happens
> > +within that jiffy will end up with the same timestamp.
> > +
> > +When the kernel goes to stamp an inode (due to a read or write), it first gets
> > +the current time and then compares it to the existing timestamp(s) to see
> > +whether anything will change. If nothing changed, then it can avoid updating
> > +the inode's metadata.
> > +
> > +Coarse timestamps are therefore good from a performance standpoint, since they
> > +reduce the need for metadata updates, but bad from the standpoint of
> > +determining whether anything has changed, since a lot of things can happen in a
> > +jiffy.
> > +
> > +They are particularly troublesome with NFSv3, where unchanging timestamps can
> > +make it difficult to tell whether to invalidate caches. NFSv4 provides a
> > +dedicated change attribute that should always show a visible change, but not
> > +all filesystems implement this properly, causing the NFS server to substitute
> > +the ctime in many cases.
> > +
> > +Multigrain timestamps aim to remedy this by selectively using fine-grained
> > +timestamps when a file has had its timestamps queried recently, and the current
> > +coarse-grained time does not cause a change.
> > +
> > +Inode Timestamps
> > +================
> > +There are currently 3 timestamps in the inode that are updated to the current
> > +wallclock time on different activity:
> > +
> > +ctime:
> > +  The inode change time. This is stamped with the current time whenever
> > +  the inode's metadata is changed. Note that this value is not settable
> > +  from userland.
> > +
> > +mtime:
> > +  The inode modification time. This is stamped with the current time
> > +  any time a file's contents change.
> > +
> > +atime:
> > +  The inode access time. This is stamped whenever an inode's contents are
> > +  read. Widely considered to be a terrible mistake. Usually avoided with
> > +  options like noatime or relatime.
> 
> And for btime/crtime (aka creation time) a filesystem can take the
> coarse timestamp, right?  It's not settable by userspace, and I think
> statx is the only way those are ever exposed.  QUERIED is never set when
> the file is being created.
> 

Yep. I'd just copy the ctime to the btime after it's set on creation so
that everything lines up nicely.

> > +Updating the mtime always implies a change to the ctime, but updating the
> > +atime due to a read request does not.
> > +
> > +Multigrain timestamps are only tracked for the ctime and the mtime. atimes are
> > +not affected and always use the coarse-grained value (subject to the floor).
> 
> Is it ok if an atime update uses the same timespec as was used for a
> ctime update?  There's a pending update for 6.11 that changes
> xfs_trans_ichgtime to do:
>
> 	tv = current_time(inode);
> 
> 	if (flags & XFS_ICHGTIME_MOD)
> 		inode_set_mtime_to_ts(inode, tv);
> 	if (flags & XFS_ICHGTIME_CHG)
> 		inode_set_ctime_to_ts(inode, tv);
> 	if (flags & XFS_ICHGTIME_ACCESS)
> 		inode_set_atime_to_ts(inode, tv);
> 	if (flags & XFS_ICHGTIME_CREATE)
> 		ip->i_crtime = tv;
> 

Yeah, that should be fine. If you were doing some (hypothetical)
operation that needs to set both the ctime and the atime, then the
natural thing to do is to just let the atime's value "flow" from the
updated ctime.

> So I guess xfs could do something like this to set @tv:
> 
> 	if (flags & XFS_ICHGTIME_CHG)
> 		tv = inode_set_ctime_current(inode);
> 	else
> 		tv = current_time();
> ...
> 	if (flags & XFS_ICHGTIME_ACCESS)
> 		inode_set_atime_to_ts(inode, tv);
>
> Thoughts?
> 

Yes, that should be fine. It's pretty similar to what we do in
inode_update_timestamps():

	if (flags & (S_MTIME|S_CTIME|S_VERSION)) {
		...
                now = inode_set_ctime_current(inode);
		...
        } else {
                now = current_time(inode);
	}


In practice, a mtime or version change implies a ctime change, whereas
an atime change generally doesn't. Still, I set up the infrastructure
to handle it properly if the ctime and atime are ever updated together.

> > +Inode Timestamp Ordering
> > +========================
> > +
> > +In addition to just providing info about changes to individual files, file
> > +timestamps also serve an important purpose in applications like "make". These
> > +programs measure timestamps in order to determine whether source files might be
> > +newer than cached objects.
> > +
> > +Userland applications like make can only determine ordering based on
> > +operational boundaries. For a syscall those are the syscall entry and exit
> > +points. For io_uring or nfsd operations, that's the request submission and
> > +response. In the case of concurrent operations, userland can make no
> > +determination about the order in which things will occur.
> > +
> > +For instance, if a single thread modifies one file, and then another file in
> > +sequence, the second file must show an equal or later mtime than the first. The
> > +same is true if two threads are issuing similar operations that do not overlap
> > +in time.
> > +
> > +If however, two threads have racing syscalls that overlap in time, then there
> > +is no such guarantee, and the second file may appear to have been modified
> > +before, after or at the same time as the first, regardless of which one was
> > +submitted first.
> > +
> > +Multigrain Timestamps
> > +=====================
> > +Multigrain timestamps are aimed at ensuring that changes to a single file are
> > +always recognizable, without violating the ordering guarantees when multiple
> > +different files are modified. This affects the mtime and the ctime, but the
> > +atime will always use coarse-grained timestamps.
> > +
> > +It uses an unused bit in the i_ctime_nsec field to indicate whether the mtime
> > +or ctime has been queried. If either or both have, then the kernel takes
> > +special care to ensure the next timestamp update will display a visible change.
> > +This ensures tight cache coherency for use-cases like NFS, without sacrificing
> > +the benefits of reduced metadata updates when files aren't being watched.
> > +
> > +The Ctime Floor Value
> > +=====================
> > +It's not sufficient to simply use fine or coarse-grained timestamps based on
> > +whether the mtime or ctime has been queried. A file could get a fine grained
> > +timestamp, and then a second file modified later could get a coarse-grained one
> > +that appears earlier than the first, which would break the kernel's timestamp
> > +ordering guarantees.
> > +
> > +To mitigate this problem, we maintain a global floor value that ensures that
> > +this can't happen. The two files in the above example may appear to have been
> > +modified at the same time in such a case, but they will never show the reverse
> > +order. To avoid problems with realtime clock jumps, the floor is managed as a
> > +monotonic ktime_t, and the values are converted to realtime clock values as
> > +needed.
> 
> monotonic atomic64_t?
> 

It is an atomic64_t, but the values come from the ktime_get_*
functions, so we use the value as a ktime_t. Both are typedefs of s64
though, so casting between them is seamless.

I'll see if I can make that clearer in the doc.

> --D
> 
> > +
> > +Implementation Notes
> > +====================
> > +Multigrain timestamps are intended for use by local filesystems that get
> > +ctime values from the local clock. This is in contrast to network filesystems
> > +and the like that just mirror timestamp values from a server.
> > +
> > +For most filesystems, it's sufficient to just set the FS_MGTIME flag in the
> > +fstype->fs_flags in order to opt-in, providing the ctime is only ever set via
> > +inode_set_ctime_current(). If the filesystem has a ->getattr routine that
> > +doesn't call generic_fillattr, then you should have it call fill_mg_cmtime to
> > +fill those values.
> > 
> > -- 
> > 2.45.2
> > 
> > 

Thanks!
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v5 6/9] xfs: switch to multigrain timestamps

[Jeff Layton]

On Thu, 2024-07-11 at 12:14 -0700, Darrick J. Wong wrote:
> On Thu, Jul 11, 2024 at 11:58:59AM -0400, Jeff Layton wrote:
> > On Thu, 2024-07-11 at 08:09 -0700, Darrick J. Wong wrote:
> > > On Thu, Jul 11, 2024 at 07:08:10AM -0400, Jeff Layton wrote:
> > > > Enable multigrain timestamps, which should ensure that there is an
> > > > apparent change to the timestamp whenever it has been written after
> > > > being actively observed via getattr.
> > > > 
> > > > Also, anytime the mtime changes, the ctime must also change, and those
> > > > are now the only two options for xfs_trans_ichgtime. Have that function
> > > > unconditionally bump the ctime, and ASSERT that XFS_ICHGTIME_CHG is
> > > > always set.
> > > > 
> > > > Finally, stop setting STATX_CHANGE_COOKIE in getattr, since the ctime
> > > > should give us better semantics now.
> > > 
> > > Following up on "As long as the fs isn't touching i_ctime_nsec directly,
> > > you shouldn't need to worry about this" from:
> > > https://lore.kernel.org/linux-xfs/cae5c28f172ac57b7eaaa98a00b23f342f01ba64.camel@kernel.org/
> > > 
> > > xfs /does/ touch i_ctime_nsec directly when it's writing inodes to disk.
> > > From xfs_inode_to_disk, see:
> > > 
> > > 	to->di_ctime = xfs_inode_to_disk_ts(ip, inode_get_ctime(inode));
> > > 
> > > AFAICT, inode_get_ctime itself remains unchanged, and still returns
> > > inode->__i_ctime, right?  In which case it's returning a raw timespec64,
> > > which can include the QUERIED flag in tv_nsec, right?
> > > 
> > 
> > No, in the first patch in the series, inode_get_ctime becomes this:
> > 
> > #define I_CTIME_QUERIED         ((u32)BIT(31))
> > 
> > static inline time64_t inode_get_ctime_sec(const struct inode *inode)
> > {
> >         return inode->i_ctime_sec;
> > }
> > 
> > static inline long inode_get_ctime_nsec(const struct inode *inode)
> > {
> >         return inode->i_ctime_nsec & ~I_CTIME_QUERIED;
> > }
> > 
> > static inline struct timespec64 inode_get_ctime(const struct inode *inode)
> > {
> >         struct timespec64 ts = { .tv_sec  = inode_get_ctime_sec(inode),
> >                                  .tv_nsec = inode_get_ctime_nsec(inode) };
> > 
> >         return ts;
> > }
> 
> Doh!  I forgot that this has already been soaking in the vfs tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/include/linux/fs.h?h=next-20240711&id=3aa63a569c64e708df547a8913c84e64a06e7853
> 
> > ...which should ensure that you never store the QUERIED bit.
> 
> So yep, we're fine here.  Sorry about the noise; this was the very
> subtle clue in the diff that the change had already been applied:
> 
>  static inline struct timespec64 inode_get_ctime(const struct inode *inode)
> @@ -1626,13 +1637,7 @@ static inline struct timespec64 inode_get_ctime(const struct inode *inode)
>  	return ts;
>  }
> 
> (Doh doh doh doh doh...)
> 
> > > Now let's look at the consumer:
> > > 
> > > static inline xfs_timestamp_t
> > > xfs_inode_to_disk_ts(
> > > 	struct xfs_inode		*ip,
> > > 	const struct timespec64		tv)
> > > {
> > > 	struct xfs_legacy_timestamp	*lts;
> > > 	xfs_timestamp_t			ts;
> > > 
> > > 	if (xfs_inode_has_bigtime(ip))
> > > 		return cpu_to_be64(xfs_inode_encode_bigtime(tv));
> > > 
> > > 	lts = (struct xfs_legacy_timestamp *)&ts;
> > > 	lts->t_sec = cpu_to_be32(tv.tv_sec);
> > > 	lts->t_nsec = cpu_to_be32(tv.tv_nsec);
> > > 
> > > 	return ts;
> > > }
> > > 
> > > For the !bigtime case (aka before we added y2038 support) the queried
> > > flag gets encoded into the tv_nsec field since xfs doesn't filter the
> > > queried flag.
> > > 
> > > For the bigtime case, the timespec is turned into an absolute nsec count
> > > since the xfs epoch (which is the minimum timestamp possible under the
> > > old encoding scheme):
> > > 
> > > static inline uint64_t xfs_inode_encode_bigtime(struct timespec64 tv)
> > > {
> > > 	return xfs_unix_to_bigtime(tv.tv_sec) * NSEC_PER_SEC + tv.tv_nsec;
> > > }
> > > 
> > > Here we'd also be mixing in the QUERIED flag, only now we've encoded a
> > > time that's a second in the future.  I think the solution is to add a:
> > > 
> > > static inline struct timespec64
> > > inode_peek_ctime(const struct inode *inode)
> > > {
> > > 	return (struct timespec64){
> > > 		.tv_sec = inode->__i_ctime.tv_sec,
> > > 		.tv_nsec = inode->__i_ctime.tv_nsec & ~I_CTIME_QUERIED,
> > > 	};
> > > }
> > > 
> > > similar to what inode_peek_iversion does for iversion; and then
> > > xfs_inode_to_disk can do:
> > > 
> > > 	to->di_ctime = xfs_inode_to_disk_ts(ip, inode_peek_ctime(inode));
> > > 
> > > which would prevent I_CTIME_QUERIED from going out to disk.
> > > 
> > > At load time, xfs_inode_from_disk uses inode_set_ctime_to_ts so I think
> > > xfs won't accidentally introduce QUERIED when it's loading an inode from
> > > disk.
> > > 
> > > 
> > 
> > Also already done in this patchset:
> > 
> > struct timespec64 inode_set_ctime_to_ts(struct inode *inode, struct timespec64 ts)
> > {
> >         inode->i_ctime_sec = ts.tv_sec;
> >         inode->i_ctime_nsec = ts.tv_nsec & ~I_CTIME_QUERIED;
> >         trace_inode_set_ctime_to_ts(inode, &ts);
> >         return ts;
> > }
> > EXPORT_SYMBOL(inode_set_ctime_to_ts);
> > 
> > Basically, we never want to store or fetch the QUERIED flag from disk,
> > and since it's in an unused bit, we can just universally mask it off
> > when dealing with "external" users of it.
> > 
> > One caveat -- I am using the sign bit for the QUERIED flag, so I'm
> > assuming that no one should ever pass inode_set_ctime_to_ts a negative
> > tv_nsec value.
> > 
> > Maybe I should add a WARN_ON_ONCE here to check for that? It seems
> > nonsensical, but you never know...
> 
> Well in theory filesystems should validate incoming timestamps and
> reject tv_nsec with the high bit set, but I'd bet there's a filesystem
> out there that allows negative nanoseconds, even if the kernel will
> never pass it such a thing. ;)
> 

Hmm, in that case, we probably should normalize the timestamp in this
function before setting the ctime to it. That way we can ensure that
the bit will be meaningless when we use it. I think the kernel has a
way to do that. I'll take a look tomorrow.

Thanks!

> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_trans_inode.c |  6 +++---
> > > >  fs/xfs/xfs_iops.c               | 10 +++-------
> > > >  fs/xfs/xfs_super.c              |  2 +-
> > > >  3 files changed, 7 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> > > > index 69fc5b981352..1f3639bbf5f0 100644
> > > > --- a/fs/xfs/libxfs/xfs_trans_inode.c
> > > > +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> > > > @@ -62,12 +62,12 @@ xfs_trans_ichgtime(
> > > >  	ASSERT(tp);
> > > >  	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> > > >  
> > > > -	tv = current_time(inode);
> > > > +	/* If the mtime changes, then ctime must also change */
> > > > +	ASSERT(flags & XFS_ICHGTIME_CHG);
> > > >  
> > > > +	tv = inode_set_ctime_current(inode);
> > > >  	if (flags & XFS_ICHGTIME_MOD)
> > > >  		inode_set_mtime_to_ts(inode, tv);
> > > > -	if (flags & XFS_ICHGTIME_CHG)
> > > > -		inode_set_ctime_to_ts(inode, tv);
> > > >  	if (flags & XFS_ICHGTIME_CREATE)
> > > >  		ip->i_crtime = tv;
> 
> And as I mentioned elsewhere in this thread, 6.11 contains a change to
> make it so that xfs_trans_ichgtime can set the access time.  That breaks
> the old assertion that XFS_ICHGTIME_CHG is always set, but I think we
> can work around that easily.
> 
> 	if (flags & XFS_ICHGTIME_CHG)
> 		tv = inode_set_ctime_current(inode);
> 	else
> 		tv = current_time(inode);
> 
> --D
> 
> > > >  }
> > > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > > index a00dcbc77e12..d25872f818fa 100644
> > > > --- a/fs/xfs/xfs_iops.c
> > > > +++ b/fs/xfs/xfs_iops.c
> > > > @@ -592,8 +592,9 @@ xfs_vn_getattr(
> > > >  	stat->gid = vfsgid_into_kgid(vfsgid);
> > > >  	stat->ino = ip->i_ino;
> > > >  	stat->atime = inode_get_atime(inode);
> > > > -	stat->mtime = inode_get_mtime(inode);
> > > > -	stat->ctime = inode_get_ctime(inode);
> > > > +
> > > > +	fill_mg_cmtime(stat, request_mask, inode);
> > > > +
> > > >  	stat->blocks = XFS_FSB_TO_BB(mp, ip->i_nblocks + ip->i_delayed_blks);
> > > >  
> > > >  	if (xfs_has_v3inodes(mp)) {
> > > > @@ -603,11 +604,6 @@ xfs_vn_getattr(
> > > >  		}
> > > >  	}
> > > >  
> > > > -	if ((request_mask & STATX_CHANGE_COOKIE) && IS_I_VERSION(inode)) {
> > > > -		stat->change_cookie = inode_query_iversion(inode);
> > > > -		stat->result_mask |= STATX_CHANGE_COOKIE;
> > > > -	}
> > > > -
> > > >  	/*
> > > >  	 * Note: If you add another clause to set an attribute flag, please
> > > >  	 * update attributes_mask below.
> > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > > index 27e9f749c4c7..210481b03fdb 100644
> > > > --- a/fs/xfs/xfs_super.c
> > > > +++ b/fs/xfs/xfs_super.c
> > > > @@ -2052,7 +2052,7 @@ static struct file_system_type xfs_fs_type = {
> > > >  	.init_fs_context	= xfs_init_fs_context,
> > > >  	.parameters		= xfs_fs_parameters,
> > > >  	.kill_sb		= xfs_kill_sb,
> > > > -	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
> > > > +	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP | FS_MGTIME,
> > > >  };
> > > >  MODULE_ALIAS_FS("xfs");
> > > >  
> > > > 
> > > > -- 
> > > > 2.45.2
> > > > 
> > 
> > -- 
> > Jeff Layton <jlayton@kernel.org>

-- 
Jeff Layton <jlayton@kernel.org>