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

[xuyang2018.jy@fujitsu.com]

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