* [LTP] [PATCH v3 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file
2023-04-04 7:30 ` [LTP] [PATCH v3 1/4] lapi/stat.h: Add STATX_DIOALIGN related definition Yang Xu
@ 2023-04-04 7:30 ` Yang Xu
2023-04-04 21:52 ` Eric Biggers
2023-04-04 7:30 ` [LTP] [PATCH v3 3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device Yang Xu
2023-04-04 7:30 ` [LTP] [PATCH v3 4/4] lapi/stat.h: Remove deprecated STATX_ALL macro Yang Xu
2 siblings, 1 reply; 46+ messages in thread
From: Yang Xu @ 2023-04-04 7:30 UTC (permalink / raw)
To: ltp
If the caller didn't include STATX_DIOALIGN in the request mask,
direct I/O alignment information isn't returned since statx() isn't
required to return unrequested information.
STATX_DIOALIGN is used to get stx_dio_mem_align and stx_dio_offset_align
for files on fs that support direct io. We just check whether these
value are nonzero on ext4 and xfs.
On ext4, files that use certain filesystem features (data journalling,
encryption, and verity) fall back to buffered I/O. But ltp doesn't use
these features by default, So I think dio should not fall back to
buffered I/O.
For struct statx member check, we only check stx_dio_mem_align because
these two member is introduced toger in separate commit in kernel, so it is
safe.
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
v2->v3:
1. fix testfile path problme
2. also test stx_dio_mem_align and stx_dio_offset_align is not filled
when not request STATX_DIOALIGN
configure.ac | 2 +-
runtest/syscalls | 1 +
testcases/kernel/syscalls/statx/.gitignore | 1 +
testcases/kernel/syscalls/statx/statx10.c | 98 ++++++++++++++++++++++
4 files changed, 101 insertions(+), 1 deletion(-)
create mode 100644 testcases/kernel/syscalls/statx/statx10.c
diff --git a/configure.ac b/configure.ac
index 4c8763376..548288310 100644
--- a/configure.ac
+++ b/configure.ac
@@ -158,7 +158,7 @@ AC_CHECK_FUNCS(mkdtemp,[],AC_MSG_ERROR(mkdtemp() not found!))
AC_CHECK_MEMBERS([struct fanotify_event_info_fid.fsid.__val],,,[#include <sys/fanotify.h>])
AC_CHECK_MEMBERS([struct perf_event_mmap_page.aux_head],,,[#include <linux/perf_event.h>])
AC_CHECK_MEMBERS([struct sigaction.sa_sigaction],[],[],[#include <signal.h>])
-AC_CHECK_MEMBERS([struct statx.stx_mnt_id],,,[
+AC_CHECK_MEMBERS([struct statx.stx_mnt_id, struct statx.stx_dio_mem_align],,,[
#define _GNU_SOURCE
#include <sys/stat.h>
])
diff --git a/runtest/syscalls b/runtest/syscalls
index 8b002e989..92123772c 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1769,6 +1769,7 @@ statx06 statx06
statx07 statx07
statx08 statx08
statx09 statx09
+statx10 statx10
membarrier01 membarrier01
diff --git a/testcases/kernel/syscalls/statx/.gitignore b/testcases/kernel/syscalls/statx/.gitignore
index 1cea43c0d..67341ff2d 100644
--- a/testcases/kernel/syscalls/statx/.gitignore
+++ b/testcases/kernel/syscalls/statx/.gitignore
@@ -7,3 +7,4 @@
/statx07
/statx08
/statx09
+/statx10
diff --git a/testcases/kernel/syscalls/statx/statx10.c b/testcases/kernel/syscalls/statx/statx10.c
new file mode 100644
index 000000000..7e5d287d9
--- /dev/null
+++ b/testcases/kernel/syscalls/statx/statx10.c
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023 FUJITSU LIMITED. All rights reserved.
+ * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * It is a basic test for STATX_DIOALIGN mask on ext4 and xfs filesystem.
+ *
+ * - STATX_DIOALIGN Want stx_dio_mem_align and stx_dio_offset_align value
+ *
+ * These two values are only filled when STATX_DIOALIGN in the request mask.
+ * Also check these two values are nonzero under dio situation when
+ * STATX_DIOALIGN in the request mask
+ *
+ * Minimum Linux version required is v6.1.
+ */
+
+#define _GNU_SOURCE
+#include <sys/types.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include "tst_test.h"
+#include "lapi/stat.h"
+
+#ifdef HAVE_STRUCT_STATX_STX_DIO_MEM_ALIGN
+#define MNTPOINT "mnt_point"
+#define TESTFILE MNTPOINT"/testfile"
+
+static int fd = -1;
+
+static void verify_statx(void)
+{
+ struct statx buf;
+
+ memset(&buf, 0, sizeof(buf));
+ TST_EXP_PASS(statx(AT_FDCWD, TESTFILE, 0, STATX_DIOALIGN, &buf),
+ "statx(AT_FDCWD, %s, 0, STATX_DIOALIGN, &buf)", TESTFILE);
+
+ if (!(buf.stx_mask & STATX_DIOALIGN)) {
+ tst_res(TCONF, "STATX_DIOALIGN is not supported until linux 6.1");
+ return;
+ }
+
+ if (buf.stx_dio_mem_align != 0)
+ tst_res(TPASS, "stx_dio_mem_align:%u", buf.stx_dio_mem_align);
+ else
+ tst_res(TFAIL, "don't get stx_dio_mem_align on supported dio fs");
+
+ if (buf.stx_dio_offset_align != 0)
+ tst_res(TPASS, "stx_dio_offset_align:%u", buf.stx_dio_offset_align);
+ else
+ tst_res(TFAIL, "don't get stx_dio_offset_align on supported dio fs");
+
+ TST_EXP_PASS(statx(AT_FDCWD, TESTFILE, 0, 0, &buf),
+ "statx(AT_FDCWD, %s, 0, 0, &buf)", TESTFILE);
+
+ if ((buf.stx_mask & STATX_DIOALIGN)) {
+ tst_res(TFAIL, "STATX_DIOALIGN mask return even not request");
+ return;
+ }
+ TST_EXP_EQ_LU(buf.stx_dio_mem_align, 0);
+ TST_EXP_EQ_LU(buf.stx_dio_offset_align, 0);
+}
+
+static void setup(void)
+{
+ if (strcmp(tst_device->fs_type, "xfs") && strcmp(tst_device->fs_type, "ext4"))
+ tst_brk(TCONF, "This test only supports ext4 and xfs");
+
+ SAFE_FILE_PRINTF(TESTFILE, "AAAA");
+ fd = open(TESTFILE, O_RDWR | O_DIRECT);
+ if (fd == -1 && errno == EINVAL)
+ tst_brk(TCONF, "The regular file is not on a filesystem that support DIO");
+}
+
+static void cleanup(void)
+{
+ if (fd > -1)
+ SAFE_CLOSE(fd);
+}
+
+static struct tst_test test = {
+ .test_all = verify_statx,
+ .setup = setup,
+ .cleanup = cleanup,
+ .needs_root = 1,
+ .mntpoint = MNTPOINT,
+ .mount_device = 1,
+ .all_filesystems = 1,
+};
+#else
+TST_TEST_TCONF("test requires struct statx to have the stx_dio_mem_align fields");
+#endif
--
2.39.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v3 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file
2023-04-04 7:30 ` [LTP] [PATCH v3 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file Yang Xu
@ 2023-04-04 21:52 ` Eric Biggers
2023-04-06 4:52 ` xuyang2018.jy
0 siblings, 1 reply; 46+ messages in thread
From: Eric Biggers @ 2023-04-04 21:52 UTC (permalink / raw)
To: Yang Xu; +Cc: ltp
On Tue, Apr 04, 2023 at 03:30:28PM +0800, Yang Xu wrote:
> On ext4, files that use certain filesystem features (data journalling,
> encryption, and verity) fall back to buffered I/O. But ltp doesn't use
> these features by default, So I think dio should not fall back to
> buffered I/O.
Please document this in the code itself.
> +static void verify_statx(void)
> +{
> + struct statx buf;
> +
> + memset(&buf, 0, sizeof(buf));
There is no need for this memset to 0.
> + if (buf.stx_dio_mem_align != 0)
> + tst_res(TPASS, "stx_dio_mem_align:%u", buf.stx_dio_mem_align);
> + else
> + tst_res(TFAIL, "don't get stx_dio_mem_align on supported dio fs");
> +
> + if (buf.stx_dio_offset_align != 0)
> + tst_res(TPASS, "stx_dio_offset_align:%u", buf.stx_dio_offset_align);
> + else
> + tst_res(TFAIL, "don't get stx_dio_offset_align on supported dio fs");
Please leave a space between : and %u.
> + if ((buf.stx_mask & STATX_DIOALIGN)) {
Unnecessary parentheses
> + tst_res(TFAIL, "STATX_DIOALIGN mask return even not request");
> + return;
> + }
This part is not a valid test. Please see the statx(2) manual page:
"It should be noted that the kernel may return fields that weren't re‐
quested and may fail to return fields that were requested, depending on
what the backing filesystem supports. (Fields that are given values
despite being unrequested can just be ignored.) In either case,
stx_mask will not be equal mask."
> +static void setup(void)
> +{
> + if (strcmp(tst_device->fs_type, "xfs") && strcmp(tst_device->fs_type, "ext4"))
> + tst_brk(TCONF, "This test only supports ext4 and xfs");
> +
> + SAFE_FILE_PRINTF(TESTFILE, "AAAA");
> + fd = open(TESTFILE, O_RDWR | O_DIRECT);
> + if (fd == -1 && errno == EINVAL)
> + tst_brk(TCONF, "The regular file is not on a filesystem that support DIO");
> +}
> +
> +static void cleanup(void)
> +{
> + if (fd > -1)
> + SAFE_CLOSE(fd);
> +}
Shouldn't 'fd' just be a local variable in setup()?
> +#else
> +TST_TEST_TCONF("test requires struct statx to have the stx_dio_mem_align fields");
> +#endif
LTP already has its own definition of struct statx in include/lapi/stat.h. So,
why is it necessary to skip this test if the system headers lack an up-to-date
definition of struct statx?
- Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v3 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file
2023-04-04 21:52 ` Eric Biggers
@ 2023-04-06 4:52 ` xuyang2018.jy
0 siblings, 0 replies; 46+ messages in thread
From: xuyang2018.jy @ 2023-04-06 4:52 UTC (permalink / raw)
To: Eric Biggers; +Cc: ltp@lists.linux.it
on 2023/04/05 5:52, Eric Biggers wrote:
> On Tue, Apr 04, 2023 at 03:30:28PM +0800, Yang Xu wrote:
>> On ext4, files that use certain filesystem features (data journalling,
>> encryption, and verity) fall back to buffered I/O. But ltp doesn't use
>> these features by default, So I think dio should not fall back to
>> buffered I/O.
>
> Please document this in the code itself.
OK.
>
>> +static void verify_statx(void)
>> +{
>> + struct statx buf;
>> +
>> + memset(&buf, 0, sizeof(buf));
>
> There is no need for this memset to 0.
YES.
>
>> + if (buf.stx_dio_mem_align != 0)
>> + tst_res(TPASS, "stx_dio_mem_align:%u", buf.stx_dio_mem_align);
>> + else
>> + tst_res(TFAIL, "don't get stx_dio_mem_align on supported dio fs");
>> +
>> + if (buf.stx_dio_offset_align != 0)
>> + tst_res(TPASS, "stx_dio_offset_align:%u", buf.stx_dio_offset_align);
>> + else
>> + tst_res(TFAIL, "don't get stx_dio_offset_align on supported dio fs");
>
> Please leave a space between : and %u.
Good catch.
>
>> + if ((buf.stx_mask & STATX_DIOALIGN)) {
>
> Unnecessary parentheses
Yes,
>
>> + tst_res(TFAIL, "STATX_DIOALIGN mask return even not request");
>> + return;
>> + }
>
> This part is not a valid test. Please see the statx(2) manual page:
>
> "It should be noted that the kernel may return fields that weren't re‐
> quested and may fail to return fields that were requested, depending on
> what the backing filesystem supports. (Fields that are given values
> despite being unrequested can just be ignored.) In either case,
> stx_mask will not be equal mask."
OK. Will remove.
>
>> +static void setup(void)
>> +{
>> + if (strcmp(tst_device->fs_type, "xfs") && strcmp(tst_device->fs_type, "ext4"))
>> + tst_brk(TCONF, "This test only supports ext4 and xfs");
>> +
>> + SAFE_FILE_PRINTF(TESTFILE, "AAAA");
>> + fd = open(TESTFILE, O_RDWR | O_DIRECT);
>> + if (fd == -1 && errno == EINVAL)
>> + tst_brk(TCONF, "The regular file is not on a filesystem that support DIO");
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> + if (fd > -1)
>> + SAFE_CLOSE(fd);
>> +}
>
> Shouldn't 'fd' just be a local variable in setup()?
Yes.
>
>> +#else
>> +TST_TEST_TCONF("test requires struct statx to have the stx_dio_mem_align fields");
>> +#endif
>
> LTP already has its own definition of struct statx in include/lapi/stat.h. So,
> why is it necessary to skip this test if the system headers lack an up-to-date
> definition of struct statx?
In actually, ltp own statx definition as below:
#if defined(HAVE_STRUCT_STATX)
#include <sys/stat.h>
#else
struct statx {
/* 0x00 */
uint32_t stx_mask;
uint32_t stx_blksize;
uint64_t stx_attributes;
So we only use ltp own statx struct when system header file sys/statx.h
doesn't have statx struct. If people use old system header file, it
still will report non defined stx_dio_mem_align or stx_dio_offset_align.
Best Regards
Yang Xu
>
> - Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread
* [LTP] [PATCH v3 3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device
2023-04-04 7:30 ` [LTP] [PATCH v3 1/4] lapi/stat.h: Add STATX_DIOALIGN related definition Yang Xu
2023-04-04 7:30 ` [LTP] [PATCH v3 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file Yang Xu
@ 2023-04-04 7:30 ` Yang Xu
2023-04-04 21:59 ` Eric Biggers
2023-04-04 7:30 ` [LTP] [PATCH v3 4/4] lapi/stat.h: Remove deprecated STATX_ALL macro Yang Xu
2 siblings, 1 reply; 46+ messages in thread
From: Yang Xu @ 2023-04-04 7:30 UTC (permalink / raw)
To: ltp
Since STATX_DIOLAIGN is only supported on regular file and block device,
so this case is used to test the latter.
This test is tightly coupled to the kernel's current DIO restrictions on block
devices. These changed in v6.0, and they are subject to further change in the
future.
It is fine for now because STATX_DIOALIGN is only in v6.1 and later
anyway.
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
v2->v3:
1.remove useless TESTFILE and MNTPINT macro
2.like statx10.c, test not filled situation when not request STATX_DIOLAIGN
3.add commet that this case is tightly coupled to the kernel's current DIO restrictions on block
devices
runtest/syscalls | 1 +
testcases/kernel/syscalls/statx/.gitignore | 1 +
testcases/kernel/syscalls/statx/statx11.c | 107 +++++++++++++++++++++
3 files changed, 109 insertions(+)
create mode 100644 testcases/kernel/syscalls/statx/statx11.c
diff --git a/runtest/syscalls b/runtest/syscalls
index 92123772c..de5f0be35 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1770,6 +1770,7 @@ statx07 statx07
statx08 statx08
statx09 statx09
statx10 statx10
+statx11 statx11
membarrier01 membarrier01
diff --git a/testcases/kernel/syscalls/statx/.gitignore b/testcases/kernel/syscalls/statx/.gitignore
index 67341ff2d..48ac4078b 100644
--- a/testcases/kernel/syscalls/statx/.gitignore
+++ b/testcases/kernel/syscalls/statx/.gitignore
@@ -8,3 +8,4 @@
/statx08
/statx09
/statx10
+/statx11
diff --git a/testcases/kernel/syscalls/statx/statx11.c b/testcases/kernel/syscalls/statx/statx11.c
new file mode 100644
index 000000000..35d6fbaf3
--- /dev/null
+++ b/testcases/kernel/syscalls/statx/statx11.c
@@ -0,0 +1,107 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023 FUJITSU LIMITED. All rights reserved.
+ * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * It is a basic test for STATX_DIOALIGN mask on block device.
+ *
+ * - STATX_DIOALIGN Want stx_dio_mem_align and stx_dio_offset_align value
+ *
+ * These two values are only filled when STATX_DIOALIGN in the request mask.
+ * These two values are tightly coupled to the kernel's current DIO
+ * restrictions on block devices.
+ *
+ * Minimum Linux version required is v6.1.
+ */
+
+#define _GNU_SOURCE
+#include <sys/types.h>
+#include <sys/ioctl.h>
+#include <sys/mount.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include "tst_test.h"
+#include "lapi/stat.h"
+
+#ifdef HAVE_STRUCT_STATX_STX_DIO_MEM_ALIGN
+static int fd = -1, logical_sector_size;
+static char sys_bdev_dma_path[1024], sys_bdev_lgs_path[1024];
+
+static void verify_statx(void)
+{
+ struct statx buf;
+
+ memset(&buf, 0, sizeof(buf));
+ TST_EXP_PASS(statx(AT_FDCWD, tst_device->dev, 0, STATX_DIOALIGN, &buf),
+ "statx(AT_FDCWD, %s, 0, STATX_DIOALIGN, &buf)", tst_device->dev);
+
+ if (!(buf.stx_mask & STATX_DIOALIGN)) {
+ tst_res(TCONF, "STATX_DIOALIGN is not supported until linux 6.1");
+ return;
+ }
+
+ /*
+ * This test is tightly coupled to the kernel's current DIO restrictions
+ * on block devices. The general rule of DIO needing to be aligned to the
+ * block device's logical block size was recently relaxed to allow user buffers
+ * (but not file offsets) aligned to the DMA alignment instead. See v6.0
+ * commit bf8d08532bc1 ("iomap: add support for dma aligned direct-io") and
+ * they are subject to further change in the future.
+ * Also can see commit 2d985f8c6b9 ("vfs: support STATX_DIOALIGN on block devices).
+ */
+ TST_ASSERT_ULONG(sys_bdev_dma_path, buf.stx_dio_mem_align - 1);
+ TST_ASSERT_ULONG(sys_bdev_lgs_path, buf.stx_dio_offset_align);
+ TST_EXP_EQ_LU(buf.stx_dio_offset_align, logical_sector_size);
+
+ TST_EXP_PASS(statx(AT_FDCWD, tst_device->dev, 0, 0, &buf),
+ "statx(AT_FDCWD, %s, 0, STATX_DIOALIGN, &buf)", tst_device->dev);
+ TST_EXP_EQ_LU(buf.stx_dio_mem_align, 0);
+ TST_EXP_EQ_LU(buf.stx_dio_offset_align, 0);
+}
+
+static void setup(void)
+{
+ char *dev_name;
+ int dev_fd;
+
+ dev_fd = SAFE_OPEN(tst_device->dev, O_RDWR);
+ SAFE_IOCTL(dev_fd, BLKSSZGET, &logical_sector_size);
+ SAFE_CLOSE(dev_fd);
+
+ if (logical_sector_size <= 0)
+ tst_brk(TBROK, "BLKSSZGET returned invalid block size %i", logical_sector_size);
+
+ dev_name = basename((char *)tst_device->dev);
+ sprintf(sys_bdev_lgs_path, "/sys/block/%s/queue/logical_block_size", dev_name);
+ while (access(sys_bdev_lgs_path, F_OK) != 0) {
+ dev_name[strlen(dev_name)-1] = '\0';
+ sprintf(sys_bdev_lgs_path, "/sys/block/%s/queue/logical_block_size", dev_name);
+ }
+
+ sprintf(sys_bdev_dma_path, "/sys/block/%s/queue/dma_alignment", dev_name);
+ if (access(sys_bdev_dma_path, F_OK) != 0)
+ tst_brk(TCONF, "dma_alignment syfsfile doesn't exist");
+}
+
+static void cleanup(void)
+{
+ if (fd > -1)
+ SAFE_CLOSE(fd);
+}
+
+static struct tst_test test = {
+ .test_all = verify_statx,
+ .setup = setup,
+ .cleanup = cleanup,
+ .needs_root = 1,
+ .needs_device = 1,
+};
+#else
+TST_TEST_TCONF("test requires struct statx to have the stx_dio_mem_align fields");
+#endif
--
2.39.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v3 3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device
2023-04-04 7:30 ` [LTP] [PATCH v3 3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device Yang Xu
@ 2023-04-04 21:59 ` Eric Biggers
2023-04-06 4:57 ` xuyang2018.jy
2023-04-06 5:40 ` [LTP] [PATCH v4 1/4] lapi/stat.h: Add STATX_DIOALIGN related definition Yang Xu
0 siblings, 2 replies; 46+ messages in thread
From: Eric Biggers @ 2023-04-04 21:59 UTC (permalink / raw)
To: Yang Xu; +Cc: ltp
Hi Yang,
On Tue, Apr 04, 2023 at 03:30:29PM +0800, Yang Xu wrote:
> + /*
> + * This test is tightly coupled to the kernel's current DIO restrictions
> + * on block devices. The general rule of DIO needing to be aligned to the
> + * block device's logical block size was recently relaxed to allow user buffers
Please don't use the word "recently" in code comments like this. It is vague,
and what is "recent" now will no longer be recent in the future.
> +
> + TST_EXP_PASS(statx(AT_FDCWD, tst_device->dev, 0, 0, &buf),
> + "statx(AT_FDCWD, %s, 0, STATX_DIOALIGN, &buf)", tst_device->dev);
> + TST_EXP_EQ_LU(buf.stx_dio_mem_align, 0);
> + TST_EXP_EQ_LU(buf.stx_dio_offset_align, 0);
Like I mentioned on patch 2, this is not a valid test case because the contract
of statx() allows it to return information that wasn't explicitly requested.
> +static void setup(void)
> +{
> + char *dev_name;
> + int dev_fd;
> +
> + dev_fd = SAFE_OPEN(tst_device->dev, O_RDWR);
> + SAFE_IOCTL(dev_fd, BLKSSZGET, &logical_sector_size);
> + SAFE_CLOSE(dev_fd);
> +
> + if (logical_sector_size <= 0)
> + tst_brk(TBROK, "BLKSSZGET returned invalid block size %i", logical_sector_size);
> +
> + dev_name = basename((char *)tst_device->dev);
> + sprintf(sys_bdev_lgs_path, "/sys/block/%s/queue/logical_block_size", dev_name);
> + while (access(sys_bdev_lgs_path, F_OK) != 0) {
> + dev_name[strlen(dev_name)-1] = '\0';
> + sprintf(sys_bdev_lgs_path, "/sys/block/%s/queue/logical_block_size", dev_name);
> + }
What does "lgs" stand for?
Why are both BLKSSZGET and /sys/block/%s/queue/logical_block_size being used?
Don't they provide exactly the same information?
> + if (access(sys_bdev_dma_path, F_OK) != 0)
> + tst_brk(TCONF, "dma_alignment syfsfile doesn't exist");
> +}
syfsfile => sysfs file
> +static void cleanup(void)
> +{
> + if (fd > -1)
> + SAFE_CLOSE(fd);
> +}
What is the purpose of the 'fd' variable?
> +static struct tst_test test = {
> + .test_all = verify_statx,
> + .setup = setup,
> + .cleanup = cleanup,
> + .needs_root = 1,
> + .needs_device = 1,
> +};
Why does this test need root?
- Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v3 3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device
2023-04-04 21:59 ` Eric Biggers
@ 2023-04-06 4:57 ` xuyang2018.jy
2023-04-06 5:36 ` xuyang2018.jy
2023-04-06 5:40 ` [LTP] [PATCH v4 1/4] lapi/stat.h: Add STATX_DIOALIGN related definition Yang Xu
1 sibling, 1 reply; 46+ messages in thread
From: xuyang2018.jy @ 2023-04-06 4:57 UTC (permalink / raw)
To: Eric Biggers; +Cc: ltp@lists.linux.it
on 2023/04/05 5:59, Eric Biggers wrote:
> Hi Yang,
>
> On Tue, Apr 04, 2023 at 03:30:29PM +0800, Yang Xu wrote:
>> + /*
>> + * This test is tightly coupled to the kernel's current DIO restrictions
>> + * on block devices. The general rule of DIO needing to be aligned to the
>> + * block device's logical block size was recently relaxed to allow user buffers
>
> Please don't use the word "recently" in code comments like this. It is vague,
> and what is "recent" now will no longer be recent in the future.
OK.
>
>> +
>> + TST_EXP_PASS(statx(AT_FDCWD, tst_device->dev, 0, 0, &buf),
>> + "statx(AT_FDCWD, %s, 0, STATX_DIOALIGN, &buf)", tst_device->dev);
>> + TST_EXP_EQ_LU(buf.stx_dio_mem_align, 0);
>> + TST_EXP_EQ_LU(buf.stx_dio_offset_align, 0);
>
> Like I mentioned on patch 2, this is not a valid test case because the contract
> of statx() allows it to return information that wasn't explicitly requested.
OK. Will remove.
>
>> +static void setup(void)
>> +{
>> + char *dev_name;
>> + int dev_fd;
>> +
>> + dev_fd = SAFE_OPEN(tst_device->dev, O_RDWR);
>> + SAFE_IOCTL(dev_fd, BLKSSZGET, &logical_sector_size);
>> + SAFE_CLOSE(dev_fd);
>> +
>> + if (logical_sector_size <= 0)
>> + tst_brk(TBROK, "BLKSSZGET returned invalid block size %i", logical_sector_size);
>> +
>> + dev_name = basename((char *)tst_device->dev);
>> + sprintf(sys_bdev_lgs_path, "/sys/block/%s/queue/logical_block_size", dev_name);
>> + while (access(sys_bdev_lgs_path, F_OK) != 0) {
>> + dev_name[strlen(dev_name)-1] = '\0';
>> + sprintf(sys_bdev_lgs_path, "/sys/block/%s/queue/logical_block_size", dev_name);
>> + }
>
> What does "lgs" stand for?
lgs->logical_block_size, will use more meaningful variable name.
>
> Why are both BLKSSZGET and /sys/block/%s/queue/logical_block_size being used?
> Don't they provide exactly the same information?
Yes, they provide same info, I will only test for sys file instead of ioctl.
>
>> + if (access(sys_bdev_dma_path, F_OK) != 0)
>> + tst_brk(TCONF, "dma_alignment syfsfile doesn't exist");
>> +}
>
> syfsfile => sysfs file
Good catch.
>
>> +static void cleanup(void)
>> +{
>> + if (fd > -1)
>> + SAFE_CLOSE(fd);
>> +}
>
> What is the purpose of the 'fd' variable?
Sorry, I copy code from statx10.c, will remove.
>
>> +static struct tst_test test = {
>> + .test_all = verify_statx,
>> + .setup = setup,
>> + .cleanup = cleanup,
>> + .needs_root = 1,
>> + .needs_device = 1,
>> +};
>
> Why does this test need root?
I remember I have removed this, will remove.
Best Regards
Yang Xu
>
> - Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v3 3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device
2023-04-06 4:57 ` xuyang2018.jy
@ 2023-04-06 5:36 ` xuyang2018.jy
0 siblings, 0 replies; 46+ messages in thread
From: xuyang2018.jy @ 2023-04-06 5:36 UTC (permalink / raw)
To: Eric Biggers; +Cc: ltp@lists.linux.it
Hi Eric>
>
> on 2023/04/05 5:59, Eric Biggers wrote:
>
>> Hi Yang,
>>
>> On Tue, Apr 04, 2023 at 03:30:29PM +0800, Yang Xu wrote:
>>> + /*
>>> + * This test is tightly coupled to the kernel's current DIO restrictions
>>> + * on block devices. The general rule of DIO needing to be aligned to the
>>> + * block device's logical block size was recently relaxed to allow user buffers
>>
>> Please don't use the word "recently" in code comments like this. It is vague,
>> and what is "recent" now will no longer be recent in the future.
>
> OK.
>>
>>> +
>>> + TST_EXP_PASS(statx(AT_FDCWD, tst_device->dev, 0, 0, &buf),
>>> + "statx(AT_FDCWD, %s, 0, STATX_DIOALIGN, &buf)", tst_device->dev);
>>> + TST_EXP_EQ_LU(buf.stx_dio_mem_align, 0);
>>> + TST_EXP_EQ_LU(buf.stx_dio_offset_align, 0);
>>
>> Like I mentioned on patch 2, this is not a valid test case because the contract
>> of statx() allows it to return information that wasn't explicitly requested.
>
> OK. Will remove.
>>
>>> +static void setup(void)
>>> +{
>>> + char *dev_name;
>>> + int dev_fd;
>>> +
>>> + dev_fd = SAFE_OPEN(tst_device->dev, O_RDWR);
>>> + SAFE_IOCTL(dev_fd, BLKSSZGET, &logical_sector_size);
>>> + SAFE_CLOSE(dev_fd);
>>> +
>>> + if (logical_sector_size <= 0)
>>> + tst_brk(TBROK, "BLKSSZGET returned invalid block size %i", logical_sector_size);
>>> +
>>> + dev_name = basename((char *)tst_device->dev);
>>> + sprintf(sys_bdev_lgs_path, "/sys/block/%s/queue/logical_block_size", dev_name);
>>> + while (access(sys_bdev_lgs_path, F_OK) != 0) {
>>> + dev_name[strlen(dev_name)-1] = '\0';
>>> + sprintf(sys_bdev_lgs_path, "/sys/block/%s/queue/logical_block_size", dev_name);
>>> + }
>>
>> What does "lgs" stand for?
>
> lgs->logical_block_size, will use more meaningful variable name.
>
>>
>> Why are both BLKSSZGET and /sys/block/%s/queue/logical_block_size being used?
>> Don't they provide exactly the same information?
>
> Yes, they provide same info, I will only test for sys file instead of ioctl.
>>
>>> + if (access(sys_bdev_dma_path, F_OK) != 0)
>>> + tst_brk(TCONF, "dma_alignment syfsfile doesn't exist");
>>> +}
>>
>> syfsfile => sysfs file
>
> Good catch.
>>
>>> +static void cleanup(void)
>>> +{
>>> + if (fd > -1)
>>> + SAFE_CLOSE(fd);
>>> +}
>>
>> What is the purpose of the 'fd' variable?
>
> Sorry, I copy code from statx10.c, will remove.
>>
>>> +static struct tst_test test = {
>>> + .test_all = verify_statx,
>>> + .setup = setup,
>>> + .cleanup = cleanup,
>>> + .needs_root = 1,
>>> + .needs_device = 1,
>>> +};
>>
>> Why does this test need root?
>
> I remember I have removed this, will remove.
I almost forgot that /dev/loop-control needs root, otherwise will meet
EACCES error
tst_device.c:108: TINFO: Not allowed to open /dev/loop-control. Are you
root?: EACCES (13)
tst_device.c:147: TINFO: No free devices found
tst_device.c:354: TBROK: Failed to acquire device
Best Regards
Yang Xu
>
>
> Best Regards
> Yang Xu
>>
>> - Eric
>
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread
* [LTP] [PATCH v4 1/4] lapi/stat.h: Add STATX_DIOALIGN related definition
2023-04-04 21:59 ` Eric Biggers
2023-04-06 4:57 ` xuyang2018.jy
@ 2023-04-06 5:40 ` Yang Xu
2023-04-06 5:40 ` [LTP] [PATCH v4 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file Yang Xu
` (4 more replies)
1 sibling, 5 replies; 46+ messages in thread
From: Yang Xu @ 2023-04-06 5:40 UTC (permalink / raw)
To: ltp
Also add missing stx_mnt_id.
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
include/lapi/stat.h | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/include/lapi/stat.h b/include/lapi/stat.h
index ce1f2b678..c2db8a589 100644
--- a/include/lapi/stat.h
+++ b/include/lapi/stat.h
@@ -97,7 +97,11 @@ struct statx {
uint32_t stx_dev_major;
uint32_t stx_dev_minor;
/* 0x90 */
- uint64_t __spare2[14];
+ uint64_t stx_mnt_id;
+ uint32_t stx_dio_mem_align;
+ uint32_t stx_dio_offset_align;
+ /* 0xa0 */
+ uint64_t __spare1[12];
/* 0x100 */
};
#endif
@@ -180,6 +184,10 @@ static inline int statx(int dirfd, const char *pathname, unsigned int flags,
# define STATX_MNT_ID 0x00001000U
#endif
+#ifndef STATX_DIOALIGN
+# define STATX_DIOALIGN 0x00002000U
+#endif
+
#ifndef STATX_ALL
# define STATX_ALL 0x00000fffU
#endif
--
2.39.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 46+ messages in thread* [LTP] [PATCH v4 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file
2023-04-06 5:40 ` [LTP] [PATCH v4 1/4] lapi/stat.h: Add STATX_DIOALIGN related definition Yang Xu
@ 2023-04-06 5:40 ` Yang Xu
2023-04-26 22:06 ` Eric Biggers
2023-04-06 5:40 ` [LTP] [PATCH v4 3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device Yang Xu
` (3 subsequent siblings)
4 siblings, 1 reply; 46+ messages in thread
From: Yang Xu @ 2023-04-06 5:40 UTC (permalink / raw)
To: ltp
STATX_DIOALIGN is used to get stx_dio_mem_align and stx_dio_offset_align
for files on fs that support direct io. We just check whether these
value are nonzero on ext4 and xfs.
On ext4, files that use certain filesystem features (data journalling,
encryption, and verity) fall back to buffered I/O. But ltp doesn't use
these features by default, So I think dio should not fall back to
buffered I/O.
For struct statx member check, we only check stx_dio_mem_align because
these two member is introduced toger in separate commit in kernel, so it is
safe.
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
configure.ac | 2 +-
runtest/syscalls | 1 +
testcases/kernel/syscalls/statx/.gitignore | 1 +
testcases/kernel/syscalls/statx/statx10.c | 84 ++++++++++++++++++++++
4 files changed, 87 insertions(+), 1 deletion(-)
create mode 100644 testcases/kernel/syscalls/statx/statx10.c
diff --git a/configure.ac b/configure.ac
index 4c8763376..548288310 100644
--- a/configure.ac
+++ b/configure.ac
@@ -158,7 +158,7 @@ AC_CHECK_FUNCS(mkdtemp,[],AC_MSG_ERROR(mkdtemp() not found!))
AC_CHECK_MEMBERS([struct fanotify_event_info_fid.fsid.__val],,,[#include <sys/fanotify.h>])
AC_CHECK_MEMBERS([struct perf_event_mmap_page.aux_head],,,[#include <linux/perf_event.h>])
AC_CHECK_MEMBERS([struct sigaction.sa_sigaction],[],[],[#include <signal.h>])
-AC_CHECK_MEMBERS([struct statx.stx_mnt_id],,,[
+AC_CHECK_MEMBERS([struct statx.stx_mnt_id, struct statx.stx_dio_mem_align],,,[
#define _GNU_SOURCE
#include <sys/stat.h>
])
diff --git a/runtest/syscalls b/runtest/syscalls
index 8b002e989..92123772c 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1769,6 +1769,7 @@ statx06 statx06
statx07 statx07
statx08 statx08
statx09 statx09
+statx10 statx10
membarrier01 membarrier01
diff --git a/testcases/kernel/syscalls/statx/.gitignore b/testcases/kernel/syscalls/statx/.gitignore
index 1cea43c0d..67341ff2d 100644
--- a/testcases/kernel/syscalls/statx/.gitignore
+++ b/testcases/kernel/syscalls/statx/.gitignore
@@ -7,3 +7,4 @@
/statx07
/statx08
/statx09
+/statx10
diff --git a/testcases/kernel/syscalls/statx/statx10.c b/testcases/kernel/syscalls/statx/statx10.c
new file mode 100644
index 000000000..59e0c06e4
--- /dev/null
+++ b/testcases/kernel/syscalls/statx/statx10.c
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023 FUJITSU LIMITED. All rights reserved.
+ * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * It is a basic test for STATX_DIOALIGN mask on ext4 and xfs filesystem.
+ *
+ * - STATX_DIOALIGN Want stx_dio_mem_align and stx_dio_offset_align value
+ *
+ * Check these two values are nonzero under dio situation when STATX_DIOALIGN
+ * in the request mask.
+ *
+ * On ext4, files that use certain filesystem features (data journaling,
+ * encryption, and verity) fall back to buffered I/O. But ltp doesn't use these
+ * features by default, So I think dio should not fall back to buffered I/O.
+ *
+ * Minimum Linux version required is v6.1.
+ */
+
+#define _GNU_SOURCE
+#include <sys/types.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include "tst_test.h"
+#include "lapi/stat.h"
+
+#define MNTPOINT "mnt_point"
+#define TESTFILE MNTPOINT"/testfile"
+
+static void verify_statx(void)
+{
+ struct statx buf;
+
+ TST_EXP_PASS_SILENT(statx(AT_FDCWD, TESTFILE, 0, STATX_DIOALIGN, &buf),
+ "statx(AT_FDCWD, %s, 0, STATX_DIOALIGN, &buf)", TESTFILE);
+
+ if (!(buf.stx_mask & STATX_DIOALIGN)) {
+ tst_res(TCONF, "STATX_DIOALIGN is not supported until linux 6.1");
+ return;
+ }
+
+#ifdef HAVE_STRUCT_STATX_STX_DIO_MEM_ALIGN
+ if (buf.stx_dio_mem_align != 0)
+ tst_res(TPASS, "stx_dio_mem_align: %u", buf.stx_dio_mem_align);
+ else
+ tst_res(TFAIL, "don't get stx_dio_mem_align on supported dio fs");
+
+ if (buf.stx_dio_offset_align != 0)
+ tst_res(TPASS, "stx_dio_offset_align: %u", buf.stx_dio_offset_align);
+ else
+ tst_res(TFAIL, "don't get stx_dio_offset_align on supported dio fs");
+#endif
+}
+
+static void setup(void)
+{
+ int fd = -1;
+
+ if (strcmp(tst_device->fs_type, "xfs") && strcmp(tst_device->fs_type, "ext4"))
+ tst_brk(TCONF, "This test only supports ext4 and xfs");
+
+ SAFE_FILE_PRINTF(TESTFILE, "AAAA");
+ fd = open(TESTFILE, O_RDWR | O_DIRECT);
+ if (fd == -1 && errno == EINVAL) {
+ SAFE_CLOSE(fd);
+ tst_brk(TCONF, "The regular file is not on a filesystem that support DIO");
+ }
+ SAFE_CLOSE(fd);
+}
+
+static struct tst_test test = {
+ .test_all = verify_statx,
+ .setup = setup,
+ .needs_root = 1,
+ .mntpoint = MNTPOINT,
+ .mount_device = 1,
+ .all_filesystems = 1,
+};
--
2.39.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v4 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file
2023-04-06 5:40 ` [LTP] [PATCH v4 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file Yang Xu
@ 2023-04-26 22:06 ` Eric Biggers
2023-04-27 3:03 ` Yang Xu (Fujitsu)
0 siblings, 1 reply; 46+ messages in thread
From: Eric Biggers @ 2023-04-26 22:06 UTC (permalink / raw)
To: Yang Xu; +Cc: ltp
On Thu, Apr 06, 2023 at 01:40:20PM +0800, Yang Xu wrote:
> + * On ext4, files that use certain filesystem features (data journaling,
> + * encryption, and verity) fall back to buffered I/O. But ltp doesn't use these
> + * features by default, So I think dio should not fall back to buffered I/O.
Does LTP create and mount the filesystem itself?
If not, then it wouldn't have control over this.
> + if (!(buf.stx_mask & STATX_DIOALIGN)) {
> + tst_res(TCONF, "STATX_DIOALIGN is not supported until linux 6.1");
> + return;
> + }
"Filesystem does not support STATX_DIOALIGN"
> +
> +#ifdef HAVE_STRUCT_STATX_STX_DIO_MEM_ALIGN
This looks wrong. If the system headers are missing this field, then the
definition in the LTP source tree should be used instead.
> + if (buf.stx_dio_mem_align != 0)
> + tst_res(TPASS, "stx_dio_mem_align: %u", buf.stx_dio_mem_align);
> + else
> + tst_res(TFAIL, "don't get stx_dio_mem_align on supported dio fs");
For the failure case: "stx_dio_mem_align was 0, but DIO should be supported"
> +
> + if (buf.stx_dio_offset_align != 0)
> + tst_res(TPASS, "stx_dio_offset_align: %u", buf.stx_dio_offset_align);
> + else
> + tst_res(TFAIL, "don't get stx_dio_offset_align on supported dio fs");
> +#endif
For the failure case: "stx_dio_offset_align was 0, but DIO should be supported"
> + SAFE_FILE_PRINTF(TESTFILE, "AAAA");
> + fd = open(TESTFILE, O_RDWR | O_DIRECT);
> + if (fd == -1 && errno == EINVAL) {
> + SAFE_CLOSE(fd);
> + tst_brk(TCONF, "The regular file is not on a filesystem that support DIO");
> + }
> + SAFE_CLOSE(fd);
The open() is not checked for error in all cases.
Also, this is closing the file descriptor even when it is -1.
> +static struct tst_test test = {
> + .test_all = verify_statx,
> + .setup = setup,
> + .needs_root = 1,
Why does this test need root?
- Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v4 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file
2023-04-26 22:06 ` Eric Biggers
@ 2023-04-27 3:03 ` Yang Xu (Fujitsu)
2023-05-01 17:44 ` Eric Biggers
0 siblings, 1 reply; 46+ messages in thread
From: Yang Xu (Fujitsu) @ 2023-04-27 3:03 UTC (permalink / raw)
To: Eric Biggers; +Cc: ltp@lists.linux.it
on 2023/04/27 6:06, Eric Biggers wrote:
> On Thu, Apr 06, 2023 at 01:40:20PM +0800, Yang Xu wrote:
>> + * On ext4, files that use certain filesystem features (data journaling,
>> + * encryption, and verity) fall back to buffered I/O. But ltp doesn't use these
>> + * features by default, So I think dio should not fall back to buffered I/O.
>
> Does LTP create and mount the filesystem itself?
Yes, I have enabled mount_device in tst_test struct, mount_device usage
you can see the following url.
https://github.com/linux-test-project/ltp/wiki/C-Test-API#115-testing-with-a-block-device
If we set block device to LTP_DEV environment, we use this block device
to mount. Otherwise, use loop device to simuate it.
>
> If not, then it wouldn't have control over this.
>
>> + if (!(buf.stx_mask & STATX_DIOALIGN)) {
>> + tst_res(TCONF, "STATX_DIOALIGN is not supported until linux 6.1");
>> + return;
>> + }
>
> "Filesystem does not support STATX_DIOALIGN"
OK.
>
>> +
>> +#ifdef HAVE_STRUCT_STATX_STX_DIO_MEM_ALIGN
>
> This looks wrong. If the system headers are missing this field, then the
> definition in the LTP source tree should be used instead.
Yes, usually, if system headers miss this field, we should use ltp
definition ie some macro. But here it has a little difference, it is a
member in a struct.
see include/lapi/stat.h
#if defined(HAVE_STRUCT_STATX)
#include <sys/stat.h>
#else
struct statx {
/* 0x00 */
uint32_t stx_mask;
uint32_t stx_blksize;
uint64_t stx_attributes;
/* 0x10 */
uint32_t stx_nlink;
uint32_t stx_uid;
uint32_t stx_gid;
uint16_t stx_mode;
uint16_t __spare0[1];
/* 0x20 */
uint64_t stx_ino;
uint64_t stx_size;
uint64_t stx_blocks;
uint64_t stx_attributes_mask;
/* 0x40 */
const struct statx_timestamp stx_atime;
const struct statx_timestamp stx_btime;
const struct statx_timestamp stx_ctime;
const struct statx_timestamp stx_mtime;
/* 0x80 */
uint32_t stx_rdev_major;
uint32_t stx_rdev_minor;
uint32_t stx_dev_major;
uint32_t stx_dev_minor;
/* 0x90 */
uint64_t __spare2[14];
/* 0x100 */
};
#endif
the ltp definition only can be used when <sys/stat.h> miss statx struct
instead of statx struct member. It seems we don't have a better idea.
Or do you have some idea?
It seems we think this question more complex, if system header miss,
then use ltp definition, then we can not figure out whether fail or we
just on old kernel. Except we add a mininl kernel check in the beginning.
>
>> + if (buf.stx_dio_mem_align != 0)
>> + tst_res(TPASS, "stx_dio_mem_align: %u", buf.stx_dio_mem_align);
>> + else
>> + tst_res(TFAIL, "don't get stx_dio_mem_align on supported dio fs");
>
> For the failure case: "stx_dio_mem_align was 0, but DIO should be supported"
OK.
>
>> +
>> + if (buf.stx_dio_offset_align != 0)
>> + tst_res(TPASS, "stx_dio_offset_align: %u", buf.stx_dio_offset_align);
>> + else
>> + tst_res(TFAIL, "don't get stx_dio_offset_align on supported dio fs");
>> +#endif
>
> For the failure case: "stx_dio_offset_align was 0, but DIO should be supported"
OK.
>
>> + SAFE_FILE_PRINTF(TESTFILE, "AAAA");
>> + fd = open(TESTFILE, O_RDWR | O_DIRECT);
>> + if (fd == -1 && errno == EINVAL) {
>> + SAFE_CLOSE(fd);
>> + tst_brk(TCONF, "The regular file is not on a filesystem that support DIO");
>> + }
>> + SAFE_CLOSE(fd);
>
> The open() is not checked for error in all cases.
how about the following code:
fd = open(TESTFILE, O_RDWR | O_DIRECT);
if (fd == -1) {
if (errno == EINVAL)
tst_brk(TCONF, "The regular file is not on a filesystem that support
DIO");
else
tst_brk(TBROK | TERRNO, "The regular file was open with O_RDWR |
O_DIRECT failed");
}
SAFE_CLOSE(fd);
>
> Also, this is closing the file descriptor even when it is -1.
Oh, my mistake, sorry.
>
>> +static struct tst_test test = {
>> + .test_all = verify_statx,
>> + .setup = setup,
>> + .needs_root = 1,
>
> Why does this test need root?
When using /dev/loop-control to search free loop device, we needs root.
see below:
tst_device.c:108: TINFO: Not allowed to open /dev/loop-control. Are you
root?: EACCES (13)
tst_device.c:147: TINFO: No free devices found
tst_device.c:354: TBROK: Failed to acquire device
Best Regards
Yang Xu
>
> - Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v4 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file
2023-04-27 3:03 ` Yang Xu (Fujitsu)
@ 2023-05-01 17:44 ` Eric Biggers
2023-05-01 17:47 ` Eric Biggers
2023-05-08 8:25 ` Yang Xu (Fujitsu)
0 siblings, 2 replies; 46+ messages in thread
From: Eric Biggers @ 2023-05-01 17:44 UTC (permalink / raw)
To: Yang Xu (Fujitsu); +Cc: ltp@lists.linux.it
On Thu, Apr 27, 2023 at 03:03:23AM +0000, Yang Xu (Fujitsu) wrote:
> on 2023/04/27 6:06, Eric Biggers wrote:
> > On Thu, Apr 06, 2023 at 01:40:20PM +0800, Yang Xu wrote:
> >> + * On ext4, files that use certain filesystem features (data journaling,
> >> + * encryption, and verity) fall back to buffered I/O. But ltp doesn't use these
> >> + * features by default, So I think dio should not fall back to buffered I/O.
> >
> > Does LTP create and mount the filesystem itself?
>
> Yes, I have enabled mount_device in tst_test struct, mount_device usage
> you can see the following url.
> https://github.com/linux-test-project/ltp/wiki/C-Test-API#115-testing-with-a-block-device
>
> If we set block device to LTP_DEV environment, we use this block device
> to mount. Otherwise, use loop device to simuate it.
Great, can you update the comment to make it clear that this test creates its
own filesystem?
> >
> > If not, then it wouldn't have control over this.
> >
> >> + if (!(buf.stx_mask & STATX_DIOALIGN)) {
> >> + tst_res(TCONF, "STATX_DIOALIGN is not supported until linux 6.1");
> >> + return;
> >> + }
> >
> > "Filesystem does not support STATX_DIOALIGN"
>
> OK.
> >
> >> +
> >> +#ifdef HAVE_STRUCT_STATX_STX_DIO_MEM_ALIGN
> >
> > This looks wrong. If the system headers are missing this field, then the
> > definition in the LTP source tree should be used instead.
>
> Yes, usually, if system headers miss this field, we should use ltp
> definition ie some macro. But here it has a little difference, it is a
> member in a struct.
>
> see include/lapi/stat.h
>
> #if defined(HAVE_STRUCT_STATX)
> #include <sys/stat.h>
> #else
> struct statx {
> /* 0x00 */
> uint32_t stx_mask;
> uint32_t stx_blksize;
> uint64_t stx_attributes;
> /* 0x10 */
> uint32_t stx_nlink;
> uint32_t stx_uid;
> uint32_t stx_gid;
> uint16_t stx_mode;
> uint16_t __spare0[1];
> /* 0x20 */
> uint64_t stx_ino;
> uint64_t stx_size;
> uint64_t stx_blocks;
> uint64_t stx_attributes_mask;
> /* 0x40 */
> const struct statx_timestamp stx_atime;
> const struct statx_timestamp stx_btime;
> const struct statx_timestamp stx_ctime;
> const struct statx_timestamp stx_mtime;
> /* 0x80 */
> uint32_t stx_rdev_major;
> uint32_t stx_rdev_minor;
> uint32_t stx_dev_major;
> uint32_t stx_dev_minor;
> /* 0x90 */
> uint64_t __spare2[14];
> /* 0x100 */
> };
> #endif
>
> the ltp definition only can be used when <sys/stat.h> miss statx struct
> instead of statx struct member. It seems we don't have a better idea.
> Or do you have some idea?
>
> It seems we think this question more complex, if system header miss,
> then use ltp definition, then we can not figure out whether fail or we
> just on old kernel. Except we add a mininl kernel check in the beginning.
>
As I said, if the system headers are missing the needed fields, then LTP should
use its in-tree definition. I.e., the in-tree definition should only be used if
HAVE_STRUCT_STATX && HAVE_STRUCT_STATX_STX_MNT_ID && [all other tested fields].
> >> + SAFE_FILE_PRINTF(TESTFILE, "AAAA");
> >> + fd = open(TESTFILE, O_RDWR | O_DIRECT);
> >> + if (fd == -1 && errno == EINVAL) {
> >> + SAFE_CLOSE(fd);
> >> + tst_brk(TCONF, "The regular file is not on a filesystem that support DIO");
> >> + }
> >> + SAFE_CLOSE(fd);
> >
> > The open() is not checked for error in all cases.
>
> how about the following code:
>
>
> fd = open(TESTFILE, O_RDWR | O_DIRECT);
> if (fd == -1) {
> if (errno == EINVAL)
> tst_brk(TCONF, "The regular file is not on a filesystem that support
> DIO");
> else
> tst_brk(TBROK | TERRNO, "The regular file was open with O_RDWR |
> O_DIRECT failed");
> }
> SAFE_CLOSE(fd);
I think that's okay.
- Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v4 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file
2023-05-01 17:44 ` Eric Biggers
@ 2023-05-01 17:47 ` Eric Biggers
2023-05-08 8:25 ` Yang Xu (Fujitsu)
1 sibling, 0 replies; 46+ messages in thread
From: Eric Biggers @ 2023-05-01 17:47 UTC (permalink / raw)
To: Yang Xu (Fujitsu); +Cc: ltp@lists.linux.it
On Mon, May 01, 2023 at 10:44:42AM -0700, Eric Biggers wrote:
> As I said, if the system headers are missing the needed fields, then LTP should
> use its in-tree definition. I.e., the in-tree definition should only be used if
> HAVE_STRUCT_STATX && HAVE_STRUCT_STATX_STX_MNT_ID && [all other tested fields].
"the in-tree definition should only be used" =>
"the system definition should only be used"
- Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [LTP] [PATCH v4 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file
2023-05-01 17:44 ` Eric Biggers
2023-05-01 17:47 ` Eric Biggers
@ 2023-05-08 8:25 ` Yang Xu (Fujitsu)
2023-05-08 8:30 ` Yang Xu (Fujitsu)
1 sibling, 1 reply; 46+ messages in thread
From: Yang Xu (Fujitsu) @ 2023-05-08 8:25 UTC (permalink / raw)
To: Eric Biggers; +Cc: ltp@lists.linux.it
on 2023/05/02 1:44, Eric Biggers wrote:
> On Thu, Apr 27, 2023 at 03:03:23AM +0000, Yang Xu (Fujitsu) wrote:
>> on 2023/04/27 6:06, Eric Biggers wrote:
>>> On Thu, Apr 06, 2023 at 01:40:20PM +0800, Yang Xu wrote:
>>>> + * On ext4, files that use certain filesystem features (data journaling,
>>>> + * encryption, and verity) fall back to buffered I/O. But ltp doesn't use these
>>>> + * features by default, So I think dio should not fall back to buffered I/O.
>>>
>>> Does LTP create and mount the filesystem itself?
>>
>> Yes, I have enabled mount_device in tst_test struct, mount_device usage
>> you can see the following url.
>> https://github.com/linux-test-project/ltp/wiki/C-Test-API#115-testing-with-a-block-device
>>
>> If we set block device to LTP_DEV environment, we use this block device
>> to mount. Otherwise, use loop device to simuate it.
>
> Great, can you update the comment to make it clear that this test creates its
> own filesystem?
Of course.
>
>>>
>>> If not, then it wouldn't have control over this.
>>>
>>>> + if (!(buf.stx_mask & STATX_DIOALIGN)) {
>>>> + tst_res(TCONF, "STATX_DIOALIGN is not supported until linux 6.1");
>>>> + return;
>>>> + }
>>>
>>> "Filesystem does not support STATX_DIOALIGN"
>>
>> OK.
>>>
>>>> +
>>>> +#ifdef HAVE_STRUCT_STATX_STX_DIO_MEM_ALIGN
>>>
>>> This looks wrong. If the system headers are missing this field, then the
>>> definition in the LTP source tree should be used instead.
>>
>> Yes, usually, if system headers miss this field, we should use ltp
>> definition ie some macro. But here it has a little difference, it is a
>> member in a struct.
>>
>> see include/lapi/stat.h
>>
>> #if defined(HAVE_STRUCT_STATX)
>> #include <sys/stat.h>
>> #else
>> struct statx {
>> /* 0x00 */
>> uint32_t stx_mask;
>> uint32_t stx_blksize;
>> uint64_t stx_attributes;
>> /* 0x10 */
>> uint32_t stx_nlink;
>> uint32_t stx_uid;
>> uint32_t stx_gid;
>> uint16_t stx_mode;
>> uint16_t __spare0[1];
>> /* 0x20 */
>> uint64_t stx_ino;
>> uint64_t stx_size;
>> uint64_t stx_blocks;
>> uint64_t stx_attributes_mask;
>> /* 0x40 */
>> const struct statx_timestamp stx_atime;
>> const struct statx_timestamp stx_btime;
>> const struct statx_timestamp stx_ctime;
>> const struct statx_timestamp stx_mtime;
>> /* 0x80 */
>> uint32_t stx_rdev_major;
>> uint32_t stx_rdev_minor;
>> uint32_t stx_dev_major;
>> uint32_t stx_dev_minor;
>> /* 0x90 */
>> uint64_t __spare2[14];
>> /* 0x100 */
>> };
>> #endif
>>
>> the ltp definition only can be used when <sys/stat.h> miss statx struct
>> instead of statx struct member. It seems we don't have a better idea.
>> Or do you have some idea?
>>
>> It seems we think this question more complex, if system header miss,
>> then use ltp definition, then we can not figure out whether fail or we
>> just on old kernel. Except we add a mininl kernel check in the beginning.
>>
>
> As I said, if the system headers are missing the needed fields, then LTP should
> use its in-tree definition. I.e., the in-tree definition should only be used if
> HAVE_STRUCT_STATX && HAVE_STRUCT_STATX_STX_MNT_ID && [all other tested fields].
Yes, it should work well but ltp has other owner headers(they still
include <sys/stat.h>), so it can't work well
I try it as below:
+#if defined(HAVE_STATX) && \
+ defined(HAVE_STRUCT_STATX_TIMESTAMP) && \
+ defined(HAVE_STRUCT_STATX) && \
+ defined(HAVE_STRUCT_STATX_STX_MNT_ID)
+#include <sys/stat.h>
+#else
....
#endif
see ltp/include, ltp owner header also uses <sys/stat.h>(use stat
struct or stat syscall)
safe_file_ops_fn.h:21:#include <sys/stat.h>
safe_macros_fn.h:19:#include <sys/stat.h>
tst_device.h:11:#include <sys/stat.h>
tst_safe_file_at.h:10:#include <sys/stat.h>
tst_safe_macros.h:13:#include <sys/stat.h>
If I remove mnt_id ifdef check in statx01.c, then statx01 will report
redefine error for statx struct as below:
In file included from statx01.c:35:
../../../../include/lapi/stat.h:30:8: error: redefinition of ‘struct
statx_timestamp’
struct statx_timestamp {
^~~~~~~~~~~~~~~
In file included from /usr/include/bits/statx.h:31,
from /usr/include/sys/stat.h:446,
from ../../../../include/tst_device.h:11,
from ../../../../include/tst_test.h:23,
from statx01.c:33:
/usr/include/linux/stat.h:56:8: note: originally defined here
struct statx_timestamp {
^~~~~~~~~~~~~~~
In file included from statx01.c:35:
../../../../include/lapi/stat.h:72:8: error: redefinition of ‘struct statx’
struct statx {
^~~~~
In file included from /usr/include/bits/statx.h:31,
from /usr/include/sys/stat.h:446,
from ../../../../include/tst_device.h:11,
from ../../../../include/tst_test.h:23,
from statx01.c:33:
/usr/include/linux/stat.h:99:8: note: originally defined here
struct statx {
^~~~~
In file included from statx01.c:35:
../../../../include/lapi/stat.h:113:19: error: conflicting types for ‘statx’
static inline int statx(int dirfd, const char *pathname, unsigned int
flags,
^~~~~
In file included from /usr/include/bits/statx.h:39,
from /usr/include/sys/stat.h:446,
from ../../../../include/tst_device.h:11,
from ../../../../include/tst_test.h:23,
from statx01.c:33:
/usr/include/bits/statx-generic.h:56:5: note: previous declaration of
‘statx’ was here
int statx (int __dirfd, const char *__restrict __path, int __flags,
^~~~~
statx01.c:96:2: error: #endif without #if
#endif
^~~~~
IMO, to change ltp owner header to avoid use <sys/mount.h> seems
difficulty.
Best Regards
Yang Xu
>
>>>> + SAFE_FILE_PRINTF(TESTFILE, "AAAA");
>>>> + fd = open(TESTFILE, O_RDWR | O_DIRECT);
>>>> + if (fd == -1 && errno == EINVAL) {
>>>> + SAFE_CLOSE(fd);
>>>> + tst_brk(TCONF, "The regular file is not on a filesystem that support DIO");
>>>> + }
>>>> + SAFE_CLOSE(fd);
>>>
>>> The open() is not checked for error in all cases.
>>
>> how about the following code:
>>
>>
>> fd = open(TESTFILE, O_RDWR | O_DIRECT);
>> if (fd == -1) {
>> if (errno == EINVAL)
>> tst_brk(TCONF, "The regular file is not on a filesystem that support
>> DIO");
>> else
>> tst_brk(TBROK | TERRNO, "The regular file was open with O_RDWR |
>> O_DIRECT failed");
>> }
>> SAFE_CLOSE(fd);
>
> I think that's okay.
>
> - Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread
* [LTP] [PATCH v4 3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device
2023-04-06 5:40 ` [LTP] [PATCH v4 1/4] lapi/stat.h: Add STATX_DIOALIGN related definition Yang Xu
2023-04-06 5:40 ` [LTP] [PATCH v4 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file Yang Xu
@ 2023-04-06 5:40 ` Yang Xu
2023-04-26 22:12 ` Eric Biggers
2023-04-06 5:40 ` [LTP] [PATCH v4 4/4] lapi/stat.h: Remove deprecated STATX_ALL macro Yang Xu
` (2 subsequent siblings)
4 siblings, 1 reply; 46+ messages in thread
From: Yang Xu @ 2023-04-06 5:40 UTC (permalink / raw)
To: ltp
Since STATX_DIOLAIGN is only supported on regular file and block device,
so this case is used to test the latter.
This test is tightly coupled to the kernel's current DIO restrictions on block
devices. These changed in v6.0, and they are subject to further change in the
future.
It is fine for now because STATX_DIOALIGN is only in v6.1 and later
anyway.
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
runtest/syscalls | 1 +
testcases/kernel/syscalls/statx/.gitignore | 1 +
testcases/kernel/syscalls/statx/statx11.c | 81 ++++++++++++++++++++++
3 files changed, 83 insertions(+)
create mode 100644 testcases/kernel/syscalls/statx/statx11.c
diff --git a/runtest/syscalls b/runtest/syscalls
index 92123772c..de5f0be35 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1770,6 +1770,7 @@ statx07 statx07
statx08 statx08
statx09 statx09
statx10 statx10
+statx11 statx11
membarrier01 membarrier01
diff --git a/testcases/kernel/syscalls/statx/.gitignore b/testcases/kernel/syscalls/statx/.gitignore
index 67341ff2d..48ac4078b 100644
--- a/testcases/kernel/syscalls/statx/.gitignore
+++ b/testcases/kernel/syscalls/statx/.gitignore
@@ -8,3 +8,4 @@
/statx08
/statx09
/statx10
+/statx11
diff --git a/testcases/kernel/syscalls/statx/statx11.c b/testcases/kernel/syscalls/statx/statx11.c
new file mode 100644
index 000000000..02449888a
--- /dev/null
+++ b/testcases/kernel/syscalls/statx/statx11.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023 FUJITSU LIMITED. All rights reserved.
+ * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * It is a basic test for STATX_DIOALIGN mask on block device.
+ *
+ * - STATX_DIOALIGN Want stx_dio_mem_align and stx_dio_offset_align value
+ *
+ * These two values are tightly coupled to the kernel's current DIO
+ * restrictions on block devices.
+ *
+ * Minimum Linux version required is v6.1.
+ */
+
+#define _GNU_SOURCE
+#include <sys/types.h>
+#include <sys/mount.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include "tst_test.h"
+#include "lapi/stat.h"
+
+static char sys_bdev_dma_path[1024], sys_bdev_logical_path[1024];
+
+static void verify_statx(void)
+{
+ struct statx buf;
+
+ memset(&buf, 0, sizeof(buf));
+ TST_EXP_PASS_SILENT(statx(AT_FDCWD, tst_device->dev, 0, STATX_DIOALIGN, &buf),
+ "statx(AT_FDCWD, %s, 0, STATX_DIOALIGN, &buf)", tst_device->dev);
+
+ if (!(buf.stx_mask & STATX_DIOALIGN)) {
+ tst_res(TCONF, "STATX_DIOALIGN is not supported until linux 6.1");
+ return;
+ }
+
+#ifdef HAVE_STRUCT_STATX_STX_DIO_MEM_ALIGN
+ /*
+ * This test is tightly coupled to the kernel's current DIO restrictions
+ * on block devices. The general rule of DIO needing to be aligned to the
+ * block device's logical block size was relaxed to allow user buffers
+ * (but not file offsets) aligned to the DMA alignment instead. See v6.0
+ * commit bf8d08532bc1 ("iomap: add support for dma aligned direct-io") and
+ * they are subject to further change in the future.
+ * Also can see commit 2d985f8c6b9 ("vfs: support STATX_DIOALIGN on block devices).
+ */
+ TST_ASSERT_ULONG(sys_bdev_dma_path, buf.stx_dio_mem_align - 1);
+ TST_ASSERT_ULONG(sys_bdev_logical_path, buf.stx_dio_offset_align);
+#endif
+}
+
+static void setup(void)
+{
+ char *dev_name;
+
+ dev_name = basename((char *)tst_device->dev);
+ sprintf(sys_bdev_logical_path, "/sys/block/%s/queue/logical_block_size", dev_name);
+ while (access(sys_bdev_logical_path, F_OK) != 0) {
+ dev_name[strlen(dev_name)-1] = '\0';
+ sprintf(sys_bdev_logical_path, "/sys/block/%s/queue/logical_block_size", dev_name);
+ }
+
+ sprintf(sys_bdev_dma_path, "/sys/block/%s/queue/dma_alignment", dev_name);
+ if (access(sys_bdev_dma_path, F_OK) != 0)
+ tst_brk(TCONF, "dma_alignment sysfs file doesn't exist");
+}
+
+static struct tst_test test = {
+ .test_all = verify_statx,
+ .setup = setup,
+ .needs_device = 1,
+ .needs_root = 1,
+};
--
2.39.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v4 3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device
2023-04-06 5:40 ` [LTP] [PATCH v4 3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device Yang Xu
@ 2023-04-26 22:12 ` Eric Biggers
2023-04-27 3:37 ` Yang Xu (Fujitsu)
0 siblings, 1 reply; 46+ messages in thread
From: Eric Biggers @ 2023-04-26 22:12 UTC (permalink / raw)
To: Yang Xu; +Cc: ltp
On Thu, Apr 06, 2023 at 01:40:21PM +0800, Yang Xu wrote:
> +static void verify_statx(void)
> +{
> + struct statx buf;
> +
> + memset(&buf, 0, sizeof(buf));
It is not necessary to memset struct statx to 0 before calling statx().
> +#ifdef HAVE_STRUCT_STATX_STX_DIO_MEM_ALIGN
Again, this looks wrong. If the system headers are outdated, then LTP should
use its in-tree header instead.
> +static void setup(void)
> +{
> + char *dev_name;
> +
> + dev_name = basename((char *)tst_device->dev);
This is modifying a const string, which seems problematic.
> + sprintf(sys_bdev_logical_path, "/sys/block/%s/queue/logical_block_size", dev_name);
> + while (access(sys_bdev_logical_path, F_OK) != 0) {
> + dev_name[strlen(dev_name)-1] = '\0';
> + sprintf(sys_bdev_logical_path, "/sys/block/%s/queue/logical_block_size", dev_name);
> + }
What is this code doing? Is it trying to strip off the partition number of the
block device name? If so, it is incorrect because it assumes the partition
number is only 1 digit long, which is not guaranteed.
How about just using /sys/class/block/%s/queue, which works for partitions?
- Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v4 3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device
2023-04-26 22:12 ` Eric Biggers
@ 2023-04-27 3:37 ` Yang Xu (Fujitsu)
2023-04-27 3:50 ` Yang Xu (Fujitsu)
0 siblings, 1 reply; 46+ messages in thread
From: Yang Xu (Fujitsu) @ 2023-04-27 3:37 UTC (permalink / raw)
To: Eric Biggers; +Cc: ltp@lists.linux.it
2023/04/27 6:12, Eric Biggers 写道:
> On Thu, Apr 06, 2023 at 01:40:21PM +0800, Yang Xu wrote:
>> +static void verify_statx(void)
>> +{
>> + struct statx buf;
>> +
>> + memset(&buf, 0, sizeof(buf));
>
> It is not necessary to memset struct statx to 0 before calling statx().
Will remove.
>
>> +#ifdef HAVE_STRUCT_STATX_STX_DIO_MEM_ALIGN
>
> Again, this looks wrong. If the system headers are outdated, then LTP should
> use its in-tree header instead.
Have mention this in the previous email. We can discussion this in that
email.
>
>> +static void setup(void)
>> +{
>> + char *dev_name;
>> +
>> + dev_name = basename((char *)tst_device->dev);
>
> This is modifying a const string, which seems problematic.
Yes, I plan to modify code as below:
char full_name[256];
strcpy(full_name, tst_device->dev);
dev_name = SAFE_BASENAME(full_name);
>
>> + sprintf(sys_bdev_logical_path, "/sys/block/%s/queue/logical_block_size", dev_name);
>> + while (access(sys_bdev_logical_path, F_OK) != 0) {
>> + dev_name[strlen(dev_name)-1] = '\0';
>> + sprintf(sys_bdev_logical_path, "/sys/block/%s/queue/logical_block_size", dev_name);
>> + }
>
> What is this code doing? Is it trying to strip off the partition number of the
> block device name?
Yes.
>If so, it is incorrect because it assumes the partition
> number is only 1 digit long, which is not guaranteed.
I don't assume the partition number is only 1 digit long, this code has
a while circulate. Also, I try the /dev/vdb11 and it also works.
>
> How about just using /sys/class/block/%s/queue, which works for partitions?
In fact, /sys/block or /sys/class/block, these files are all link files
to /sys/device/pci...... see below:
#cd /sys/class/block
[root@localhost block]# ls -l
total 0
lrwxrwxrwx. 1 root root 0 Apr 20 00:07 loop0 ->
../../devices/virtual/block/loop0
lrwxrwxrwx. 1 root root 0 Apr 20 00:07 loop1 ->
../../devices/virtual/block/loop1
lrwxrwxrwx. 1 root root 0 Apr 27 08:25 sda ->
../../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda
lrwxrwxrwx. 1 root root 0 Apr 27 08:25 sda1 ->
../../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda/sda1
lrwxrwxrwx. 1 root root 0 Apr 27 08:25 sda2 ->
../../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda/sda2
lrwxrwxrwx. 1 root root 0 Apr 27 08:25 sda3 ->
../../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda/sda3
lrwxrwxrwx. 1 root root 0 Apr 27 08:25 sda4 ->
../../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda/sda4
lrwxrwxrwx. 1 root root 0 Apr 27 08:25 sda5 ->
../../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda/sda5
lrwxrwxrwx. 1 root root 0 Apr 27 08:25 sda6 ->
../../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda/sda6
lrwxrwxrwx. 1 root root 0 Apr 27 08:25 zram0 ->
../../devices/virtual/block/zram0
[root@localhost block]# cd /sys/block/
[root@localhost block]# ls -l
total 0
lrwxrwxrwx. 1 root root 0 Apr 20 00:03 loop0 ->
../devices/virtual/block/loop0
lrwxrwxrwx. 1 root root 0 Apr 20 00:03 loop1 ->
../devices/virtual/block/loop1
lrwxrwxrwx. 1 root root 0 Apr 20 00:03 sda ->
../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda
lrwxrwxrwx. 1 root root 0 Apr 20 00:03 zram0 ->
../devices/virtual/block/zram0
[root@localhost block]# pwd
Best Regards
Yang Xu
>
> - Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v4 3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device
2023-04-27 3:37 ` Yang Xu (Fujitsu)
@ 2023-04-27 3:50 ` Yang Xu (Fujitsu)
2023-05-01 17:49 ` Eric Biggers
0 siblings, 1 reply; 46+ messages in thread
From: Yang Xu (Fujitsu) @ 2023-04-27 3:50 UTC (permalink / raw)
To: Eric Biggers; +Cc: ltp@lists.linux.it
on 2023/04/27 11:37, Yang Xu (Fujitsu) wrote:
>
>
> 2023/04/27 6:12, Eric Biggers 写道:
>> On Thu, Apr 06, 2023 at 01:40:21PM +0800, Yang Xu wrote:
>>> +static void verify_statx(void)
>>> +{
>>> + struct statx buf;
>>> +
>>> + memset(&buf, 0, sizeof(buf));
>>
>> It is not necessary to memset struct statx to 0 before calling statx().
>
> Will remove.
>>
>>> +#ifdef HAVE_STRUCT_STATX_STX_DIO_MEM_ALIGN
>>
>> Again, this looks wrong. If the system headers are outdated, then LTP should
>> use its in-tree header instead.
>
> Have mention this in the previous email. We can discussion this in that
> email.
>
>>
>>> +static void setup(void)
>>> +{
>>> + char *dev_name;
>>> +
>>> + dev_name = basename((char *)tst_device->dev);
>>
>> This is modifying a const string, which seems problematic.
>
> Yes, I plan to modify code as below:
> char full_name[256];
>
> strcpy(full_name, tst_device->dev);
> dev_name = SAFE_BASENAME(full_name);
>
>>
>>> + sprintf(sys_bdev_logical_path, "/sys/block/%s/queue/logical_block_size", dev_name);
>>> + while (access(sys_bdev_logical_path, F_OK) != 0) {
>>> + dev_name[strlen(dev_name)-1] = '\0';
>>> + sprintf(sys_bdev_logical_path, "/sys/block/%s/queue/logical_block_size", dev_name);
>>> + }
>>
>> What is this code doing? Is it trying to strip off the partition number of the
>> block device name?
>
> Yes.
>
>> If so, it is incorrect because it assumes the partition
>> number is only 1 digit long, which is not guaranteed.
>
> I don't assume the partition number is only 1 digit long, this code has
> a while circulate. Also, I try the /dev/vdb11 and it also works.
>
>
>>
>> How about just using /sys/class/block/%s/queue, which works for partitions?
/sys/class/block/%s/queue for partitions does't exist.
[root@localhost sda5]# pwd
/sys/class/block/sda5
[root@localhost sda5]# ls
alignment_offset dev discard_alignment holders inflight partition
power ro size start stat subsystem trace uevent
[root@localhost sda5]#
Best Regards
Yang Xu
>
> In fact, /sys/block or /sys/class/block, these files are all link files
> to /sys/device/pci...... see below:
> #cd /sys/class/block
> [root@localhost block]# ls -l
> total 0
> lrwxrwxrwx. 1 root root 0 Apr 20 00:07 loop0 ->
> ../../devices/virtual/block/loop0
> lrwxrwxrwx. 1 root root 0 Apr 20 00:07 loop1 ->
> ../../devices/virtual/block/loop1
> lrwxrwxrwx. 1 root root 0 Apr 27 08:25 sda ->
> ../../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda
> lrwxrwxrwx. 1 root root 0 Apr 27 08:25 sda1 ->
> ../../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda/sda1
> lrwxrwxrwx. 1 root root 0 Apr 27 08:25 sda2 ->
> ../../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda/sda2
> lrwxrwxrwx. 1 root root 0 Apr 27 08:25 sda3 ->
> ../../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda/sda3
> lrwxrwxrwx. 1 root root 0 Apr 27 08:25 sda4 ->
> ../../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda/sda4
> lrwxrwxrwx. 1 root root 0 Apr 27 08:25 sda5 ->
> ../../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda/sda5
> lrwxrwxrwx. 1 root root 0 Apr 27 08:25 sda6 ->
> ../../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda/sda6
> lrwxrwxrwx. 1 root root 0 Apr 27 08:25 zram0 ->
> ../../devices/virtual/block/zram0
> [root@localhost block]# cd /sys/block/
> [root@localhost block]# ls -l
> total 0
> lrwxrwxrwx. 1 root root 0 Apr 20 00:03 loop0 ->
> ../devices/virtual/block/loop0
> lrwxrwxrwx. 1 root root 0 Apr 20 00:03 loop1 ->
> ../devices/virtual/block/loop1
> lrwxrwxrwx. 1 root root 0 Apr 20 00:03 sda ->
> ../devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda
> lrwxrwxrwx. 1 root root 0 Apr 20 00:03 zram0 ->
> ../devices/virtual/block/zram0
> [root@localhost block]# pwd
>
> Best Regards
> Yang Xu
>
>>
>> - Eric
>
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v4 3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device
2023-04-27 3:50 ` Yang Xu (Fujitsu)
@ 2023-05-01 17:49 ` Eric Biggers
2023-05-08 8:26 ` Yang Xu (Fujitsu)
0 siblings, 1 reply; 46+ messages in thread
From: Eric Biggers @ 2023-05-01 17:49 UTC (permalink / raw)
To: Yang Xu (Fujitsu); +Cc: ltp@lists.linux.it
On Thu, Apr 27, 2023 at 03:50:25AM +0000, Yang Xu (Fujitsu) wrote:
> >>> + sprintf(sys_bdev_logical_path, "/sys/block/%s/queue/logical_block_size", dev_name);
> >>> + while (access(sys_bdev_logical_path, F_OK) != 0) {
> >>> + dev_name[strlen(dev_name)-1] = '\0';
> >>> + sprintf(sys_bdev_logical_path, "/sys/block/%s/queue/logical_block_size", dev_name);
> >>> + }
> >>
> >> What is this code doing? Is it trying to strip off the partition number of the
> >> block device name?
> >
> > Yes.
> >
> >> If so, it is incorrect because it assumes the partition
> >> number is only 1 digit long, which is not guaranteed.
> >
> > I don't assume the partition number is only 1 digit long, this code has
> > a while circulate. Also, I try the /dev/vdb11 and it also works.
> >
> >
> >>
> >> How about just using /sys/class/block/%s/queue, which works for partitions?
>
> /sys/class/block/%s/queue for partitions does't exist.
Okay, sorry, I forgot that /sys/class/block/%s/queue doesn't exist for
partitions.
Please at least add a comment that explains what this code is doing, as it's
hard to understand.
- Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v4 3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device
2023-05-01 17:49 ` Eric Biggers
@ 2023-05-08 8:26 ` Yang Xu (Fujitsu)
0 siblings, 0 replies; 46+ messages in thread
From: Yang Xu (Fujitsu) @ 2023-05-08 8:26 UTC (permalink / raw)
To: Eric Biggers; +Cc: ltp@lists.linux.it
on 2023/05/02 1:49, Eric Biggers wrote:
> On Thu, Apr 27, 2023 at 03:50:25AM +0000, Yang Xu (Fujitsu) wrote:
>>>>> + sprintf(sys_bdev_logical_path, "/sys/block/%s/queue/logical_block_size", dev_name);
>>>>> + while (access(sys_bdev_logical_path, F_OK) != 0) {
>>>>> + dev_name[strlen(dev_name)-1] = '\0';
>>>>> + sprintf(sys_bdev_logical_path, "/sys/block/%s/queue/logical_block_size", dev_name);
>>>>> + }
>>>>
>>>> What is this code doing? Is it trying to strip off the partition number of the
>>>> block device name?
>>>
>>> Yes.
>>>
>>>> If so, it is incorrect because it assumes the partition
>>>> number is only 1 digit long, which is not guaranteed.
>>>
>>> I don't assume the partition number is only 1 digit long, this code has
>>> a while circulate. Also, I try the /dev/vdb11 and it also works.
>>>
>>>
>>>>
>>>> How about just using /sys/class/block/%s/queue, which works for partitions?
>>
>> /sys/class/block/%s/queue for partitions does't exist.
>
> Okay, sorry, I forgot that /sys/class/block/%s/queue doesn't exist for
> partitions.
>
> Please at least add a comment that explains what this code is doing, as it's
> hard to understand.
Yes, will add a comment for this
Best Regards
Yang Xu
>
> - Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread
* [LTP] [PATCH v4 4/4] lapi/stat.h: Remove deprecated STATX_ALL macro
2023-04-06 5:40 ` [LTP] [PATCH v4 1/4] lapi/stat.h: Add STATX_DIOALIGN related definition Yang Xu
2023-04-06 5:40 ` [LTP] [PATCH v4 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file Yang Xu
2023-04-06 5:40 ` [LTP] [PATCH v4 3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device Yang Xu
@ 2023-04-06 5:40 ` Yang Xu
2023-04-26 21:56 ` Eric Biggers
2023-04-26 9:57 ` [LTP] [PATCH v4 1/4] lapi/stat.h: Add STATX_DIOALIGN related definition Yang Xu (Fujitsu)
2023-04-26 21:56 ` Eric Biggers
4 siblings, 1 reply; 46+ messages in thread
From: Yang Xu @ 2023-04-06 5:40 UTC (permalink / raw)
To: ltp
Since kernel 5.10-rc1 commit 581701b7efd6 ("uapi: deprecate STATX_ALL"),
this flag has been mark as deprecated.
Kernel should keep this macro for compatibility, but ltp doesn't think
about it. So remove it.
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
include/lapi/stat.h | 4 ----
testcases/kernel/syscalls/statx/statx06.c | 4 ++--
testcases/kernel/syscalls/statx/statx07.c | 6 +++---
3 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/include/lapi/stat.h b/include/lapi/stat.h
index c2db8a589..7c9a7a00c 100644
--- a/include/lapi/stat.h
+++ b/include/lapi/stat.h
@@ -188,10 +188,6 @@ static inline int statx(int dirfd, const char *pathname, unsigned int flags,
# define STATX_DIOALIGN 0x00002000U
#endif
-#ifndef STATX_ALL
-# define STATX_ALL 0x00000fffU
-#endif
-
#ifndef STATX__RESERVED
# define STATX__RESERVED 0x80000000U
#endif
diff --git a/testcases/kernel/syscalls/statx/statx06.c b/testcases/kernel/syscalls/statx/statx06.c
index ce82b905b..60e736c5a 100644
--- a/testcases/kernel/syscalls/statx/statx06.c
+++ b/testcases/kernel/syscalls/statx/statx06.c
@@ -111,10 +111,10 @@ static void test_statx(unsigned int test_nr)
clock_wait_tick();
SAFE_CLOCK_GETTIME(CLOCK_REALTIME_COARSE, &after_time);
- TEST(statx(AT_FDCWD, TEST_FILE, 0, STATX_ALL, &buff));
+ TEST(statx(AT_FDCWD, TEST_FILE, 0, STATX_BASIC_STATS | STATX_BTIME, &buff));
if (TST_RET != 0) {
tst_brk(TFAIL | TTERRNO,
- "statx(AT_FDCWD, %s, 0, STATX_ALL, &buff)",
+ "statx(AT_FDCWD, %s, 0, STATX_BASIC_STATS | STATX_BTIME, &buff)",
TEST_FILE);
}
diff --git a/testcases/kernel/syscalls/statx/statx07.c b/testcases/kernel/syscalls/statx/statx07.c
index b13c11f72..c798c7a10 100644
--- a/testcases/kernel/syscalls/statx/statx07.c
+++ b/testcases/kernel/syscalls/statx/statx07.c
@@ -62,15 +62,15 @@ static int get_mode(char *file_name, int flag_type, char *flag_name)
{
struct statx buf;
- TEST(statx(AT_FDCWD, file_name, flag_type, STATX_ALL, &buf));
+ TEST(statx(AT_FDCWD, file_name, flag_type, STATX_BASIC_STATS | STATX_BTIME, &buf));
if (TST_RET == -1) {
tst_brk(TFAIL | TST_ERR,
- "statx(AT_FDCWD, %s, %s, STATX_ALL, &buf)",
+ "statx(AT_FDCWD, %s, %s, STATX_BASIC_STATS | STATX_BTIME, &buf)",
file_name, flag_name);
}
- tst_res(TINFO, "statx(AT_FDCWD, %s, %s, STATX_ALL, &buf) = %o",
+ tst_res(TINFO, "statx(AT_FDCWD, %s, %s, STATX_BASIC_STATS | STATX_BTIME, &buf) = %o",
file_name, flag_name, buf.stx_mode);
return buf.stx_mode;
--
2.39.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v4 4/4] lapi/stat.h: Remove deprecated STATX_ALL macro
2023-04-06 5:40 ` [LTP] [PATCH v4 4/4] lapi/stat.h: Remove deprecated STATX_ALL macro Yang Xu
@ 2023-04-26 21:56 ` Eric Biggers
2023-04-27 1:52 ` Yang Xu (Fujitsu)
0 siblings, 1 reply; 46+ messages in thread
From: Eric Biggers @ 2023-04-26 21:56 UTC (permalink / raw)
To: Yang Xu; +Cc: ltp
On Thu, Apr 06, 2023 at 01:40:22PM +0800, Yang Xu wrote:
> diff --git a/testcases/kernel/syscalls/statx/statx07.c b/testcases/kernel/syscalls/statx/statx07.c
> index b13c11f72..c798c7a10 100644
> --- a/testcases/kernel/syscalls/statx/statx07.c
> +++ b/testcases/kernel/syscalls/statx/statx07.c
> @@ -62,15 +62,15 @@ static int get_mode(char *file_name, int flag_type, char *flag_name)
> {
> struct statx buf;
>
> - TEST(statx(AT_FDCWD, file_name, flag_type, STATX_ALL, &buf));
> + TEST(statx(AT_FDCWD, file_name, flag_type, STATX_BASIC_STATS | STATX_BTIME, &buf));
>
> if (TST_RET == -1) {
> tst_brk(TFAIL | TST_ERR,
> - "statx(AT_FDCWD, %s, %s, STATX_ALL, &buf)",
> + "statx(AT_FDCWD, %s, %s, STATX_BASIC_STATS | STATX_BTIME, &buf)",
> file_name, flag_name);
> }
>
> - tst_res(TINFO, "statx(AT_FDCWD, %s, %s, STATX_ALL, &buf) = %o",
> + tst_res(TINFO, "statx(AT_FDCWD, %s, %s, STATX_BASIC_STATS | STATX_BTIME, &buf) = %o",
> file_name, flag_name, buf.stx_mode);
>
> return buf.stx_mode;
Looks like this place just wants STATX_BASIC_STATS.
- Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v4 4/4] lapi/stat.h: Remove deprecated STATX_ALL macro
2023-04-26 21:56 ` Eric Biggers
@ 2023-04-27 1:52 ` Yang Xu (Fujitsu)
0 siblings, 0 replies; 46+ messages in thread
From: Yang Xu (Fujitsu) @ 2023-04-27 1:52 UTC (permalink / raw)
To: Eric Biggers; +Cc: ltp@lists.linux.it
on 2023/04/27 5:56, Eric Biggers wrote:
> On Thu, Apr 06, 2023 at 01:40:22PM +0800, Yang Xu wrote:
>> diff --git a/testcases/kernel/syscalls/statx/statx07.c b/testcases/kernel/syscalls/statx/statx07.c
>> index b13c11f72..c798c7a10 100644
>> --- a/testcases/kernel/syscalls/statx/statx07.c
>> +++ b/testcases/kernel/syscalls/statx/statx07.c
>> @@ -62,15 +62,15 @@ static int get_mode(char *file_name, int flag_type, char *flag_name)
>> {
>> struct statx buf;
>>
>> - TEST(statx(AT_FDCWD, file_name, flag_type, STATX_ALL, &buf));
>> + TEST(statx(AT_FDCWD, file_name, flag_type, STATX_BASIC_STATS | STATX_BTIME, &buf));
>>
>> if (TST_RET == -1) {
>> tst_brk(TFAIL | TST_ERR,
>> - "statx(AT_FDCWD, %s, %s, STATX_ALL, &buf)",
>> + "statx(AT_FDCWD, %s, %s, STATX_BASIC_STATS | STATX_BTIME, &buf)",
>> file_name, flag_name);
>> }
>>
>> - tst_res(TINFO, "statx(AT_FDCWD, %s, %s, STATX_ALL, &buf) = %o",
>> + tst_res(TINFO, "statx(AT_FDCWD, %s, %s, STATX_BASIC_STATS | STATX_BTIME, &buf) = %o",
>> file_name, flag_name, buf.stx_mode);
>>
>> return buf.stx_mode;
>
> Looks like this place just wants STATX_BASIC_STATS.
Yes, I will only use STATX_BASIC_STATS for statx07.c
Best Regards
Yang Xu
>
> - Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [LTP] [PATCH v4 1/4] lapi/stat.h: Add STATX_DIOALIGN related definition
2023-04-06 5:40 ` [LTP] [PATCH v4 1/4] lapi/stat.h: Add STATX_DIOALIGN related definition Yang Xu
` (2 preceding siblings ...)
2023-04-06 5:40 ` [LTP] [PATCH v4 4/4] lapi/stat.h: Remove deprecated STATX_ALL macro Yang Xu
@ 2023-04-26 9:57 ` Yang Xu (Fujitsu)
2023-04-26 21:56 ` Eric Biggers
4 siblings, 0 replies; 46+ messages in thread
From: Yang Xu (Fujitsu) @ 2023-04-26 9:57 UTC (permalink / raw)
To: ltp@lists.linux.it
Hi
Ping!
Best Regards
Yang Xu> Also add missing stx_mnt_id.
>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
> include/lapi/stat.h | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/include/lapi/stat.h b/include/lapi/stat.h
> index ce1f2b678..c2db8a589 100644
> --- a/include/lapi/stat.h
> +++ b/include/lapi/stat.h
> @@ -97,7 +97,11 @@ struct statx {
> uint32_t stx_dev_major;
> uint32_t stx_dev_minor;
> /* 0x90 */
> - uint64_t __spare2[14];
> + uint64_t stx_mnt_id;
> + uint32_t stx_dio_mem_align;
> + uint32_t stx_dio_offset_align;
> + /* 0xa0 */
> + uint64_t __spare1[12];
> /* 0x100 */
> };
> #endif
> @@ -180,6 +184,10 @@ static inline int statx(int dirfd, const char *pathname, unsigned int flags,
> # define STATX_MNT_ID 0x00001000U
> #endif
>
> +#ifndef STATX_DIOALIGN
> +# define STATX_DIOALIGN 0x00002000U
> +#endif
> +
> #ifndef STATX_ALL
> # define STATX_ALL 0x00000fffU
> #endif
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v4 1/4] lapi/stat.h: Add STATX_DIOALIGN related definition
2023-04-06 5:40 ` [LTP] [PATCH v4 1/4] lapi/stat.h: Add STATX_DIOALIGN related definition Yang Xu
` (3 preceding siblings ...)
2023-04-26 9:57 ` [LTP] [PATCH v4 1/4] lapi/stat.h: Add STATX_DIOALIGN related definition Yang Xu (Fujitsu)
@ 2023-04-26 21:56 ` Eric Biggers
2023-04-27 1:36 ` Yang Xu (Fujitsu)
4 siblings, 1 reply; 46+ messages in thread
From: Eric Biggers @ 2023-04-26 21:56 UTC (permalink / raw)
To: Yang Xu; +Cc: ltp
On Thu, Apr 06, 2023 at 01:40:19PM +0800, Yang Xu wrote:
> Also add missing stx_mnt_id.
>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
> include/lapi/stat.h | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/include/lapi/stat.h b/include/lapi/stat.h
> index ce1f2b678..c2db8a589 100644
> --- a/include/lapi/stat.h
> +++ b/include/lapi/stat.h
> @@ -97,7 +97,11 @@ struct statx {
> uint32_t stx_dev_major;
> uint32_t stx_dev_minor;
> /* 0x90 */
> - uint64_t __spare2[14];
> + uint64_t stx_mnt_id;
> + uint32_t stx_dio_mem_align;
> + uint32_t stx_dio_offset_align;
> + /* 0xa0 */
> + uint64_t __spare1[12];
> /* 0x100 */
> };
Not like it matters, but the kernel header has __spare3, not __spare1.
- Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [LTP] [PATCH v4 1/4] lapi/stat.h: Add STATX_DIOALIGN related definition
2023-04-26 21:56 ` Eric Biggers
@ 2023-04-27 1:36 ` Yang Xu (Fujitsu)
0 siblings, 0 replies; 46+ messages in thread
From: Yang Xu (Fujitsu) @ 2023-04-27 1:36 UTC (permalink / raw)
To: Eric Biggers; +Cc: ltp@lists.linux.it
on 2023/04/27 5:56, Eric Biggers wrote:
> On Thu, Apr 06, 2023 at 01:40:19PM +0800, Yang Xu wrote:
>> Also add missing stx_mnt_id.
>>
>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>> ---
>> include/lapi/stat.h | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/lapi/stat.h b/include/lapi/stat.h
>> index ce1f2b678..c2db8a589 100644
>> --- a/include/lapi/stat.h
>> +++ b/include/lapi/stat.h
>> @@ -97,7 +97,11 @@ struct statx {
>> uint32_t stx_dev_major;
>> uint32_t stx_dev_minor;
>> /* 0x90 */
>> - uint64_t __spare2[14];
>> + uint64_t stx_mnt_id;
>> + uint32_t stx_dio_mem_align;
>> + uint32_t stx_dio_offset_align;
>> + /* 0xa0 */
>> + uint64_t __spare1[12];
>> /* 0x100 */
>> };
>
> Not like it matters, but the kernel header has __spare3, not __spare1.
Yes, I know this.
Sorry, I don't explain this reason for using _spare1[12] in commit message.
Looks the history of this struct in the kernel header.
Since kernel commit a528d35e ("statx: Add a system call to make enhanced
file info available")[1], it introduced
__spare0[1],__spare1[1],__spare2[14].
Then in kernel commit 3209f68 ("statx: Include a mask for stx_attributes
in struct statx")[2], it uses stx_attributes_mask to replace __spare1[1],
so it leaves a gap.
After kernel commit fa2fcf4f1 ("statx: add mount ID")[3], it uses
stx_mnit_id and _spare2 , _spare3[12] to replace _spare2[14].
Finally, in kernel commit 825cf206 ("statx: add direct I/O alignment
information")[4], use stx_dio_mem_align and stx_dio_offset_align to
replace _spare2. It also leaves a gap.
That is why I use __spare1[12] instead of _spare3[12].
[1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/stat.h?id=a528d35e8b
[2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/stat.h?id=3209f68b
[3]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/stat.h?id=fa2fcf4f
[4]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/stat.h?id=825cf206
Best Regards
Yang Xu
>
> - Eric
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 46+ messages in thread
* [LTP] [PATCH v3 4/4] lapi/stat.h: Remove deprecated STATX_ALL macro
2023-04-04 7:30 ` [LTP] [PATCH v3 1/4] lapi/stat.h: Add STATX_DIOALIGN related definition Yang Xu
2023-04-04 7:30 ` [LTP] [PATCH v3 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file Yang Xu
2023-04-04 7:30 ` [LTP] [PATCH v3 3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device Yang Xu
@ 2023-04-04 7:30 ` Yang Xu
2 siblings, 0 replies; 46+ messages in thread
From: Yang Xu @ 2023-04-04 7:30 UTC (permalink / raw)
To: ltp
Since kernel 5.10-rc1 commit 581701b7efd6 ("uapi: deprecate STATX_ALL"),
this flag has been mark as deprecated.
Kernel should keep this macro for compatibility, but ltp doesn't think
about it. So remove it.
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
include/lapi/stat.h | 4 ----
testcases/kernel/syscalls/statx/statx06.c | 4 ++--
testcases/kernel/syscalls/statx/statx07.c | 6 +++---
3 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/include/lapi/stat.h b/include/lapi/stat.h
index c2db8a589..7c9a7a00c 100644
--- a/include/lapi/stat.h
+++ b/include/lapi/stat.h
@@ -188,10 +188,6 @@ static inline int statx(int dirfd, const char *pathname, unsigned int flags,
# define STATX_DIOALIGN 0x00002000U
#endif
-#ifndef STATX_ALL
-# define STATX_ALL 0x00000fffU
-#endif
-
#ifndef STATX__RESERVED
# define STATX__RESERVED 0x80000000U
#endif
diff --git a/testcases/kernel/syscalls/statx/statx06.c b/testcases/kernel/syscalls/statx/statx06.c
index ce82b905b..60e736c5a 100644
--- a/testcases/kernel/syscalls/statx/statx06.c
+++ b/testcases/kernel/syscalls/statx/statx06.c
@@ -111,10 +111,10 @@ static void test_statx(unsigned int test_nr)
clock_wait_tick();
SAFE_CLOCK_GETTIME(CLOCK_REALTIME_COARSE, &after_time);
- TEST(statx(AT_FDCWD, TEST_FILE, 0, STATX_ALL, &buff));
+ TEST(statx(AT_FDCWD, TEST_FILE, 0, STATX_BASIC_STATS | STATX_BTIME, &buff));
if (TST_RET != 0) {
tst_brk(TFAIL | TTERRNO,
- "statx(AT_FDCWD, %s, 0, STATX_ALL, &buff)",
+ "statx(AT_FDCWD, %s, 0, STATX_BASIC_STATS | STATX_BTIME, &buff)",
TEST_FILE);
}
diff --git a/testcases/kernel/syscalls/statx/statx07.c b/testcases/kernel/syscalls/statx/statx07.c
index b13c11f72..c798c7a10 100644
--- a/testcases/kernel/syscalls/statx/statx07.c
+++ b/testcases/kernel/syscalls/statx/statx07.c
@@ -62,15 +62,15 @@ static int get_mode(char *file_name, int flag_type, char *flag_name)
{
struct statx buf;
- TEST(statx(AT_FDCWD, file_name, flag_type, STATX_ALL, &buf));
+ TEST(statx(AT_FDCWD, file_name, flag_type, STATX_BASIC_STATS | STATX_BTIME, &buf));
if (TST_RET == -1) {
tst_brk(TFAIL | TST_ERR,
- "statx(AT_FDCWD, %s, %s, STATX_ALL, &buf)",
+ "statx(AT_FDCWD, %s, %s, STATX_BASIC_STATS | STATX_BTIME, &buf)",
file_name, flag_name);
}
- tst_res(TINFO, "statx(AT_FDCWD, %s, %s, STATX_ALL, &buf) = %o",
+ tst_res(TINFO, "statx(AT_FDCWD, %s, %s, STATX_BASIC_STATS | STATX_BTIME, &buf) = %o",
file_name, flag_name, buf.stx_mode);
return buf.stx_mode;
--
2.39.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 46+ messages in thread