[LTP] [PATCH v4 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file

[Yang Xu (Fujitsu)]

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