[PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Jeff Layton]

My initial goal was to implement multigrain timestamps on most major
filesystems, so we could present them to userland, and use them for
NFSv3, etc.

With the current implementation however, we can't guarantee that a file
with a coarse grained timestamp modified after one with a fine grained
timestamp will always appear to have a later value. This could confuse
some programs like make, rsync, find, etc. that depend on strict
ordering requirements for timestamps.

The goal of this version is more modest: fix XFS' change attribute.
XFS's change attribute is bumped on atime updates in addition to other
deliberate changes. This makes it unsuitable for export via nfsd.

Jan Kara suggested keeping this functionality internal-only for now and
plumbing the fine grained timestamps through getattr [1]. This set takes
a slightly different approach and has XFS use the fine-grained attr to
fake up STATX_CHANGE_COOKIE in its getattr routine itself.

While we keep fine-grained timestamps in struct inode, when presenting
the timestamps via getattr, we truncate them at a granularity of number
of ns per jiffy, which allows us to smooth over the fuzz that causes
ordering problems.

This set only converts XFS to use this scheme. All of the other
commonly-exported local filesystems have a native change attribute and
wouldn't clearly benefit from mgtime support at this time. Still, it
should be possible to add this support to other filesystems in the
future, as the need arises (bcachefs?).

I'd like to see this go in for v6.7 if possible, so getting it into
linux-next now would be great if there are no objections.

[1]: https://lore.kernel.org/linux-fsdevel/20230920124823.ghl6crb5sh4x2pmt@quack3/

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Jeff Layton (5):
      fs: add infrastructure for multigrain timestamps
      fs: optimize away some fine-grained timestamp updates
      fs: have setattr_copy handle multigrain timestamps appropriately
      fs: add timestamp_truncate_to_gran helper
      xfs: switch to multigrain timestamps

 fs/attr.c                       |  52 ++++++++++++--
 fs/inode.c                      | 151 ++++++++++++++++++++++++++++++++++++----
 fs/xfs/libxfs/xfs_trans_inode.c |   6 +-
 fs/xfs/xfs_iops.c               |  26 +++++--
 fs/xfs/xfs_super.c              |   2 +-
 include/linux/fs.h              |  64 ++++++++++++++++-
 6 files changed, 269 insertions(+), 32 deletions(-)
---
base-commit: f8f2d6d669b91ea98ec8f182c22e06d3d0663e15
change-id: 20230921-ctime-5b7a628a4b95

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

[PATCH v8 1/5] fs: add infrastructure for multigrain timestamps

[Jeff Layton]

The VFS always uses 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
NFS, which traditionally relied on timestamps to validate caches. A lot
of changes can happen in a jiffy, and that can lead to cache-coherency
issues between hosts.

NFSv4 added a dedicated change attribute that must change value after
any change to an inode. Some filesystems (btrfs, ext4 and tmpfs) utilize
the i_version field for this, but the NFSv4 spec allows a server to
generate this value from the inode's ctime.

What we need is a way to only use fine-grained timestamps when they are
being actively queried.

POSIX generally mandates that when the the mtime changes, the ctime must
also change. The kernel always stores normalized ctime values, so only
the first 30 bits of the tv_nsec field are ever used.

Use the 31st bit of the ctime tv_nsec field to indicate that something
has queried the inode for the mtime or ctime. When this flag is set,
on the next mtime or ctime update, the kernel will fetch a fine-grained
timestamp instead of the usual coarse-grained one.

Filesytems can opt into this behavior by setting the FS_MGTIME flag in
the fstype. Filesystems that don't set this flag will continue to use
coarse-grained timestamps.

Note that there is one problem with this scheme. A file with a
coarse-grained timestamp that is modified after a different file with a
fine grained one can appear to have been modified before.

Thus, these timestamps are not suitable for presentation to userland
as-is as it could confuse some programs that depend on strict ordering
via timestamps. For some use cases however (constructing change
cookies), they should be fine.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/inode.c         | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/fs.h |  63 +++++++++++++++++++++++++++++++--
 2 files changed, 160 insertions(+), 5 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 84bc3c76e5cc..f3d68e4b8df7 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -168,6 +168,8 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 	inode->i_fop = &no_open_fops;
 	inode->i_ino = 0;
 	inode->__i_nlink = 1;
+	inode->__i_ctime.tv_sec = 0;
+	inode->__i_ctime.tv_nsec = 0;
 	inode->i_opflags = 0;
 	if (sb->s_xattr)
 		inode->i_opflags |= IOP_XATTR;
@@ -2102,10 +2104,52 @@ int file_remove_privs(struct file *file)
 }
 EXPORT_SYMBOL(file_remove_privs);
 
+/**
+ * current_mgtime - 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_mgtime(struct inode *inode)
+{
+	struct timespec64 now, ctime;
+	atomic_long_t *pnsec = (atomic_long_t *)&inode->__i_ctime.tv_nsec;
+	long nsec = atomic_long_read(pnsec);
+
+	if (nsec & I_CTIME_QUERIED) {
+		ktime_get_real_ts64(&now);
+		return timestamp_truncate(now, inode);
+	}
+
+	ktime_get_coarse_real_ts64(&now);
+	now = timestamp_truncate(now, inode);
+
+	/*
+	 * If we've recently fetched a fine-grained timestamp
+	 * then the coarse-grained one may still be earlier than the
+	 * existing ctime. Just keep the existing value if so.
+	 */
+	ctime = inode_get_ctime(inode);
+	if (timespec64_compare(&ctime, &now) > 0)
+		now = ctime;
+
+	return now;
+}
+EXPORT_SYMBOL(current_mgtime);
+
+static struct timespec64 current_ctime(struct inode *inode)
+{
+	if (is_mgtime(inode))
+		return current_mgtime(inode);
+	return current_time(inode);
+}
+
 static int inode_needs_update_time(struct inode *inode)
 {
 	int sync_it = 0;
-	struct timespec64 now = current_time(inode);
+	struct timespec64 now = current_ctime(inode);
 	struct timespec64 ctime;
 
 	/* First try to exhaust all avenues to not sync */
@@ -2527,6 +2571,13 @@ struct timespec64 current_time(struct inode *inode)
 }
 EXPORT_SYMBOL(current_time);
 
+/*
+ * Coarse timer ticks happen (roughly) every jiffy. If we see a coarse time
+ * more than 2 jiffies earlier than the current ctime, then we need to
+ * update it. This is the max delta allowed (in ns).
+ */
+#define COARSE_TIME_MAX_DELTA (2 / HZ * NSEC_PER_SEC)
+
 /**
  * inode_set_ctime_current - set the ctime to current_time
  * @inode: inode
@@ -2536,9 +2587,54 @@ EXPORT_SYMBOL(current_time);
  */
 struct timespec64 inode_set_ctime_current(struct inode *inode)
 {
-	struct timespec64 now = current_time(inode);
+	struct timespec64 now;
+	struct timespec64 ctime;
+
+	ctime.tv_nsec = READ_ONCE(inode->__i_ctime.tv_nsec);
+	if (!(ctime.tv_nsec & I_CTIME_QUERIED)) {
+		now = current_time(inode);
+
+		/* Just copy it into place if it's not multigrain */
+		if (!is_mgtime(inode)) {
+			inode_set_ctime_to_ts(inode, now);
+			return now;
+		}
 
-	inode_set_ctime(inode, now.tv_sec, now.tv_nsec);
+		/*
+		 * If we've recently updated with a fine-grained timestamp,
+		 * then the coarse-grained one may still be earlier than the
+		 * existing ctime. Just keep the existing value if so.
+		 */
+		ctime.tv_sec = inode->__i_ctime.tv_sec;
+		if (timespec64_compare(&ctime, &now) > 0) {
+			struct timespec64	limit = now;
+
+			/*
+			 * If the current coarse-grained clock is earlier than
+			 * it should be, then that's an indication that there
+			 * may have been a backward clock jump, and that the
+			 * update should not be skipped.
+			 */
+			timespec64_add_ns(&limit, COARSE_TIME_MAX_DELTA);
+			if (timespec64_compare(&ctime, &limit) < 0)
+				return ctime;
+		}
+
+		/*
+		 * Ctime updates are usually protected by the inode_lock, but
+		 * we can still race with someone setting the QUERIED flag.
+		 * Try to swap the new nsec value into place. If it's changed
+		 * in the interim, then just go with a fine-grained timestamp.
+		 */
+		if (cmpxchg(&inode->__i_ctime.tv_nsec, ctime.tv_nsec,
+			    now.tv_nsec) != ctime.tv_nsec)
+			goto fine_grained;
+		inode->__i_ctime.tv_sec = now.tv_sec;
+		return now;
+	}
+fine_grained:
+	ktime_get_real_ts64(&now);
+	inode_set_ctime_to_ts(inode, timestamp_truncate(now, inode));
 	return now;
 }
 EXPORT_SYMBOL(inode_set_ctime_current);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b528f063e8ff..91239a4c1a65 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1508,18 +1508,65 @@ static inline bool fsuidgid_has_mapping(struct super_block *sb,
 	       kgid_has_mapping(fs_userns, kgid);
 }
 
+struct timespec64 current_mgtime(struct inode *inode);
 struct timespec64 current_time(struct inode *inode);
 struct timespec64 inode_set_ctime_current(struct inode *inode);
 
