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

[xuyang2018.jy@fujitsu.com]

on 2023/04/05 5:52, Eric Biggers wrote:
> On Tue, Apr 04, 2023 at 03:30:28PM +0800, Yang Xu wrote:
>> On ext4, files that use certain filesystem features (data journalling,
>> 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.
> 
> Please document this in the code itself.

OK.

> 
>> +static void verify_statx(void)
>> +{
>> +	struct statx buf;
>> +
>> +	memset(&buf, 0, sizeof(buf));
> 
> There is no need for this memset to 0.

YES.
> 
>> +	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");
>> +
>> +	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");
> 
> Please leave a space between : and %u.

Good catch.
> 
>> +	if ((buf.stx_mask & STATX_DIOALIGN)) {
> 
> Unnecessary parentheses

Yes,
> 
>> +		tst_res(TFAIL, "STATX_DIOALIGN mask return even not request");
>> +		return;
>> +	}
> 
> This part is not a valid test.  Please see the statx(2) manual page:
> 
>         "It should be noted that the kernel may return fields that  weren't  re‐
>         quested and may fail to return fields that were requested, depending on
>         what the backing filesystem supports.  (Fields that  are  given  values
>         despite  being  unrequested  can  just  be  ignored.)   In either case,
>         stx_mask will not be equal mask."

OK. Will remove.
> 
>> +static void setup(void)
>> +{
>> +	if (strcmp(tst_device->fs_type, "xfs") && strcmp(tst_device->fs_type, "ext4"))
>> +		tst_brk(TCONF, "This test only supports ext4 and xfs");
>> +
>> +	SAFE_FILE_PRINTF(TESTFILE, "AAAA");
>> +	fd = open(TESTFILE, O_RDWR | O_DIRECT);
>> +	if (fd == -1 && errno == EINVAL)
>> +		tst_brk(TCONF, "The regular file is not on a filesystem that support DIO");
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> +	if (fd > -1)
>> +		SAFE_CLOSE(fd);
>> +}
> 
> Shouldn't 'fd' just be a local variable in setup()?

Yes.
> 
>> +#else
>> +TST_TEST_TCONF("test requires struct statx to have the stx_dio_mem_align fields");
>> +#endif
> 
> LTP already has its own definition of struct statx in include/lapi/stat.h.  So,
> why is it necessary to skip this test if the system headers lack an up-to-date
> definition of struct statx?
In actually, ltp own statx definition as below:
#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;

So we only use ltp own statx struct when system header file sys/statx.h 
doesn't have statx struct. If people use old system header file, it 
still will report non defined stx_dio_mem_align or stx_dio_offset_align.

Best Regards
Yang Xu
> 
> - Eric