diff mbox series

[v3,3/4] syscalls/statx11: Add basic test for STATX_DIOALIGN on block device

Message ID 1680593430-14728-3-git-send-email-xuyang2018.jy@fujitsu.com
State Superseded
Headers show
Series [v3,1/4] lapi/stat.h: Add STATX_DIOALIGN related definition | expand

Commit Message

Yang Xu April 4, 2023, 7:30 a.m. UTC 
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

Comments

Eric Biggers April 4, 2023, 9:59 p.m. UTC | #1 
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
Yang Xu April 6, 2023, 4:57 a.m. UTC | #2 
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
Yang Xu April 6, 2023, 5:36 a.m. UTC | #3 
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
>
diff mbox series

Patch

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