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

[xuyang2018.jy@fujitsu.com]

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
>