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

[Eric Biggers <ebiggers@kernel.org>]

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?

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"

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

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

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

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

Also, this is closing the file descriptor even when it is -1.

> +static struct tst_test test = {
> +	.test_all = verify_statx,
> +	.setup = setup,
> +	.needs_root = 1,

Why does this test need root?

- Eric

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