fs: implement multigrain timestamps

generic_fillattr just fills in the entire stat struct indiscriminately today, copying data from the inode. There is at least one attribute (STATX_CHANGE_COOKIE) that can have side effects when it is reported, and we're looking at adding more with the addition of multigrain timestamps.

Add a request_mask argument to generic_fillattr and have most callers just pass in the value that is passed to getattr. Have other callers (e.g. ksmbd) just pass in STATX_BASIC_STATS. Also move the setting of STATX_CHANGE_COOKIE into generic_fillattr.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/9p/vfs_inode.c      |  4 ++--
 fs/9p/vfs_inode_dotl.c |  4 ++--
 fs/afs/inode.c         |  2 +-
 fs/btrfs/inode.c       |  2 +-
 fs/ceph/inode.c        |  2 +-
 fs/cifs/inode.c        |  2 +-
 fs/coda/inode.c        |  3 ++-
 fs/ecryptfs/inode.c    |  5 +++--
 fs/erofs/inode.c       |  2 +-
 fs/exfat/file.c        |  2 +-
 fs/ext2/inode.c        |  2 +-
 fs/ext4/inode.c        |  2 +-
 fs/f2fs/file.c         |  2 +-
 fs/fat/file.c          |  2 +-
 fs/fuse/dir.c          |  2 +-
 fs/gfs2/inode.c        |  2 +-
 fs/hfsplus/inode.c     |  2 +-
 fs/kernfs/inode.c      |  2 +-
 fs/ksmbd/smb2pdu.c     | 22 +++++++++++-----------
 fs/ksmbd/vfs.c         |  3 ++-
 fs/libfs.c             |  4 ++--
 fs/minix/inode.c       |  2 +-
 fs/nfs/inode.c         |  2 +-
 fs/nfs/namespace.c     |  3 ++-
 fs/ntfs3/file.c        |  2 +-
 fs/ocfs2/file.c        |  2 +-
 fs/orangefs/inode.c    |  2 +-
 fs/proc/base.c         |  4 ++--
 fs/proc/fd.c           |  2 +-
 fs/proc/generic.c      |  2 +-
 fs/proc/proc_net.c     |  2 +-
 fs/proc/proc_sysctl.c  |  2 +-
 fs/proc/root.c         |  3 ++-
 fs/stat.c              | 18 ++++++++++--------
 fs/sysv/itree.c        |  3 ++-
 fs/ubifs/dir.c         |  2 +-
 fs/udf/symlink.c       |  2 +-
 fs/vboxsf/utils.c      |  2 +-
 include/linux/fs.h     |  2 +-
 mm/shmem.c             |  2 +-
 40 files changed, 70 insertions(+), 62 deletions(-)

fs/9p/vfs_inode.c +2 -2

