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:

quoted

On Fri, 2022-08-05 at 18:06 -0400, Jeff Layton wrote:

quoted

On Sat, 2022-08-06 at 08:01 +1000, Dave Chinner wrote:

quoted

On Fri, Aug 05, 2022 at 02:35:40PM -0400, Jeff Layton wrote:

quoted

From: Jeff Layton <redacted>

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