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

[Eric Biggers <ebiggers@kernel.org>]

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.

> +static void verify_statx(void)
> +{
> +	struct statx buf;
> +
> +	memset(&buf, 0, sizeof(buf));

There is no need for this memset to 0.

> +	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.

> +	if ((buf.stx_mask & STATX_DIOALIGN)) {

Unnecessary parentheses

> +		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."

> +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()?

> +#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?

- Eric

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp