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

[Eric Biggers <ebiggers@kernel.org>]

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