[RFC PATCH 0/4] vfs: allow querying i_version via statx

[RFC PATCH 0/4] vfs: allow querying i_version via statx

[Jeff Layton]

Recently I posted a patch to turn on the i_version counter
unconditionally in ext4, and Lukas rightly pointed out that we don't
currently have an easy way to validate its functionality. You can fetch
it via NFS (and see it in network traces), but there's no way to get to
it from userland.

Besides testing, this may also be of use for userland NFS servers, or by
any program that wants to accurately check for file changes, and not be
subject to mtime granularity problems.

Comments and suggestions welcome. I'm not 100% convinced that this is a
great idea, but we've had people ask for it before and it seems like a
reasonable thing to provide.

Jeff Layton (4):
  vfs: report change attribute in statx for IS_I_VERSION inodes
  nfs: report the change attribute if requested
  afs: fill out change attribute in statx replies
  ceph: fill in the change attribute in statx requests

 fs/afs/inode.c            |  2 ++
 fs/ceph/inode.c           | 14 +++++++++-----
 fs/nfs/inode.c            |  3 +++
 fs/stat.c                 |  7 +++++++
 include/linux/stat.h      |  1 +
 include/uapi/linux/stat.h |  3 ++-
 samples/vfs/test-statx.c  |  4 +++-
 7 files changed, 27 insertions(+), 7 deletions(-)

-- 
2.37.1

[RFC PATCH 1/4] vfs: report change attribute in statx for IS_I_VERSION inodes

[Jeff Layton]

From: Jeff Layton <jlayton@redhat.com>

Claim one of the spare fields in struct statx to hold a 64-bit change
attribute. When statx requests this attribute, do an
inode_query_iversion and fill the result in the field.

Also update the test-statx.c program to fetch the change attribute as
well.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/stat.c                 | 7 +++++++
 include/linux/stat.h      | 1 +
 include/uapi/linux/stat.h | 3 ++-
 samples/vfs/test-statx.c  | 4 +++-
 4 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/fs/stat.c b/fs/stat.c
index 9ced8860e0f3..976e0a59ab23 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -17,6 +17,7 @@
 #include <linux/syscalls.h>
 #include <linux/pagemap.h>
 #include <linux/compat.h>
+#include <linux/iversion.h>
 
 #include <linux/uaccess.h>
 #include <asm/unistd.h>
@@ -118,6 +119,11 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
 	stat->attributes_mask |= (STATX_ATTR_AUTOMOUNT |
 				  STATX_ATTR_DAX);
 
+	if ((request_mask & STATX_CHGATTR) && IS_I_VERSION(inode)) {
+		stat->result_mask |= STATX_CHGATTR;
+		stat->chgattr = inode_query_iversion(inode);
+	}
+
 	mnt_userns = mnt_user_ns(path->mnt);
 	if (inode->i_op->getattr)
 		return inode->i_op->getattr(mnt_userns, path, stat,
@@ -611,6 +617,7 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
 	tmp.stx_dev_major = MAJOR(stat->dev);
 	tmp.stx_dev_minor = MINOR(stat->dev);
 	tmp.stx_mnt_id = stat->mnt_id;
+	tmp.stx_chgattr = stat->chgattr;
 
 	return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;
 }
diff --git a/include/linux/stat.h b/include/linux/stat.h
index 7df06931f25d..4a17887472f6 100644
--- a/include/linux/stat.h
+++ b/include/linux/stat.h
@@ -50,6 +50,7 @@ struct kstat {
 	struct timespec64 btime;			/* File creation time */
 	u64		blocks;
 	u64		mnt_id;
+	u64		chgattr;
 };
 
 #endif
diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
index 1500a0f58041..b45243a0fbc5 100644
--- a/include/uapi/linux/stat.h
+++ b/include/uapi/linux/stat.h
@@ -124,7 +124,7 @@ struct statx {
 	__u32	stx_dev_minor;
 	/* 0x90 */
 	__u64	stx_mnt_id;
-	__u64	__spare2;
+	__u64	stx_chgattr;	/* Inode change attribute */
 	/* 0xa0 */
 	__u64	__spare3[12];	/* Spare space for future expansion */
 	/* 0x100 */
@@ -152,6 +152,7 @@ struct statx {
 #define STATX_BASIC_STATS	0x000007ffU	/* The stuff in the normal stat struct */
 #define STATX_BTIME		0x00000800U	/* Want/got stx_btime */
 #define STATX_MNT_ID		0x00001000U	/* Got stx_mnt_id */
+#define STATX_CHGATTR		0x00002000U	/* Want/git stx_chgattr */
 
 #define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */
 
diff --git a/samples/vfs/test-statx.c b/samples/vfs/test-statx.c
index 49c7a46cee07..767208d2f564 100644
--- a/samples/vfs/test-statx.c
+++ b/samples/vfs/test-statx.c
@@ -109,6 +109,8 @@ static void dump_statx(struct statx *stx)
 		printf(" Inode: %-11llu", (unsigned long long) stx->stx_ino);
 	if (stx->stx_mask & STATX_NLINK)
 		printf(" Links: %-5u", stx->stx_nlink);
+	if (stx->stx_mask & STATX_CHGATTR)
+		printf(" Change Attr: 0x%llx", stx->stx_chgattr);
 	if (stx->stx_mask & STATX_TYPE) {
 		switch (stx->stx_mode & S_IFMT) {
 		case S_IFBLK:
@@ -218,7 +220,7 @@ int main(int argc, char **argv)
 	struct statx stx;
 	int ret, raw = 0, atflag = AT_SYMLINK_NOFOLLOW;
 
-	unsigned int mask = STATX_BASIC_STATS | STATX_BTIME;
+	unsigned int mask = STATX_BASIC_STATS | STATX_BTIME | STATX_CHGATTR;
 
 	for (argv++; *argv; argv++) {
 		if (strcmp(*argv, "-F") == 0) {
-- 
2.37.1

[RFC PATCH 2/4] nfs: report the change attribute if requested

[Jeff Layton]

Allow NFS to report the i_version in statx. Since the cost to fetch it
is relatively cheap, do it unconditionally and just set the flag if it
looks like it's valid.

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

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index b4e46b0ffa2d..8e0e7ecf6429 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -829,6 +829,8 @@ static u32 nfs_get_valid_attrmask(struct inode *inode)
 		reply_mask |= STATX_UID | STATX_GID;
 	if (!(cache_validity & NFS_INO_INVALID_BLOCKS))
 		reply_mask |= STATX_BLOCKS;
+	if (!(cache_validity & NFS_INO_INVALID_CHANGE))
+		reply_mask |= STATX_CHGATTR;
 	return reply_mask;
 }
 
@@ -914,6 +916,7 @@ int nfs_getattr(struct user_namespace *mnt_userns, const struct path *path,
 
 	generic_fillattr(&init_user_ns, inode, stat);
 	stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
+	stat->chgattr = inode_peek_iversion_raw(inode);
 	if (S_ISDIR(inode->i_mode))
 		stat->blksize = NFS_SERVER(inode)->dtsize;
 out:
-- 
2.37.1

[RFC PATCH 3/4] afs: fill out change attribute in statx replies

[Jeff Layton]

Always copy the change attribute in a statx reply, and set the
STATX_CHGATTR bit unconditionally.

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

diff --git a/fs/afs/inode.c b/fs/afs/inode.c
index 64dab70d4a4f..dffd6edd6628 100644
--- a/fs/afs/inode.c
+++ b/fs/afs/inode.c
@@ -760,6 +760,8 @@ int afs_getattr(struct user_namespace *mnt_userns, const struct path *path,
 	do {
 		read_seqbegin_or_lock(&vnode->cb_lock, &seq);
 		generic_fillattr(&init_user_ns, inode, stat);
+		stat->chgattr = inode_peek_iversion_raw(inode);
+		stat->result_mask |= STATX_CHGATTR;
 		if (test_bit(AFS_VNODE_SILLY_DELETED, &vnode->flags) &&
 		    stat->nlink > 0)
 			stat->nlink -= 1;
-- 
2.37.1

[RFC PATCH 4/4] ceph: fill in the change attribute in statx requests

[Jeff Layton]

When statx requests the change attribute, request the full gamut of caps
(similarly to how ctime is handled). When the change attribute seems to
be valid, return it in the chgattr field.

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

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 56c53ab3618e..fb2ed85f9083 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -2408,10 +2408,10 @@ static int statx_to_caps(u32 want, umode_t mode)
 {
 	int mask = 0;
 
-	if (want & (STATX_MODE|STATX_UID|STATX_GID|STATX_CTIME|STATX_BTIME))
+	if (want & (STATX_MODE|STATX_UID|STATX_GID|STATX_CTIME|STATX_BTIME|STATX_CHGATTR))
 		mask |= CEPH_CAP_AUTH_SHARED;
 
-	if (want & (STATX_NLINK|STATX_CTIME)) {
+	if (want & (STATX_NLINK|STATX_CTIME|STATX_CHGATTR)) {
 		/*
 		 * The link count for directories depends on inode->i_subdirs,
 		 * and that is only updated when Fs caps are held.
@@ -2422,11 +2422,10 @@ static int statx_to_caps(u32 want, umode_t mode)
 			mask |= CEPH_CAP_LINK_SHARED;
 	}
 
-	if (want & (STATX_ATIME|STATX_MTIME|STATX_CTIME|STATX_SIZE|
-		    STATX_BLOCKS))
+	if (want & (STATX_ATIME|STATX_MTIME|STATX_CTIME|STATX_SIZE| STATX_BLOCKS|STATX_CHGATTR))
 		mask |= CEPH_CAP_FILE_SHARED;
 
-	if (want & (STATX_CTIME))
+	if (want & (STATX_CTIME|STATX_CHGATTR))
 		mask |= CEPH_CAP_XATTR_SHARED;
 
 	return mask;
@@ -2468,6 +2467,11 @@ int ceph_getattr(struct user_namespace *mnt_userns, const struct path *path,
 		valid_mask |= STATX_BTIME;
 	}
 
+	if (request_mask & STATX_CHGATTR) {
+		stat->chgattr = inode_peek_iversion_raw(inode);
+		valid_mask |= STATX_CHGATTR;
+	}
+
 	if (ceph_snap(inode) == CEPH_NOSNAP)
 		stat->dev = inode->i_sb->s_dev;
 	else
-- 
2.37.1

RE: [RFC PATCH 0/4] vfs: allow querying i_version via statx

[Frank Filz]

> Recently I posted a patch to turn on the i_version counter unconditionally
in
> ext4, and Lukas rightly pointed out that we don't currently have an easy
way to
> validate its functionality. You can fetch it via NFS (and see it in
network traces),
> but there's no way to get to it from userland.
> 
> Besides testing, this may also be of use for userland NFS servers, or by
any
> program that wants to accurately check for file changes, and not be
subject to
> mtime granularity problems.

This would definitely be useful for NFS Ganesha.

Thanks

Frank

> Comments and suggestions welcome. I'm not 100% convinced that this is a
> great idea, but we've had people ask for it before and it seems like a
reasonable
> thing to provide.
> 
> Jeff Layton (4):
>   vfs: report change attribute in statx for IS_I_VERSION inodes
>   nfs: report the change attribute if requested
>   afs: fill out change attribute in statx replies
>   ceph: fill in the change attribute in statx requests
> 
>  fs/afs/inode.c            |  2 ++
>  fs/ceph/inode.c           | 14 +++++++++-----
>  fs/nfs/inode.c            |  3 +++
>  fs/stat.c                 |  7 +++++++
>  include/linux/stat.h      |  1 +
>  include/uapi/linux/stat.h |  3 ++-
>  samples/vfs/test-statx.c  |  4 +++-
>  7 files changed, 27 insertions(+), 7 deletions(-)
> 
> --
> 2.37.1

Re: [RFC PATCH 1/4] vfs: report change attribute in statx for IS_I_VERSION inodes

[David Howells]

Jeff Layton <jlayton@kernel.org> wrote:

> +	__u64	stx_chgattr;	/* Inode change attribute */

Why not call it stx_change_attr?

David

Re: [RFC PATCH 1/4] vfs: report change attribute in statx for IS_I_VERSION inodes

[Jeff Layton]

On Fri, 2022-08-05 at 20:17 +0100, David Howells wrote:
> Jeff Layton <jlayton@kernel.org> wrote:
> 
> > +	__u64	stx_chgattr;	/* Inode change attribute */
> 
> Why not call it stx_change_attr?
> 
> David
> 

Ok. I'm open to suggestions on the naming in general. It's a small
patchset so it's not hard to change that.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH 1/4] vfs: report change attribute in statx for IS_I_VERSION inodes

[Dave Chinner]

On Fri, Aug 05, 2022 at 02:35:40PM -0400, Jeff Layton wrote:
> From: Jeff Layton <jlayton@redhat.com>
> 
> Claim one of the spare fields in struct statx to hold a 64-bit change
> attribute. When statx requests this attribute, do an
> inode_query_iversion and fill the result in the field.
> 
> Also update the test-statx.c program to fetch the change attribute as
> well.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/stat.c                 | 7 +++++++
>  include/linux/stat.h      | 1 +
>  include/uapi/linux/stat.h | 3 ++-
>  samples/vfs/test-statx.c  | 4 +++-
>  4 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/stat.c b/fs/stat.c
> index 9ced8860e0f3..976e0a59ab23 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -17,6 +17,7 @@
>  #include <linux/syscalls.h>
>  #include <linux/pagemap.h>
>  #include <linux/compat.h>
> +#include <linux/iversion.h>
>  
>  #include <linux/uaccess.h>
>  #include <asm/unistd.h>
> @@ -118,6 +119,11 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
>  	stat->attributes_mask |= (STATX_ATTR_AUTOMOUNT |
>  				  STATX_ATTR_DAX);
>  
> +	if ((request_mask & STATX_CHGATTR) && IS_I_VERSION(inode)) {
> +		stat->result_mask |= STATX_CHGATTR;
> +		stat->chgattr = inode_query_iversion(inode);
> +	}

If you're going to add generic support for it, shouldn't there be a
generic test in fstests that ensures that filesystems that advertise
STATX_CHGATTR support actually behave correctly? Including across
mounts, and most importantly, that it is made properly stable by
fsync?

i.e. what good is this if different filesystems have random quirks
that mean it can't be relied on by userspace to tell it changes have
occurred?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC PATCH 1/4] vfs: report change attribute in statx for IS_I_VERSION inodes

[Jeff Layton]

On Sat, 2022-08-06 at 08:01 +1000, Dave Chinner wrote:
> On Fri, Aug 05, 2022 at 02:35:40PM -0400, Jeff Layton wrote:
> > From: Jeff Layton <jlayton@redhat.com>
> > 
> > Claim one of the spare fields in struct statx to hold a 64-bit change
> > attribute. When statx requests this attribute, do an
> > inode_query_iversion and fill the result in the field.
> > 
> > Also update the test-statx.c program to fetch the change attribute as
> > well.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/stat.c                 | 7 +++++++
> >  include/linux/stat.h      | 1 +
> >  include/uapi/linux/stat.h | 3 ++-
> >  samples/vfs/test-statx.c  | 4 +++-
> >  4 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/stat.c b/fs/stat.c
> > index 9ced8860e0f3..976e0a59ab23 100644
> > --- a/fs/stat.c
> > +++ b/fs/stat.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/syscalls.h>
> >  #include <linux/pagemap.h>
> >  #include <linux/compat.h>
> > +#include <linux/iversion.h>
> >  
> >  #include <linux/uaccess.h>
> >  #include <asm/unistd.h>
> > @@ -118,6 +119,11 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
> >  	stat->attributes_mask |= (STATX_ATTR_AUTOMOUNT |
> >  				  STATX_ATTR_DAX);
> >  
> > +	if ((request_mask & STATX_CHGATTR) && IS_I_VERSION(inode)) {
> > +		stat->result_mask |= STATX_CHGATTR;
> > +		stat->chgattr = inode_query_iversion(inode);
> > +	}
> 
> If you're going to add generic support for it, shouldn't there be a
> generic test in fstests that ensures that filesystems that advertise
> STATX_CHGATTR support actually behave correctly? Including across
> mounts, and most importantly, that it is made properly stable by
> fsync?
> 
> i.e. what good is this if different filesystems have random quirks
> that mean it can't be relied on by userspace to tell it changes have
> occurred?

Absolutely. Being able to better test the i_version field for consistent
behavior is a primary goal. I haven't yet written any yet, but we'd
definitely want something in xfstests if we decide this is worthwhile.
-- 
Jeff Layton <jlayton@redhat.com>

Re: [RFC PATCH 1/4] vfs: report change attribute in statx for IS_I_VERSION inodes

[Xiubo Li]

On 8/6/22 2:35 AM, Jeff Layton wrote:
> From: Jeff Layton <jlayton@redhat.com>
>
> Claim one of the spare fields in struct statx to hold a 64-bit change
> attribute. When statx requests this attribute, do an
> inode_query_iversion and fill the result in the field.
>
> Also update the test-statx.c program to fetch the change attribute as
> well.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>   fs/stat.c                 | 7 +++++++
>   include/linux/stat.h      | 1 +
>   include/uapi/linux/stat.h | 3 ++-
>   samples/vfs/test-statx.c  | 4 +++-
>   4 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/fs/stat.c b/fs/stat.c
> index 9ced8860e0f3..976e0a59ab23 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -17,6 +17,7 @@
>   #include <linux/syscalls.h>
>   #include <linux/pagemap.h>
>   #include <linux/compat.h>
> +#include <linux/iversion.h>
>   
>   #include <linux/uaccess.h>
>   #include <asm/unistd.h>
> @@ -118,6 +119,11 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
>   	stat->attributes_mask |= (STATX_ATTR_AUTOMOUNT |
>   				  STATX_ATTR_DAX);
>   
> +	if ((request_mask & STATX_CHGATTR) && IS_I_VERSION(inode)) {
> +		stat->result_mask |= STATX_CHGATTR;
> +		stat->chgattr = inode_query_iversion(inode);
> +	}
> +
>   	mnt_userns = mnt_user_ns(path->mnt);
>   	if (inode->i_op->getattr)
>   		return inode->i_op->getattr(mnt_userns, path, stat,
> @@ -611,6 +617,7 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
>   	tmp.stx_dev_major = MAJOR(stat->dev);
>   	tmp.stx_dev_minor = MINOR(stat->dev);
>   	tmp.stx_mnt_id = stat->mnt_id;
> +	tmp.stx_chgattr = stat->chgattr;
>   
>   	return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;
>   }
> diff --git a/include/linux/stat.h b/include/linux/stat.h
> index 7df06931f25d..4a17887472f6 100644
> --- a/include/linux/stat.h
> +++ b/include/linux/stat.h
> @@ -50,6 +50,7 @@ struct kstat {
>   	struct timespec64 btime;			/* File creation time */
>   	u64		blocks;
>   	u64		mnt_id;
> +	u64		chgattr;
>   };
>   
>   #endif
> diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
> index 1500a0f58041..b45243a0fbc5 100644
> --- a/include/uapi/linux/stat.h
> +++ b/include/uapi/linux/stat.h
> @@ -124,7 +124,7 @@ struct statx {
>   	__u32	stx_dev_minor;
>   	/* 0x90 */
>   	__u64	stx_mnt_id;
> -	__u64	__spare2;
> +	__u64	stx_chgattr;	/* Inode change attribute */
>   	/* 0xa0 */
>   	__u64	__spare3[12];	/* Spare space for future expansion */
>   	/* 0x100 */
> @@ -152,6 +152,7 @@ struct statx {
>   #define STATX_BASIC_STATS	0x000007ffU	/* The stuff in the normal stat struct */
>   #define STATX_BTIME		0x00000800U	/* Want/got stx_btime */
>   #define STATX_MNT_ID		0x00001000U	/* Got stx_mnt_id */
> +#define STATX_CHGATTR		0x00002000U	/* Want/git stx_chgattr */

s/git/get/ ?

>   
>   #define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */
>   
> diff --git a/samples/vfs/test-statx.c b/samples/vfs/test-statx.c
> index 49c7a46cee07..767208d2f564 100644
> --- a/samples/vfs/test-statx.c
> +++ b/samples/vfs/test-statx.c
> @@ -109,6 +109,8 @@ static void dump_statx(struct statx *stx)
>   		printf(" Inode: %-11llu", (unsigned long long) stx->stx_ino);
>   	if (stx->stx_mask & STATX_NLINK)
>   		printf(" Links: %-5u", stx->stx_nlink);
> +	if (stx->stx_mask & STATX_CHGATTR)
> +		printf(" Change Attr: 0x%llx", stx->stx_chgattr);
>   	if (stx->stx_mask & STATX_TYPE) {
>   		switch (stx->stx_mode & S_IFMT) {
>   		case S_IFBLK:
> @@ -218,7 +220,7 @@ int main(int argc, char **argv)
>   	struct statx stx;
>   	int ret, raw = 0, atflag = AT_SYMLINK_NOFOLLOW;
>   
> -	unsigned int mask = STATX_BASIC_STATS | STATX_BTIME;
> +	unsigned int mask = STATX_BASIC_STATS | STATX_BTIME | STATX_CHGATTR;
>   
>   	for (argv++; *argv; argv++) {
>   		if (strcmp(*argv, "-F") == 0) {

Re: [RFC PATCH 1/4] vfs: report change attribute in statx for IS_I_VERSION inodes

[Jeff Layton]

On Mon, 2022-08-08 at 10:09 +0800, Xiubo Li wrote:
> On 8/6/22 2:35 AM, Jeff Layton wrote:
> > From: Jeff Layton <jlayton@redhat.com>
> > 
> > Claim one of the spare fields in struct statx to hold a 64-bit change
> > attribute. When statx requests this attribute, do an
> > inode_query_iversion and fill the result in the field.
> > 
> > Also update the test-statx.c program to fetch the change attribute as
> > well.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >   fs/stat.c                 | 7 +++++++
> >   include/linux/stat.h      | 1 +
> >   include/uapi/linux/stat.h | 3 ++-
> >   samples/vfs/test-statx.c  | 4 +++-
> >   4 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/stat.c b/fs/stat.c
> > index 9ced8860e0f3..976e0a59ab23 100644
> > --- a/fs/stat.c
> > +++ b/fs/stat.c
> > @@ -17,6 +17,7 @@
> >   #include <linux/syscalls.h>
> >   #include <linux/pagemap.h>
> >   #include <linux/compat.h>
> > +#include <linux/iversion.h>
> >   
> >   #include <linux/uaccess.h>
> >   #include <asm/unistd.h>
> > @@ -118,6 +119,11 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
> >   	stat->attributes_mask |= (STATX_ATTR_AUTOMOUNT |
> >   				  STATX_ATTR_DAX);
> >   
> > +	if ((request_mask & STATX_CHGATTR) && IS_I_VERSION(inode)) {
> > +		stat->result_mask |= STATX_CHGATTR;
> > +		stat->chgattr = inode_query_iversion(inode);
> > +	}
> > +
> >   	mnt_userns = mnt_user_ns(path->mnt);
> >   	if (inode->i_op->getattr)
> >   		return inode->i_op->getattr(mnt_userns, path, stat,
> > @@ -611,6 +617,7 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
> >   	tmp.stx_dev_major = MAJOR(stat->dev);
> >   	tmp.stx_dev_minor = MINOR(stat->dev);
> >   	tmp.stx_mnt_id = stat->mnt_id;
> > +	tmp.stx_chgattr = stat->chgattr;
> >   
> >   	return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;
> >   }
> > diff --git a/include/linux/stat.h b/include/linux/stat.h
> > index 7df06931f25d..4a17887472f6 100644
> > --- a/include/linux/stat.h
> > +++ b/include/linux/stat.h
> > @@ -50,6 +50,7 @@ struct kstat {
> >   	struct timespec64 btime;			/* File creation time */
> >   	u64		blocks;
> >   	u64		mnt_id;
> > +	u64		chgattr;
> >   };
> >   
> >   #endif
> > diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
> > index 1500a0f58041..b45243a0fbc5 100644
> > --- a/include/uapi/linux/stat.h
> > +++ b/include/uapi/linux/stat.h
> > @@ -124,7 +124,7 @@ struct statx {
> >   	__u32	stx_dev_minor;
> >   	/* 0x90 */
> >   	__u64	stx_mnt_id;
> > -	__u64	__spare2;
> > +	__u64	stx_chgattr;	/* Inode change attribute */
> >   	/* 0xa0 */
> >   	__u64	__spare3[12];	/* Spare space for future expansion */
> >   	/* 0x100 */
> > @@ -152,6 +152,7 @@ struct statx {
> >   #define STATX_BASIC_STATS	0x000007ffU	/* The stuff in the normal stat struct */
> >   #define STATX_BTIME		0x00000800U	/* Want/got stx_btime */
> >   #define STATX_MNT_ID		0x00001000U	/* Got stx_mnt_id */
> > +#define STATX_CHGATTR		0x00002000U	/* Want/git stx_chgattr */
> 
> s/git/get/ ?
> 

Muscle-memory typo. Fixed in my tree.

> >   
> >   #define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */
> >   
> > diff --git a/samples/vfs/test-statx.c b/samples/vfs/test-statx.c
> > index 49c7a46cee07..767208d2f564 100644
> > --- a/samples/vfs/test-statx.c
> > +++ b/samples/vfs/test-statx.c
> > @@ -109,6 +109,8 @@ static void dump_statx(struct statx *stx)
> >   		printf(" Inode: %-11llu", (unsigned long long) stx->stx_ino);
> >   	if (stx->stx_mask & STATX_NLINK)
> >   		printf(" Links: %-5u", stx->stx_nlink);
> > +	if (stx->stx_mask & STATX_CHGATTR)
> > +		printf(" Change Attr: 0x%llx", stx->stx_chgattr);
> >   	if (stx->stx_mask & STATX_TYPE) {
> >   		switch (stx->stx_mode & S_IFMT) {
> >   		case S_IFBLK:
> > @@ -218,7 +220,7 @@ int main(int argc, char **argv)
> >   	struct statx stx;
> >   	int ret, raw = 0, atflag = AT_SYMLINK_NOFOLLOW;
> >   
> > -	unsigned int mask = STATX_BASIC_STATS | STATX_BTIME;
> > +	unsigned int mask = STATX_BASIC_STATS | STATX_BTIME | STATX_CHGATTR;
> >   
> >   	for (argv++; *argv; argv++) {
> >   		if (strcmp(*argv, "-F") == 0) {
> 

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

Re: [RFC PATCH 4/4] ceph: fill in the change attribute in statx requests

[Xiubo Li]

On 8/6/22 2:35 AM, Jeff Layton wrote:
> When statx requests the change attribute, request the full gamut of caps
> (similarly to how ctime is handled). When the change attribute seems to
> be valid, return it in the chgattr field.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>   fs/ceph/inode.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 56c53ab3618e..fb2ed85f9083 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -2408,10 +2408,10 @@ static int statx_to_caps(u32 want, umode_t mode)
>   {
>   	int mask = 0;
>   
> -	if (want & (STATX_MODE|STATX_UID|STATX_GID|STATX_CTIME|STATX_BTIME))
> +	if (want & (STATX_MODE|STATX_UID|STATX_GID|STATX_CTIME|STATX_BTIME|STATX_CHGATTR))
>   		mask |= CEPH_CAP_AUTH_SHARED;
>   
> -	if (want & (STATX_NLINK|STATX_CTIME)) {
> +	if (want & (STATX_NLINK|STATX_CTIME|STATX_CHGATTR)) {
>   		/*
>   		 * The link count for directories depends on inode->i_subdirs,
>   		 * and that is only updated when Fs caps are held.
> @@ -2422,11 +2422,10 @@ static int statx_to_caps(u32 want, umode_t mode)
>   			mask |= CEPH_CAP_LINK_SHARED;
>   	}
>   
> -	if (want & (STATX_ATIME|STATX_MTIME|STATX_CTIME|STATX_SIZE|
> -		    STATX_BLOCKS))
> +	if (want & (STATX_ATIME|STATX_MTIME|STATX_CTIME|STATX_SIZE| STATX_BLOCKS|STATX_CHGATTR))
>   		mask |= CEPH_CAP_FILE_SHARED;
>   
> -	if (want & (STATX_CTIME))
> +	if (want & (STATX_CTIME|STATX_CHGATTR))
>   		mask |= CEPH_CAP_XATTR_SHARED;
>   
>   	return mask;
> @@ -2468,6 +2467,11 @@ int ceph_getattr(struct user_namespace *mnt_userns, const struct path *path,
>   		valid_mask |= STATX_BTIME;
>   	}
>   
> +	if (request_mask & STATX_CHGATTR) {
> +		stat->chgattr = inode_peek_iversion_raw(inode);
> +		valid_mask |= STATX_CHGATTR;
> +	}
> +
>   	if (ceph_snap(inode) == CEPH_NOSNAP)
>   		stat->dev = inode->i_sb->s_dev;
>   	else

Reviewed-by: Xiubo Li <xiubli@redhat.com>

Re: [RFC PATCH 1/4] vfs: report change attribute in statx for IS_I_VERSION inodes

[Jeff Layton]

On Fri, 2022-08-05 at 18:06 -0400, Jeff Layton wrote:
> On Sat, 2022-08-06 at 08:01 +1000, Dave Chinner wrote:
> > On Fri, Aug 05, 2022 at 02:35:40PM -0400, Jeff Layton wrote:
> > > From: Jeff Layton <jlayton@redhat.com>
> > > 
> > > Claim one of the spare fields in struct statx to hold a 64-bit change
> > > attribute. When statx requests this attribute, do an
> > > inode_query_iversion and fill the result in the field.
> > > 
> > > Also update the test-statx.c program to fetch the change attribute as
> > > well.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/stat.c                 | 7 +++++++
> > >  include/linux/stat.h      | 1 +
> > >  include/uapi/linux/stat.h | 3 ++-
> > >  samples/vfs/test-statx.c  | 4 +++-
> > >  4 files changed, 13 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/stat.c b/fs/stat.c
> > > index 9ced8860e0f3..976e0a59ab23 100644
> > > --- a/fs/stat.c
> > > +++ b/fs/stat.c
> > > @@ -17,6 +17,7 @@
> > >  #include <linux/syscalls.h>
> > >  #include <linux/pagemap.h>
> > >  #include <linux/compat.h>
> > > +#include <linux/iversion.h>
> > >  
> > >  #include <linux/uaccess.h>
> > >  #include <asm/unistd.h>
> > > @@ -118,6 +119,11 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
> > >  	stat->attributes_mask |= (STATX_ATTR_AUTOMOUNT |
> > >  				  STATX_ATTR_DAX);
> > >  
> > > +	if ((request_mask & STATX_CHGATTR) && IS_I_VERSION(inode)) {
> > > +		stat->result_mask |= STATX_CHGATTR;
> > > +		stat->chgattr = inode_query_iversion(inode);
> > > +	}
> > 
> > If you're going to add generic support for it, shouldn't there be a
> > generic test in fstests that ensures that filesystems that advertise
> > STATX_CHGATTR support actually behave correctly? Including across
> > mounts, and most importantly, that it is made properly stable by
> > fsync?
> > 
> > i.e. what good is this if different filesystems have random quirks
> > that mean it can't be relied on by userspace to tell it changes have
> > occurred?
> 
> Absolutely. Being able to better test the i_version field for consistent
> behavior is a primary goal. I haven't yet written any yet, but we'd
> definitely want something in xfstests if we decide this is worthwhile.

I started writing some tests for this today, and hit a bit of a chicken-
and-egg problem:

I'd prefer to use xfs_io for easier maintainability, but the STATX_*
constants are defined via UAPI header. Older kernels don't have them and
old xfs_io programs don't understand or report this value.

Should I just write a one-off binary program for xfstests to fetch this
value for now, or are we better off merging the patchset first, and then
fix xfs_io and then write the tests using the updated xfs_io program?

-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH 1/4] vfs: report change attribute in statx for IS_I_VERSION inodes

[Darrick J. Wong]

On Mon, Aug 08, 2022 at 09:19:05AM -0400, Jeff Layton wrote:
> On Fri, 2022-08-05 at 18:06 -0400, Jeff Layton wrote:
> > On Sat, 2022-08-06 at 08:01 +1000, Dave Chinner wrote:
> > > On Fri, Aug 05, 2022 at 02:35:40PM -0400, Jeff Layton wrote:
> > > > From: Jeff Layton <jlayton@redhat.com>
> > > > 
> > > > Claim one of the spare fields in struct statx to hold a 64-bit change
> > > > attribute. When statx requests this attribute, do an
> > > > inode_query_iversion and fill the result in the field.
> > > > 
> > > > Also update the test-statx.c program to fetch the change attribute as
> > > > well.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  fs/stat.c                 | 7 +++++++
> > > >  include/linux/stat.h      | 1 +
> > > >  include/uapi/linux/stat.h | 3 ++-
> > > >  samples/vfs/test-statx.c  | 4 +++-
> > > >  4 files changed, 13 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/fs/stat.c b/fs/stat.c
> > > > index 9ced8860e0f3..976e0a59ab23 100644
> > > > --- a/fs/stat.c
> > > > +++ b/fs/stat.c
> > > > @@ -17,6 +17,7 @@
> > > >  #include <linux/syscalls.h>
> > > >  #include <linux/pagemap.h>
> > > >  #include <linux/compat.h>
> > > > +#include <linux/iversion.h>
> > > >  
> > > >  #include <linux/uaccess.h>
> > > >  #include <asm/unistd.h>
> > > > @@ -118,6 +119,11 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
> > > >  	stat->attributes_mask |= (STATX_ATTR_AUTOMOUNT |
> > > >  				  STATX_ATTR_DAX);
> > > >  
> > > > +	if ((request_mask & STATX_CHGATTR) && IS_I_VERSION(inode)) {
> > > > +		stat->result_mask |= STATX_CHGATTR;
> > > > +		stat->chgattr = inode_query_iversion(inode);
> > > > +	}
> > > 
> > > If you're going to add generic support for it, shouldn't there be a
> > > generic test in fstests that ensures that filesystems that advertise
> > > STATX_CHGATTR support actually behave correctly? Including across
> > > mounts, and most importantly, that it is made properly stable by
> > > fsync?
> > > 
> > > i.e. what good is this if different filesystems have random quirks
> > > that mean it can't be relied on by userspace to tell it changes have
> > > occurred?
> > 
> > Absolutely. Being able to better test the i_version field for consistent
> > behavior is a primary goal. I haven't yet written any yet, but we'd
> > definitely want something in xfstests if we decide this is worthwhile.
> 
> I started writing some tests for this today, and hit a bit of a chicken-
> and-egg problem:
> 
> I'd prefer to use xfs_io for easier maintainability, but the STATX_*
> constants are defined via UAPI header. Older kernels don't have them and
> old xfs_io programs don't understand or report this value.
> 
> Should I just write a one-off binary program for xfstests to fetch this
> value for now, or are we better off merging the patchset first, and then
> fix xfs_io and then write the tests using the updated xfs_io program?

What we've done in the past to support new APIs until they land in
kernel headers is:

Add an autoconf macro to decide if the system header files are recent
enough to support whatever functionality is needed by xfs_io;

Modify the build system to #define OVERRIDE_FUBAR if the system headers
aren't new enough to have FUBAR; and

Modify (or create) the relevant header file to override the system
header definitions as needed to support building the relevant pieces of
code.  A year or so after the functionality lands, we can then remove
the overrides, or just leave them in place until the next time we need
it.

For example, Eric Biggers wanted to teach the fscrypt commands to use a
new feature he was adding to an existing API, so he AC_DEFUN'd a macro
that checks to see if the system linux/fs.h *does not* define a
structure containing the desired field.  If this is the case, it sets
need_internal_fscrypt_add_key_arg=yes.

AC_DEFUN([AC_NEED_INTERNAL_FSCRYPT_ADD_KEY_ARG],
  [
    AC_CHECK_TYPE(struct fscrypt_add_key_arg,
      [
        AC_CHECK_MEMBER(struct fscrypt_add_key_arg.key_id,
          ,
          need_internal_fscrypt_add_key_arg=yes,
          [#include <linux/fs.h>]
        )
      ],,
      [#include <linux/fs.h>]
    )
    AC_SUBST(need_internal_fscrypt_add_key_arg)
  ])

This macro is called from configure.ac.

Next, include/builddefs.in was modified to include the selected value in
the make variables:

NEED_INTERNAL_FSCRYPT_ADD_KEY_ARG = @need_internal_fscrypt_add_key_arg@

And then the shouty variable is used in the same file to set a compiler
define:

ifeq ($(NEED_INTERNAL_FSCRYPT_ADD_KEY_ARG),yes)
PCFLAGS+= -DOVERRIDE_SYSTEM_FSCRYPT_ADD_KEY_ARG
endif

Then io/encrypt.c does the following to move the system's definition of
struct fscrypt_add_key_arg out of the way...

#ifdef OVERRIDE_SYSTEM_FSCRYPT_ADD_KEY_ARG
#  define fscrypt_add_key_arg sys_fscrypt_add_key_arg
#endif
#include <linux/fs.h>  /* via io.h -> xfs.h -> xfs/linux.h */

...so that the file can provide its own definition further down:

/*
 * Since the key_id field was added later than struct
 * fscrypt_add_key_arg itself, we may need to override the system
 * definition to get that field.
 */
#if !defined(FS_IOC_ADD_ENCRYPTION_KEY) || \
	defined(OVERRIDE_SYSTEM_FSCRYPT_ADD_KEY_ARG)
#undef fscrypt_add_key_arg
struct fscrypt_add_key_arg {
	struct fscrypt_key_specifier key_spec;
	__u32 raw_size;
	__u32 key_id;
	__u32 __reserved[8];
	__u8 raw[];
};
#endif

Obviously, #defined constants are much easier to override:

#ifndef FS_IOC_ADD_ENCRYPTION_KEY
#  define FS_IOC_ADD_ENCRYPTION_KEY		_IOWR('f', 23, struct fscrypt_add_key_arg)
#endif

But I went for the full solution since you're adding fields to struct
statx.

Also, whatever you do, don't put your overrides in any file that gets
exported via xfslibs-dev, because those files get put in /usr/include.
We just learned that lesson the hard way with MAP_SYNC.

--D

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

Re: [RFC PATCH 1/4] vfs: report change attribute in statx for IS_I_VERSION inodes

[Jeff Layton]

On Tue, 2022-08-09 at 08:33 -0700, Darrick J. Wong wrote:
> On Mon, Aug 08, 2022 at 09:19:05AM -0400, Jeff Layton wrote:
> > On Fri, 2022-08-05 at 18:06 -0400, Jeff Layton wrote:
> > > On Sat, 2022-08-06 at 08:01 +1000, Dave Chinner wrote:
> > > > On Fri, Aug 05, 2022 at 02:35:40PM -0400, Jeff Layton wrote:
> > > > > From: Jeff Layton <jlayton@redhat.com>
> > > > > 
> > > > > Claim one of the spare fields in struct statx to hold a 64-bit change
> > > > > attribute. When statx requests this attribute, do an
> > > > > inode_query_iversion and fill the result in the field.
> > > > > 
> > > > > Also update the test-statx.c program to fetch the change attribute as
> > > > > well.
> > > > > 
> > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > ---
> > > > >  fs/stat.c                 | 7 +++++++
> > > > >  include/linux/stat.h      | 1 +
> > > > >  include/uapi/linux/stat.h | 3 ++-
> > > > >  samples/vfs/test-statx.c  | 4 +++-
> > > > >  4 files changed, 13 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/fs/stat.c b/fs/stat.c
> > > > > index 9ced8860e0f3..976e0a59ab23 100644
> > > > > --- a/fs/stat.c
> > > > > +++ b/fs/stat.c
> > > > > @@ -17,6 +17,7 @@
> > > > >  #include <linux/syscalls.h>
> > > > >  #include <linux/pagemap.h>
> > > > >  #include <linux/compat.h>
> > > > > +#include <linux/iversion.h>
> > > > >  
> > > > >  #include <linux/uaccess.h>
> > > > >  #include <asm/unistd.h>
> > > > > @@ -118,6 +119,11 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
> > > > >  	stat->attributes_mask |= (STATX_ATTR_AUTOMOUNT |
> > > > >  				  STATX_ATTR_DAX);
> > > > >  
> > > > > +	if ((request_mask & STATX_CHGATTR) && IS_I_VERSION(inode)) {
> > > > > +		stat->result_mask |= STATX_CHGATTR;
> > > > > +		stat->chgattr = inode_query_iversion(inode);
> > > > > +	}
> > > > 
> > > > If you're going to add generic support for it, shouldn't there be a
> > > > generic test in fstests that ensures that filesystems that advertise
> > > > STATX_CHGATTR support actually behave correctly? Including across
> > > > mounts, and most importantly, that it is made properly stable by
> > > > fsync?
> > > > 
> > > > i.e. what good is this if different filesystems have random quirks
> > > > that mean it can't be relied on by userspace to tell it changes have
> > > > occurred?
> > > 
> > > Absolutely. Being able to better test the i_version field for consistent
> > > behavior is a primary goal. I haven't yet written any yet, but we'd
> > > definitely want something in xfstests if we decide this is worthwhile.
> > 
> > I started writing some tests for this today, and hit a bit of a chicken-
> > and-egg problem:
> > 
> > I'd prefer to use xfs_io for easier maintainability, but the STATX_*
> > constants are defined via UAPI header. Older kernels don't have them and
> > old xfs_io programs don't understand or report this value.
> > 
> > Should I just write a one-off binary program for xfstests to fetch this
> > value for now, or are we better off merging the patchset first, and then
> > fix xfs_io and then write the tests using the updated xfs_io program?
> 
> What we've done in the past to support new APIs until they land in
> kernel headers is:
> 
> Add an autoconf macro to decide if the system header files are recent
> enough to support whatever functionality is needed by xfs_io;
> 
> Modify the build system to #define OVERRIDE_FUBAR if the system headers
> aren't new enough to have FUBAR; and
> 
> Modify (or create) the relevant header file to override the system
> header definitions as needed to support building the relevant pieces of
> code.  A year or so after the functionality lands, we can then remove
> the overrides, or just leave them in place until the next time we need
> it.
> 
> For example, Eric Biggers wanted to teach the fscrypt commands to use a
> new feature he was adding to an existing API, so he AC_DEFUN'd a macro
> that checks to see if the system linux/fs.h *does not* define a
> structure containing the desired field.  If this is the case, it sets
> need_internal_fscrypt_add_key_arg=yes.
> 
> AC_DEFUN([AC_NEED_INTERNAL_FSCRYPT_ADD_KEY_ARG],
>   [
>     AC_CHECK_TYPE(struct fscrypt_add_key_arg,
>       [
>         AC_CHECK_MEMBER(struct fscrypt_add_key_arg.key_id,
>           ,
>           need_internal_fscrypt_add_key_arg=yes,
>           [#include <linux/fs.h>]
>         )
>       ],,
>       [#include <linux/fs.h>]
>     )
>     AC_SUBST(need_internal_fscrypt_add_key_arg)
>   ])
> 
> This macro is called from configure.ac.
> 
> Next, include/builddefs.in was modified to include the selected value in
> the make variables:
> 
> NEED_INTERNAL_FSCRYPT_ADD_KEY_ARG = @need_internal_fscrypt_add_key_arg@
> 
> And then the shouty variable is used in the same file to set a compiler
> define:
> 
> ifeq ($(NEED_INTERNAL_FSCRYPT_ADD_KEY_ARG),yes)
> PCFLAGS+= -DOVERRIDE_SYSTEM_FSCRYPT_ADD_KEY_ARG
> endif
> 
> Then io/encrypt.c does the following to move the system's definition of
> struct fscrypt_add_key_arg out of the way...
> 
> #ifdef OVERRIDE_SYSTEM_FSCRYPT_ADD_KEY_ARG
> #  define fscrypt_add_key_arg sys_fscrypt_add_key_arg
> #endif
> #include <linux/fs.h>  /* via io.h -> xfs.h -> xfs/linux.h */
> 
> ...so that the file can provide its own definition further down:
> 
> /*
>  * Since the key_id field was added later than struct
>  * fscrypt_add_key_arg itself, we may need to override the system
>  * definition to get that field.
>  */
> #if !defined(FS_IOC_ADD_ENCRYPTION_KEY) || \
> 	defined(OVERRIDE_SYSTEM_FSCRYPT_ADD_KEY_ARG)
> #undef fscrypt_add_key_arg
> struct fscrypt_add_key_arg {
> 	struct fscrypt_key_specifier key_spec;
> 	__u32 raw_size;
> 	__u32 key_id;
> 	__u32 __reserved[8];
> 	__u8 raw[];
> };
> #endif
> 

Darrick, thanks for the detailed instructions. They've been very
helpful! My approach is slightly different since xfsprogs already has a
statx.h. I'm just updating that to allow for overriding. I think I have
the autoconf part worked out.

I'm having a problem with the above though. I have this in statx.h:

#undef statx_timestamp
struct statx_timestamp {
        __s64   tv_sec;
        __s32   tv_nsec;
        __s32   __reserved;
};

...but when I go to build, I get this:

In file included from stat.c:11:
statx.h:60:8: error: redefinition of ‘struct statx_timestamp’
   60 | struct statx_timestamp {
      |        ^~~~~~~~~~~~~~~

...it seems like the "#undef statx_timestamp" isn't doing the right
thing. Is my syntax wrong?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH 1/4] vfs: report change attribute in statx for IS_I_VERSION inodes

[Jeff Layton]

On Tue, 2022-08-09 at 14:04 -0400, Jeff Layton wrote:
> On Tue, 2022-08-09 at 08:33 -0700, Darrick J. Wong wrote:
> > On Mon, Aug 08, 2022 at 09:19:05AM -0400, Jeff Layton wrote:
> > > On Fri, 2022-08-05 at 18:06 -0400, Jeff Layton wrote:
> > > > On Sat, 2022-08-06 at 08:01 +1000, Dave Chinner wrote:
> > > > > On Fri, Aug 05, 2022 at 02:35:40PM -0400, Jeff Layton wrote:
> > > > > > From: Jeff Layton <jlayton@redhat.com>
> > > > > > 
> > > > > > Claim one of the spare fields in struct statx to hold a 64-bit change
> > > > > > attribute. When statx requests this attribute, do an
> > > > > > inode_query_iversion and fill the result in the field.
> > > > > > 
> > > > > > Also update the test-statx.c program to fetch the change attribute as
> > > > > > well.
> > > > > > 
> > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > > ---
> > > > > >  fs/stat.c                 | 7 +++++++
> > > > > >  include/linux/stat.h      | 1 +
> > > > > >  include/uapi/linux/stat.h | 3 ++-
> > > > > >  samples/vfs/test-statx.c  | 4 +++-
> > > > > >  4 files changed, 13 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/stat.c b/fs/stat.c
> > > > > > index 9ced8860e0f3..976e0a59ab23 100644
> > > > > > --- a/fs/stat.c
> > > > > > +++ b/fs/stat.c
> > > > > > @@ -17,6 +17,7 @@
> > > > > >  #include <linux/syscalls.h>
> > > > > >  #include <linux/pagemap.h>
> > > > > >  #include <linux/compat.h>
> > > > > > +#include <linux/iversion.h>
> > > > > >  
> > > > > >  #include <linux/uaccess.h>
> > > > > >  #include <asm/unistd.h>
> > > > > > @@ -118,6 +119,11 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
> > > > > >  	stat->attributes_mask |= (STATX_ATTR_AUTOMOUNT |
> > > > > >  				  STATX_ATTR_DAX);
> > > > > >  
> > > > > > +	if ((request_mask & STATX_CHGATTR) && IS_I_VERSION(inode)) {
> > > > > > +		stat->result_mask |= STATX_CHGATTR;
> > > > > > +		stat->chgattr = inode_query_iversion(inode);
> > > > > > +	}
> > > > > 
> > > > > If you're going to add generic support for it, shouldn't there be a
> > > > > generic test in fstests that ensures that filesystems that advertise
> > > > > STATX_CHGATTR support actually behave correctly? Including across
> > > > > mounts, and most importantly, that it is made properly stable by
> > > > > fsync?
> > > > > 
> > > > > i.e. what good is this if different filesystems have random quirks
> > > > > that mean it can't be relied on by userspace to tell it changes have
> > > > > occurred?
> > > > 
> > > > Absolutely. Being able to better test the i_version field for consistent
> > > > behavior is a primary goal. I haven't yet written any yet, but we'd
> > > > definitely want something in xfstests if we decide this is worthwhile.
> > > 
> > > I started writing some tests for this today, and hit a bit of a chicken-
> > > and-egg problem:
> > > 
> > > I'd prefer to use xfs_io for easier maintainability, but the STATX_*
> > > constants are defined via UAPI header. Older kernels don't have them and
> > > old xfs_io programs don't understand or report this value.
> > > 
> > > Should I just write a one-off binary program for xfstests to fetch this
> > > value for now, or are we better off merging the patchset first, and then
> > > fix xfs_io and then write the tests using the updated xfs_io program?
> > 
> > What we've done in the past to support new APIs until they land in
> > kernel headers is:
> > 
> > Add an autoconf macro to decide if the system header files are recent
> > enough to support whatever functionality is needed by xfs_io;
> > 
> > Modify the build system to #define OVERRIDE_FUBAR if the system headers
> > aren't new enough to have FUBAR; and
> > 
> > Modify (or create) the relevant header file to override the system
> > header definitions as needed to support building the relevant pieces of
> > code.  A year or so after the functionality lands, we can then remove
> > the overrides, or just leave them in place until the next time we need
> > it.
> > 
> > For example, Eric Biggers wanted to teach the fscrypt commands to use a
> > new feature he was adding to an existing API, so he AC_DEFUN'd a macro
> > that checks to see if the system linux/fs.h *does not* define a
> > structure containing the desired field.  If this is the case, it sets
> > need_internal_fscrypt_add_key_arg=yes.
> > 
> > AC_DEFUN([AC_NEED_INTERNAL_FSCRYPT_ADD_KEY_ARG],
> >   [
> >     AC_CHECK_TYPE(struct fscrypt_add_key_arg,
> >       [
> >         AC_CHECK_MEMBER(struct fscrypt_add_key_arg.key_id,
> >           ,
> >           need_internal_fscrypt_add_key_arg=yes,
> >           [#include <linux/fs.h>]
> >         )
> >       ],,
> >       [#include <linux/fs.h>]
> >     )
> >     AC_SUBST(need_internal_fscrypt_add_key_arg)
> >   ])
> > 
> > This macro is called from configure.ac.
> > 
> > Next, include/builddefs.in was modified to include the selected value in
> > the make variables:
> > 
> > NEED_INTERNAL_FSCRYPT_ADD_KEY_ARG = @need_internal_fscrypt_add_key_arg@
> > 
> > And then the shouty variable is used in the same file to set a compiler
> > define:
> > 
> > ifeq ($(NEED_INTERNAL_FSCRYPT_ADD_KEY_ARG),yes)
> > PCFLAGS+= -DOVERRIDE_SYSTEM_FSCRYPT_ADD_KEY_ARG
> > endif
> > 
> > Then io/encrypt.c does the following to move the system's definition of
> > struct fscrypt_add_key_arg out of the way...
> > 
> > #ifdef OVERRIDE_SYSTEM_FSCRYPT_ADD_KEY_ARG
> > #  define fscrypt_add_key_arg sys_fscrypt_add_key_arg
> > #endif
> > #include <linux/fs.h>  /* via io.h -> xfs.h -> xfs/linux.h */
> > 
> > ...so that the file can provide its own definition further down:
> > 
> > /*
> >  * Since the key_id field was added later than struct
> >  * fscrypt_add_key_arg itself, we may need to override the system
> >  * definition to get that field.
> >  */
> > #if !defined(FS_IOC_ADD_ENCRYPTION_KEY) || \
> > 	defined(OVERRIDE_SYSTEM_FSCRYPT_ADD_KEY_ARG)
> > #undef fscrypt_add_key_arg
> > struct fscrypt_add_key_arg {
> > 	struct fscrypt_key_specifier key_spec;
> > 	__u32 raw_size;
> > 	__u32 key_id;
> > 	__u32 __reserved[8];
> > 	__u8 raw[];
> > };
> > #endif
> > 
> 
> Darrick, thanks for the detailed instructions. They've been very
> helpful! My approach is slightly different since xfsprogs already has a
> statx.h. I'm just updating that to allow for overriding. I think I have
> the autoconf part worked out.
> 
> I'm having a problem with the above though. I have this in statx.h:
> 
> #undef statx_timestamp
> struct statx_timestamp {
>         __s64   tv_sec;
>         __s32   tv_nsec;
>         __s32   __reserved;
> };
> 
> ...but when I go to build, I get this:
> 
> In file included from stat.c:11:
> statx.h:60:8: error: redefinition of ‘struct statx_timestamp’
>    60 | struct statx_timestamp {
>       |        ^~~~~~~~~~~~~~~
> 
> ...it seems like the "#undef statx_timestamp" isn't doing the right
> thing. Is my syntax wrong?

I think I got it sorted. I had to rename the structs and then use the
preprocessor to override the old struct names in the code. It's a little
ugly, but it works:

struct statx_timestamp_internal {
        __s64   tv_sec;
        __s32   tv_nsec;
        __s32   __reserved;
};
#define statx_timestamp statx_timestamp_internal

I just sent a patch to linux-xfs that seems to do the right thing.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH 1/4] vfs: report change attribute in statx for IS_I_VERSION inodes

[JunChao Sun]

On Sat, Aug 6, 2022 at 2:37 AM Jeff Layton <jlayton@kernel.org> wrote:
>
> From: Jeff Layton <jlayton@redhat.com>
>
> Claim one of the spare fields in struct statx to hold a 64-bit change
>>
>> attribute. When statx requests this attribute, do an
>> inode_query_iversion and fill the result in the field.

I guess, is it better to update the corresponding part of the man-pages...?
>
>
> Also update the test-statx.c program to fetch the change attribute as
> well.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/stat.c                 | 7 +++++++
>  include/linux/stat.h      | 1 +
>  include/uapi/linux/stat.h | 3 ++-
>  samples/vfs/test-statx.c  | 4 +++-
>  4 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/fs/stat.c b/fs/stat.c
> index 9ced8860e0f3..976e0a59ab23 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -17,6 +17,7 @@
>  #include <linux/syscalls.h>
>  #include <linux/pagemap.h>
>  #include <linux/compat.h>
> +#include <linux/iversion.h>
>
>  #include <linux/uaccess.h>
>  #include <asm/unistd.h>
> @@ -118,6 +119,11 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
>         stat->attributes_mask |= (STATX_ATTR_AUTOMOUNT |
>                                   STATX_ATTR_DAX);
>
> +       if ((request_mask & STATX_CHGATTR) && IS_I_VERSION(inode)) {
> +               stat->result_mask |= STATX_CHGATTR;
> +               stat->chgattr = inode_query_iversion(inode);
> +       }
> +
>         mnt_userns = mnt_user_ns(path->mnt);
>         if (inode->i_op->getattr)
>                 return inode->i_op->getattr(mnt_userns, path, stat,
> @@ -611,6 +617,7 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
>         tmp.stx_dev_major = MAJOR(stat->dev);
>         tmp.stx_dev_minor = MINOR(stat->dev);
>         tmp.stx_mnt_id = stat->mnt_id;
> +       tmp.stx_chgattr = stat->chgattr;
>
>         return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;
>  }
> diff --git a/include/linux/stat.h b/include/linux/stat.h
> index 7df06931f25d..4a17887472f6 100644
> --- a/include/linux/stat.h
> +++ b/include/linux/stat.h
> @@ -50,6 +50,7 @@ struct kstat {
>         struct timespec64 btime;                        /* File creation time */
>         u64             blocks;
>         u64             mnt_id;
> +       u64             chgattr;
>  };
>
>  #endif
> diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
> index 1500a0f58041..b45243a0fbc5 100644
> --- a/include/uapi/linux/stat.h
> +++ b/include/uapi/linux/stat.h
> @@ -124,7 +124,7 @@ struct statx {
>         __u32   stx_dev_minor;
>         /* 0x90 */
>         __u64   stx_mnt_id;
> -       __u64   __spare2;
> +       __u64   stx_chgattr;    /* Inode change attribute */
>         /* 0xa0 */
>         __u64   __spare3[12];   /* Spare space for future expansion */
>         /* 0x100 */
> @@ -152,6 +152,7 @@ struct statx {
>  #define STATX_BASIC_STATS      0x000007ffU     /* The stuff in the normal stat struct */
>  #define STATX_BTIME            0x00000800U     /* Want/got stx_btime */
>  #define STATX_MNT_ID           0x00001000U     /* Got stx_mnt_id */
> +#define STATX_CHGATTR          0x00002000U     /* Want/git stx_chgattr */
>
>  #define STATX__RESERVED                0x80000000U     /* Reserved for future struct statx expansion */
>
> diff --git a/samples/vfs/test-statx.c b/samples/vfs/test-statx.c
> index 49c7a46cee07..767208d2f564 100644
> --- a/samples/vfs/test-statx.c
> +++ b/samples/vfs/test-statx.c
> @@ -109,6 +109,8 @@ static void dump_statx(struct statx *stx)
>                 printf(" Inode: %-11llu", (unsigned long long) stx->stx_ino);
>         if (stx->stx_mask & STATX_NLINK)
>                 printf(" Links: %-5u", stx->stx_nlink);
> +       if (stx->stx_mask & STATX_CHGATTR)
> +               printf(" Change Attr: 0x%llx", stx->stx_chgattr);
>         if (stx->stx_mask & STATX_TYPE) {
>                 switch (stx->stx_mode & S_IFMT) {
>                 case S_IFBLK:
> @@ -218,7 +220,7 @@ int main(int argc, char **argv)
>         struct statx stx;
>         int ret, raw = 0, atflag = AT_SYMLINK_NOFOLLOW;
>
> -       unsigned int mask = STATX_BASIC_STATS | STATX_BTIME;
> +       unsigned int mask = STATX_BASIC_STATS | STATX_BTIME | STATX_CHGATTR;
>
>         for (argv++; *argv; argv++) {
>                 if (strcmp(*argv, "-F") == 0) {
> --
> 2.37.1
>

Re: [RFC PATCH 1/4] vfs: report change attribute in statx for IS_I_VERSION inodes

[Jeff Layton]

On Wed, 2022-08-10 at 11:00 +0800, JunChao Sun wrote:
> On Sat, Aug 6, 2022 at 2:37 AM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > From: Jeff Layton <jlayton@redhat.com>
> > 
> > Claim one of the spare fields in struct statx to hold a 64-bit change
> > > 
> > > attribute. When statx requests this attribute, do an
> > > inode_query_iversion and fill the result in the field.
> 
> I guess, is it better to update the corresponding part of the man-pages...?

Yes. If we end up accepting a patch like this, we'll need to update the
statx(2) manpage. We'll probably also want to add support for it to the
/bin/stat coreutils command as well.

At this point, I'm trying to put together some xfstests so we can ensure
that this feature doesn't regress (if we take it).

> > 
> > 
> > Also update the test-statx.c program to fetch the change attribute as
> > well.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/stat.c                 | 7 +++++++
> >  include/linux/stat.h      | 1 +
> >  include/uapi/linux/stat.h | 3 ++-
> >  samples/vfs/test-statx.c  | 4 +++-
> >  4 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/stat.c b/fs/stat.c
> > index 9ced8860e0f3..976e0a59ab23 100644
> > --- a/fs/stat.c
> > +++ b/fs/stat.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/syscalls.h>
> >  #include <linux/pagemap.h>
> >  #include <linux/compat.h>
> > +#include <linux/iversion.h>
> > 
> >  #include <linux/uaccess.h>
> >  #include <asm/unistd.h>
> > @@ -118,6 +119,11 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
> >         stat->attributes_mask |= (STATX_ATTR_AUTOMOUNT |
> >                                   STATX_ATTR_DAX);
> > 
> > +       if ((request_mask & STATX_CHGATTR) && IS_I_VERSION(inode)) {
> > +               stat->result_mask |= STATX_CHGATTR;
> > +               stat->chgattr = inode_query_iversion(inode);
> > +       }
> > +
> >         mnt_userns = mnt_user_ns(path->mnt);
> >         if (inode->i_op->getattr)
> >                 return inode->i_op->getattr(mnt_userns, path, stat,
> > @@ -611,6 +617,7 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
> >         tmp.stx_dev_major = MAJOR(stat->dev);
> >         tmp.stx_dev_minor = MINOR(stat->dev);
> >         tmp.stx_mnt_id = stat->mnt_id;
> > +       tmp.stx_chgattr = stat->chgattr;
> > 
> >         return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;
> >  }
> > diff --git a/include/linux/stat.h b/include/linux/stat.h
> > index 7df06931f25d..4a17887472f6 100644
> > --- a/include/linux/stat.h
> > +++ b/include/linux/stat.h
> > @@ -50,6 +50,7 @@ struct kstat {
> >         struct timespec64 btime;                        /* File creation time */
> >         u64             blocks;
> >         u64             mnt_id;
> > +       u64             chgattr;
> >  };
> > 
> >  #endif
> > diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
> > index 1500a0f58041..b45243a0fbc5 100644
> > --- a/include/uapi/linux/stat.h
> > +++ b/include/uapi/linux/stat.h
> > @@ -124,7 +124,7 @@ struct statx {
> >         __u32   stx_dev_minor;
> >         /* 0x90 */
> >         __u64   stx_mnt_id;
> > -       __u64   __spare2;
> > +       __u64   stx_chgattr;    /* Inode change attribute */
> >         /* 0xa0 */
> >         __u64   __spare3[12];   /* Spare space for future expansion */
> >         /* 0x100 */
> > @@ -152,6 +152,7 @@ struct statx {
> >  #define STATX_BASIC_STATS      0x000007ffU     /* The stuff in the normal stat struct */
> >  #define STATX_BTIME            0x00000800U     /* Want/got stx_btime */
> >  #define STATX_MNT_ID           0x00001000U     /* Got stx_mnt_id */
> > +#define STATX_CHGATTR          0x00002000U     /* Want/git stx_chgattr */
> > 
> >  #define STATX__RESERVED                0x80000000U     /* Reserved for future struct statx expansion */
> > 
> > diff --git a/samples/vfs/test-statx.c b/samples/vfs/test-statx.c
> > index 49c7a46cee07..767208d2f564 100644
> > --- a/samples/vfs/test-statx.c
> > +++ b/samples/vfs/test-statx.c
> > @@ -109,6 +109,8 @@ static void dump_statx(struct statx *stx)
> >                 printf(" Inode: %-11llu", (unsigned long long) stx->stx_ino);
> >         if (stx->stx_mask & STATX_NLINK)
> >                 printf(" Links: %-5u", stx->stx_nlink);
> > +       if (stx->stx_mask & STATX_CHGATTR)
> > +               printf(" Change Attr: 0x%llx", stx->stx_chgattr);
> >         if (stx->stx_mask & STATX_TYPE) {
> >                 switch (stx->stx_mode & S_IFMT) {
> >                 case S_IFBLK:
> > @@ -218,7 +220,7 @@ int main(int argc, char **argv)
> >         struct statx stx;
> >         int ret, raw = 0, atflag = AT_SYMLINK_NOFOLLOW;
> > 
> > -       unsigned int mask = STATX_BASIC_STATS | STATX_BTIME;
> > +       unsigned int mask = STATX_BASIC_STATS | STATX_BTIME | STATX_CHGATTR;
> > 
> >         for (argv++; *argv; argv++) {
> >                 if (strcmp(*argv, "-F") == 0) {
> > --
> > 2.37.1
> > 
> 

-- 
Jeff Layton <jlayton@redhat.com>