index 36b466e35887..11bd069d038b 100644
@@ -1016,7 +1016,7 @@ v9fs_vfs_getattr(struct mnt_idmap *idmap, const struct path *path,
 	p9_debug(P9_DEBUG_VFS, "dentry: %p\n", dentry);
 	v9ses = v9fs_dentry2v9ses(dentry);
 	if (v9ses->cache & (CACHE_META|CACHE_LOOSE)) {
-		generic_fillattr(&nop_mnt_idmap, inode, stat);
+		generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 		return 0;
 	} else if (v9ses->cache & CACHE_WRITEBACK) {
 		if (S_ISREG(inode->i_mode)) {

@@ -1037,7 +1037,7 @@ v9fs_vfs_getattr(struct mnt_idmap *idmap, const struct path *path,
 		return PTR_ERR(st);
 
 	v9fs_stat2inode(st, d_inode(dentry), dentry->d_sb, 0);
-	generic_fillattr(&nop_mnt_idmap, d_inode(dentry), stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, d_inode(dentry), stat);
 
 	p9stat_free(st);
 	kfree(st);

fs/9p/vfs_inode_dotl.c +2 -2

index 5361cd2d7996..04083fbf0c91 100644
@@ -451,7 +451,7 @@ v9fs_vfs_getattr_dotl(struct mnt_idmap *idmap,
 	p9_debug(P9_DEBUG_VFS, "dentry: %p\n", dentry);
 	v9ses = v9fs_dentry2v9ses(dentry);
 	if (v9ses->cache & (CACHE_META|CACHE_LOOSE)) {
-		generic_fillattr(&nop_mnt_idmap, inode, stat);
+		generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 		return 0;
 	} else if (v9ses->cache) {
 		if (S_ISREG(inode->i_mode)) {

@@ -476,7 +476,7 @@ v9fs_vfs_getattr_dotl(struct mnt_idmap *idmap,
 		return PTR_ERR(st);
 
 	v9fs_stat2inode_dotl(st, d_inode(dentry), 0);
-	generic_fillattr(&nop_mnt_idmap, d_inode(dentry), stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, d_inode(dentry), stat);
 	/* Change block size to what the server returned */
 	stat->blksize = st->st_blksize;
 

fs/afs/inode.c +1 -1

index 866bab860a88..54a4a1dd3bba 100644
@@ -773,7 +773,7 @@ int afs_getattr(struct mnt_idmap *idmap, const struct path *path,
 
 	do {
 		read_seqbegin_or_lock(&vnode->cb_lock, &seq);
-		generic_fillattr(&nop_mnt_idmap, inode, stat);
+		generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 		if (test_bit(AFS_VNODE_SILLY_DELETED, &vnode->flags) &&
 		    stat->nlink > 0)
 			stat->nlink -= 1;

fs/btrfs/inode.c +1 -1

index 19c707bc8801..2335b5e1cecc 100644
@@ -8625,7 +8625,7 @@ static int btrfs_getattr(struct mnt_idmap *idmap,
 				  STATX_ATTR_IMMUTABLE |
 				  STATX_ATTR_NODUMP);
 
-	generic_fillattr(idmap, inode, stat);
+	generic_fillattr(idmap, request_mask, inode, stat);
 	stat->dev = BTRFS_I(inode)->root->anon_dev;
 
 	spin_lock(&BTRFS_I(inode)->lock);

fs/ceph/inode.c +1 -1

index 8e5f41d45283..2e479dca6845 100644
@@ -2465,7 +2465,7 @@ int ceph_getattr(struct mnt_idmap *idmap, const struct path *path,
 			return err;
 	}
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 	stat->ino = ceph_present_inode(inode);
 
 	/*

fs/cifs/inode.c +1 -1

index 1087ac6104a9..1ba09e39a1de 100644
@@ -2540,7 +2540,7 @@ int cifs_getattr(struct mnt_idmap *idmap, const struct path *path,
 			return rc;
 	}
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 	stat->blksize = cifs_sb->ctx->bsize;
 	stat->ino = CIFS_I(inode)->uniqueid;
 

fs/coda/inode.c +2 -1

index d661e6cf17ac..f0be47be3587 100644
@@ -256,7 +256,8 @@ int coda_getattr(struct mnt_idmap *idmap, const struct path *path,
 {
 	int err = coda_revalidate_inode(d_inode(path->dentry));
 	if (!err)
-		generic_fillattr(&nop_mnt_idmap, d_inode(path->dentry), stat);
+		generic_fillattr(&nop_mnt_idmap, request_mask,
+				 d_inode(path->dentry), stat);
 	return err;
 }
 

fs/ecryptfs/inode.c +3 -2

index 83274915ba6d..e4c1b62a2de2 100644
@@ -982,7 +982,7 @@ static int ecryptfs_getattr_link(struct mnt_idmap *idmap,
 
 	mount_crypt_stat = &ecryptfs_superblock_to_private(
 						dentry->d_sb)->mount_crypt_stat;
-	generic_fillattr(&nop_mnt_idmap, d_inode(dentry), stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, d_inode(dentry), stat);
 	if (mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES) {
 		char *target;
 		size_t targetsiz;

@@ -1011,7 +1011,8 @@ static int ecryptfs_getattr(struct mnt_idmap *idmap,
 	if (!rc) {
 		fsstack_copy_attr_all(d_inode(dentry),
 				      ecryptfs_inode_to_lower(d_inode(dentry)));
-		generic_fillattr(&nop_mnt_idmap, d_inode(dentry), stat);
+		generic_fillattr(&nop_mnt_idmap, request_mask,
+				 d_inode(dentry), stat);
 		stat->blocks = lower_stat.blocks;
 	}
 	return rc;

fs/erofs/inode.c +1 -1

index d70b12b81507..b8aeb1251997 100644
@@ -372,7 +372,7 @@ int erofs_getattr(struct mnt_idmap *idmap, const struct path *path,
 	stat->attributes_mask |= (STATX_ATTR_COMPRESSED |
 				  STATX_ATTR_IMMUTABLE);
 
-	generic_fillattr(idmap, inode, stat);
+	generic_fillattr(idmap, request_mask, inode, stat);
 	return 0;
 }
 

fs/exfat/file.c +1 -1

index e99183a74611..29b6229fddad 100644
@@ -232,7 +232,7 @@ int exfat_getattr(struct mnt_idmap *idmap, const struct path *path,
 	struct inode *inode = d_backing_inode(path->dentry);
 	struct exfat_inode_info *ei = EXFAT_I(inode);
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 	exfat_truncate_atime(&stat->atime);
 	stat->result_mask |= STATX_BTIME;
 	stat->btime.tv_sec = ei->i_crtime.tv_sec;

fs/ext2/inode.c +1 -1

index 26f135e7ffce..eb4d32fcbf17 100644
@@ -1614,7 +1614,7 @@ int ext2_getattr(struct mnt_idmap *idmap, const struct path *path,
 			STATX_ATTR_IMMUTABLE |
 			STATX_ATTR_NODUMP);
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 	return 0;
 }
 

fs/ext4/inode.c +1 -1

index ce5f21b6c2b3..e0bbcf7a07b5 100644
@@ -5539,7 +5539,7 @@ int ext4_getattr(struct mnt_idmap *idmap, const struct path *path,
 				  STATX_ATTR_NODUMP |
 				  STATX_ATTR_VERITY);
 
-	generic_fillattr(idmap, inode, stat);
+	generic_fillattr(idmap, request_mask, inode, stat);
 	return 0;
 }
 

fs/f2fs/file.c +1 -1

index 5ac53d2627d2..bb16319a9491 100644
@@ -888,7 +888,7 @@ int f2fs_getattr(struct mnt_idmap *idmap, const struct path *path,
 				  STATX_ATTR_NODUMP |
 				  STATX_ATTR_VERITY);
 
-	generic_fillattr(idmap, inode, stat);
+	generic_fillattr(idmap, request_mask, inode, stat);
 
 	/* we need to show initial sectors used for inline_data/dentries */
 	if ((S_ISREG(inode->i_mode) && f2fs_has_inline_data(inode)) ||

fs/fat/file.c +1 -1

index 795a4fad5c40..650f77422057 100644
@@ -401,7 +401,7 @@ int fat_getattr(struct mnt_idmap *idmap, const struct path *path,
 	struct inode *inode = d_inode(path->dentry);
 	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
 
-	generic_fillattr(idmap, inode, stat);
+	generic_fillattr(idmap, request_mask, inode, stat);
 	stat->blksize = sbi->cluster_size;
 
 	if (sbi->options.nfs == FAT_NFS_NOSTALE_RO) {

fs/fuse/dir.c +1 -1

index 35bc174f9ba2..a445d72bd98f 100644
@@ -1224,7 +1224,7 @@ static int fuse_update_get_attr(struct inode *inode, struct file *file,
 		forget_all_cached_acls(inode);
 		err = fuse_do_getattr(inode, stat, file);
 	} else if (stat) {
-		generic_fillattr(&nop_mnt_idmap, inode, stat);
+		generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 		stat->mode = fi->orig_i_mode;
 		stat->ino = fi->orig_ino;
 	}

fs/gfs2/inode.c +1 -1

index 17c994a0c0d0..5b3c62d20db5 100644
@@ -2071,7 +2071,7 @@ static int gfs2_getattr(struct mnt_idmap *idmap,
 				  STATX_ATTR_IMMUTABLE |
 				  STATX_ATTR_NODUMP);
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 
 	if (gfs2_holder_initialized(&gh))
 		gfs2_glock_dq_uninit(&gh);

fs/hfsplus/inode.c +1 -1

index b21660475ac1..6aefdbd3e7b6 100644
@@ -298,7 +298,7 @@ int hfsplus_getattr(struct mnt_idmap *idmap, const struct path *path,
 	stat->attributes_mask |= STATX_ATTR_APPEND | STATX_ATTR_IMMUTABLE |
 				 STATX_ATTR_NODUMP;
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 	return 0;
 }
 

fs/kernfs/inode.c +1 -1

index b22b74d1a115..b01cc38ed843 100644
@@ -191,7 +191,7 @@ int kernfs_iop_getattr(struct mnt_idmap *idmap,
 
 	down_read(&root->kernfs_iattr_rwsem);
 	kernfs_refresh_inode(kn, inode);
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 	up_read(&root->kernfs_iattr_rwsem);
 
 	return 0;

fs/ksmbd/smb2pdu.c +11 -11

index cb93fd231f4e..d39ddb344417 100644
@@ -4342,8 +4342,8 @@ static int get_file_basic_info(struct smb2_query_info_rsp *rsp,
 	}
 
 	basic_info = (struct smb2_file_basic_info *)rsp->Buffer;
-	generic_fillattr(file_mnt_idmap(fp->filp), file_inode(fp->filp),
-			 &stat);
+	generic_fillattr(file_mnt_idmap(fp->filp), STATX_BASIC_STATS,
+			 file_inode(fp->filp), &stat);
 	basic_info->CreationTime = cpu_to_le64(fp->create_time);
 	time = ksmbd_UnixTimeToNT(stat.atime);
 	basic_info->LastAccessTime = cpu_to_le64(time);

@@ -4383,7 +4383,7 @@ static void get_file_standard_info(struct smb2_query_info_rsp *rsp,
 	struct kstat stat;
 
 	inode = file_inode(fp->filp);
-	generic_fillattr(file_mnt_idmap(fp->filp), inode, &stat);
+	generic_fillattr(file_mnt_idmap(fp->filp), STATX_BASIC_STATS, inode, &stat);
 
 	sinfo = (struct smb2_file_standard_info *)rsp->Buffer;
 	delete_pending = ksmbd_inode_pending_delete(fp);

@@ -4437,7 +4437,7 @@ static int get_file_all_info(struct ksmbd_work *work,
 		return PTR_ERR(filename);
 
 	inode = file_inode(fp->filp);
-	generic_fillattr(file_mnt_idmap(fp->filp), inode, &stat);
+	generic_fillattr(file_mnt_idmap(fp->filp), STATX_BASIC_STATS, inode, &stat);
 
 	ksmbd_debug(SMB, "filename = %s\n", filename);
 	delete_pending = ksmbd_inode_pending_delete(fp);

@@ -4514,8 +4514,8 @@ static void get_file_stream_info(struct ksmbd_work *work,
 	int buf_free_len;
 	struct smb2_query_info_req *req = ksmbd_req_buf_next(work);
 
-	generic_fillattr(file_mnt_idmap(fp->filp), file_inode(fp->filp),
-			 &stat);
+	generic_fillattr(file_mnt_idmap(fp->filp), STATX_BASIC_STATS,
+			 file_inode(fp->filp), &stat);
 	file_info = (struct smb2_file_stream_info *)rsp->Buffer;
 
 	buf_free_len =

@@ -4605,8 +4605,8 @@ static void get_file_internal_info(struct smb2_query_info_rsp *rsp,
 	struct smb2_file_internal_info *file_info;
 	struct kstat stat;
 
-	generic_fillattr(file_mnt_idmap(fp->filp), file_inode(fp->filp),
-			 &stat);
+	generic_fillattr(file_mnt_idmap(fp->filp), STATX_BASIC_STATS,
+			 file_inode(fp->filp), &stat);
 	file_info = (struct smb2_file_internal_info *)rsp->Buffer;
 	file_info->IndexNumber = cpu_to_le64(stat.ino);
 	rsp->OutputBufferLength =

@@ -4631,7 +4631,7 @@ static int get_file_network_open_info(struct smb2_query_info_rsp *rsp,
 	file_info = (struct smb2_file_ntwrk_info *)rsp->Buffer;
 
 	inode = file_inode(fp->filp);
-	generic_fillattr(file_mnt_idmap(fp->filp), inode, &stat);
+	generic_fillattr(file_mnt_idmap(fp->filp), STATX_BASIC_STATS, inode, &stat);
 
 	file_info->CreationTime = cpu_to_le64(fp->create_time);
 	time = ksmbd_UnixTimeToNT(stat.atime);

@@ -4692,8 +4692,8 @@ static void get_file_compression_info(struct smb2_query_info_rsp *rsp,
 	struct smb2_file_comp_info *file_info;
 	struct kstat stat;
 
-	generic_fillattr(file_mnt_idmap(fp->filp), file_inode(fp->filp),
-			 &stat);
+	generic_fillattr(file_mnt_idmap(fp->filp), STATX_BASIC_STATS,
+			 file_inode(fp->filp), &stat);
 
 	file_info = (struct smb2_file_comp_info *)rsp->Buffer;
 	file_info->CompressedFileSize = cpu_to_le64(stat.blocks << 9);

fs/ksmbd/vfs.c +2 -1

index 778c152708e4..47a4fe1e5043 100644
@@ -1597,7 +1597,8 @@ int ksmbd_vfs_fill_dentry_attrs(struct ksmbd_work *work,
 	u64 time;
 	int rc;
 
-	generic_fillattr(idmap, d_inode(dentry), ksmbd_kstat->kstat);
+	generic_fillattr(idmap, STATX_BASIC_STATS, d_inode(dentry),
+			 ksmbd_kstat->kstat);
 
 	time = ksmbd_UnixTimeToNT(ksmbd_kstat->kstat->ctime);
 	ksmbd_kstat->create_time = time;

fs/libfs.c +2 -2

index 89cf614a3271..b8f7be638f17 100644
@@ -33,7 +33,7 @@ int simple_getattr(struct mnt_idmap *idmap, const struct path *path,
 		   unsigned int query_flags)
 {
 	struct inode *inode = d_inode(path->dentry);
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 	stat->blocks = inode->i_mapping->nrpages << (PAGE_SHIFT - 9);
 	return 0;
 }

@@ -1315,7 +1315,7 @@ static int empty_dir_getattr(struct mnt_idmap *idmap,
 			     u32 request_mask, unsigned int query_flags)
 {
 	struct inode *inode = d_inode(path->dentry);
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 	return 0;
 }
 

fs/minix/inode.c +1 -1

index e9fbb5303a22..a7f927e3760c 100644
@@ -660,7 +660,7 @@ int minix_getattr(struct mnt_idmap *idmap, const struct path *path,
 	struct super_block *sb = path->dentry->d_sb;
 	struct inode *inode = d_inode(path->dentry);
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 	if (INODE_VERSION(inode) == MINIX_V1)
 		stat->blocks = (BLOCK_SIZE / 512) * V1_minix_blocks(stat->size, sb);
 	else

fs/nfs/inode.c +1 -1

index a910b9a638c5..7f490c29ab6b 100644
@@ -912,7 +912,7 @@ int nfs_getattr(struct mnt_idmap *idmap, const struct path *path,
 	/* Only return attributes that were revalidated. */
 	stat->result_mask = nfs_get_valid_attrmask(inode) | request_mask;
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 	stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
 	stat->change_cookie = inode_peek_iversion_raw(inode);
 	stat->attributes_mask |= STATX_ATTR_CHANGE_MONOTONIC;

fs/nfs/namespace.c +2 -1

index 19d51ebf842c..e7494cdd957e 100644
@@ -215,7 +215,8 @@ nfs_namespace_getattr(struct mnt_idmap *idmap,
 	if (NFS_FH(d_inode(path->dentry))->size != 0)
 		return nfs_getattr(idmap, path, stat, request_mask,
 				   query_flags);
-	generic_fillattr(&nop_mnt_idmap, d_inode(path->dentry), stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, d_inode(path->dentry),
+			 stat);
 	return 0;
 }
 

fs/ntfs3/file.c +1 -1

index 9a3d55c367d9..43ffd48eb048 100644
@@ -85,7 +85,7 @@ int ntfs_getattr(struct mnt_idmap *idmap, const struct path *path,
 
 	stat->attributes_mask |= STATX_ATTR_COMPRESSED | STATX_ATTR_ENCRYPTED;
 
-	generic_fillattr(idmap, inode, stat);
+	generic_fillattr(idmap, request_mask, inode, stat);
 
 	stat->result_mask |= STATX_BTIME;
 	stat->btime = ni->i_crtime;

fs/ocfs2/file.c +1 -1

index efb09de4343d..5dc659a53311 100644
@@ -1317,7 +1317,7 @@ int ocfs2_getattr(struct mnt_idmap *idmap, const struct path *path,
 		goto bail;
 	}
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 	/*
 	 * If there is inline data in the inode, the inode will normally not
 	 * have data blocks allocated (it may have an external xattr block).

fs/orangefs/inode.c +1 -1

index 9014bbcc8031..a52c30e80f45 100644
@@ -871,7 +871,7 @@ int orangefs_getattr(struct mnt_idmap *idmap, const struct path *path,
 	ret = orangefs_inode_getattr(inode,
 	    request_mask & STATX_SIZE ? ORANGEFS_GETATTR_SIZE : 0);
 	if (ret == 0) {
-		generic_fillattr(&nop_mnt_idmap, inode, stat);
+		generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 
 		/* override block size reported to stat */
 		if (!(request_mask & STATX_SIZE))

fs/proc/base.c +2 -2

index 05452c3b9872..98eab84553f9 100644
@@ -1966,7 +1966,7 @@ int pid_getattr(struct mnt_idmap *idmap, const struct path *path,
 	struct proc_fs_info *fs_info = proc_sb_info(inode->i_sb);
 	struct task_struct *task;
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 
 	stat->uid = GLOBAL_ROOT_UID;
 	stat->gid = GLOBAL_ROOT_GID;

@@ -3899,7 +3899,7 @@ static int proc_task_getattr(struct mnt_idmap *idmap,
 {
 	struct inode *inode = d_inode(path->dentry);
 	struct task_struct *p = get_proc_task(inode);
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 
 	if (p) {
 		stat->nlink += get_nr_threads(p);

fs/proc/fd.c +1 -1

index b3140deebbbf..6276b3938842 100644
@@ -352,7 +352,7 @@ static int proc_fd_getattr(struct mnt_idmap *idmap,
 	struct inode *inode = d_inode(path->dentry);
 	int rv = 0;
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 
 	/* If it's a directory, put the number of open fds there */
 	if (S_ISDIR(inode->i_mode)) {

fs/proc/generic.c +1 -1

index 42ae38ff6e7e..775ce0bcf08c 100644
@@ -146,7 +146,7 @@ static int proc_getattr(struct mnt_idmap *idmap,
 		}
 	}
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 	return 0;
 }
 

fs/proc/proc_net.c +1 -1

index a0c0419872e3..75f35f128e63 100644
@@ -308,7 +308,7 @@ static int proc_tgid_net_getattr(struct mnt_idmap *idmap,
 
 	net = get_proc_task_net(inode);
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 
 	if (net != NULL) {
 		stat->nlink = net->proc_net->nlink;

fs/proc/proc_sysctl.c +1 -1

index 8038833ff5b0..c00b15b0ba81 100644
@@ -855,7 +855,7 @@ static int proc_sys_getattr(struct mnt_idmap *idmap,
 	if (IS_ERR(head))
 		return PTR_ERR(head);
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 	if (table)
 		stat->mode = (stat->mode & S_IFMT) | table->mode;
 

fs/proc/root.c +2 -1

index a86e65a608da..9191248f2dac 100644
@@ -314,7 +314,8 @@ static int proc_root_getattr(struct mnt_idmap *idmap,
 			     const struct path *path, struct kstat *stat,
 			     u32 request_mask, unsigned int query_flags)
 {
-	generic_fillattr(&nop_mnt_idmap, d_inode(path->dentry), stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, d_inode(path->dentry),
+			 stat);
 	stat->nlink = proc_root.nlink + nr_processes();
 	return 0;
 }

fs/stat.c +10 -8

index 7c238da22ef0..9b513a142a56 100644
@@ -29,6 +29,7 @@
 /**
  * generic_fillattr - Fill in the basic attributes from the inode struct
  * @idmap:	idmap of the mount the inode was found from
+ * @req_mask	statx request_mask
  * @inode:	Inode to use as the source
  * @stat:	Where to fill in the attributes
  *

@@ -42,8 +43,8 @@
  * uid and gid filds. On non-idmapped mounts or if permission checking is to be
  * performed on the raw inode simply passs @nop_mnt_idmap.
  */
-void generic_fillattr(struct mnt_idmap *idmap, struct inode *inode,
-		      struct kstat *stat)
+void generic_fillattr(struct mnt_idmap *idmap, u32 request_mask,
+		      struct inode *inode, struct kstat *stat)
 {
 	vfsuid_t vfsuid = i_uid_into_vfsuid(idmap, inode);
 	vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);

@@ -61,6 +62,12 @@ void generic_fillattr(struct mnt_idmap *idmap, struct inode *inode,
 	stat->ctime = inode->i_ctime;
 	stat->blksize = i_blocksize(inode);
 	stat->blocks = inode->i_blocks;
+
+	if ((request_mask & STATX_CHANGE_COOKIE) && IS_I_VERSION(inode)) {
+		stat->result_mask |= STATX_CHANGE_COOKIE;
+		stat->change_cookie = inode_query_iversion(inode);
+	}
+
 }
 EXPORT_SYMBOL(generic_fillattr);
 

@@ -123,17 +130,12 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
 	stat->attributes_mask |= (STATX_ATTR_AUTOMOUNT |
 				  STATX_ATTR_DAX);
 
-	if ((request_mask & STATX_CHANGE_COOKIE) && IS_I_VERSION(inode)) {
-		stat->result_mask |= STATX_CHANGE_COOKIE;
-		stat->change_cookie = inode_query_iversion(inode);
-	}
-
 	idmap = mnt_idmap(path->mnt);
 	if (inode->i_op->getattr)
 		return inode->i_op->getattr(idmap, path, stat,
 					    request_mask, query_flags);
 
-	generic_fillattr(idmap, inode, stat);
+	generic_fillattr(idmap, request_mask, inode, stat);
 	return 0;
 }
 EXPORT_SYMBOL(vfs_getattr_nosec);

fs/sysv/itree.c +2 -1

index b22764fe669c..d41189cd9ec2 100644
@@ -445,7 +445,8 @@ int sysv_getattr(struct mnt_idmap *idmap, const struct path *path,
 		 struct kstat *stat, u32 request_mask, unsigned int flags)
 {
 	struct super_block *s = path->dentry->d_sb;
-	generic_fillattr(&nop_mnt_idmap, d_inode(path->dentry), stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, d_inode(path->dentry),
+			 stat);
 	stat->blocks = (s->s_blocksize / 512) * sysv_nblocks(s, stat->size);
 	stat->blksize = s->s_blocksize;
 	return 0;

fs/ubifs/dir.c +1 -1

index ef0499edc248..ca044f31c49d 100644
@@ -1665,7 +1665,7 @@ int ubifs_getattr(struct mnt_idmap *idmap, const struct path *path,
 				STATX_ATTR_ENCRYPTED |
 				STATX_ATTR_IMMUTABLE);
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 	stat->blksize = UBIFS_BLOCK_SIZE;
 	stat->size = ui->ui_size;
 

fs/udf/symlink.c +1 -1

index a34c8c4e6d21..69f6982aec9a 100644
@@ -153,7 +153,7 @@ static int udf_symlink_getattr(struct mnt_idmap *idmap,
 	struct inode *inode = d_backing_inode(dentry);
 	struct page *page;
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 	page = read_mapping_page(inode->i_mapping, 0, NULL);
 	if (IS_ERR(page))
 		return PTR_ERR(page);

fs/vboxsf/utils.c +1 -1

index dd0ae1188e87..482f778709d6 100644
@@ -252,7 +252,7 @@ int vboxsf_getattr(struct mnt_idmap *idmap, const struct path *path,
 	if (err)
 		return err;
 
-	generic_fillattr(&nop_mnt_idmap, d_inode(dentry), kstat);
+	generic_fillattr(&nop_mnt_idmap, request_mask, d_inode(dentry), kstat);
 	return 0;
 }
 

include/linux/fs.h +1 -1

index 21a981680856..d5896f90093a 100644
@@ -2857,7 +2857,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 generic_fillattr(struct mnt_idmap *, struct inode *, struct kstat *);
+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);
 extern int vfs_getattr(const struct path *, struct kstat *, u32, unsigned int);

mm/shmem.c +1 -1

index e40a08c5c6d7..8208d4f85dff 100644
@@ -1073,7 +1073,7 @@ static int shmem_getattr(struct mnt_idmap *idmap,
 	stat->attributes_mask |= (STATX_ATTR_APPEND |
 			STATX_ATTR_IMMUTABLE |
 			STATX_ATTR_NODUMP);
-	generic_fillattr(idmap, inode, stat);
+	generic_fillattr(idmap, request_mask, inode, stat);
 
 	if (shmem_is_huge(inode, 0, false, NULL, 0))
 		stat->blksize = HPAGE_PMD_SIZE;

Signature · 1 line

2.40.1

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

Quoted 4 lines · Jeff Layton

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

TIL.... that atomic_long_fetch_or() and atomic_long_fetch_andnot() exist. :-)

When I went looking for documentation about why they do or this particular usage pattern found in the patch, I didn't find any --- at least, certainly not in the Documentation in the kernel sources. The closest that I fond was this LWN article written by Neil Brown from 2016:

	https://lwn.net/Articles/698315/

... and this only covered the use atomic_fetch_or(); I wasn't able to find anything discussing atomic_fetch_andnot().

It looks like Peter Zijlstra added some bare-bones documentation in 2017, in commit: 706eeb3e9c6f ("Documentation/locking/atomic: Add documents for new atomic_t APIs") so we do have Documentation that these functions *exist*, but there is nothing explaining what they do, or how they can be used (e.g., in this rather clever way to set and clear a flag in the high bits of the nsec field).

I know that it's best to report missing documentation in the form of a patch, but I fear I don't have the time or the expertise to really do this topic justice, so I'd just thought I'd just note this lack for now, and maybe in my copious spare time I'll try to get at least something that will no doubt contain errors, but might inspire some folks to correct the text. (Or maybe on someone on linux-doc will feel inspired and get there ahead of me. :-)

- Ted

[Cc Paul and Mark]

Quoted 27 lines · Theodore Ts'o

Quoted 4 lines · Jeff Layton

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

TIL.... that atomic_long_fetch_or() and atomic_long_fetch_andnot() exist. :-)

When I went looking for documentation about why they do or this particular usage pattern found in the patch, I didn't find any --- at least, certainly not in the Documentation in the kernel sources. The closest that I fond was this LWN article written by Neil Brown from 2016:

	https://lwn.net/Articles/698315/

... and this only covered the use atomic_fetch_or(); I wasn't able to find anything discussing atomic_fetch_andnot().

It looks like Peter Zijlstra added some bare-bones documentation in 2017, in commit: 706eeb3e9c6f ("Documentation/locking/atomic: Add documents for new atomic_t APIs") so we do have Documentation that these functions *exist*, but there is nothing explaining what they do, or how they can be used (e.g., in this rather clever way to set and clear a flag in the high bits of the nsec field).

Quoted 8 lines

I know that it's best to report missing documentation in the form of a patch, but I fear I don't have the time or the expertise to really do this topic justice, so I'd just thought I'd just note this lack for now, and maybe in my copious spare time I'll try to get at least something that will no doubt contain errors, but might inspire some folks to correct the text. (Or maybe on someone on linux-doc will feel inspired and get there ahead of me. :-)

Paul already started the work:

	https://lore.kernel.org/lkml/19135936-06d7-4705-8bc8-bb31c2a478ca@paulmck-laptop/

Quoted 39 lines · Boqun Feng

[Cc Paul and Mark]

Quoted 35 lines · Theodore Ts'o

Quoted 4 lines · Jeff Layton

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

TIL.... that atomic_long_fetch_or() and atomic_long_fetch_andnot() exist. :-)

When I went looking for documentation about why they do or this particular usage pattern found in the patch, I didn't find any --- at least, certainly not in the Documentation in the kernel sources. The closest that I fond was this LWN article written by Neil Brown from 2016:

	https://lwn.net/Articles/698315/

... and this only covered the use atomic_fetch_or(); I wasn't able to find anything discussing atomic_fetch_andnot().

It looks like Peter Zijlstra added some bare-bones documentation in 2017, in commit: 706eeb3e9c6f ("Documentation/locking/atomic: Add documents for new atomic_t APIs") so we do have Documentation that these functions *exist*, but there is nothing explaining what they do, or how they can be used (e.g., in this rather clever way to set and clear a flag in the high bits of the nsec field).

I know that it's best to report missing documentation in the form of a patch, but I fear I don't have the time or the expertise to really do this topic justice, so I'd just thought I'd just note this lack for now, and maybe in my copious spare time I'll try to get at least something that will no doubt contain errors, but might inspire some folks to correct the text. (Or maybe on someone on linux-doc will feel inspired and get there ahead of me. :-)

Quoted 3 lines

Paul already started the work:

	https://lore.kernel.org/lkml/19135936-06d7-4705-8bc8-bb31c2a478ca@paulmck-laptop/

Which reminds me, I need to forward-port that documentation to Mark's latest version. ;-)

The VFS always uses coarse-grained timestamp updates for filling out 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. 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 (e.g backup applications).

Switching to always using fine-grained timestamps would improve the situation, but that becomes rather expensive, as the underlying filesystem will 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.

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

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

This patch adds the infrastructure this scheme. Filesytems can opt into it by setting the FS_MULTIGRAIN_TS flag in the fstype.

Later patches will convert individual filesystems over to use it.

Signed-off-by Jeff Layton <[email protected]>

+/**
+ * current_mg_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.
+ */

fs/inode.c +26 -0

+static struct timespec64 current_mg_time(struct inode *inode)
+{
+	struct timespec64 now;
+	atomic_long_t *pnsec = (atomic_long_t *)&inode->i_ctime.tv_nsec;
+	long nsec = atomic_long_fetch_andnot(I_CTIME_QUERIED, pnsec);
+
+	if (nsec & I_CTIME_QUERIED) {
+		ktime_get_real_ts64(&now);
+	} else {
+		struct timespec64 ctime;
+
+		ktime_get_coarse_real_ts64(&now);
+
+		/*
+		 * If we've recently fetched a fine-grained timestamp
+		 * then the coarse-grained one may still be earlier than the
+		 * existing one. Just keep the existing ctime if so.
+		 */
+		ctime = ctime_peek(inode);
+		if (timespec64_compare(&ctime, &now) > 0)
+			now = ctime;
+	}
+
+	return now;
+}
+

include/linux/fs.h +6 -0

+/**
+ * ctime_nsec_peek - peek at (but don't query) the ctime tv_nsec field
+ * @inode: inode to fetch the ctime from
+ *
+ * Grab the current ctime tv_nsec field from the inode, mask off the
+ * I_CTIME_QUERIED flag and return it. This is mostly intended for use by

include/linux/fs.h +12 -0

+}
+
+/**
+ * ctime_peek - peek at (but don't query) the ctime
+ * @inode: inode to fetch the ctime from
+ *
+ * Grab the current ctime from the inode, sans I_CTIME_QUERIED flag. For
+ * use by internal consumers that don't require a fine-grained update on
+ * the next change.
+ *
+ * This is safe to call regardless of whether the underlying filesystem
+ * is using multigrain timestamps.

Quoted 27 lines · Jeff Layton

The VFS always uses coarse-grained timestamp updates for filling out 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. 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 (e.g backup applications).

Switching to always using fine-grained timestamps would improve the situation, but that becomes rather expensive, as the underlying filesystem will 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.

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

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

Quoted 6 lines

This patch adds the infrastructure this scheme. Filesytems can opt into it by setting the FS_MULTIGRAIN_TS flag in the fstype.

Later patches will convert individual filesystems over to use it.

Signed-off-by Jeff Layton <[email protected]>

So there are two things I dislike about this series because I think they are fragile:

1) If we have a filesystem supporting multigrain ts and someone accidentally directly uses the value of inode->i_ctime, he can get bogus value (with QUERIED flag). This mistake is very easy to do. So I think we should rename i_ctime to something like __i_ctime and always use accessor function for it.

2) As I already commented in a previous version of the series, the scheme with just one flag for both ctime and mtime and flag getting cleared in current_time() relies on the fact that filesystems always do an equivalent of:

	inode->i_mtime = inode->i_ctime = current_time();

Otherwise we can do coarse grained update where we should have done a fine grained one. Filesystems often update timestamps like this but not universally. Grepping shows some instances where only inode->i_mtime is set from current_time() e.g. in autofs or bfs. Again a mistake that is rather easy to make and results in subtle issues. I think this would be also nicely solved by renaming i_ctime to __i_ctime and using a function to set ctime. Mtime could then be updated with inode->i_mtime = ctime_peek().

I understand this is quite some churn but a very mechanical one that could be just done with Coccinelle and a few manual fixups. So IMHO it is worth the more robust result.

Quoted 59 lines · Jan Kara

Quoted 27 lines · Jeff Layton

The VFS always uses coarse-grained timestamp updates for filling out 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. 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 (e.g backup applications).

Switching to always using fine-grained timestamps would improve the situation, but that becomes rather expensive, as the underlying filesystem will 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.

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

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

Quoted 6 lines

This patch adds the infrastructure this scheme. Filesytems can opt into it by setting the FS_MULTIGRAIN_TS flag in the fstype.

Later patches will convert individual filesystems over to use it.

Signed-off-by Jeff Layton <[email protected]>

So there are two things I dislike about this series because I think they are fragile:

1) If we have a filesystem supporting multigrain ts and someone accidentally directly uses the value of inode->i_ctime, he can get bogus value (with QUERIED flag). This mistake is very easy to do. So I think we should rename i_ctime to something like __i_ctime and always use accessor function for it.

2) As I already commented in a previous version of the series, the scheme with just one flag for both ctime and mtime and flag getting cleared in current_time() relies on the fact that filesystems always do an equivalent of:

	inode->i_mtime = inode->i_ctime = current_time();

Otherwise we can do coarse grained update where we should have done a fine grained one. Filesystems often update timestamps like this but not universally. Grepping shows some instances where only inode->i_mtime is set from current_time() e.g. in autofs or bfs. Again a mistake that is rather easy to make and results in subtle issues. I think this would be also nicely solved by renaming i_ctime to __i_ctime and using a function to set ctime. Mtime could then be updated with inode->i_mtime = ctime_peek().

Quoted 3 lines

I understand this is quite some churn but a very mechanical one that could be just done with Coccinelle and a few manual fixups. So IMHO it is worth the more robust result.

Also as I'm thinking about it your current scheme is slightly racy. Suppose the filesystem does:

CPU1 CPU2

					statx()
inode->i_ctime = current_time()
  current_mg_time()
    nsec = atomic_long_fetch_andnot(QUERIED, &inode->i_ctime.tv_nsec)
					  nsec = atomic_long_fetch_or(QUERIED, &inode->i_ctime.tv_nsec)
    if (nsec & QUERIED) - not set
      ktime_get_coarse_real_ts64(&now)
    return timestamp_truncate(now, inode);

- QUERIED flag in the inode->i_ctime gets overwritten by the assignment => we need not update ctime due to granularity although it was queried

One more reason to use explicit function to update inode->i_ctime ;)

Quoted 77 lines · Jan Kara

Quoted 59 lines · Jan Kara

Quoted 27 lines · Jeff Layton

The VFS always uses coarse-grained timestamp updates for filling out 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. 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 (e.g backup applications).

Switching to always using fine-grained timestamps would improve the situation, but that becomes rather expensive, as the underlying filesystem will 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.

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

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

Quoted 6 lines

This patch adds the infrastructure this scheme. Filesytems can opt into it by setting the FS_MULTIGRAIN_TS flag in the fstype.

Later patches will convert individual filesystems over to use it.

Signed-off-by Jeff Layton <[email protected]>

So there are two things I dislike about this series because I think they are fragile:

1) If we have a filesystem supporting multigrain ts and someone accidentally directly uses the value of inode->i_ctime, he can get bogus value (with QUERIED flag). This mistake is very easy to do. So I think we should rename i_ctime to something like __i_ctime and always use accessor function for it.

2) As I already commented in a previous version of the series, the scheme with just one flag for both ctime and mtime and flag getting cleared in current_time() relies on the fact that filesystems always do an equivalent of:

	inode->i_mtime = inode->i_ctime = current_time();

Otherwise we can do coarse grained update where we should have done a fine grained one. Filesystems often update timestamps like this but not universally. Grepping shows some instances where only inode->i_mtime is set from current_time() e.g. in autofs or bfs. Again a mistake that is rather easy to make and results in subtle issues. I think this would be also nicely solved by renaming i_ctime to __i_ctime and using a function to set ctime. Mtime could then be updated with inode->i_mtime = ctime_peek().

Quoted 3 lines

I understand this is quite some churn but a very mechanical one that could be just done with Coccinelle and a few manual fixups. So IMHO it is worth the more robust result.

Also as I'm thinking about it your current scheme is slightly racy. Suppose the filesystem does:

CPU1 CPU2

					statx()
inode->i_ctime = current_time()
  current_mg_time()
    nsec = atomic_long_fetch_andnot(QUERIED, &inode->i_ctime.tv_nsec)
					  nsec = atomic_long_fetch_or(QUERIED, &inode->i_ctime.tv_nsec)
    if (nsec & QUERIED) - not set
      ktime_get_coarse_real_ts64(&now)
    return timestamp_truncate(now, inode);

Quoted 4 lines

- QUERIED flag in the inode->i_ctime gets overwritten by the assignment => we need not update ctime due to granularity although it was queried

One more reason to use explicit function to update inode->i_ctime ;)

When we store the new time in the i_ctime field, the flag gets cleared because at that point we're storing a new (unseen) time.

However, you're correct: if the i_ctime in your above example starts at the same value that is currently being returned by ktime_get_coarse_real_ts64, then we'll lose the flag set in statx.

I think the right fix there would be to not update the ctime at all if it's a coarse grained time, and the value wouldn't have an apparent change to an observer. That would leave the flag intact.

That does mean we'd need to move to a function that does clock fetch and assigns it to i_ctime in one go (like you suggest). Something like:

    inode_update_ctime(inode);

How we do that with atomic operations over two values (the tv_sec and tv_nsec) is a bit tricky. I'll have to think about it.

Christian, given Jan's concerns do you want to drop this series for now and let me respin it?

Quoted 102 lines · Jeff Layton

Quoted 77 lines · Jan Kara

Quoted 59 lines · Jan Kara

Quoted 27 lines · Jeff Layton

The VFS always uses coarse-grained timestamp updates for filling out 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. 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 (e.g backup applications).

Switching to always using fine-grained timestamps would improve the situation, but that becomes rather expensive, as the underlying filesystem will 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.

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

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

Quoted 6 lines

This patch adds the infrastructure this scheme. Filesytems can opt into it by setting the FS_MULTIGRAIN_TS flag in the fstype.

Later patches will convert individual filesystems over to use it.

Signed-off-by Jeff Layton <[email protected]>

So there are two things I dislike about this series because I think they are fragile:

1) If we have a filesystem supporting multigrain ts and someone accidentally directly uses the value of inode->i_ctime, he can get bogus value (with QUERIED flag). This mistake is very easy to do. So I think we should rename i_ctime to something like __i_ctime and always use accessor function for it.

2) As I already commented in a previous version of the series, the scheme with just one flag for both ctime and mtime and flag getting cleared in current_time() relies on the fact that filesystems always do an equivalent of:

	inode->i_mtime = inode->i_ctime = current_time();

Otherwise we can do coarse grained update where we should have done a fine grained one. Filesystems often update timestamps like this but not universally. Grepping shows some instances where only inode->i_mtime is set from current_time() e.g. in autofs or bfs. Again a mistake that is rather easy to make and results in subtle issues. I think this would be also nicely solved by renaming i_ctime to __i_ctime and using a function to set ctime. Mtime could then be updated with inode->i_mtime = ctime_peek().

Quoted 3 lines

I understand this is quite some churn but a very mechanical one that could be just done with Coccinelle and a few manual fixups. So IMHO it is worth the more robust result.

Also as I'm thinking about it your current scheme is slightly racy. Suppose the filesystem does:

CPU1 CPU2

					statx()
inode->i_ctime = current_time()
  current_mg_time()
    nsec = atomic_long_fetch_andnot(QUERIED, &inode->i_ctime.tv_nsec)
					  nsec = atomic_long_fetch_or(QUERIED, &inode->i_ctime.tv_nsec)
    if (nsec & QUERIED) - not set
      ktime_get_coarse_real_ts64(&now)
    return timestamp_truncate(now, inode);

Quoted 4 lines

- QUERIED flag in the inode->i_ctime gets overwritten by the assignment => we need not update ctime due to granularity although it was queried

One more reason to use explicit function to update inode->i_ctime ;)

When we store the new time in the i_ctime field, the flag gets cleared because at that point we're storing a new (unseen) time.

However, you're correct: if the i_ctime in your above example starts at the same value that is currently being returned by ktime_get_coarse_real_ts64, then we'll lose the flag set in statx.

I think the right fix there would be to not update the ctime at all if it's a coarse grained time, and the value wouldn't have an apparent change to an observer. That would leave the flag intact.

That does mean we'd need to move to a function that does clock fetch and assigns it to i_ctime in one go (like you suggest). Something like:

    inode_update_ctime(inode);

How we do that with atomic operations over two values (the tv_sec and tv_nsec) is a bit tricky. I'll have to think about it.

Quoted 2 lines

Christian, given Jan's concerns do you want to drop this series for now and let me respin it?

I deliberately put it into a vfs.unstable.* branch. I would leave it there until you send a new one then drop it. If we get lucky the bots that run on -next will have time to report potential perf issues while it's not currently causing conflicts.

Quoted 59 lines · Jan Kara

Quoted 27 lines · Jeff Layton

The VFS always uses coarse-grained timestamp updates for filling out 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. 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 (e.g backup applications).

Switching to always using fine-grained timestamps would improve the situation, but that becomes rather expensive, as the underlying filesystem will 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.

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

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

Quoted 6 lines

This patch adds the infrastructure this scheme. Filesytems can opt into it by setting the FS_MULTIGRAIN_TS flag in the fstype.

Later patches will convert individual filesystems over to use it.

Signed-off-by Jeff Layton <[email protected]>

So there are two things I dislike about this series because I think they are fragile:

1) If we have a filesystem supporting multigrain ts and someone accidentally directly uses the value of inode->i_ctime, he can get bogus value (with QUERIED flag). This mistake is very easy to do. So I think we should rename i_ctime to something like __i_ctime and always use accessor function for it.

2) As I already commented in a previous version of the series, the scheme with just one flag for both ctime and mtime and flag getting cleared in current_time() relies on the fact that filesystems always do an equivalent of:

	inode->i_mtime = inode->i_ctime = current_time();

Otherwise we can do coarse grained update where we should have done a fine grained one. Filesystems often update timestamps like this but not universally. Grepping shows some instances where only inode->i_mtime is set from current_time() e.g. in autofs or bfs. Again a mistake that is rather easy to make and results in subtle issues. I think this would be also nicely solved by renaming i_ctime to __i_ctime and using a function to set ctime. Mtime could then be updated with inode->i_mtime = ctime_peek().

Quoted 3 lines

I understand this is quite some churn but a very mechanical one that could be just done with Coccinelle and a few manual fixups. So IMHO it is worth the more robust result.

Also as I'm thinking about it your current scheme is slightly racy. Suppose the filesystem does:

CPU1 CPU2

					statx()
inode->i_ctime = current_time()
  current_mg_time()
    nsec = atomic_long_fetch_andnot(QUERIED, &inode->i_ctime.tv_nsec)
					  nsec = atomic_long_fetch_or(QUERIED, &inode->i_ctime.tv_nsec)
    if (nsec & QUERIED) - not set
      ktime_get_coarse_real_ts64(&now)
    return timestamp_truncate(now, inode);

- QUERIED flag in the inode->i_ctime gets overwritten by the assignment => we need not update ctime due to granularity although it was queried

One more reason to use explicit function to update inode->i_ctime ;)

Quoted 27 lines · Jeff Layton

The VFS always uses coarse-grained timestamp updates for filling out 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. 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 (e.g backup applications).

Switching to always using fine-grained timestamps would improve the situation, but that becomes rather expensive, as the underlying filesystem will 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.

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

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

Quoted 6 lines

This patch adds the infrastructure this scheme. Filesytems can opt into it by setting the FS_MULTIGRAIN_TS flag in the fstype.

Later patches will convert individual filesystems over to use it.

Signed-off-by Jeff Layton <[email protected]>

So there are two things I dislike about this series because I think they are fragile:

1) If we have a filesystem supporting multigrain ts and someone accidentally directly uses the value of inode->i_ctime, he can get bogus value (with QUERIED flag). This mistake is very easy to do. So I think we should rename i_ctime to something like __i_ctime and always use accessor function for it.

2) As I already commented in a previous version of the series, the scheme with just one flag for both ctime and mtime and flag getting cleared in current_time() relies on the fact that filesystems always do an equivalent of:

	inode->i_mtime = inode->i_ctime = current_time();

Otherwise we can do coarse grained update where we should have done a fine grained one. Filesystems often update timestamps like this but not universally. Grepping shows some instances where only inode->i_mtime is set from current_time() e.g. in autofs or bfs. Again a mistake that is rather easy to make and results in subtle issues. I think this would be also nicely solved by renaming i_ctime to __i_ctime and using a function to set ctime. Mtime could then be updated with inode->i_mtime = ctime_peek().

I understand this is quite some churn but a very mechanical one that could be just done with Coccinelle and a few manual fixups. So IMHO it is worth the more robust result.

Some more nits below.

Quoted 8 lines

+/**
+ * current_mg_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.
+ */

The comment should also mention that QUERIED flag is cleared from the ctime.

Quoted 26 lines

fs/inode.c +26 -0

+static struct timespec64 current_mg_time(struct inode *inode)
+{
+	struct timespec64 now;
+	atomic_long_t *pnsec = (atomic_long_t *)&inode->i_ctime.tv_nsec;
+	long nsec = atomic_long_fetch_andnot(I_CTIME_QUERIED, pnsec);
+
+	if (nsec & I_CTIME_QUERIED) {
+		ktime_get_real_ts64(&now);
+	} else {
+		struct timespec64 ctime;
+
+		ktime_get_coarse_real_ts64(&now);
+
+		/*
+		 * If we've recently fetched a fine-grained timestamp
+		 * then the coarse-grained one may still be earlier than the
+		 * existing one. Just keep the existing ctime if so.
+		 */
+		ctime = ctime_peek(inode);
+		if (timespec64_compare(&ctime, &now) > 0)
+			now = ctime;
+	}
+
+	return now;
+}
+

...

Quoted 6 lines

include/linux/fs.h +6 -0

+/**
+ * ctime_nsec_peek - peek at (but don't query) the ctime tv_nsec field
+ * @inode: inode to fetch the ctime from
+ *
+ * Grab the current ctime tv_nsec field from the inode, mask off the
+ * I_CTIME_QUERIED flag and return it. This is mostly intended for use by

include/linux/fs.h struct file_system_type { · line 2276

Quoted 10 lines

include/linux/fs.h +10 -0

+ * 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 long ctime_nsec_peek(const struct inode *inode)
+{
+	return inode->i_ctime.tv_nsec &~ I_CTIME_QUERIED;

This is somewhat unusual spacing. I'd use:

	inode->i_ctime.tv_nsec & ~I_CTIME_QUERIED

Quoted 18 lines

include/linux/fs.h +18 -0

+}
+
+/**
+ * ctime_peek - peek at (but don't query) the ctime
+ * @inode: inode to fetch the ctime from
+ *
+ * Grab the current ctime from the inode, sans I_CTIME_QUERIED flag. For
+ * use by internal consumers that don't require a fine-grained update on
+ * the next change.
+ *
+ * This is safe to call regardless of whether the underlying filesystem
+ * is using multigrain timestamps.
+ */
+static inline struct timespec64 ctime_peek(const struct inode *inode)
+{
+	struct timespec64 ctime;
+
+	ctime.tv_sec = inode->i_ctime.tv_sec;

include/linux/fs.h struct file_system_type { · line 2298

Quoted 4 lines

include/linux/fs.h +4 -0

+	ctime.tv_nsec = ctime_nsec_peek(inode);
+
+	return ctime;
+}

Given this is in a header that gets included in a lot of places, maybe we should call it like inode_ctime_peek() or inode_ctime_get() to reduce chances of a name clash?

Quoted 27 lines · Jeff Layton

The VFS always uses coarse-grained timestamp updates for filling out 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. 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 (e.g backup applications).

Switching to always using fine-grained timestamps would improve the situation, but that becomes rather expensive, as the underlying filesystem will 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.

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

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

Quoted 6 lines

This patch adds the infrastructure this scheme. Filesytems can opt into it by setting the FS_MULTIGRAIN_TS flag in the fstype.

Later patches will convert individual filesystems over to use it.

Signed-off-by Jeff Layton <[email protected]>

So there are two things I dislike about this series because I think they are fragile:

1) If we have a filesystem supporting multigrain ts and someone accidentally directly uses the value of inode->i_ctime, he can get bogus value (with QUERIED flag). This mistake is very easy to do. So I think we should rename i_ctime to something like __i_ctime and always use accessor function for it.

2) As I already commented in a previous version of the series, the scheme with just one flag for both ctime and mtime and flag getting cleared in current_time() relies on the fact that filesystems always do an equivalent of:

	inode->i_mtime = inode->i_ctime = current_time();

Otherwise we can do coarse grained update where we should have done a fine grained one. Filesystems often update timestamps like this but not universally. Grepping shows some instances where only inode->i_mtime is set from current_time() e.g. in autofs or bfs. Again a mistake that is rather easy to make and results in subtle issues. I think this would be also nicely solved by renaming i_ctime to __i_ctime and using a function to set ctime. Mtime could then be updated with inode->i_mtime = ctime_peek().

I understand this is quite some churn but a very mechanical one that could be just done with Coccinelle and a few manual fixups. So IMHO it is worth the more robust result.

Some more nits below.

Quoted 8 lines

+/**
+ * current_mg_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.
+ */

The comment should also mention that QUERIED flag is cleared from the ctime.

Quoted 26 lines

fs/inode.c +26 -0

+static struct timespec64 current_mg_time(struct inode *inode)
+{
+	struct timespec64 now;
+	atomic_long_t *pnsec = (atomic_long_t *)&inode->i_ctime.tv_nsec;
+	long nsec = atomic_long_fetch_andnot(I_CTIME_QUERIED, pnsec);
+
+	if (nsec & I_CTIME_QUERIED) {
+		ktime_get_real_ts64(&now);
+	} else {
+		struct timespec64 ctime;
+
+		ktime_get_coarse_real_ts64(&now);
+
+		/*
+		 * If we've recently fetched a fine-grained timestamp
+		 * then the coarse-grained one may still be earlier than the
+		 * existing one. Just keep the existing ctime if so.
+		 */
+		ctime = ctime_peek(inode);
+		if (timespec64_compare(&ctime, &now) > 0)
+			now = ctime;
+	}
+
+	return now;
+}
+

...

Quoted 6 lines

include/linux/fs.h +6 -0

+/**
+ * ctime_nsec_peek - peek at (but don't query) the ctime tv_nsec field
+ * @inode: inode to fetch the ctime from
+ *
+ * Grab the current ctime tv_nsec field from the inode, mask off the
+ * I_CTIME_QUERIED flag and return it. This is mostly intended for use by

include/linux/fs.h struct file_system_type { · line 2276

Quoted 10 lines

include/linux/fs.h +10 -0

+ * 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 long ctime_nsec_peek(const struct inode *inode)
+{
+	return inode->i_ctime.tv_nsec &~ I_CTIME_QUERIED;

This is somewhat unusual spacing. I'd use:

	inode->i_ctime.tv_nsec & ~I_CTIME_QUERIED

Quoted 18 lines

include/linux/fs.h +18 -0

+}
+
+/**
+ * ctime_peek - peek at (but don't query) the ctime
+ * @inode: inode to fetch the ctime from
+ *
+ * Grab the current ctime from the inode, sans I_CTIME_QUERIED flag. For
+ * use by internal consumers that don't require a fine-grained update on
+ * the next change.
+ *
+ * This is safe to call regardless of whether the underlying filesystem
+ * is using multigrain timestamps.
+ */
+static inline struct timespec64 ctime_peek(const struct inode *inode)
+{
+	struct timespec64 ctime;
+
+	ctime.tv_sec = inode->i_ctime.tv_sec;

include/linux/fs.h struct file_system_type { · line 2298

Quoted 4 lines

include/linux/fs.h +4 -0

+	ctime.tv_nsec = ctime_nsec_peek(inode);
+
+	return ctime;
+}

Given this is in a header that gets included in a lot of places, maybe we should call it like inode_ctime_peek() or inode_ctime_get() to reduce chances of a name clash?

Quoted 35 lines · Jan Kara

Quoted 27 lines · Jeff Layton

The VFS always uses coarse-grained timestamp updates for filling out 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. 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 (e.g backup applications).

Switching to always using fine-grained timestamps would improve the situation, but that becomes rather expensive, as the underlying filesystem will 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.

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

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

Quoted 6 lines

This patch adds the infrastructure this scheme. Filesytems can opt into it by setting the FS_MULTIGRAIN_TS flag in the fstype.

Later patches will convert individual filesystems over to use it.

Signed-off-by Jeff Layton <[email protected]>

Quoted 9 lines

So there are two things I dislike about this series because I think they are fragile:

1) If we have a filesystem supporting multigrain ts and someone accidentally directly uses the value of inode->i_ctime, he can get bogus value (with QUERIED flag). This mistake is very easy to do. So I think we should rename i_ctime to something like __i_ctime and always use accessor function for it.

We could do this, but it'll be quite invasive. We'd have to change any place that touches i_ctime (and there are a lot of them), even on filesystems that are not being converted.

Quoted 15 lines

2) As I already commented in a previous version of the series, the scheme with just one flag for both ctime and mtime and flag getting cleared in current_time() relies on the fact that filesystems always do an equivalent of:

	inode->i_mtime = inode->i_ctime = current_time();

Otherwise we can do coarse grained update where we should have done a fine grained one. Filesystems often update timestamps like this but not universally. Grepping shows some instances where only inode->i_mtime is set from current_time() e.g. in autofs or bfs. Again a mistake that is rather easy to make and results in subtle issues. I think this would be also nicely solved by renaming i_ctime to __i_ctime and using a function to set ctime. Mtime could then be updated with inode->i_mtime = ctime_peek().

Quoted 3 lines

I understand this is quite some churn but a very mechanical one that could be just done with Coccinelle and a few manual fixups. So IMHO it is worth the more robust result.

AFAICT, under POSIX, you must _always_ set the ctime when you set the mtime, but the reverse is not true. That's why keeping the flag in the ctime makes sense. If we're updating the mtime, then we necessarily must update the ctime.

Quoted 18 lines

Quoted 18 lines

include/linux/fs.h +18 -0

+}
+
+/**
+ * ctime_peek - peek at (but don't query) the ctime
+ * @inode: inode to fetch the ctime from
+ *
+ * Grab the current ctime from the inode, sans I_CTIME_QUERIED flag. For
+ * use by internal consumers that don't require a fine-grained update on
+ * the next change.
+ *
+ * This is safe to call regardless of whether the underlying filesystem
+ * is using multigrain timestamps.
+ */
+static inline struct timespec64 ctime_peek(const struct inode *inode)
+{
+	struct timespec64 ctime;
+
+	ctime.tv_sec = inode->i_ctime.tv_sec;

Quoted 8 lines

include/linux/fs.h struct file_system_type { · line 2298

Quoted 4 lines

include/linux/fs.h +4 -0

+	ctime.tv_nsec = ctime_nsec_peek(inode);
+
+	return ctime;
+}

Given this is in a header that gets included in a lot of places, maybe we should call it like inode_ctime_peek() or inode_ctime_get() to reduce chances of a name clash?

I'd be fine with that, but "ctime" sort of implies inode->i_ctime to me. We don't really use that nomenclature elsewhere.

Quoted 12 lines · Jeff Layton

Quoted 10 lines · Jan Kara

So there are two things I dislike about this series because I think they are fragile:

1) If we have a filesystem supporting multigrain ts and someone accidentally directly uses the value of inode->i_ctime, he can get bogus value (with QUERIED flag). This mistake is very easy to do. So I think we should rename i_ctime to something like __i_ctime and always use accessor function for it.

Quoted 3 lines

We could do this, but it'll be quite invasive. We'd have to change any place that touches i_ctime (and there are a lot of them), even on filesystems that are not being converted.

Yes, that's why I suggested Coccinelle to deal with this.

Quoted 13 lines · Jan Kara

Quoted 12 lines · Jeff Layton

Quoted 10 lines · Jan Kara

So there are two things I dislike about this series because I think they are fragile:

1) If we have a filesystem supporting multigrain ts and someone accidentally directly uses the value of inode->i_ctime, he can get bogus value (with QUERIED flag). This mistake is very easy to do. So I think we should rename i_ctime to something like __i_ctime and always use accessor function for it.

Quoted 5 lines

Quoted 3 lines

We could do this, but it'll be quite invasive. We'd have to change any place that touches i_ctime (and there are a lot of them), even on filesystems that are not being converted.

Yes, that's why I suggested Coccinelle to deal with this.

I've done the work to convert all of the accesses of i_ctime into accessor functions in the kernel. The current state of it is here:

https://git.kernel.org/pub/scm/linux/kernel/git/jlayton/linux.git/commit/?h=ctime

As expected, it touches a lot of code, all over the place. So far I have most of the conversion in one giant patch, and I need to split it up (probably per-subsystem).

What's the best way to feed this change into mainline? Should I try to get subsystem maintainers to pick these up, or are we better off feeding this in via a separate branch?

If getattr fails, then nfsd can end up scraping the time values directly out of the inode for pre and post-op attrs. This may or may not be the right thing to do, but for now make it at least use ctime_peek in this situation to ensure that the QUERIED flag is masked.

Signed-off-by Jeff Layton <[email protected]>

---
fs/nfsd/nfsfh.c | 11 ++++++++---

1 file changed, 8 insertions(+), 3 deletions(-)

fs/nfsd/nfsfh.c +8 -3

index ccd8485fee04..f053cf20dd8a 100644
@@ -624,9 +624,14 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
 inode = d_inode(fhp->fh_dentry);
 err = fh_getattr(fhp, &stat);
 if (err) {
- /* Grab the times from inode anyway */
+ /*
+ * Grab the times from inode anyway.
+ *
+ * FIXME: is this the right thing to do? Or should we just
+ *  not send pre and post-op attrs in this case?
+ */
 stat.mtime = inode->i_mtime;
- stat.ctime = inode->i_ctime;
+ stat.ctime = ctime_peek(inode);
 stat.size  = inode->i_size;
 if (v4 && IS_I_VERSION(inode)) {
 stat.change_cookie = inode_query_iversion(inode);

@@ -662,7 +667,7 @@ void fh_fill_post_attrs(struct svc_fh *fhp)
 err = fh_getattr(fhp, &fhp->fh_post_attr);
 if (err) {
 fhp->fh_post_saved = false;
- fhp->fh_post_attr.ctime = inode->i_ctime;
+ fhp->fh_post_attr.ctime = ctime_peek(inode);
 if (v4 && IS_I_VERSION(inode)) {
 fhp->fh_post_attr.change_cookie = inode_query_iversion(inode);
 fhp->fh_post_attr.result_mask |= STATX_CHANGE_COOKIE;

Signature · 1 line

2.40.1

Quoted 7 lines · Jeff Layton

If getattr fails, then nfsd can end up scraping the time values directly out of the inode for pre and post-op attrs. This may or may not be the right thing to do, but for now make it at least use ctime_peek in this situation to ensure that the QUERIED flag is masked.

That code comes from:

commit 39ca1bf624b6b82cc895b0217889eaaf572a7913

Author:     Amir Goldstein <[email protected]>
AuthorDate: Wed Jan 3 17:14:35 2018 +0200
Commit:     J. Bruce Fields <[email protected]>
CommitDate: Thu Feb 8 13:40:17 2018 -0500

nfsd: store stat times in fill_pre_wcc() instead of inode times

The time values in stat and inode may differ for overlayfs and stat time values are the correct ones to use. This is also consistent with the fact that fill_post_wcc() also stores stat time values.

This means introducing a stat call that could fail, where previously we were just copying values out of the inode. To be conservative about changing behavior, we fall back to copying values out of the inode in the error case. It might be better just to clear fh_pre_saved (though note the BUG_ON in set_change_info).

Signed-off-by Amir Goldstein <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

I was thinking it might have been added to handle odd corner cases around re-exporting NFS mounts, but that does not seem to be the case.

The fh_getattr() can fail for legitimate reasons -- like the file is in the middle of being deleted or renamed over -- I would think. This code should really deal with that by not adding pre-op attrs, since they are optional.

Quoted 31 lines · Chuck Lever III

Quoted 7 lines · Jeff Layton

If getattr fails, then nfsd can end up scraping the time values directly out of the inode for pre and post-op attrs. This may or may not be the right thing to do, but for now make it at least use ctime_peek in this situation to ensure that the QUERIED flag is masked.

That code comes from:

commit 39ca1bf624b6b82cc895b0217889eaaf572a7913

Author:     Amir Goldstein <[email protected]>
AuthorDate: Wed Jan 3 17:14:35 2018 +0200
Commit:     J. Bruce Fields <[email protected]>
CommitDate: Thu Feb 8 13:40:17 2018 -0500

nfsd: store stat times in fill_pre_wcc() instead of inode times

The time values in stat and inode may differ for overlayfs and stat time values are the correct ones to use. This is also consistent with the fact that fill_post_wcc() also stores stat time values.

This means introducing a stat call that could fail, where previously we were just copying values out of the inode. To be conservative about changing behavior, we fall back to copying values out of the inode in the error case. It might be better just to clear fh_pre_saved (though note the BUG_ON in set_change_info).

Signed-off-by Amir Goldstein <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

Quoted 9 lines

I was thinking it might have been added to handle odd corner cases around re-exporting NFS mounts, but that does not seem to be the case.

The fh_getattr() can fail for legitimate reasons -- like the file is in the middle of being deleted or renamed over -- I would think. This code should really deal with that by not adding pre-op attrs, since they are optional.

That sounds fine to me. I'll plan to drop this patch from the series and I'll send a separate patch to just remove those branches altogether (which should DTRT).

Quoted 42 lines · Jeff Layton

Quoted 40 lines · Chuck Lever III

Quoted 7 lines · Jeff Layton

If getattr fails, then nfsd can end up scraping the time values directly out of the inode for pre and post-op attrs. This may or may not be the right thing to do, but for now make it at least use ctime_peek in this situation to ensure that the QUERIED flag is masked.

That code comes from:

commit 39ca1bf624b6b82cc895b0217889eaaf572a7913

Author:     Amir Goldstein <[email protected]>
AuthorDate: Wed Jan 3 17:14:35 2018 +0200
Commit:     J. Bruce Fields <[email protected]>
CommitDate: Thu Feb 8 13:40:17 2018 -0500

nfsd: store stat times in fill_pre_wcc() instead of inode times

The time values in stat and inode may differ for overlayfs and stat time values are the correct ones to use. This is also consistent with the fact that fill_post_wcc() also stores stat time values.

This means introducing a stat call that could fail, where previously we were just copying values out of the inode. To be conservative about changing behavior, we fall back to copying values out of the inode in the error case. It might be better just to clear fh_pre_saved (though note the BUG_ON in set_change_info).

Signed-off-by Amir Goldstein <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

I was thinking it might have been added to handle odd corner cases around re-exporting NFS mounts, but that does not seem to be the case.

The fh_getattr() can fail for legitimate reasons -- like the file is in the middle of being deleted or renamed over -- I would think. This code should really deal with that by not adding pre-op attrs, since they are optional.

Quoted 3 lines

That sounds fine to me. I'll plan to drop this patch from the series and I'll send a separate patch to just remove those branches altogether (which should DTRT).

I'll wait with reviewing this until you send the next version then.

Signed-off-by Jeff Layton <[email protected]>

Quoted 1 line · Jeff Layton

Signed-off-by Jeff Layton <[email protected]>

Acked-by David Sterba <[email protected]>

Please add a brief description to the changelog too, there's a similar text in the patches adding the infrastructure. Something like "Allow to optimize lot of metadata updates by encoding the status in the cmtime. The fine grained time is needed for NFS."

Quoted 9 lines · David Sterba

Quoted 1 line · Jeff Layton

Signed-off-by Jeff Layton <[email protected]>

Acked-by David Sterba <[email protected]>

Please add a brief description to the changelog too, there's a similar text in the patches adding the infrastructure. Something like "Allow to optimize lot of metadata updates by encoding the status in the cmtime. The fine grained time is needed for NFS."

Sure thing.

Christian, do you want to just alter the changelog with David's suggestion, or would you rather I resend the series?

v4:
- add request_mask argument to generic_fillattr
- Drop current_ctime helper and just code functionality into current_time
- rework i_ctime accessor functions

A few weeks ago, during one of the discussions around i_version, Dave Chinner wrote this:

[...]