| Message ID | 20240118073215.10026-1-wegao@suse.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] ioctl_fiemap01: New test for fiemap ioctl() | expand |
Hi Wei, > Fixes: #535 > diff --git a/testcases/kernel/syscalls/ioctl/ioctl_fiemap01.c b/testcases/kernel/syscalls/ioctl/ioctl_fiemap01.c > new file mode 100644 > index 000000000..a626bb03c > --- /dev/null > +++ b/testcases/kernel/syscalls/ioctl/ioctl_fiemap01.c > @@ -0,0 +1,116 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2023 Wei Gao <wegao@suse.com> > + */ > + > +/*\ > + * [Description] > + * > + * Verify basic fiemap ioctl > + * nit: missing space and dot at the end. > + */ > + > +#include <linux/fs.h> > +#include <linux/fiemap.h> > +#include <stdlib.h> > + > +#include "tst_test.h" > + > +#define TESTFILE "testfile" > +#define NUM_EXTENT 2 > +#define FILE_OFFSET ((rand() % 8 + 2) * getpagesize()) > + > +static char *buf; > + > +static void print_extens(struct fiemap *fiemap) > +{ > + nit: please no space above. > + tst_res(TDEBUG, "File extent count: %u", fiemap->fm_mapped_extents); nit: please add space here (readability) > + for (unsigned int i = 0; i < fiemap->fm_mapped_extents; ++i) { > + tst_res(TDEBUG, "Extent %u: Logical offset: %llu, Physical offset: %llu, flags: %u, Length: %llu", > + i + 1, > + fiemap->fm_extents[i].fe_logical, > + fiemap->fm_extents[i].fe_physical, > + fiemap->fm_extents[i].fe_flags, > + fiemap->fm_extents[i].fe_length); > + } > +} > + > +static void verify_ioctl(void) > +{ > + int fd; > + struct fiemap *fiemap; > + > + fd = SAFE_OPEN(TESTFILE, O_RDWR | O_CREAT, 0644); > + > + fiemap = SAFE_MALLOC(sizeof(struct fiemap) + sizeof(struct fiemap_extent) * NUM_EXTENT); > + fiemap->fm_start = 0; > + fiemap->fm_length = ~0ULL; > + fiemap->fm_extent_count = 1; > + > + fiemap->fm_flags = -1; > + TST_EXP_FAIL(ioctl(fd, FS_IOC_FIEMAP, fiemap), EBADR); I get on Tumbleweed with 6.8.0-rc4-1.g9b23bf2-default EOPNOTSUPP: ioctl_fiemap01.c:52: TFAIL: ioctl(fd, FS_IOC_FIEMAP, fiemap) expected EBADR: EOPNOTSUPP (95) What is the missing dependency for FS_IOC_FIEMAP? Or is there wrong fiemap use which causes that? > + > + fiemap->fm_flags = 0; > + SAFE_IOCTL(fd, FS_IOC_FIEMAP, fiemap); And on the same kernel another EOPNOTSUPP: ioctl_fiemap01.c:55: TBROK: ioctl(3,((((2U|1U) << (((0+8)+8)+14)) | ((('f')) << (0+8)) | (((11)) << 0) | ((((sizeof(struct fiemap)))) << ((0+8)+8)))),...) failed: EOPNOTSUPP (95) > + print_extens(fiemap); > + if (fiemap->fm_mapped_extents == 0) > + tst_res(TPASS, "Check fiemap iotct zero fm_mapped_extents pass"); s/iotct/ioctl/ > + else > + tst_res(TFAIL, "Check fiemap iotct zero fm_mapped_extents failed"); s/iotct/ioctl/ > + > + SAFE_WRITE(SAFE_WRITE_ANY, fd, buf, getpagesize()); > + SAFE_IOCTL(fd, FS_IOC_FIEMAP, fiemap); > + print_extens(fiemap); Maybe print only on failure? > + if ((fiemap->fm_mapped_extents == 1) && (fiemap->fm_extents[0].fe_physical == 0)) NOTE: brackets are not needed (== has higher precedence than &&), thus: if (fiemap->fm_mapped_extents == 1 && fiemap->fm_extents[0].fe_physical == 0) > + tst_res(TPASS, "Check fiemap iotct one fm_mapped_extents pass"); s/iotct/ioctl/ > + else > + tst_res(TFAIL, "Check fiemap iotct one fm_mapped_extents failed"); s/iotct/ioctl/ > + > + fiemap->fm_flags = FIEMAP_FLAG_SYNC; > + SAFE_IOCTL(fd, FS_IOC_FIEMAP, fiemap); > + print_extens(fiemap); > + if ((fiemap->fm_mapped_extents == 1) && nit: again == does not need to be in () > + (fiemap->fm_extents[0].fe_flags == FIEMAP_EXTENT_LAST) && > + (fiemap->fm_extents[0].fe_physical > 0) && > + (fiemap->fm_extents[0].fe_length == (__u64)getpagesize())) > + tst_res(TPASS, "Check fiemap iotct FIEMAP_FLAG_SYNC fm_flags pass"); s/iotct/ioctl/ > + else > + tst_res(TFAIL, "Check fiemap iotct FIEMAP_FLAG_SYNC fm_flags failed"); s/iotct/ioctl/ There are 4x checks, I guess instead it would be worth to write what exactly failed. > + > + fiemap->fm_extent_count = NUM_EXTENT; > + srand(time(NULL)); > + SAFE_LSEEK(fd, FILE_OFFSET, SEEK_SET); > + SAFE_WRITE(SAFE_WRITE_ALL, fd, buf, getpagesize()); > + SAFE_IOCTL(fd, FS_IOC_FIEMAP, fiemap); > + print_extens(fiemap); > + if ((fiemap->fm_mapped_extents == NUM_EXTENT) && nit: If this check was repeated more than twice, I would put it into separate function. > + (fiemap->fm_extents[NUM_EXTENT - 1].fe_flags == FIEMAP_EXTENT_LAST) && > + (fiemap->fm_extents[NUM_EXTENT - 1].fe_physical > 0) && > + (fiemap->fm_extents[NUM_EXTENT - 1].fe_length == (__u64)getpagesize())) > + tst_res(TPASS, "Check fiemap iotct multiple fm_mapped_extents pass"); > + else > + tst_res(TFAIL, "Check fiemap iotct multiple fm_mapped_extents failed"); > + > + free(fiemap); > + SAFE_CLOSE(fd); > + unlink(TESTFILE); > +} ... Kind regards, Petr
On Wed, Feb 28, 2024 at 06:07:29PM +0100, Petr Vorel wrote: > Hi Wei, > > > Fixes: #535 > > > diff --git a/testcases/kernel/syscalls/ioctl/ioctl_fiemap01.c b/testcases/kernel/syscalls/ioctl/ioctl_fiemap01.c > > new file mode 100644 > > index 000000000..a626bb03c > > --- /dev/null > > +++ b/testcases/kernel/syscalls/ioctl/ioctl_fiemap01.c > > @@ -0,0 +1,116 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (c) 2023 Wei Gao <wegao@suse.com> > > + */ > > + > > +/*\ > > + * [Description] > > + * > > + * Verify basic fiemap ioctl > > + * > nit: missing space and dot at the end. > > + */ > > + > > +#include <linux/fs.h> > > +#include <linux/fiemap.h> > > +#include <stdlib.h> > > + > > +#include "tst_test.h" > > + > > +#define TESTFILE "testfile" > > +#define NUM_EXTENT 2 > > +#define FILE_OFFSET ((rand() % 8 + 2) * getpagesize()) > > + > > +static char *buf; > > + > > +static void print_extens(struct fiemap *fiemap) > > +{ > > + > nit: please no space above. > > + tst_res(TDEBUG, "File extent count: %u", fiemap->fm_mapped_extents); > nit: please add space here (readability) > > + for (unsigned int i = 0; i < fiemap->fm_mapped_extents; ++i) { > > + tst_res(TDEBUG, "Extent %u: Logical offset: %llu, Physical offset: %llu, flags: %u, Length: %llu", > > + i + 1, > > + fiemap->fm_extents[i].fe_logical, > > + fiemap->fm_extents[i].fe_physical, > > + fiemap->fm_extents[i].fe_flags, > > + fiemap->fm_extents[i].fe_length); > > + } > > +} > > + > > +static void verify_ioctl(void) > > +{ > > + int fd; > > + struct fiemap *fiemap; > > + > > + fd = SAFE_OPEN(TESTFILE, O_RDWR | O_CREAT, 0644); > > + > > + fiemap = SAFE_MALLOC(sizeof(struct fiemap) + sizeof(struct fiemap_extent) * NUM_EXTENT); > > + fiemap->fm_start = 0; > > + fiemap->fm_length = ~0ULL; > > + fiemap->fm_extent_count = 1; > > + > > + fiemap->fm_flags = -1; > > + TST_EXP_FAIL(ioctl(fd, FS_IOC_FIEMAP, fiemap), EBADR); > > I get on Tumbleweed with 6.8.0-rc4-1.g9b23bf2-default EOPNOTSUPP: > ioctl_fiemap01.c:52: TFAIL: ioctl(fd, FS_IOC_FIEMAP, fiemap) expected EBADR: EOPNOTSUPP (95) > What is the missing dependency for FS_IOC_FIEMAP? Or is there wrong fiemap use > which causes that? Thanks for your test and i also reproduce this issue. This is caused by Tumbleweed mount tmpfs on /tmp and tmpfs seems not support fiemap action. tmpfs on /tmp type tmpfs (rw,nosuid,nodev,size=494452k,nr_inodes=1048576,inode64) I will sent new patch for cover all supported filesystem. > > > + > > + fiemap->fm_flags = 0; > > + SAFE_IOCTL(fd, FS_IOC_FIEMAP, fiemap); > And on the same kernel another EOPNOTSUPP: > > ioctl_fiemap01.c:55: TBROK: ioctl(3,((((2U|1U) << (((0+8)+8)+14)) | ((('f')) << (0+8)) | (((11)) << 0) | ((((sizeof(struct fiemap)))) << ((0+8)+8)))),...) failed: EOPNOTSUPP (95) > > + print_extens(fiemap); > > + if (fiemap->fm_mapped_extents == 0) > > + tst_res(TPASS, "Check fiemap iotct zero fm_mapped_extents pass"); > s/iotct/ioctl/ > > > + else > > + tst_res(TFAIL, "Check fiemap iotct zero fm_mapped_extents failed"); > s/iotct/ioctl/ > > + > > + SAFE_WRITE(SAFE_WRITE_ANY, fd, buf, getpagesize()); > > + SAFE_IOCTL(fd, FS_IOC_FIEMAP, fiemap); > > + print_extens(fiemap); > Maybe print only on failure? > > + if ((fiemap->fm_mapped_extents == 1) && (fiemap->fm_extents[0].fe_physical == 0)) > NOTE: brackets are not needed (== has higher precedence than &&), thus: > if (fiemap->fm_mapped_extents == 1 && fiemap->fm_extents[0].fe_physical == 0) > > > + tst_res(TPASS, "Check fiemap iotct one fm_mapped_extents pass"); > s/iotct/ioctl/ > > + else > > + tst_res(TFAIL, "Check fiemap iotct one fm_mapped_extents failed"); > s/iotct/ioctl/ > > + > > + fiemap->fm_flags = FIEMAP_FLAG_SYNC; > > + SAFE_IOCTL(fd, FS_IOC_FIEMAP, fiemap); > > + print_extens(fiemap); > > + if ((fiemap->fm_mapped_extents == 1) && > nit: again == does not need to be in () > > + (fiemap->fm_extents[0].fe_flags == FIEMAP_EXTENT_LAST) && > > + (fiemap->fm_extents[0].fe_physical > 0) && > > + (fiemap->fm_extents[0].fe_length == (__u64)getpagesize())) > > + tst_res(TPASS, "Check fiemap iotct FIEMAP_FLAG_SYNC fm_flags pass"); > s/iotct/ioctl/ > > + else > > + tst_res(TFAIL, "Check fiemap iotct FIEMAP_FLAG_SYNC fm_flags failed"); > s/iotct/ioctl/ > There are 4x checks, I guess instead it would be worth to write what exactly failed. > > > + > > + fiemap->fm_extent_count = NUM_EXTENT; > > + srand(time(NULL)); > > + SAFE_LSEEK(fd, FILE_OFFSET, SEEK_SET); > > + SAFE_WRITE(SAFE_WRITE_ALL, fd, buf, getpagesize()); > > + SAFE_IOCTL(fd, FS_IOC_FIEMAP, fiemap); > > + print_extens(fiemap); > > + if ((fiemap->fm_mapped_extents == NUM_EXTENT) && > nit: If this check was repeated more than twice, I would put it into separate > function. > > + (fiemap->fm_extents[NUM_EXTENT - 1].fe_flags == FIEMAP_EXTENT_LAST) && > > + (fiemap->fm_extents[NUM_EXTENT - 1].fe_physical > 0) && > > + (fiemap->fm_extents[NUM_EXTENT - 1].fe_length == (__u64)getpagesize())) > > + tst_res(TPASS, "Check fiemap iotct multiple fm_mapped_extents pass"); > > + else > > + tst_res(TFAIL, "Check fiemap iotct multiple fm_mapped_extents failed"); > > + > > + free(fiemap); > > + SAFE_CLOSE(fd); > > + unlink(TESTFILE); > > +} > ... > > Kind regards, > Petr
Hi Wei, ... > > > + fiemap->fm_flags = -1; > > > + TST_EXP_FAIL(ioctl(fd, FS_IOC_FIEMAP, fiemap), EBADR); > > I get on Tumbleweed with 6.8.0-rc4-1.g9b23bf2-default EOPNOTSUPP: > > ioctl_fiemap01.c:52: TFAIL: ioctl(fd, FS_IOC_FIEMAP, fiemap) expected EBADR: EOPNOTSUPP (95) > > What is the missing dependency for FS_IOC_FIEMAP? Or is there wrong fiemap use > > which causes that? > Thanks for your test and i also reproduce this issue. > This is caused by Tumbleweed mount tmpfs on /tmp and tmpfs seems not support fiemap action. > tmpfs on /tmp type tmpfs (rw,nosuid,nodev,size=494452k,nr_inodes=1048576,inode64) +1. You probably knows that you need .skip_filesystems, which works even .all_filesystems is not set. .skip_filesystems = (const char *const []) { "tmpfs", NULL }, But actually when this is filesystem dependent, I guess .all_filesystems = 1 would make sense, right? > I will sent new patch for cover all supported filesystem. I suppose you agree :). Kind regards, Petr
On Fri, Mar 29, 2024 at 10:32:39PM +0100, Petr Vorel wrote: > Hi Wei, > > ... > > > > + fiemap->fm_flags = -1; > > > > + TST_EXP_FAIL(ioctl(fd, FS_IOC_FIEMAP, fiemap), EBADR); > > > > I get on Tumbleweed with 6.8.0-rc4-1.g9b23bf2-default EOPNOTSUPP: > > > ioctl_fiemap01.c:52: TFAIL: ioctl(fd, FS_IOC_FIEMAP, fiemap) expected EBADR: EOPNOTSUPP (95) > > > What is the missing dependency for FS_IOC_FIEMAP? Or is there wrong fiemap use > > > which causes that? > > Thanks for your test and i also reproduce this issue. > > This is caused by Tumbleweed mount tmpfs on /tmp and tmpfs seems not support fiemap action. > > tmpfs on /tmp type tmpfs (rw,nosuid,nodev,size=494452k,nr_inodes=1048576,inode64) > > +1. You probably knows that you need .skip_filesystems, which works even > .all_filesystems is not set. > > .skip_filesystems = (const char *const []) { > "tmpfs", > NULL > }, > > But actually when this is filesystem dependent, I guess .all_filesystems = 1 > would make sense, right? > > > I will sent new patch for cover all supported filesystem. > > I suppose you agree :). Sure! New draft patch will try to cover all filesystem. The new patch has one more thing need improve, you suggest print_extens only on failure, when i start using TST_EXP_EXPR i can not get return result. Any suggestion or example code handle this in clean way or we can keep current implementation? NOTE: print_extens currently control by TDEBUG. Thanks a lot :) > > Kind regards, > Petr
diff --git a/runtest/syscalls b/runtest/syscalls index 6e2407879..4e6ce5aef 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -589,6 +589,8 @@ ioctl_ns07 ioctl_ns07 ioctl_sg01 ioctl_sg01 +ioctl_fiemap01 ioctl_fiemap01 + inotify_init1_01 inotify_init1_01 inotify_init1_02 inotify_init1_02 diff --git a/testcases/kernel/syscalls/ioctl/.gitignore b/testcases/kernel/syscalls/ioctl/.gitignore index 5fff7a61d..64adcdfe6 100644 --- a/testcases/kernel/syscalls/ioctl/.gitignore +++ b/testcases/kernel/syscalls/ioctl/.gitignore @@ -22,3 +22,4 @@ /ioctl_ns06 /ioctl_ns07 /ioctl_sg01 +/ioctl_fiemap01 diff --git a/testcases/kernel/syscalls/ioctl/ioctl_fiemap01.c b/testcases/kernel/syscalls/ioctl/ioctl_fiemap01.c new file mode 100644 index 000000000..a626bb03c --- /dev/null +++ b/testcases/kernel/syscalls/ioctl/ioctl_fiemap01.c @@ -0,0 +1,116 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2023 Wei Gao <wegao@suse.com> + */ + +/*\ + * [Description] + * + * Verify basic fiemap ioctl + * + */ + +#include <linux/fs.h> +#include <linux/fiemap.h> +#include <stdlib.h> + +#include "tst_test.h" + +#define TESTFILE "testfile" +#define NUM_EXTENT 2 +#define FILE_OFFSET ((rand() % 8 + 2) * getpagesize()) + +static char *buf; + +static void print_extens(struct fiemap *fiemap) +{ + + tst_res(TDEBUG, "File extent count: %u", fiemap->fm_mapped_extents); + for (unsigned int i = 0; i < fiemap->fm_mapped_extents; ++i) { + tst_res(TDEBUG, "Extent %u: Logical offset: %llu, Physical offset: %llu, flags: %u, Length: %llu", + i + 1, + fiemap->fm_extents[i].fe_logical, + fiemap->fm_extents[i].fe_physical, + fiemap->fm_extents[i].fe_flags, + fiemap->fm_extents[i].fe_length); + } +} + +static void verify_ioctl(void) +{ + int fd; + struct fiemap *fiemap; + + fd = SAFE_OPEN(TESTFILE, O_RDWR | O_CREAT, 0644); + + fiemap = SAFE_MALLOC(sizeof(struct fiemap) + sizeof(struct fiemap_extent) * NUM_EXTENT); + fiemap->fm_start = 0; + fiemap->fm_length = ~0ULL; + fiemap->fm_extent_count = 1; + + fiemap->fm_flags = -1; + TST_EXP_FAIL(ioctl(fd, FS_IOC_FIEMAP, fiemap), EBADR); + + fiemap->fm_flags = 0; + SAFE_IOCTL(fd, FS_IOC_FIEMAP, fiemap); + print_extens(fiemap); + if (fiemap->fm_mapped_extents == 0) + tst_res(TPASS, "Check fiemap iotct zero fm_mapped_extents pass"); + else + tst_res(TFAIL, "Check fiemap iotct zero fm_mapped_extents failed"); + + SAFE_WRITE(SAFE_WRITE_ANY, fd, buf, getpagesize()); + SAFE_IOCTL(fd, FS_IOC_FIEMAP, fiemap); + print_extens(fiemap); + if ((fiemap->fm_mapped_extents == 1) && (fiemap->fm_extents[0].fe_physical == 0)) + tst_res(TPASS, "Check fiemap iotct one fm_mapped_extents pass"); + else + tst_res(TFAIL, "Check fiemap iotct one fm_mapped_extents failed"); + + fiemap->fm_flags = FIEMAP_FLAG_SYNC; + SAFE_IOCTL(fd, FS_IOC_FIEMAP, fiemap); + print_extens(fiemap); + if ((fiemap->fm_mapped_extents == 1) && + (fiemap->fm_extents[0].fe_flags == FIEMAP_EXTENT_LAST) && + (fiemap->fm_extents[0].fe_physical > 0) && + (fiemap->fm_extents[0].fe_length == (__u64)getpagesize())) + tst_res(TPASS, "Check fiemap iotct FIEMAP_FLAG_SYNC fm_flags pass"); + else + tst_res(TFAIL, "Check fiemap iotct FIEMAP_FLAG_SYNC fm_flags failed"); + + fiemap->fm_extent_count = NUM_EXTENT; + srand(time(NULL)); + SAFE_LSEEK(fd, FILE_OFFSET, SEEK_SET); + SAFE_WRITE(SAFE_WRITE_ALL, fd, buf, getpagesize()); + SAFE_IOCTL(fd, FS_IOC_FIEMAP, fiemap); + print_extens(fiemap); + if ((fiemap->fm_mapped_extents == NUM_EXTENT) && + (fiemap->fm_extents[NUM_EXTENT - 1].fe_flags == FIEMAP_EXTENT_LAST) && + (fiemap->fm_extents[NUM_EXTENT - 1].fe_physical > 0) && + (fiemap->fm_extents[NUM_EXTENT - 1].fe_length == (__u64)getpagesize())) + tst_res(TPASS, "Check fiemap iotct multiple fm_mapped_extents pass"); + else + tst_res(TFAIL, "Check fiemap iotct multiple fm_mapped_extents failed"); + + free(fiemap); + SAFE_CLOSE(fd); + unlink(TESTFILE); +} + +static void setup(void) +{ + buf = SAFE_MALLOC(getpagesize()); +} + +static void cleanup(void) +{ + free(buf); +} + +static struct tst_test test = { + .setup = setup, + .cleanup = cleanup, + .test_all = verify_ioctl, + .needs_root = 1, + .needs_tmpdir = 1, +};
Fixes: #535 Signed-off-by: Wei Gao <wegao@suse.com> --- runtest/syscalls | 2 + testcases/kernel/syscalls/ioctl/.gitignore | 1 + .../kernel/syscalls/ioctl/ioctl_fiemap01.c | 116 ++++++++++++++++++ 3 files changed, 119 insertions(+) create mode 100644 testcases/kernel/syscalls/ioctl/ioctl_fiemap01.c