2022-08-05 18:37:22

by Jeffrey Layton

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

From: Jeff Layton <[email protected]>

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 <[email protected]>
---
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


2022-08-05 19:21:42

by David Howells

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

Jeff Layton <[email protected]> wrote:

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

Why not call it stx_change_attr?

David

2022-08-05 22:27:28

by Jeff Layton

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

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 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>

2022-08-08 02:45:35

by Xiubo Li

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


On 8/6/22 2:35 AM, Jeff Layton wrote:
> From: Jeff Layton <[email protected]>
>
> 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 <[email protected]>
> ---
> 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) {

2022-08-08 10:19:10

by Jeffrey Layton

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

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 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>

2022-08-08 13:25:00

by Jeffrey Layton

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

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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>

2022-08-09 18:13:38

by Jeffrey Layton

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

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 <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]>