| Message ID | 20240902164554.928371-2-cyphar@cyphar.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [xfstests,v2,1/2] statx: update headers to include newer statx fields | expand |
On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote: > > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to > make sure they match properly as part of the regular open_by_handle > tests. > > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/ > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> > --- > v2: > - Remove -M argument and always do the mount ID tests. [Amir Goldstein] > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein] > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/> Looks good. You may add: Reviewed-by: Amir Goldstein <amir73il@gmail.com> It'd be nice to get a verification that this is indeed tested on the latest upstream and does not regress the tests that run the open_by_handle program. Thanks, Amir. > > src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++---------- > 1 file changed, 99 insertions(+), 29 deletions(-) > > diff --git a/src/open_by_handle.c b/src/open_by_handle.c > index d9c802ca9bd1..0ad591da632e 100644 > --- a/src/open_by_handle.c > +++ b/src/open_by_handle.c > @@ -86,10 +86,16 @@ Examples: > #include <errno.h> > #include <linux/limits.h> > #include <libgen.h> > +#include <stdint.h> > +#include <stdbool.h> > > #include <sys/stat.h> > #include "statx.h" > > +#ifndef AT_HANDLE_MNT_ID_UNIQUE > +# define AT_HANDLE_MNT_ID_UNIQUE 0x001 > +#endif > + > #define MAXFILES 1024 > > struct handle { > @@ -120,6 +126,94 @@ void usage(void) > exit(EXIT_FAILURE); > } > > +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz) > +{ > + int ret; > + int mntid_short; > + > + static bool skip_mntid_unique; > + > + uint64_t statx_mntid_short = 0, statx_mntid_unique = 0; > + struct statx statxbuf; > + > + /* Get both the short and unique mount id. */ > + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) { > + fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname); > + return EXIT_FAILURE; > + } > + if (!(statxbuf.stx_mask & STATX_MNT_ID)) { > + fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname); > + return EXIT_FAILURE; > + } > + statx_mntid_short = statxbuf.stx_mnt_id; > + > + if (!skip_mntid_unique) { > + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) { > + fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname); > + return EXIT_FAILURE; > + } > + /* > + * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the > + * kernel doesn't give us a unique mount ID just skip it. > + */ > + if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))) > + printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n"); > + else > + statx_mntid_unique = statxbuf.stx_mnt_id; > + } > + > + fh->handle_bytes = bufsz; > + ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0); > + if (bufsz < fh->handle_bytes) { > + /* Query the filesystem required bufsz and the file handle */ > + if (ret != -1 || errno != EOVERFLOW) { > + fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret); > + return EXIT_FAILURE; > + } > + ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0); > + } > + if (ret < 0) { > + fprintf(stderr, "%s: name_to_handle: %m\n", fname); > + return EXIT_FAILURE; > + } > + > + if (mntid_short != (int) statx_mntid_short) { > + fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short); > + return EXIT_FAILURE; > + } > + > + if (!skip_mntid_unique && statx_mntid_unique != 0) { > + struct handle dummy_fh; > + uint64_t mntid_unique = 0; > + > + /* > + * Get the unique mount ID. We don't need to get another copy of the > + * handle so store it in a dummy struct. > + */ > + dummy_fh.fh.handle_bytes = fh->handle_bytes; > + ret = name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE); > + if (ret < 0) { > + if (errno != EINVAL) { > + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname); > + return EXIT_FAILURE; > + } > + /* > + * EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported, so skip > + * the check in that case. > + */ > + printf("name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n"); > + skip_mntid_unique = true; > + } else { > + if (mntid_unique != statx_mntid_unique) { > + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique); > + return EXIT_FAILURE; > + } > + } > + } > + > + return 0; > +} > + > int main(int argc, char **argv) > { > int i, c; > @@ -132,7 +226,7 @@ int main(int argc, char **argv) > char fname2[PATH_MAX]; > char *test_dir; > char *mount_dir; > - int mount_fd, mount_id; > + int mount_fd; > char *infile = NULL, *outfile = NULL; > int in_fd = 0, out_fd = 0; > int numfiles = 1; > @@ -307,21 +401,9 @@ int main(int argc, char **argv) > return EXIT_FAILURE; > } > } else { > - handle[i].fh.handle_bytes = bufsz; > - ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0); > - if (bufsz < handle[i].fh.handle_bytes) { > - /* Query the filesystem required bufsz and the file handle */ > - if (ret != -1 || errno != EOVERFLOW) { > - fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname); > - return EXIT_FAILURE; > - } > - ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0); > - } > - if (ret < 0) { > - strcat(fname, ": name_to_handle"); > - perror(fname); > + ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz); > + if (ret < 0) > return EXIT_FAILURE; > - } > } > if (keepopen) { > /* Open without close to keep unlinked files around */ > @@ -349,21 +431,9 @@ int main(int argc, char **argv) > return EXIT_FAILURE; > } > } else { > - dir_handle.fh.handle_bytes = bufsz; > - ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0); > - if (bufsz < dir_handle.fh.handle_bytes) { > - /* Query the filesystem required bufsz and the file handle */ > - if (ret != -1 || errno != EOVERFLOW) { > - fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname); > - return EXIT_FAILURE; > - } > - ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0); > - } > - if (ret < 0) { > - strcat(dname, ": name_to_handle"); > - perror(dname); > + ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz); > + if (ret < 0) > return EXIT_FAILURE; > - } > } > if (out_fd) { > ret = write(out_fd, (char *)&dir_handle, sizeof(*handle)); > -- > 2.46.0 >
On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote: > On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote: > > > > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to > > make sure they match properly as part of the regular open_by_handle > > tests. > > > > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/ > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> > > --- > > v2: > > - Remove -M argument and always do the mount ID tests. [Amir Goldstein] > > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE > > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein] > > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/> > > Looks good. > > You may add: > > Reviewed-by: Amir Goldstein <amir73il@gmail.com> > > It'd be nice to get a verification that this is indeed tested on the latest > upstream and does not regress the tests that run the open_by_handle program. I've tested that the fallback works on mainline and correctly does the test on patched kernels (by running open_by_handle directly) but I haven't run the suite yet (still getting my mkosi testing setup working to run fstests...). > Thanks, > Amir. > > > > > src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++---------- > > 1 file changed, 99 insertions(+), 29 deletions(-) > > > > diff --git a/src/open_by_handle.c b/src/open_by_handle.c > > index d9c802ca9bd1..0ad591da632e 100644 > > --- a/src/open_by_handle.c > > +++ b/src/open_by_handle.c > > @@ -86,10 +86,16 @@ Examples: > > #include <errno.h> > > #include <linux/limits.h> > > #include <libgen.h> > > +#include <stdint.h> > > +#include <stdbool.h> > > > > #include <sys/stat.h> > > #include "statx.h" > > > > +#ifndef AT_HANDLE_MNT_ID_UNIQUE > > +# define AT_HANDLE_MNT_ID_UNIQUE 0x001 > > +#endif > > + > > #define MAXFILES 1024 > > > > struct handle { > > @@ -120,6 +126,94 @@ void usage(void) > > exit(EXIT_FAILURE); > > } > > > > +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz) > > +{ > > + int ret; > > + int mntid_short; > > + > > + static bool skip_mntid_unique; > > + > > + uint64_t statx_mntid_short = 0, statx_mntid_unique = 0; > > + struct statx statxbuf; > > + > > + /* Get both the short and unique mount id. */ > > + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) { > > + fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname); > > + return EXIT_FAILURE; > > + } > > + if (!(statxbuf.stx_mask & STATX_MNT_ID)) { > > + fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname); > > + return EXIT_FAILURE; > > + } > > + statx_mntid_short = statxbuf.stx_mnt_id; > > + > > + if (!skip_mntid_unique) { > > + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) { > > + fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname); > > + return EXIT_FAILURE; > > + } > > + /* > > + * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the > > + * kernel doesn't give us a unique mount ID just skip it. > > + */ > > + if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))) > > + printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n"); > > + else > > + statx_mntid_unique = statxbuf.stx_mnt_id; > > + } > > + > > + fh->handle_bytes = bufsz; > > + ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0); > > + if (bufsz < fh->handle_bytes) { > > + /* Query the filesystem required bufsz and the file handle */ > > + if (ret != -1 || errno != EOVERFLOW) { > > + fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret); > > + return EXIT_FAILURE; > > + } > > + ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0); > > + } > > + if (ret < 0) { > > + fprintf(stderr, "%s: name_to_handle: %m\n", fname); > > + return EXIT_FAILURE; > > + } > > + > > + if (mntid_short != (int) statx_mntid_short) { > > + fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short); > > + return EXIT_FAILURE; > > + } > > + > > + if (!skip_mntid_unique && statx_mntid_unique != 0) { > > + struct handle dummy_fh; > > + uint64_t mntid_unique = 0; > > + > > + /* > > + * Get the unique mount ID. We don't need to get another copy of the > > + * handle so store it in a dummy struct. > > + */ > > + dummy_fh.fh.handle_bytes = fh->handle_bytes; > > + ret = name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE); > > + if (ret < 0) { > > + if (errno != EINVAL) { > > + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname); > > + return EXIT_FAILURE; > > + } > > + /* > > + * EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported, so skip > > + * the check in that case. > > + */ > > + printf("name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n"); > > + skip_mntid_unique = true; > > + } else { > > + if (mntid_unique != statx_mntid_unique) { > > + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique); > > + return EXIT_FAILURE; > > + } > > + } > > + } > > + > > + return 0; > > +} > > + > > int main(int argc, char **argv) > > { > > int i, c; > > @@ -132,7 +226,7 @@ int main(int argc, char **argv) > > char fname2[PATH_MAX]; > > char *test_dir; > > char *mount_dir; > > - int mount_fd, mount_id; > > + int mount_fd; > > char *infile = NULL, *outfile = NULL; > > int in_fd = 0, out_fd = 0; > > int numfiles = 1; > > @@ -307,21 +401,9 @@ int main(int argc, char **argv) > > return EXIT_FAILURE; > > } > > } else { > > - handle[i].fh.handle_bytes = bufsz; > > - ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0); > > - if (bufsz < handle[i].fh.handle_bytes) { > > - /* Query the filesystem required bufsz and the file handle */ > > - if (ret != -1 || errno != EOVERFLOW) { > > - fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname); > > - return EXIT_FAILURE; > > - } > > - ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0); > > - } > > - if (ret < 0) { > > - strcat(fname, ": name_to_handle"); > > - perror(fname); > > + ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz); > > + if (ret < 0) > > return EXIT_FAILURE; > > - } > > } > > if (keepopen) { > > /* Open without close to keep unlinked files around */ > > @@ -349,21 +431,9 @@ int main(int argc, char **argv) > > return EXIT_FAILURE; > > } > > } else { > > - dir_handle.fh.handle_bytes = bufsz; > > - ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0); > > - if (bufsz < dir_handle.fh.handle_bytes) { > > - /* Query the filesystem required bufsz and the file handle */ > > - if (ret != -1 || errno != EOVERFLOW) { > > - fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname); > > - return EXIT_FAILURE; > > - } > > - ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0); > > - } > > - if (ret < 0) { > > - strcat(dname, ": name_to_handle"); > > - perror(dname); > > + ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz); > > + if (ret < 0) > > return EXIT_FAILURE; > > - } > > } > > if (out_fd) { > > ret = write(out_fd, (char *)&dir_handle, sizeof(*handle)); > > -- > > 2.46.0 > >
On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote: > > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to > make sure they match properly as part of the regular open_by_handle > tests. > > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/ > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> > --- > v2: > - Remove -M argument and always do the mount ID tests. [Amir Goldstein] > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein] > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/> > > src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++---------- > 1 file changed, 99 insertions(+), 29 deletions(-) > > diff --git a/src/open_by_handle.c b/src/open_by_handle.c > index d9c802ca9bd1..0ad591da632e 100644 > --- a/src/open_by_handle.c > +++ b/src/open_by_handle.c > @@ -86,10 +86,16 @@ Examples: > #include <errno.h> > #include <linux/limits.h> > #include <libgen.h> > +#include <stdint.h> > +#include <stdbool.h> > > #include <sys/stat.h> > #include "statx.h" > > +#ifndef AT_HANDLE_MNT_ID_UNIQUE > +# define AT_HANDLE_MNT_ID_UNIQUE 0x001 > +#endif > + > #define MAXFILES 1024 > > struct handle { > @@ -120,6 +126,94 @@ void usage(void) > exit(EXIT_FAILURE); > } > > +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz) > +{ > + int ret; > + int mntid_short; > + > + static bool skip_mntid_unique; > + > + uint64_t statx_mntid_short = 0, statx_mntid_unique = 0; > + struct statx statxbuf; > + > + /* Get both the short and unique mount id. */ > + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) { This fails build on top of latest for-next branch with commit 873e36c9 - statx.h: update to latest kernel UAPI It can be fixed by changing to use the private xfstests_statx() implementation, same as in stat_test.c. I am not sure how elegant this is, but that's the easy fix. > + fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname); > + return EXIT_FAILURE; > + } > + if (!(statxbuf.stx_mask & STATX_MNT_ID)) { > + fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname); > + return EXIT_FAILURE; > + } > + statx_mntid_short = statxbuf.stx_mnt_id; > + > + if (!skip_mntid_unique) { > + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) { > + fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname); > + return EXIT_FAILURE; > + } > + /* > + * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the > + * kernel doesn't give us a unique mount ID just skip it. > + */ > + if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))) > + printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n"); This verbose print breaks all existing "exportfs" tests which do not expect it in the golden output. I understand that silently ignoring the failure is not good, but I also would like to add this test coverage to all the existing tests. One solution is to resurrect the command line option -M from v1, but instead of meaning "test unique mount id" let it mean "do not allow to skip unique mount id" test. Then you can add a new test that runs open_by_handle -M, but also implement a helper _require_unique_mntid similar to _require_btime which is needed for the new test to run only on new kernels. I'm sorry for this complication, but fstest is a testsuite that runs on disto and stable kernels as well and we need to allow test coverage of new features along with stability of the test env. Thanks, Amir.
On Tue, Sep 3, 2024 at 8:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote: > > On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote: > > On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote: > > > > > > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to > > > make sure they match properly as part of the regular open_by_handle > > > tests. > > > > > > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/ > > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> > > > --- > > > v2: > > > - Remove -M argument and always do the mount ID tests. [Amir Goldstein] > > > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE > > > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein] > > > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/> > > > > Looks good. > > > > You may add: > > > > Reviewed-by: Amir Goldstein <amir73il@gmail.com> > > > > It'd be nice to get a verification that this is indeed tested on the latest > > upstream and does not regress the tests that run the open_by_handle program. > > I've tested that the fallback works on mainline and correctly does the > test on patched kernels (by running open_by_handle directly) but I > haven't run the suite yet (still getting my mkosi testing setup working > to run fstests...). I am afraid this has to be tested. I started testing myself and found that it breaks existing tests. Even if you make the test completely opt-in as in v1 it need to be tested and _notrun on old kernels. If you have a new version, I can test it until you get your fstests setup ready, because anyway I would want to check that your test also works with overlayfs which has some specialized exportfs tests. Test by running ./check -overlay -g exportfs, but I can also do that for you. Thanks, Amir.
On 2024-09-03, Amir Goldstein <amir73il@gmail.com> wrote: > On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote: > > > > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to > > make sure they match properly as part of the regular open_by_handle > > tests. > > > > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/ > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> > > --- > > v2: > > - Remove -M argument and always do the mount ID tests. [Amir Goldstein] > > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE > > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein] > > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/> > > > > src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++---------- > > 1 file changed, 99 insertions(+), 29 deletions(-) > > > > diff --git a/src/open_by_handle.c b/src/open_by_handle.c > > index d9c802ca9bd1..0ad591da632e 100644 > > --- a/src/open_by_handle.c > > +++ b/src/open_by_handle.c > > @@ -86,10 +86,16 @@ Examples: > > #include <errno.h> > > #include <linux/limits.h> > > #include <libgen.h> > > +#include <stdint.h> > > +#include <stdbool.h> > > > > #include <sys/stat.h> > > #include "statx.h" > > > > +#ifndef AT_HANDLE_MNT_ID_UNIQUE > > +# define AT_HANDLE_MNT_ID_UNIQUE 0x001 > > +#endif > > + > > #define MAXFILES 1024 > > > > struct handle { > > @@ -120,6 +126,94 @@ void usage(void) > > exit(EXIT_FAILURE); > > } > > > > +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz) > > +{ > > + int ret; > > + int mntid_short; > > + > > + static bool skip_mntid_unique; > > + > > + uint64_t statx_mntid_short = 0, statx_mntid_unique = 0; > > + struct statx statxbuf; > > + > > + /* Get both the short and unique mount id. */ > > + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) { > > This fails build on top of latest for-next branch with commit > 873e36c9 - statx.h: update to latest kernel UAPI > > It can be fixed by changing to use the private xfstests_statx() > implementation, same as in stat_test.c. > > I am not sure how elegant this is, but that's the easy fix. Ah, I was using master as the base. Sorry about that... > > + fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname); > > + return EXIT_FAILURE; > > + } > > + if (!(statxbuf.stx_mask & STATX_MNT_ID)) { > > + fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname); > > + return EXIT_FAILURE; > > + } > > + statx_mntid_short = statxbuf.stx_mnt_id; > > + > > + if (!skip_mntid_unique) { > > + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) { > > + fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname); > > + return EXIT_FAILURE; > > + } > > + /* > > + * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the > > + * kernel doesn't give us a unique mount ID just skip it. > > + */ > > + if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))) > > + printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n"); > > This verbose print breaks all existing "exportfs" tests which do not > expect it in the golden output. > > I understand that silently ignoring the failure is not good, but I also > would like to add this test coverage to all the existing tests. > > One solution is to resurrect the command line option -M from v1, > but instead of meaning "test unique mount id" let it mean > "do not allow to skip unique mount id" test. > > Then you can add a new test that runs open_by_handle -M, but also > implement a helper _require_unique_mntid similar to _require_btime > which is needed for the new test to run only on new kernels. > > I'm sorry for this complication, but fstest is a testsuite that runs on > disto and stable kernels as well and we need to allow test coverage > of new features along with stability of the test env. No worries, I'll write it up. I'm not familiar with the exact requirements of xfstests, sorry for the noise! (^_^") > > Thanks, > Amir.
On 2024-09-03, Amir Goldstein <amir73il@gmail.com> wrote: > On Tue, Sep 3, 2024 at 8:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote: > > > > On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote: > > > On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote: > > > > > > > > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to > > > > make sure they match properly as part of the regular open_by_handle > > > > tests. > > > > > > > > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/ > > > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> > > > > --- > > > > v2: > > > > - Remove -M argument and always do the mount ID tests. [Amir Goldstein] > > > > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE > > > > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein] > > > > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/> > > > > > > Looks good. > > > > > > You may add: > > > > > > Reviewed-by: Amir Goldstein <amir73il@gmail.com> > > > > > > It'd be nice to get a verification that this is indeed tested on the latest > > > upstream and does not regress the tests that run the open_by_handle program. > > > > I've tested that the fallback works on mainline and correctly does the > > test on patched kernels (by running open_by_handle directly) but I > > haven't run the suite yet (still getting my mkosi testing setup working > > to run fstests...). > > I am afraid this has to be tested. > I started testing myself and found that it breaks existing tests. > Even if you make the test completely opt-in as in v1 it need to be > tested and _notrun on old kernels. > > If you have a new version, I can test it until you get your fstests setup > ready, because anyway I would want to check that your test also > works with overlayfs which has some specialized exportfs tests. > Test by running ./check -overlay -g exportfs, but I can also do that for you. I managed to get fstests running, sorry about that... For the v3 I have ready (which includes a new test using -M), the following runs work in my VM: - ./check -g exportfs - ./check -overlay -g exportfs Should I check anything else before sending it? Also, when running the tests I think I may have found a bug? Using overlayfs+xfs leads to the following error when doing ./check -overlay if the scratch device is XFS: ./common/rc: line 299: _xfs_has_feature: command not found not run: upper fs needs to support d_type The fix I applied was simply: diff --git a/common/rc b/common/rc index 0beaf2ff1126..e6af1b16918f 100644 --- a/common/rc +++ b/common/rc @@ -296,6 +296,7 @@ _supports_filetype() local fstyp=`$DF_PROG $dir | tail -1 | $AWK_PROG '{print $2}'` case "$fstyp" in xfs) + . common/xfs _xfs_has_feature $dir ftype ;; ext2|ext3|ext4) Should I include this patch as well, or did I make a mistake somewhere? (I could add the import to the top instead if you'd prefer that.) Thanks. > > Thanks, > Amir.
On Wed, Sep 4, 2024 at 6:31 PM Aleksa Sarai <cyphar@cyphar.com> wrote: > > On 2024-09-03, Amir Goldstein <amir73il@gmail.com> wrote: > > On Tue, Sep 3, 2024 at 8:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote: > > > > > > On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote: > > > > On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote: > > > > > > > > > > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to > > > > > make sure they match properly as part of the regular open_by_handle > > > > > tests. > > > > > > > > > > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/ > > > > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> > > > > > --- > > > > > v2: > > > > > - Remove -M argument and always do the mount ID tests. [Amir Goldstein] > > > > > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE > > > > > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein] > > > > > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/> > > > > > > > > Looks good. > > > > > > > > You may add: > > > > > > > > Reviewed-by: Amir Goldstein <amir73il@gmail.com> > > > > > > > > It'd be nice to get a verification that this is indeed tested on the latest > > > > upstream and does not regress the tests that run the open_by_handle program. > > > > > > I've tested that the fallback works on mainline and correctly does the > > > test on patched kernels (by running open_by_handle directly) but I > > > haven't run the suite yet (still getting my mkosi testing setup working > > > to run fstests...). > > > > I am afraid this has to be tested. > > I started testing myself and found that it breaks existing tests. > > Even if you make the test completely opt-in as in v1 it need to be > > tested and _notrun on old kernels. > > > > If you have a new version, I can test it until you get your fstests setup > > ready, because anyway I would want to check that your test also > > works with overlayfs which has some specialized exportfs tests. > > Test by running ./check -overlay -g exportfs, but I can also do that for you. > > I managed to get fstests running, sorry about that... > > For the v3 I have ready (which includes a new test using -M), the > following runs work in my VM: > > - ./check -g exportfs > - ./check -overlay -g exportfs > > Should I check anything else before sending it? > That should be enough. So you have one new test that does not run on upstream kernel and runs and passes on patched kernel? > Also, when running the tests I think I may have found a bug? Using > overlayfs+xfs leads to the following error when doing ./check -overlay > if the scratch device is XFS: > > ./common/rc: line 299: _xfs_has_feature: command not found > not run: upper fs needs to support d_type > > The fix I applied was simply: > > diff --git a/common/rc b/common/rc > index 0beaf2ff1126..e6af1b16918f 100644 > --- a/common/rc > +++ b/common/rc > @@ -296,6 +296,7 @@ _supports_filetype() > local fstyp=`$DF_PROG $dir | tail -1 | $AWK_PROG '{print $2}'` > case "$fstyp" in > xfs) > + . common/xfs > _xfs_has_feature $dir ftype > ;; > ext2|ext3|ext4) > > Should I include this patch as well, or did I make a mistake somewhere? > (I could add the import to the top instead if you'd prefer that.) This should already be handled by if [ -n "$OVL_BASE_FSTYP" ];then _source_specific_fs $OVL_BASE_FSTYP fi in common/overlay I think what you are missing is to export FSTYP=xfs as README.overlay suggests. It's true that ./check does not *require* defining FSTYP and can auto detect the test filesystem, but for running -overlay is it a requirement to define the base FSTYP. Thanks, Amir.
On 2024-09-04, Amir Goldstein <amir73il@gmail.com> wrote: > On Wed, Sep 4, 2024 at 6:31 PM Aleksa Sarai <cyphar@cyphar.com> wrote: > > > > On 2024-09-03, Amir Goldstein <amir73il@gmail.com> wrote: > > > On Tue, Sep 3, 2024 at 8:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote: > > > > > > > > On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote: > > > > > On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote: > > > > > > > > > > > > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to > > > > > > make sure they match properly as part of the regular open_by_handle > > > > > > tests. > > > > > > > > > > > > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/ > > > > > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> > > > > > > --- > > > > > > v2: > > > > > > - Remove -M argument and always do the mount ID tests. [Amir Goldstein] > > > > > > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE > > > > > > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein] > > > > > > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/> > > > > > > > > > > Looks good. > > > > > > > > > > You may add: > > > > > > > > > > Reviewed-by: Amir Goldstein <amir73il@gmail.com> > > > > > > > > > > It'd be nice to get a verification that this is indeed tested on the latest > > > > > upstream and does not regress the tests that run the open_by_handle program. > > > > > > > > I've tested that the fallback works on mainline and correctly does the > > > > test on patched kernels (by running open_by_handle directly) but I > > > > haven't run the suite yet (still getting my mkosi testing setup working > > > > to run fstests...). > > > > > > I am afraid this has to be tested. > > > I started testing myself and found that it breaks existing tests. > > > Even if you make the test completely opt-in as in v1 it need to be > > > tested and _notrun on old kernels. > > > > > > If you have a new version, I can test it until you get your fstests setup > > > ready, because anyway I would want to check that your test also > > > works with overlayfs which has some specialized exportfs tests. > > > Test by running ./check -overlay -g exportfs, but I can also do that for you. > > > > I managed to get fstests running, sorry about that... > > > > For the v3 I have ready (which includes a new test using -M), the > > following runs work in my VM: > > > > - ./check -g exportfs > > - ./check -overlay -g exportfs > > > > Should I check anything else before sending it? > > > > That should be enough. > So you have one new test that does not run on upstream kernel > and runs and passes on patched kernel? Yes. Both of those suite runs succeed without issues on v6.6.49, v6.11-rc6 and with the AT_HANDLE_MNT_ID_UNIQUE patches. I also added skipping logic such that it _should_ work on pre-5.8 kernels (pre-STATX_MNT_ID) as well, but I can't test that at the moment (mkosi-kernel fails to boot kernels that old it seems...). I'll send it now. > > Also, when running the tests I think I may have found a bug? Using > > overlayfs+xfs leads to the following error when doing ./check -overlay > > if the scratch device is XFS: > > > > ./common/rc: line 299: _xfs_has_feature: command not found > > not run: upper fs needs to support d_type > > > > The fix I applied was simply: > > > > diff --git a/common/rc b/common/rc > > index 0beaf2ff1126..e6af1b16918f 100644 > > --- a/common/rc > > +++ b/common/rc > > @@ -296,6 +296,7 @@ _supports_filetype() > > local fstyp=`$DF_PROG $dir | tail -1 | $AWK_PROG '{print $2}'` > > case "$fstyp" in > > xfs) > > + . common/xfs > > _xfs_has_feature $dir ftype > > ;; > > ext2|ext3|ext4) > > > > Should I include this patch as well, or did I make a mistake somewhere? > > (I could add the import to the top instead if you'd prefer that.) > > This should already be handled by > if [ -n "$OVL_BASE_FSTYP" ];then > _source_specific_fs $OVL_BASE_FSTYP > fi > > in common/overlay > > I think what you are missing is to > export FSTYP=xfs > as README.overlay suggests. > > It's true that ./check does not *require* defining FSTYP > and can auto detect the test filesystem, but for running -overlay > is it a requirement to define the base FSTYP. Ah okay, I missed that. That fixed the issue, thanks! > Thanks, > Amir.
diff --git a/src/open_by_handle.c b/src/open_by_handle.c index d9c802ca9bd1..0ad591da632e 100644 --- a/src/open_by_handle.c +++ b/src/open_by_handle.c @@ -86,10 +86,16 @@ Examples: #include <errno.h> #include <linux/limits.h> #include <libgen.h> +#include <stdint.h> +#include <stdbool.h> #include <sys/stat.h> #include "statx.h" +#ifndef AT_HANDLE_MNT_ID_UNIQUE +# define AT_HANDLE_MNT_ID_UNIQUE 0x001 +#endif + #define MAXFILES 1024 struct handle { @@ -120,6 +126,94 @@ void usage(void) exit(EXIT_FAILURE); } +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz) +{ + int ret; + int mntid_short; + + static bool skip_mntid_unique; + + uint64_t statx_mntid_short = 0, statx_mntid_unique = 0; + struct statx statxbuf; + + /* Get both the short and unique mount id. */ + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) { + fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname); + return EXIT_FAILURE; + } + if (!(statxbuf.stx_mask & STATX_MNT_ID)) { + fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname); + return EXIT_FAILURE; + } + statx_mntid_short = statxbuf.stx_mnt_id; + + if (!skip_mntid_unique) { + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) { + fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname); + return EXIT_FAILURE; + } + /* + * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the + * kernel doesn't give us a unique mount ID just skip it. + */ + if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))) + printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n"); + else + statx_mntid_unique = statxbuf.stx_mnt_id; + } + + fh->handle_bytes = bufsz; + ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0); + if (bufsz < fh->handle_bytes) { + /* Query the filesystem required bufsz and the file handle */ + if (ret != -1 || errno != EOVERFLOW) { + fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret); + return EXIT_FAILURE; + } + ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0); + } + if (ret < 0) { + fprintf(stderr, "%s: name_to_handle: %m\n", fname); + return EXIT_FAILURE; + } + + if (mntid_short != (int) statx_mntid_short) { + fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short); + return EXIT_FAILURE; + } + + if (!skip_mntid_unique && statx_mntid_unique != 0) { + struct handle dummy_fh; + uint64_t mntid_unique = 0; + + /* + * Get the unique mount ID. We don't need to get another copy of the + * handle so store it in a dummy struct. + */ + dummy_fh.fh.handle_bytes = fh->handle_bytes; + ret = name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE); + if (ret < 0) { + if (errno != EINVAL) { + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname); + return EXIT_FAILURE; + } + /* + * EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported, so skip + * the check in that case. + */ + printf("name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n"); + skip_mntid_unique = true; + } else { + if (mntid_unique != statx_mntid_unique) { + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique); + return EXIT_FAILURE; + } + } + } + + return 0; +} + int main(int argc, char **argv) { int i, c; @@ -132,7 +226,7 @@ int main(int argc, char **argv) char fname2[PATH_MAX]; char *test_dir; char *mount_dir; - int mount_fd, mount_id; + int mount_fd; char *infile = NULL, *outfile = NULL; int in_fd = 0, out_fd = 0; int numfiles = 1; @@ -307,21 +401,9 @@ int main(int argc, char **argv) return EXIT_FAILURE; } } else { - handle[i].fh.handle_bytes = bufsz; - ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0); - if (bufsz < handle[i].fh.handle_bytes) { - /* Query the filesystem required bufsz and the file handle */ - if (ret != -1 || errno != EOVERFLOW) { - fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname); - return EXIT_FAILURE; - } - ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0); - } - if (ret < 0) { - strcat(fname, ": name_to_handle"); - perror(fname); + ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz); + if (ret < 0) return EXIT_FAILURE; - } } if (keepopen) { /* Open without close to keep unlinked files around */ @@ -349,21 +431,9 @@ int main(int argc, char **argv) return EXIT_FAILURE; } } else { - dir_handle.fh.handle_bytes = bufsz; - ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0); - if (bufsz < dir_handle.fh.handle_bytes) { - /* Query the filesystem required bufsz and the file handle */ - if (ret != -1 || errno != EOVERFLOW) { - fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname); - return EXIT_FAILURE; - } - ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0); - } - if (ret < 0) { - strcat(dname, ": name_to_handle"); - perror(dname); + ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz); + if (ret < 0) return EXIT_FAILURE; - } } if (out_fd) { ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to make sure they match properly as part of the regular open_by_handle tests. Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/ Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> --- v2: - Remove -M argument and always do the mount ID tests. [Amir Goldstein] - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein] - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/> src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++---------- 1 file changed, 99 insertions(+), 29 deletions(-)