+/*
+ * 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.
+ *
+ * The kernel always keeps normalized struct timespec64 values in the ctime,
+ * which means that only the first 30 bits of the value are used. Use the
+ * 31st bit of the ctime's tv_nsec field as a flag to indicate that the value
+ * has been queried since it was last updated.
+ */
+#define I_CTIME_QUERIED		(1L<<30)
+
 /**
  * inode_get_ctime - fetch the current ctime from the inode
  * @inode: inode from which to fetch ctime
  *
- * Grab the current ctime from the inode and return it.
+ * Grab the current ctime from the inode, mask off the I_CTIME_QUERIED
+ * flag and return it. This is mostly intended for use by internal consumers
+ * of the ctime that aren't concerned with ensuring a fine-grained update on
+ * the next change (e.g. when preparing to store the value in the backing store
+ * for later retrieval).
+ *
+ * This is safe to call regardless of whether the underlying filesystem
+ * is using multigrain timestamps.
  */
 static inline struct timespec64 inode_get_ctime(const struct inode *inode)
 {
-	return inode->__i_ctime;
+	struct timespec64 ctime;
+
+	ctime.tv_sec = inode->__i_ctime.tv_sec;
+	ctime.tv_nsec = inode->__i_ctime.tv_nsec & ~I_CTIME_QUERIED;
+
+	return ctime;
+}
+
+/**
+ * inode_query_ctime - fetch the current ctime from inode and flag it
+ * @inode: inode from which to fetch and flag
+ *
+ * Grab the current ctime from the inode, mask off the I_CTIME_QUERIED
+ * flag and return it. This version also marks the inode as needing a fine
+ * grained timestamp update in the future.
+ */
+static inline struct timespec64 inode_query_ctime(const struct inode *inode)
+{
+	struct timespec64 ctime;
+	atomic_long_t *pnsec = (atomic_long_t *)&inode->__i_ctime.tv_nsec;
+
+	ctime.tv_sec = inode->__i_ctime.tv_sec;
+	ctime.tv_nsec = atomic_long_fetch_or(I_CTIME_QUERIED, pnsec) & ~I_CTIME_QUERIED;
+	return ctime;
 }
 
 /**
@@ -2305,6 +2352,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;
@@ -2328,6 +2376,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));

-- 
2.41.0

[PATCH v8 2/5] fs: optimize away some fine-grained timestamp updates

[Jeff Layton]

When updating the ctime and the QUERIED bit is set, we can still use the
coarse-grained clock if the next coarse time tick has already happened.
Only use the fine grained clock if the coarse grained one is equal to or
earlier than the old ctime.

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

diff --git a/fs/inode.c b/fs/inode.c
index f3d68e4b8df7..293f9ba623d1 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2587,26 +2587,35 @@ EXPORT_SYMBOL(current_time);
  */
 struct timespec64 inode_set_ctime_current(struct inode *inode)
 {
-	struct timespec64 now;
+	struct timespec64 now = current_time(inode);
 	struct timespec64 ctime;
+	bool queried;
+	int tscomp;
 
+	/* Just copy it into place if it's not multigrain */
+	if (!is_mgtime(inode)) {
+		inode_set_ctime_to_ts(inode, now);
+		return now;
+	}
+
+	ctime.tv_sec = inode->__i_ctime.tv_sec;
 	ctime.tv_nsec = READ_ONCE(inode->__i_ctime.tv_nsec);
-	if (!(ctime.tv_nsec & I_CTIME_QUERIED)) {
-		now = current_time(inode);
+	queried = ctime.tv_nsec & I_CTIME_QUERIED;
+	ctime.tv_nsec &= ~I_CTIME_QUERIED;
 
-		/* Just copy it into place if it's not multigrain */
-		if (!is_mgtime(inode)) {
-			inode_set_ctime_to_ts(inode, now);
-			return now;
-		}
+	tscomp = timespec64_compare(&ctime, &now);
 
+	/*
+	 * We can use a coarse-grained timestamp if no one has queried for it,
+	 * or coarse time is already later than the existing ctime.
+	 */
+	if (!queried || tscomp < 0) {
 		/*
 		 * If we've recently updated with a fine-grained timestamp,
 		 * then the coarse-grained one may still be earlier than the
 		 * existing ctime. Just keep the existing value if so.
 		 */
-		ctime.tv_sec = inode->__i_ctime.tv_sec;
-		if (timespec64_compare(&ctime, &now) > 0) {
+		if (tscomp > 0) {
 			struct timespec64	limit = now;
 
 			/*
@@ -2620,6 +2629,10 @@ struct timespec64 inode_set_ctime_current(struct inode *inode)
 				return ctime;
 		}
 
+		/* Put back the queried bit if we stripped it before */
+		if (queried)
+			ctime.tv_nsec |= I_CTIME_QUERIED;
+
 		/*
 		 * Ctime updates are usually protected by the inode_lock, but
 		 * we can still race with someone setting the QUERIED flag.

-- 
2.41.0

[PATCH v8 3/5] 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 a8ae5f6d9b16..8ba330e6a582 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -275,6 +275,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->i_atime = attr->ia_atime;
+	else if (ia_valid & ATTR_ATIME)
+		inode->i_atime = now;
+
+	if (ia_valid & ATTR_MTIME_SET)
+		inode->i_mtime = attr->ia_mtime;
+	else if (ia_valid & ATTR_MTIME)
+		inode->i_mtime = now;
+}
+
 /**
  * setattr_copy - copy simple metadata updates into the generic inode
  * @idmap:	idmap of the mount the inode was found from
@@ -307,12 +343,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->i_atime = attr->ia_atime;
-	if (ia_valid & ATTR_MTIME)
-		inode->i_mtime = 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,
@@ -320,6 +350,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->i_atime = attr->ia_atime;
+	if (ia_valid & ATTR_MTIME)
+		inode->i_mtime = attr->ia_mtime;
+	if (ia_valid & ATTR_CTIME)
+		inode_set_ctime_to_ts(inode, attr->ia_ctime);
 }
 EXPORT_SYMBOL(setattr_copy);
 

-- 
2.41.0

[PATCH v8 4/5] fs: add timestamp_truncate_to_gran helper

[Jeff Layton]

In a future patch, we're going to need to truncate fine-grained
timestamps down to jiffies granularity. Add a new helper that allows
truncating down to an arbitrary granularity.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/inode.c         | 38 +++++++++++++++++++++++++++-----------
 include/linux/fs.h |  1 +
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 293f9ba623d1..ae6baa5b17c5 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2521,6 +2521,29 @@ void inode_nohighmem(struct inode *inode)
 }
 EXPORT_SYMBOL(inode_nohighmem);
 
+/**
+ * timestamp_truncate_to_gran - Truncate timespec to a granularity
+ * @t: Timespec
+ * @gran: the specified granularity (in ns)
+ *
+ * Truncate a timespec to the specified granularity. Always rounds down.
+ * gran must not be 0 nor greater than a second (NSEC_PER_SEC, or 10^9 ns).
+ */
+struct timespec64 timestamp_truncate_to_gran(struct timespec64 t, unsigned int gran)
+{
+	/* Avoid division in the common cases 1 ns and 1 s. */
+	if (gran == 1)
+		; /* nothing */
+	else if (gran == NSEC_PER_SEC)
+		t.tv_nsec = 0;
+	else if (gran > 1 && gran < NSEC_PER_SEC)
+		t.tv_nsec -= t.tv_nsec % gran;
+	else
+		WARN(1, "invalid file time granularity: %u", gran);
+	return t;
+}
+EXPORT_SYMBOL(timestamp_truncate_to_gran);
+
 /**
  * timestamp_truncate - Truncate timespec to a granularity
  * @t: Timespec
@@ -2536,19 +2559,12 @@ struct timespec64 timestamp_truncate(struct timespec64 t, struct inode *inode)
 	unsigned int gran = sb->s_time_gran;
 
 	t.tv_sec = clamp(t.tv_sec, sb->s_time_min, sb->s_time_max);
-	if (unlikely(t.tv_sec == sb->s_time_max || t.tv_sec == sb->s_time_min))
+	if (unlikely(t.tv_sec == sb->s_time_max || t.tv_sec == sb->s_time_min)) {
 		t.tv_nsec = 0;
+		return t;
+	}
 
-	/* Avoid division in the common cases 1 ns and 1 s. */
-	if (gran == 1)
-		; /* nothing */
-	else if (gran == NSEC_PER_SEC)
-		t.tv_nsec = 0;
-	else if (gran > 1 && gran < NSEC_PER_SEC)
-		t.tv_nsec -= t.tv_nsec % gran;
-	else
-		WARN(1, "invalid file time granularity: %u", gran);
-	return t;
+	return timestamp_truncate_to_gran(t, gran);
 }
 EXPORT_SYMBOL(timestamp_truncate);
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 91239a4c1a65..fa696322dae3 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -748,6 +748,7 @@ struct inode {
 	void			*i_private; /* fs or device private pointer */
 } __randomize_layout;
 
+struct timespec64 timestamp_truncate_to_gran(struct timespec64 t, unsigned int gran);
 struct timespec64 timestamp_truncate(struct timespec64 t, struct inode *inode);
 
 static inline unsigned int i_blocksize(const struct inode *node)

-- 
2.41.0

[PATCH v8 5/5] xfs: switch to multigrain timestamps

[Jeff Layton]

Enable multigrain timestamps, which should ensure that there is an
apparent change to the internal ctime value whenever a file has been
written after being actively observed via getattr.

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

In getattr, truncate the mtime and ctime at the number of nanoseconds
per jiffy, which should ensure that ordering is preserved. Use the fine
grained ctime to fake up a STATX_CHANGE_COOKIE if one was requested.

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

diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
index 6b2296ff248a..ad22656376d3 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);
 	ASSERT(xfs_isilocked(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->i_mtime = 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 1c1e6171209d..af4a54756113 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -559,6 +559,7 @@ xfs_vn_getattr(
 	struct xfs_mount	*mp = ip->i_mount;
 	vfsuid_t		vfsuid = i_uid_into_vfsuid(idmap, inode);
 	vfsgid_t		vfsgid = i_gid_into_vfsgid(idmap, inode);
+	struct timespec64	ctime;
 
 	trace_xfs_getattr(ip);
 
@@ -573,8 +574,23 @@ xfs_vn_getattr(
 	stat->gid = vfsgid_into_kgid(vfsgid);
 	stat->ino = ip->i_ino;
 	stat->atime = inode->i_atime;
-	stat->mtime = inode->i_mtime;
-	stat->ctime = inode_get_ctime(inode);
+	stat->mtime = timestamp_truncate_to_gran(inode->i_mtime, NSEC_PER_SEC/HZ);
+
+	/*
+	 * Don't bother flagging the inode for a fine-grained update unless
+	 * STATX_CHANGE_COOKIE is set, in which case, use the fine-grained
+	 * value to fake up a change_cookie.
+	 */
+	if (request_mask & STATX_CHANGE_COOKIE) {
+		ctime = inode_query_ctime(inode);
+		stat->change_cookie = time_to_chattr(&ctime);
+		stat->result_mask |= STATX_CHANGE_COOKIE;
+	} else {
+		ctime = inode_get_ctime(inode);
+	}
+
+	stat->ctime = timestamp_truncate_to_gran(ctime, NSEC_PER_SEC/HZ);
+
 	stat->blocks = XFS_FSB_TO_BB(mp, ip->i_nblocks + ip->i_delayed_blks);
 
 	if (xfs_has_v3inodes(mp)) {
@@ -914,12 +930,8 @@ xfs_setattr_size(
 	 * these flags set.  For all other operations the VFS set these flags
 	 * explicitly if it wants a timestamp update.
 	 */
-	if (newsize != oldsize &&
-	    !(iattr->ia_valid & (ATTR_CTIME | ATTR_MTIME))) {
-		iattr->ia_ctime = iattr->ia_mtime =
-			current_time(inode);
+	if (newsize != oldsize)
 		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME;
-	}
 
 	/*
 	 * The first thing we do is set the size to new_size permanently on
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index b5c202f5d96c..1f77014c6e1a 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -2065,7 +2065,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.41.0

Re: [PATCH v8 1/5] fs: add infrastructure for multigrain timestamps

[Kent Overstreet]

On Fri, Sep 22, 2023 at 01:14:40PM -0400, Jeff Layton wrote:
> The VFS always uses 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
> NFS, which traditionally relied on timestamps to validate caches. A lot
> of changes can happen in a jiffy, and that can lead to cache-coherency
> issues between hosts.
> 
> NFSv4 added a dedicated change attribute that must change value after
> any change to an inode. Some filesystems (btrfs, ext4 and tmpfs) utilize
> the i_version field for this, but the NFSv4 spec allows a server to
> generate this value from the inode's ctime.
> 
> What we need is a way to only use fine-grained timestamps when they are
> being actively queried.
> 
> POSIX generally mandates that when the the mtime changes, the ctime must
> also change. The kernel always stores normalized ctime values, so only
> the first 30 bits of the tv_nsec field are ever used.
> 
> Use the 31st bit of the ctime tv_nsec field to indicate that something
> has queried the inode for the mtime or ctime. When this flag is set,
> on the next mtime or ctime update, the kernel will fetch a fine-grained
> timestamp instead of the usual coarse-grained one.
> 
> Filesytems can opt into this behavior by setting the FS_MGTIME flag in
> the fstype. Filesystems that don't set this flag will continue to use
> coarse-grained timestamps.

Interesting...

So in bcachefs, for most inode fields the btree inode is the "master
copy"; we do inode updates via btree transactions, and then on
successful transaction commit we update the VFS inode to match.

(exceptions: i_size, i_blocks)

I'd been contemplating switching to that model for timestamp updates as
well, since that would allow us to get rid of our
super_operations.write_inode method - except we probably wouldn't want
to do that since it would likely make timestamp updates too expensive.

And now with your scheme of stashing extra state in timespec, I'm glad
we didn't.

Still, timestamp updates are a bit messier than I'd like, would be
lovely to figure out a way to clean that up - right now we have an
awkward mix of "sometimes timestamp updates happen in a btree
transaction first, other times just the VFS inode is updated and marked
dirty".

xfs doesn't have .write_inode, so it's probably time to study what it
does...

Re: [PATCH v8 1/5] fs: add infrastructure for multigrain timestamps

[Jeff Layton]

On Fri, 2023-09-22 at 13:31 -0400, Kent Overstreet wrote:
> On Fri, Sep 22, 2023 at 01:14:40PM -0400, Jeff Layton wrote:
> > The VFS always uses 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
> > NFS, which traditionally relied on timestamps to validate caches. A lot
> > of changes can happen in a jiffy, and that can lead to cache-coherency
> > issues between hosts.
> > 
> > NFSv4 added a dedicated change attribute that must change value after
> > any change to an inode. Some filesystems (btrfs, ext4 and tmpfs) utilize
> > the i_version field for this, but the NFSv4 spec allows a server to
> > generate this value from the inode's ctime.
> > 
> > What we need is a way to only use fine-grained timestamps when they are
> > being actively queried.
> > 
> > POSIX generally mandates that when the the mtime changes, the ctime must
> > also change. The kernel always stores normalized ctime values, so only
> > the first 30 bits of the tv_nsec field are ever used.
> > 
> > Use the 31st bit of the ctime tv_nsec field to indicate that something
> > has queried the inode for the mtime or ctime. When this flag is set,
> > on the next mtime or ctime update, the kernel will fetch a fine-grained
> > timestamp instead of the usual coarse-grained one.
> > 
> > Filesytems can opt into this behavior by setting the FS_MGTIME flag in
> > the fstype. Filesystems that don't set this flag will continue to use
> > coarse-grained timestamps.
> 
> Interesting...
> 
> So in bcachefs, for most inode fields the btree inode is the "master
> copy"; we do inode updates via btree transactions, and then on
> successful transaction commit we update the VFS inode to match.
> 
> (exceptions: i_size, i_blocks)
> 
> I'd been contemplating switching to that model for timestamp updates as
> well, since that would allow us to get rid of our
> super_operations.write_inode method - except we probably wouldn't want
> to do that since it would likely make timestamp updates too expensive.
> 
> And now with your scheme of stashing extra state in timespec, I'm glad
> we didn't.
> 
> Still, timestamp updates are a bit messier than I'd like, would be
> lovely to figure out a way to clean that up - right now we have an
> awkward mix of "sometimes timestamp updates happen in a btree
> transaction first, other times just the VFS inode is updated and marked
> dirty".
> 
> xfs doesn't have .write_inode, so it's probably time to study what it
> does...

A few months ago, we talked briefly and I asked about an i_version
counter for bcachefs. You were going to look into it, and I wasn't sure
if you had implemented one. If you haven't, then this may be a simpler
alternative.

For now, these aren't much good for anything other than faking up a
change attribute for NFSv4, but they should be fine for that and you
wouldn't need to grow your on-disk inode to accommodate them.

Cheers,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Amir Goldstein]

On Fri, Sep 22, 2023 at 8:15 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> My initial goal was to implement multigrain timestamps on most major
> filesystems, so we could present them to userland, and use them for
> NFSv3, etc.
>
> With the current implementation however, we can't guarantee that a file
> with a coarse grained timestamp modified after one with a fine grained
> timestamp will always appear to have a later value. This could confuse
> some programs like make, rsync, find, etc. that depend on strict
> ordering requirements for timestamps.
>
> The goal of this version is more modest: fix XFS' change attribute.
> XFS's change attribute is bumped on atime updates in addition to other
> deliberate changes. This makes it unsuitable for export via nfsd.
>
> Jan Kara suggested keeping this functionality internal-only for now and
> plumbing the fine grained timestamps through getattr [1]. This set takes
> a slightly different approach and has XFS use the fine-grained attr to
> fake up STATX_CHANGE_COOKIE in its getattr routine itself.
>
> While we keep fine-grained timestamps in struct inode, when presenting
> the timestamps via getattr, we truncate them at a granularity of number
> of ns per jiffy,

That's not good, because user explicitly set granular mtime would be
truncated too and booting with different kernels (HZ) would change
the observed timestamps of files.

> which allows us to smooth over the fuzz that causes
> ordering problems.
>

The reported ordering problems (i.e. cp -u) is not even limited to the
scope of a single fs, right?

Thinking out loud - if the QERIED bit was not per inode timestamp
but instead in a global fs_multigrain_ts variable, then all the inodes
of all the mgtime fs would be using globally ordered timestamps

That should eliminate the reported issues with time reorder for
fine vs coarse grained timestamps.

The risk of extra unneeded "change cookie" updates compared to
per inode QUERIED bit may exist, but I think it is a rather small overhead
and maybe worth the tradeoff of having to maintain a real per inode
"change cookie" in addition to a "globally ordered mgtime"?

If this idea is acceptable, you may still be able to salvage the reverted
ctime series for 6.7, because the change to use global mgtime should
be quite trivial?

Thanks,
Amir.

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Jeff Layton]

On Sat, 2023-09-23 at 10:15 +0300, Amir Goldstein wrote:
> On Fri, Sep 22, 2023 at 8:15 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > My initial goal was to implement multigrain timestamps on most major
> > filesystems, so we could present them to userland, and use them for
> > NFSv3, etc.
> > 
> > With the current implementation however, we can't guarantee that a file
> > with a coarse grained timestamp modified after one with a fine grained
> > timestamp will always appear to have a later value. This could confuse
> > some programs like make, rsync, find, etc. that depend on strict
> > ordering requirements for timestamps.
> > 
> > The goal of this version is more modest: fix XFS' change attribute.
> > XFS's change attribute is bumped on atime updates in addition to other
> > deliberate changes. This makes it unsuitable for export via nfsd.
> > 
> > Jan Kara suggested keeping this functionality internal-only for now and
> > plumbing the fine grained timestamps through getattr [1]. This set takes
> > a slightly different approach and has XFS use the fine-grained attr to
> > fake up STATX_CHANGE_COOKIE in its getattr routine itself.
> > 
> > While we keep fine-grained timestamps in struct inode, when presenting
> > the timestamps via getattr, we truncate them at a granularity of number
> > of ns per jiffy,
> 
> That's not good, because user explicitly set granular mtime would be
> truncated too and booting with different kernels (HZ) would change
> the observed timestamps of files.
> 

That's a very good point.

> > which allows us to smooth over the fuzz that causes
> > ordering problems.
> > 
> 
> The reported ordering problems (i.e. cp -u) is not even limited to the
> scope of a single fs, right?
> 

It isn't. Most of the tools we're concerned with don't generally care
about filesystem boundaries.

> Thinking out loud - if the QERIED bit was not per inode timestamp
> but instead in a global fs_multigrain_ts variable, then all the inodes
> of all the mgtime fs would be using globally ordered timestamps
>
> That should eliminate the reported issues with time reorder for
> fine vs coarse grained timestamps.
> 
> The risk of extra unneeded "change cookie" updates compared to
> per inode QUERIED bit may exist, but I think it is a rather small overhead
> and maybe worth the tradeoff of having to maintain a real per inode
> "change cookie" in addition to a "globally ordered mgtime"?
> 
> If this idea is acceptable, you may still be able to salvage the reverted
> ctime series for 6.7, because the change to use global mgtime should
> be quite trivial?
> 

This is basically the idea I was going to look at next once I got some
other stuff settled here: Basically, when we apply a fine-grained
timestamp to an inode, we'd advance the coarse-grained clock that
filesystems use to that value.

It could cause some write amplification: if you are streaming writes to
a bunch of files at the same time and someone stats one of them, then
they'd all end up getting an extra inode transaction. That doesn't sound
_too_ bad on its face, but I probably need to implement it and then run
some numbers to see.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Jeff Layton]

On Sat, 2023-09-23 at 10:15 +0300, Amir Goldstein wrote:
> On Fri, Sep 22, 2023 at 8:15 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > My initial goal was to implement multigrain timestamps on most major
> > filesystems, so we could present them to userland, and use them for
> > NFSv3, etc.
> > 
> > With the current implementation however, we can't guarantee that a file
> > with a coarse grained timestamp modified after one with a fine grained
> > timestamp will always appear to have a later value. This could confuse
> > some programs like make, rsync, find, etc. that depend on strict
> > ordering requirements for timestamps.
> > 
> > The goal of this version is more modest: fix XFS' change attribute.
> > XFS's change attribute is bumped on atime updates in addition to other
> > deliberate changes. This makes it unsuitable for export via nfsd.
> > 
> > Jan Kara suggested keeping this functionality internal-only for now and
> > plumbing the fine grained timestamps through getattr [1]. This set takes
> > a slightly different approach and has XFS use the fine-grained attr to
> > fake up STATX_CHANGE_COOKIE in its getattr routine itself.
> > 
> > While we keep fine-grained timestamps in struct inode, when presenting
> > the timestamps via getattr, we truncate them at a granularity of number
> > of ns per jiffy,
> 
> That's not good, because user explicitly set granular mtime would be
> truncated too and booting with different kernels (HZ) would change
> the observed timestamps of files.
> 

Thinking about this some more, I think the first problem is easily
addressable:

The ctime isn't explicitly settable and with this set, we're already not
truncating the atime. We haven't used any of the extra bits in the mtime
yet, so we could just carve out a flag in there that says "this mtime
was explicitly set and shouldn't be truncated before presentation".

The second problem (booting to older kernel) is a bit tougher to deal
with however. I'll have to think about that one a bit more.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Amir Goldstein]

On Sat, Sep 23, 2023 at 1:46 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Sat, 2023-09-23 at 10:15 +0300, Amir Goldstein wrote:
> > On Fri, Sep 22, 2023 at 8:15 PM Jeff Layton <jlayton@kernel.org> wrote:
> > >
> > > My initial goal was to implement multigrain timestamps on most major
> > > filesystems, so we could present them to userland, and use them for
> > > NFSv3, etc.
> > >
> > > With the current implementation however, we can't guarantee that a file
> > > with a coarse grained timestamp modified after one with a fine grained
> > > timestamp will always appear to have a later value. This could confuse
> > > some programs like make, rsync, find, etc. that depend on strict
> > > ordering requirements for timestamps.
> > >
> > > The goal of this version is more modest: fix XFS' change attribute.
> > > XFS's change attribute is bumped on atime updates in addition to other
> > > deliberate changes. This makes it unsuitable for export via nfsd.
> > >
> > > Jan Kara suggested keeping this functionality internal-only for now and
> > > plumbing the fine grained timestamps through getattr [1]. This set takes
> > > a slightly different approach and has XFS use the fine-grained attr to
> > > fake up STATX_CHANGE_COOKIE in its getattr routine itself.
> > >
> > > While we keep fine-grained timestamps in struct inode, when presenting
> > > the timestamps via getattr, we truncate them at a granularity of number
> > > of ns per jiffy,
> >
> > That's not good, because user explicitly set granular mtime would be
> > truncated too and booting with different kernels (HZ) would change
> > the observed timestamps of files.
> >
>
> Thinking about this some more, I think the first problem is easily
> addressable:
>
> The ctime isn't explicitly settable and with this set, we're already not
> truncating the atime. We haven't used any of the extra bits in the mtime
> yet, so we could just carve out a flag in there that says "this mtime
> was explicitly set and shouldn't be truncated before presentation".
>

I thought about this option too.
But note that the "mtime was explicitly set" flag needs
to be persisted to disk so you cannot store it in the high nsec bits.
At least XFS won't store those bits if you use them - they have to
be translated to an XFS inode flag and I don't know if changing
XFS on-disk format was on your wish list.

Thanks,
Amir.

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Amir Goldstein]

On Sat, Sep 23, 2023 at 1:22 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Sat, 2023-09-23 at 10:15 +0300, Amir Goldstein wrote:
> > On Fri, Sep 22, 2023 at 8:15 PM Jeff Layton <jlayton@kernel.org> wrote:
> > >
> > > My initial goal was to implement multigrain timestamps on most major
> > > filesystems, so we could present them to userland, and use them for
> > > NFSv3, etc.
> > >
> > > With the current implementation however, we can't guarantee that a file
> > > with a coarse grained timestamp modified after one with a fine grained
> > > timestamp will always appear to have a later value. This could confuse
> > > some programs like make, rsync, find, etc. that depend on strict
> > > ordering requirements for timestamps.
> > >
> > > The goal of this version is more modest: fix XFS' change attribute.
> > > XFS's change attribute is bumped on atime updates in addition to other
> > > deliberate changes. This makes it unsuitable for export via nfsd.
> > >
> > > Jan Kara suggested keeping this functionality internal-only for now and
> > > plumbing the fine grained timestamps through getattr [1]. This set takes
> > > a slightly different approach and has XFS use the fine-grained attr to
> > > fake up STATX_CHANGE_COOKIE in its getattr routine itself.
> > >
> > > While we keep fine-grained timestamps in struct inode, when presenting
> > > the timestamps via getattr, we truncate them at a granularity of number
> > > of ns per jiffy,
> >
> > That's not good, because user explicitly set granular mtime would be
> > truncated too and booting with different kernels (HZ) would change
> > the observed timestamps of files.
> >
>
> That's a very good point.
>
> > > which allows us to smooth over the fuzz that causes
> > > ordering problems.
> > >
> >
> > The reported ordering problems (i.e. cp -u) is not even limited to the
> > scope of a single fs, right?
> >
>
> It isn't. Most of the tools we're concerned with don't generally care
> about filesystem boundaries.
>
> > Thinking out loud - if the QERIED bit was not per inode timestamp
> > but instead in a global fs_multigrain_ts variable, then all the inodes
> > of all the mgtime fs would be using globally ordered timestamps
> >
> > That should eliminate the reported issues with time reorder for
> > fine vs coarse grained timestamps.
> >
> > The risk of extra unneeded "change cookie" updates compared to
> > per inode QUERIED bit may exist, but I think it is a rather small overhead
> > and maybe worth the tradeoff of having to maintain a real per inode
> > "change cookie" in addition to a "globally ordered mgtime"?
> >
> > If this idea is acceptable, you may still be able to salvage the reverted
> > ctime series for 6.7, because the change to use global mgtime should
> > be quite trivial?
> >
>
> This is basically the idea I was going to look at next once I got some
> other stuff settled here: Basically, when we apply a fine-grained
> timestamp to an inode, we'd advance the coarse-grained clock that
> filesystems use to that value.
>
> It could cause some write amplification: if you are streaming writes to
> a bunch of files at the same time and someone stats one of them, then
> they'd all end up getting an extra inode transaction. That doesn't sound
> _too_ bad on its face, but I probably need to implement it and then run
> some numbers to see.
>

Several journal transactions within a single jiffie tick?
If ctime/change_cookie of an inode is updated once within the scope
of a single running transaction, I don't think it matters how many
times it would be updated, but maybe I am missing something.

The problem is probably going to be that the seqlock of the coarse
grained clock is going to be invalidated way too frequently to be
"read mostly" in the presence of ls -lR workload, but again, I did
not study the implementation, so I may be way off.

Thanks,
Amir.

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Amir Goldstein]

On Sat, Sep 23, 2023 at 1:46 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Sat, 2023-09-23 at 10:15 +0300, Amir Goldstein wrote:
> > On Fri, Sep 22, 2023 at 8:15 PM Jeff Layton <jlayton@kernel.org> wrote:
> > >
> > > My initial goal was to implement multigrain timestamps on most major
> > > filesystems, so we could present them to userland, and use them for
> > > NFSv3, etc.
> > >
> > > With the current implementation however, we can't guarantee that a file
> > > with a coarse grained timestamp modified after one with a fine grained
> > > timestamp will always appear to have a later value. This could confuse
> > > some programs like make, rsync, find, etc. that depend on strict
> > > ordering requirements for timestamps.
> > >
> > > The goal of this version is more modest: fix XFS' change attribute.
> > > XFS's change attribute is bumped on atime updates in addition to other
> > > deliberate changes. This makes it unsuitable for export via nfsd.
> > >
> > > Jan Kara suggested keeping this functionality internal-only for now and
> > > plumbing the fine grained timestamps through getattr [1]. This set takes
> > > a slightly different approach and has XFS use the fine-grained attr to
> > > fake up STATX_CHANGE_COOKIE in its getattr routine itself.
> > >
> > > While we keep fine-grained timestamps in struct inode, when presenting
> > > the timestamps via getattr, we truncate them at a granularity of number
> > > of ns per jiffy,
> >
> > That's not good, because user explicitly set granular mtime would be
> > truncated too and booting with different kernels (HZ) would change
> > the observed timestamps of files.
> >
>
> Thinking about this some more, I think the first problem is easily
> addressable:
>
> The ctime isn't explicitly settable and with this set, we're already not
> truncating the atime. We haven't used any of the extra bits in the mtime
> yet, so we could just carve out a flag in there that says "this mtime
> was explicitly set and shouldn't be truncated before presentation".
>
> The second problem (booting to older kernel) is a bit tougher to deal
> with however. I'll have to think about that one a bit more.

There is a straightforward solution to both these problems -
opt in with a mount option to truncate user visible timestamps to 100ns
(and not jiffy) resolution as Linus is trying to promote.

Thanks,
Amir.

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Christian Brauner]

> My initial goal was to implement multigrain timestamps on most major
> filesystems, so we could present them to userland, and use them for
> NFSv3, etc.

If there's no clear users and workloads depending on this other than for
the sake of NFS then we shouldn't expose this to userspace. We've tried
this and I'm not convinced we're getting anything other than regressions
out of it. Keep it internal and confined to the filesystem that actually
needs this.

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Dave Chinner]

On Sat, Sep 23, 2023 at 05:52:36PM +0300, Amir Goldstein wrote:
> On Sat, Sep 23, 2023 at 1:46 PM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > On Sat, 2023-09-23 at 10:15 +0300, Amir Goldstein wrote:
> > > On Fri, Sep 22, 2023 at 8:15 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > >
> > > > My initial goal was to implement multigrain timestamps on most major
> > > > filesystems, so we could present them to userland, and use them for
> > > > NFSv3, etc.
> > > >
> > > > With the current implementation however, we can't guarantee that a file
> > > > with a coarse grained timestamp modified after one with a fine grained
> > > > timestamp will always appear to have a later value. This could confuse
> > > > some programs like make, rsync, find, etc. that depend on strict
> > > > ordering requirements for timestamps.
> > > >
> > > > The goal of this version is more modest: fix XFS' change attribute.
> > > > XFS's change attribute is bumped on atime updates in addition to other
> > > > deliberate changes. This makes it unsuitable for export via nfsd.
> > > >
> > > > Jan Kara suggested keeping this functionality internal-only for now and
> > > > plumbing the fine grained timestamps through getattr [1]. This set takes
> > > > a slightly different approach and has XFS use the fine-grained attr to
> > > > fake up STATX_CHANGE_COOKIE in its getattr routine itself.
> > > >
> > > > While we keep fine-grained timestamps in struct inode, when presenting
> > > > the timestamps via getattr, we truncate them at a granularity of number
> > > > of ns per jiffy,
> > >
> > > That's not good, because user explicitly set granular mtime would be
> > > truncated too and booting with different kernels (HZ) would change
> > > the observed timestamps of files.
> > >
> >
> > Thinking about this some more, I think the first problem is easily
> > addressable:
> >
> > The ctime isn't explicitly settable and with this set, we're already not
> > truncating the atime. We haven't used any of the extra bits in the mtime
> > yet, so we could just carve out a flag in there that says "this mtime
> > was explicitly set and shouldn't be truncated before presentation".
> >
> 
> I thought about this option too.
> But note that the "mtime was explicitly set" flag needs
> to be persisted to disk so you cannot store it in the high nsec bits.
> At least XFS won't store those bits if you use them - they have to
> be translated to an XFS inode flag and I don't know if changing
> XFS on-disk format was on your wish list.

Remember: this multi-grain timestamp thing was an idea to solve the
NFS change attribute problem without requiring *any* filesystem with
sub-jiffie timestamp capability to change their on-disk format to
implement a persistent change attribute that matches the new
requires of the kernel nfsd.

If we now need to change the on-disk format to support
some whacky new timestamp semantic to do this, then people have
completely lost sight of what problem the multi-grain timestamp idea
was supposed to address.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[NeilBrown]

On Sun, 24 Sep 2023, Christian Brauner wrote:
> > My initial goal was to implement multigrain timestamps on most major
> > filesystems, so we could present them to userland, and use them for
> > NFSv3, etc.
> 
> If there's no clear users and workloads depending on this other than for
> the sake of NFS then we shouldn't expose this to userspace. We've tried
> this and I'm not convinced we're getting anything other than regressions
> out of it. Keep it internal and confined to the filesystem that actually
> needs this.
> 

Some NFS servers run in userspace, and they would a "clear user" of this
functionality.

NeilBrown

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Jeff Layton]

On Sat, 2023-09-23 at 17:58 +0300, Amir Goldstein wrote:
> On Sat, Sep 23, 2023 at 1:22 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Sat, 2023-09-23 at 10:15 +0300, Amir Goldstein wrote:
> > > On Fri, Sep 22, 2023 at 8:15 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > 
> > > > My initial goal was to implement multigrain timestamps on most major
> > > > filesystems, so we could present them to userland, and use them for
> > > > NFSv3, etc.
> > > > 
> > > > With the current implementation however, we can't guarantee that a file
> > > > with a coarse grained timestamp modified after one with a fine grained
> > > > timestamp will always appear to have a later value. This could confuse
> > > > some programs like make, rsync, find, etc. that depend on strict
> > > > ordering requirements for timestamps.
> > > > 
> > > > The goal of this version is more modest: fix XFS' change attribute.
> > > > XFS's change attribute is bumped on atime updates in addition to other
> > > > deliberate changes. This makes it unsuitable for export via nfsd.
> > > > 
> > > > Jan Kara suggested keeping this functionality internal-only for now and
> > > > plumbing the fine grained timestamps through getattr [1]. This set takes
> > > > a slightly different approach and has XFS use the fine-grained attr to
> > > > fake up STATX_CHANGE_COOKIE in its getattr routine itself.
> > > > 
> > > > While we keep fine-grained timestamps in struct inode, when presenting
> > > > the timestamps via getattr, we truncate them at a granularity of number
> > > > of ns per jiffy,
> > > 
> > > That's not good, because user explicitly set granular mtime would be
> > > truncated too and booting with different kernels (HZ) would change
> > > the observed timestamps of files.
> > > 
> > 
> > That's a very good point.
> > 
> > > > which allows us to smooth over the fuzz that causes
> > > > ordering problems.
> > > > 
> > > 
> > > The reported ordering problems (i.e. cp -u) is not even limited to the
> > > scope of a single fs, right?
> > > 
> > 
> > It isn't. Most of the tools we're concerned with don't generally care
> > about filesystem boundaries.
> > 
> > > Thinking out loud - if the QERIED bit was not per inode timestamp
> > > but instead in a global fs_multigrain_ts variable, then all the inodes
> > > of all the mgtime fs would be using globally ordered timestamps
> > > 
> > > That should eliminate the reported issues with time reorder for
> > > fine vs coarse grained timestamps.
> > > 
> > > The risk of extra unneeded "change cookie" updates compared to
> > > per inode QUERIED bit may exist, but I think it is a rather small overhead
> > > and maybe worth the tradeoff of having to maintain a real per inode
> > > "change cookie" in addition to a "globally ordered mgtime"?
> > > 
> > > If this idea is acceptable, you may still be able to salvage the reverted
> > > ctime series for 6.7, because the change to use global mgtime should
> > > be quite trivial?
> > > 
> > 
> > This is basically the idea I was going to look at next once I got some
> > other stuff settled here: Basically, when we apply a fine-grained
> > timestamp to an inode, we'd advance the coarse-grained clock that
> > filesystems use to that value.
> > 
> > It could cause some write amplification: if you are streaming writes to
> > a bunch of files at the same time and someone stats one of them, then
> > they'd all end up getting an extra inode transaction. That doesn't sound
> > _too_ bad on its face, but I probably need to implement it and then run
> > some numbers to see.
> > 
> 
> Several journal transactions within a single jiffie tick?
> If ctime/change_cookie of an inode is updated once within the scope
> of a single running transaction, I don't think it matters how many
> times it would be updated, but maybe I am missing something.
> 
> The problem is probably going to be that the seqlock of the coarse
> grained clock is going to be invalidated way too frequently to be
> "read mostly" in the presence of ls -lR workload, but again, I did
> not study the implementation, so I may be way off.
> 

That may end up being the case, but I think if we can minimize the
number of fine-grained updates, then the number of invalidations will be
minimal too. I haven't rolled an implementation of this yet. This is all
very much still in the "waving of hands" stage anyway.

Once the dust settles from the atime and mtime API rework, I may still
take a stab at doing this.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Jeff Layton]

On Mon, 2023-09-25 at 08:18 +1000, Dave Chinner wrote:
> On Sat, Sep 23, 2023 at 05:52:36PM +0300, Amir Goldstein wrote:
> > On Sat, Sep 23, 2023 at 1:46 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > 
> > > On Sat, 2023-09-23 at 10:15 +0300, Amir Goldstein wrote:
> > > > On Fri, Sep 22, 2023 at 8:15 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > > 
> > > > > My initial goal was to implement multigrain timestamps on most major
> > > > > filesystems, so we could present them to userland, and use them for
> > > > > NFSv3, etc.
> > > > > 
> > > > > With the current implementation however, we can't guarantee that a file
> > > > > with a coarse grained timestamp modified after one with a fine grained
> > > > > timestamp will always appear to have a later value. This could confuse
> > > > > some programs like make, rsync, find, etc. that depend on strict
> > > > > ordering requirements for timestamps.
> > > > > 
> > > > > The goal of this version is more modest: fix XFS' change attribute.
> > > > > XFS's change attribute is bumped on atime updates in addition to other
> > > > > deliberate changes. This makes it unsuitable for export via nfsd.
> > > > > 
> > > > > Jan Kara suggested keeping this functionality internal-only for now and
> > > > > plumbing the fine grained timestamps through getattr [1]. This set takes
> > > > > a slightly different approach and has XFS use the fine-grained attr to
> > > > > fake up STATX_CHANGE_COOKIE in its getattr routine itself.
> > > > > 
> > > > > While we keep fine-grained timestamps in struct inode, when presenting
> > > > > the timestamps via getattr, we truncate them at a granularity of number
> > > > > of ns per jiffy,
> > > > 
> > > > That's not good, because user explicitly set granular mtime would be
> > > > truncated too and booting with different kernels (HZ) would change
> > > > the observed timestamps of files.
> > > > 
> > > 
> > > Thinking about this some more, I think the first problem is easily
> > > addressable:
> > > 
> > > The ctime isn't explicitly settable and with this set, we're already not
> > > truncating the atime. We haven't used any of the extra bits in the mtime
> > > yet, so we could just carve out a flag in there that says "this mtime
> > > was explicitly set and shouldn't be truncated before presentation".
> > > 
> > 
> > I thought about this option too.
> > But note that the "mtime was explicitly set" flag needs
> > to be persisted to disk so you cannot store it in the high nsec bits.
> > At least XFS won't store those bits if you use them - they have to
> > be translated to an XFS inode flag and I don't know if changing
> > XFS on-disk format was on your wish list.
> 
> Remember: this multi-grain timestamp thing was an idea to solve the
> NFS change attribute problem without requiring *any* filesystem with
> sub-jiffie timestamp capability to change their on-disk format to
> implement a persistent change attribute that matches the new
> requires of the kernel nfsd.
> 
> If we now need to change the on-disk format to support
> some whacky new timestamp semantic to do this, then people have
> completely lost sight of what problem the multi-grain timestamp idea
> was supposed to address.
> 

Yep. The main impetus for all of this was to fix XFS's change attribute
without requiring an on-disk format change. If we have to rev the on-
disk format, we're probably better off plumbing in a proper i_version
counter and tossing this idea aside.

That said, I think all we'd need for this scheme is a single flag per
inode (to indicate that the mtime shouldn't be truncated before
presentation). If that's possible to do without fully revving the inode
format, then we could still pursue this. I take it that's probably not
the case though.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Jeff Layton]

On Mon, 2023-09-25 at 08:44 +1000, NeilBrown wrote:
> On Sun, 24 Sep 2023, Christian Brauner wrote:
> > > My initial goal was to implement multigrain timestamps on most major
> > > filesystems, so we could present them to userland, and use them for
> > > NFSv3, etc.
> > 
> > If there's no clear users and workloads depending on this other than for
> > the sake of NFS then we shouldn't expose this to userspace. We've tried
> > this and I'm not convinced we're getting anything other than regressions
> > out of it. Keep it internal and confined to the filesystem that actually
> > needs this.
> > 
> 
> Some NFS servers run in userspace, and they would a "clear user" of this
> functionality.
> 

Indeed. Also, all of the programs that we're concerned about breaking
here (make, rsync, etc.) could benefit from proper fine-grained
timestamps:

Today, when they see two identical timestamps on files, these programs
have to assume the worst: rsync has to do the copy, make has to update
the target, etc. With a real distinguishable fine-grained timestamps,
these programs would likely be more efficient and some of these unneeded
operations would be avoided.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Dave Chinner]

On Mon, Sep 25, 2023 at 06:14:05AM -0400, Jeff Layton wrote:
> On Mon, 2023-09-25 at 08:18 +1000, Dave Chinner wrote:
> > On Sat, Sep 23, 2023 at 05:52:36PM +0300, Amir Goldstein wrote:
> > > On Sat, Sep 23, 2023 at 1:46 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > 
> > > > On Sat, 2023-09-23 at 10:15 +0300, Amir Goldstein wrote:
> > > > > On Fri, Sep 22, 2023 at 8:15 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > > > 
> > > > > > My initial goal was to implement multigrain timestamps on most major
> > > > > > filesystems, so we could present them to userland, and use them for
> > > > > > NFSv3, etc.
> > > > > > 
> > > > > > With the current implementation however, we can't guarantee that a file
> > > > > > with a coarse grained timestamp modified after one with a fine grained
> > > > > > timestamp will always appear to have a later value. This could confuse
> > > > > > some programs like make, rsync, find, etc. that depend on strict
> > > > > > ordering requirements for timestamps.
> > > > > > 
> > > > > > The goal of this version is more modest: fix XFS' change attribute.
> > > > > > XFS's change attribute is bumped on atime updates in addition to other
> > > > > > deliberate changes. This makes it unsuitable for export via nfsd.
> > > > > > 
> > > > > > Jan Kara suggested keeping this functionality internal-only for now and
> > > > > > plumbing the fine grained timestamps through getattr [1]. This set takes
> > > > > > a slightly different approach and has XFS use the fine-grained attr to
> > > > > > fake up STATX_CHANGE_COOKIE in its getattr routine itself.
> > > > > > 
> > > > > > While we keep fine-grained timestamps in struct inode, when presenting
> > > > > > the timestamps via getattr, we truncate them at a granularity of number
> > > > > > of ns per jiffy,
> > > > > 
> > > > > That's not good, because user explicitly set granular mtime would be
> > > > > truncated too and booting with different kernels (HZ) would change
> > > > > the observed timestamps of files.
> > > > > 
> > > > 
> > > > Thinking about this some more, I think the first problem is easily
> > > > addressable:
> > > > 
> > > > The ctime isn't explicitly settable and with this set, we're already not
> > > > truncating the atime. We haven't used any of the extra bits in the mtime
> > > > yet, so we could just carve out a flag in there that says "this mtime
> > > > was explicitly set and shouldn't be truncated before presentation".
> > > > 
> > > 
> > > I thought about this option too.
> > > But note that the "mtime was explicitly set" flag needs
> > > to be persisted to disk so you cannot store it in the high nsec bits.
> > > At least XFS won't store those bits if you use them - they have to
> > > be translated to an XFS inode flag and I don't know if changing
> > > XFS on-disk format was on your wish list.
> > 
> > Remember: this multi-grain timestamp thing was an idea to solve the
> > NFS change attribute problem without requiring *any* filesystem with
> > sub-jiffie timestamp capability to change their on-disk format to
> > implement a persistent change attribute that matches the new
> > requires of the kernel nfsd.
> > 
> > If we now need to change the on-disk format to support
> > some whacky new timestamp semantic to do this, then people have
> > completely lost sight of what problem the multi-grain timestamp idea
> > was supposed to address.
> > 
> 
> Yep. The main impetus for all of this was to fix XFS's change attribute
> without requiring an on-disk format change. If we have to rev the on-
> disk format, we're probably better off plumbing in a proper i_version
> counter and tossing this idea aside.
> 
> That said, I think all we'd need for this scheme is a single flag per
> inode (to indicate that the mtime shouldn't be truncated before
> presentation). If that's possible to do without fully revving the inode
> format, then we could still pursue this. I take it that's probably not
> the case though.

Older kernels that don't know what the flag means, but that should
be OK for an inode flag. The bigger issue is that none of the
userspace tools (xfs_db, xfs_repair, etc) know about it, so they
would have to be taught about it. And then there's testing it, which
likely means userspace needs visibility of the flag (e.g. FS_XFLAG
for it) and then there's more work....

It's really not worth it.

I think that Linus's suggestion of the in-memory inode timestamp
always being a 64bit, 100ns granularity value instead of a timespec
that gets truncated at sample time has merit as a general solution.

We also must not lose sight of the fact that the lazytime mount
option makes atime updates on XFS behave exactly as the nfsd/NFS
client application wants. That is, XFS will do in-memory atime
updates unless the atime update also sets S_VERSION to explicitly
bump the i_version counter if required. That leads to another
potential nfsd specific solution without requiring filesystems to
change on disk formats: the nfsd explicitly asks operations for lazy
atime updates...

And we must also keep in sight the fact that io_uring wants
non-blocking timestamp updates to be possible (for all types of
updates). Hence it looks to me like we have more than one use case
for per-operation/application specific timestamp update semantics.
Perhaps there's a generic solution to this problem (e.g.  operation
specific non-blocking, in-memory pure timestamp updates) that does
what everyone needs...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Jeff Layton]

On Tue, 2023-09-26 at 08:32 +1000, Dave Chinner wrote:
> On Mon, Sep 25, 2023 at 06:14:05AM -0400, Jeff Layton wrote:
> > On Mon, 2023-09-25 at 08:18 +1000, Dave Chinner wrote:
> > > On Sat, Sep 23, 2023 at 05:52:36PM +0300, Amir Goldstein wrote:
> > > > On Sat, Sep 23, 2023 at 1:46 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > > 
> > > > > On Sat, 2023-09-23 at 10:15 +0300, Amir Goldstein wrote:
> > > > > > On Fri, Sep 22, 2023 at 8:15 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > > > > 
> > > > > > > My initial goal was to implement multigrain timestamps on most major
> > > > > > > filesystems, so we could present them to userland, and use them for
> > > > > > > NFSv3, etc.
> > > > > > > 
> > > > > > > With the current implementation however, we can't guarantee that a file
> > > > > > > with a coarse grained timestamp modified after one with a fine grained
> > > > > > > timestamp will always appear to have a later value. This could confuse
> > > > > > > some programs like make, rsync, find, etc. that depend on strict
> > > > > > > ordering requirements for timestamps.
> > > > > > > 
> > > > > > > The goal of this version is more modest: fix XFS' change attribute.
> > > > > > > XFS's change attribute is bumped on atime updates in addition to other
> > > > > > > deliberate changes. This makes it unsuitable for export via nfsd.
> > > > > > > 
> > > > > > > Jan Kara suggested keeping this functionality internal-only for now and
> > > > > > > plumbing the fine grained timestamps through getattr [1]. This set takes
> > > > > > > a slightly different approach and has XFS use the fine-grained attr to
> > > > > > > fake up STATX_CHANGE_COOKIE in its getattr routine itself.
> > > > > > > 
> > > > > > > While we keep fine-grained timestamps in struct inode, when presenting
> > > > > > > the timestamps via getattr, we truncate them at a granularity of number
> > > > > > > of ns per jiffy,
> > > > > > 
> > > > > > That's not good, because user explicitly set granular mtime would be
> > > > > > truncated too and booting with different kernels (HZ) would change
> > > > > > the observed timestamps of files.
> > > > > > 
> > > > > 
> > > > > Thinking about this some more, I think the first problem is easily
> > > > > addressable:
> > > > > 
> > > > > The ctime isn't explicitly settable and with this set, we're already not
> > > > > truncating the atime. We haven't used any of the extra bits in the mtime
> > > > > yet, so we could just carve out a flag in there that says "this mtime
> > > > > was explicitly set and shouldn't be truncated before presentation".
> > > > > 
> > > > 
> > > > I thought about this option too.
> > > > But note that the "mtime was explicitly set" flag needs
> > > > to be persisted to disk so you cannot store it in the high nsec bits.
> > > > At least XFS won't store those bits if you use them - they have to
> > > > be translated to an XFS inode flag and I don't know if changing
> > > > XFS on-disk format was on your wish list.
> > > 
> > > Remember: this multi-grain timestamp thing was an idea to solve the
> > > NFS change attribute problem without requiring *any* filesystem with
> > > sub-jiffie timestamp capability to change their on-disk format to
> > > implement a persistent change attribute that matches the new
> > > requires of the kernel nfsd.
> > > 
> > > If we now need to change the on-disk format to support
> > > some whacky new timestamp semantic to do this, then people have
> > > completely lost sight of what problem the multi-grain timestamp idea
> > > was supposed to address.
> > > 
> > 
> > Yep. The main impetus for all of this was to fix XFS's change attribute
> > without requiring an on-disk format change. If we have to rev the on-
> > disk format, we're probably better off plumbing in a proper i_version
> > counter and tossing this idea aside.
> > 
> > That said, I think all we'd need for this scheme is a single flag per
> > inode (to indicate that the mtime shouldn't be truncated before
> > presentation). If that's possible to do without fully revving the inode
> > format, then we could still pursue this. I take it that's probably not
> > the case though.
> 
> Older kernels that don't know what the flag means, but that should
> be OK for an inode flag. The bigger issue is that none of the
> userspace tools (xfs_db, xfs_repair, etc) know about it, so they
> would have to be taught about it. And then there's testing it, which
> likely means userspace needs visibility of the flag (e.g. FS_XFLAG
> for it) and then there's more work....
> 
> It's really not worth it.
> 
>
> I think that Linus's suggestion of the in-memory inode timestamp
> always being a 64bit, 100ns granularity value instead of a timespec
> that gets truncated at sample time has merit as a general solution.
> 

Changing how we store timestamps in struct inode is a good idea, and
reducing the effective granularity to 100ns seems reasonable, but that
alone won't fix XFS's i_version counter, or the ordering problems that
we hit with the multigrain series that had to be reverted.

> We also must not lose sight of the fact that the lazytime mount
> option makes atime updates on XFS behave exactly as the nfsd/NFS
> client application wants. That is, XFS will do in-memory atime
> updates unless the atime update also sets S_VERSION to explicitly
> bump the i_version counter if required. That leads to another
> potential nfsd specific solution without requiring filesystems to
> change on disk formats: the nfsd explicitly asks operations for lazy
> atime updates...
> 

Not exactly. The problem with XFS's i_version is that it also bumps it
on atime updates. lazytime reduces the number of atime updates to
~1/day. To be exactly what nfsd wants, you'd need to make that 0. I
suppose you can work around it with noatime, but that has problems of
its own.

> And we must also keep in sight the fact that io_uring wants
> non-blocking timestamp updates to be possible (for all types of
> updates). Hence it looks to me like we have more than one use case
> for per-operation/application specific timestamp update semantics.
> Perhaps there's a generic solution to this problem (e.g.  operation
> specific non-blocking, in-memory pure timestamp updates) that does
> what everyone needs...

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Christian Brauner]

> > Some NFS servers run in userspace, and they would a "clear user" of this
> > functionality.
> > 
> 
> Indeed. Also, all of the programs that we're concerned about breaking
> here (make, rsync, etc.) could benefit from proper fine-grained
> timestamps:
> 
> Today, when they see two identical timestamps on files, these programs
> have to assume the worst: rsync has to do the copy, make has to update
> the target, etc. With a real distinguishable fine-grained timestamps,
> these programs would likely be more efficient and some of these unneeded
> operations would be avoided.

The whole sales pitch falls flat if we end up with wrong ordering of
timestamps which caused us to revert this. So unless this is fixed
we shouldn't expose this to userspace again.

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Christian Brauner]

> > If there's no clear users and workloads depending on this other than for
> > the sake of NFS then we shouldn't expose this to userspace. We've tried

> 
> Some NFS servers run in userspace, and they would a "clear user" of this
> functionality.

See my comment above. We did thist mostly for the sake of NFS as there
was in itself nothing wrong with timestamps that needed urgent fixing.

The end result has been that we caused a regression for four other major
filesystems when they were switched to fine-grained timestamps.

So NFS servers in userspace isn't a sufficient argument to just try
again with a slightly tweaked solution but without a wholesale fix of
the actual ordering problem. The bar to merge this will naturally be
higher the second time around.

That's orthogonal to improving the general timestamp infrastructure in
struct inode ofc.

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Jeff Layton]

On Tue, 2023-09-26 at 14:18 +0200, Christian Brauner wrote:
> > > If there's no clear users and workloads depending on this other than for
> > > the sake of NFS then we shouldn't expose this to userspace. We've tried
> 
> > 
> > Some NFS servers run in userspace, and they would a "clear user" of this
> > functionality.
> 
> See my comment above. We did thist mostly for the sake of NFS as there
> was in itself nothing wrong with timestamps that needed urgent fixing.
> 
> The end result has been that we caused a regression for four other major
> filesystems when they were switched to fine-grained timestamps.
> 
> So NFS servers in userspace isn't a sufficient argument to just try
> again with a slightly tweaked solution but without a wholesale fix of
> the actual ordering problem. The bar to merge this will naturally be
> higher the second time around.
> 
> That's orthogonal to improving the general timestamp infrastructure in
> struct inode ofc.

There are multiple proposals in flight here, and they all sort of
dovetail on one another. I'm not proposing we expose any changes to the
timestamps to users in any way, unless we can fix the ordering issues,
and ensure that we can preserve existing behavior re: utimensat().

I think it's possible to do that, but I'm going to table that work for
the moment, and finish up the atime/mtime accessor conversions first.
That makes experimenting with all of this a lot simpler. I think I can
also put together a nicer implementation of multigrain timestamps too,
if we can first convert the current timespec64's in struct inode into a
single 64-bit word.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Christian Brauner]

On Tue, Sep 26, 2023 at 08:51:32AM -0400, Jeff Layton wrote:
> On Tue, 2023-09-26 at 14:18 +0200, Christian Brauner wrote:
> > > > If there's no clear users and workloads depending on this other than for
> > > > the sake of NFS then we shouldn't expose this to userspace. We've tried
> > 
> > > 
> > > Some NFS servers run in userspace, and they would a "clear user" of this
> > > functionality.
> > 
> > See my comment above. We did thist mostly for the sake of NFS as there
> > was in itself nothing wrong with timestamps that needed urgent fixing.
> > 
> > The end result has been that we caused a regression for four other major
> > filesystems when they were switched to fine-grained timestamps.
> > 
> > So NFS servers in userspace isn't a sufficient argument to just try
> > again with a slightly tweaked solution but without a wholesale fix of
> > the actual ordering problem. The bar to merge this will naturally be
> > higher the second time around.
> > 
> > That's orthogonal to improving the general timestamp infrastructure in
> > struct inode ofc.
> 
> There are multiple proposals in flight here, and they all sort of
> dovetail on one another. I'm not proposing we expose any changes to the
> timestamps to users in any way, unless we can fix the ordering issues,
> and ensure that we can preserve existing behavior re: utimensat().

Yeah, I know you're aware of that and I'm mostly clarifying the
conditions for this work for the ones not that closely involved.

> I think it's possible to do that, but I'm going to table that work for
> the moment, and finish up the atime/mtime accessor conversions first.

That sounds great.

> That makes experimenting with all of this a lot simpler. I think I can

Oh that's good then.

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Dave Chinner]

On Tue, Sep 26, 2023 at 07:31:55AM -0400, Jeff Layton wrote:
> On Tue, 2023-09-26 at 08:32 +1000, Dave Chinner wrote:
> > We also must not lose sight of the fact that the lazytime mount
> > option makes atime updates on XFS behave exactly as the nfsd/NFS
> > client application wants. That is, XFS will do in-memory atime
> > updates unless the atime update also sets S_VERSION to explicitly
> > bump the i_version counter if required. That leads to another
> > potential nfsd specific solution without requiring filesystems to
> > change on disk formats: the nfsd explicitly asks operations for lazy
> > atime updates...
> > 
> 
> Not exactly. The problem with XFS's i_version is that it also bumps it
> on atime updates. lazytime reduces the number of atime updates to
> ~1/day. To be exactly what nfsd wants, you'd need to make that 0.

As long as there are future modifications going to those files,
lazytime completely elides the visibility of atime updates as they
get silently aggregated into future modifications and so there are
0 i_version changes as a resutl of pure atime updates in those cases.

If there are no future modifications, then just like relatime, there
is a timestamp update every 24hrs. That's no big deal, nobody is
complaining about this being a problem.

It's the "persistent atime update after modification" heuristic
implemented by relatime that is causing all the problems here. If
that behaviour is elided on the server side, then most of the client
side invalidation problems with these workloads go away.

IOWs, nfsd needs direct control over how atime updates should be
treated by the VFS/filesystem (i.e. as pure in-memory updates)
rather than leaving it to some heuristic that may do the exact
opposite of what the nfsd application needs.

That's the point I was making: we have emerging requirements for
per-operation timestamp update behaviour control with io_uring and
other non-blocking applications. The nfsd application also has
specific semantics it wants the VFS/filesystem to implement
(non-persistent atime unless something else changes)....

My point is that we've now failed a couple of times now to implement
what NFSD requires via trying to change VFS and/or filesystem
infrastructure to provide i_version or ctime semantics the nfsd
requires. That's a fairly good sign that we might not be approaching
this problem from the right direction, and so doubling down and
considering changing the timestamp infrastructure from the ground up
just to solve a relatively niche, filesystem specific issue doesn't
seem like the best approach.

OTOH, having the application actually tell the timestamp updates
exactly what semantics it needs (non blocking, persistent vs in
memory, etc) will allow the VFS and filesystems can do the right
thing for the application without having to worry about general
heuristics that sometimes do exactly the wrong thing....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v8 0/5] fs: multigrain timestamps for XFS's change_cookie

[Jeff Layton]

On Wed, 2023-09-27 at 09:33 +1000, Dave Chinner wrote:
> On Tue, Sep 26, 2023 at 07:31:55AM -0400, Jeff Layton wrote:
> > On Tue, 2023-09-26 at 08:32 +1000, Dave Chinner wrote:
> > > We also must not lose sight of the fact that the lazytime mount
> > > option makes atime updates on XFS behave exactly as the nfsd/NFS
> > > client application wants. That is, XFS will do in-memory atime
> > > updates unless the atime update also sets S_VERSION to explicitly
> > > bump the i_version counter if required. That leads to another
> > > potential nfsd specific solution without requiring filesystems to
> > > change on disk formats: the nfsd explicitly asks operations for lazy
> > > atime updates...
> > > 
> > 
> > Not exactly. The problem with XFS's i_version is that it also bumps it
> > on atime updates. lazytime reduces the number of atime updates to
> > ~1/day. To be exactly what nfsd wants, you'd need to make that 0.
> 
> As long as there are future modifications going to those files,
> lazytime completely elides the visibility of atime updates as they
> get silently aggregated into future modifications and so there are
> 0 i_version changes as a resutl of pure atime updates in those cases.
> 
> If there are no future modifications, then just like relatime, there
> is a timestamp update every 24hrs. That's no big deal, nobody is
> complaining about this being a problem.
> 

Right. The main issue here is that (with relatime) we'll still end up
with a cache invalidation once every 24 hours for any r/o files that
have been accessed. It's not a _huge_ problem on most workloads; it's
just not ideal.

> It's the "persistent atime update after modification" heuristic
> implemented by relatime that is causing all the problems here. If
> that behaviour is elided on the server side, then most of the client
> side invalidation problems with these workloads go away.
> 
> IOWs, nfsd needs direct control over how atime updates should be
> treated by the VFS/filesystem (i.e. as pure in-memory updates)
> rather than leaving it to some heuristic that may do the exact
> opposite of what the nfsd application needs.
>
> That's the point I was making: we have emerging requirements for
> per-operation timestamp update behaviour control with io_uring and
> other non-blocking applications. The nfsd application also has
> specific semantics it wants the VFS/filesystem to implement
> (non-persistent atime unless something else changes)....
> 
> My point is that we've now failed a couple of times now to implement
> what NFSD requires via trying to change VFS and/or filesystem
> infrastructure to provide i_version or ctime semantics the nfsd
> requires. That's a fairly good sign that we might not be approaching
> this problem from the right direction, and so doubling down and
> considering changing the timestamp infrastructure from the ground up
> just to solve a relatively niche, filesystem specific issue doesn't
> seem like the best approach.
> 
> OTOH, having the application actually tell the timestamp updates
> exactly what semantics it needs (non blocking, persistent vs in
> memory, etc) will allow the VFS and filesystems can do the right
> thing for the application without having to worry about general
> heuristics that sometimes do exactly the wrong thing....
> 

I'm a little unclear on exactly what you're proposing here, but I think
that's overstating what's needed. nfsd's needs are pretty simple: it
wants a change attribute that changes any time the ctime would change.

btrfs, ext4 and tmpfs have this. xfs does not because its change
attribute changes when the atime changes as well. With the right mount
options, that problem can be mitigated to some degree, but it's still
not ideal.

We have a couple of options: try to make the ctime behave the way we
need, or just implement a proper change attribute in xfs (which involves
revving the on-disk format).
-- 
Jeff Layton <jlayton@kernel.org>