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 27, 2023 at 03:03:23AM +0000, Yang Xu (Fujitsu) wrote:
> 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.

Great, can you update the comment to make it clear that this test creates its
own filesystem?

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

As I said, if the system headers are missing the needed fields, then LTP should
use its in-tree definition.  I.e., the in-tree definition should only be used if
HAVE_STRUCT_STATX && HAVE_STRUCT_STATX_STX_MNT_ID && [all other tested fields].

> >> +	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);

I think that's okay.

- Eric

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