[LTP] [PATCH v4 2/4] syscalls/statx10: Add basic test for STATX_DIOALIGN on regular file
Yang Xu (Fujitsu)
xuyang2018.jy@fujitsu.com
Mon May 8 10:25:10 CEST 2023
on 2023/05/02 1:44, Eric Biggers wrote:
> 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?
Of course.
>
>>>
>>> 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].
Yes, it should work well but ltp has other owner headers(they still
include <sys/stat.h>), so it can't work well
I try it as below:
+#if defined(HAVE_STATX) && \
+ defined(HAVE_STRUCT_STATX_TIMESTAMP) && \
+ defined(HAVE_STRUCT_STATX) && \
+ defined(HAVE_STRUCT_STATX_STX_MNT_ID)
+#include <sys/stat.h>
+#else
....
#endif
see ltp/include, ltp owner header also uses <sys/stat.h>(use stat
struct or stat syscall)
safe_file_ops_fn.h:21:#include <sys/stat.h>
safe_macros_fn.h:19:#include <sys/stat.h>
tst_device.h:11:#include <sys/stat.h>
tst_safe_file_at.h:10:#include <sys/stat.h>
tst_safe_macros.h:13:#include <sys/stat.h>
If I remove mnt_id ifdef check in statx01.c, then statx01 will report
redefine error for statx struct as below:
In file included from statx01.c:35:
../../../../include/lapi/stat.h:30:8: error: redefinition of ‘struct
statx_timestamp’
struct statx_timestamp {
^~~~~~~~~~~~~~~
In file included from /usr/include/bits/statx.h:31,
from /usr/include/sys/stat.h:446,
from ../../../../include/tst_device.h:11,
from ../../../../include/tst_test.h:23,
from statx01.c:33:
/usr/include/linux/stat.h:56:8: note: originally defined here
struct statx_timestamp {
^~~~~~~~~~~~~~~
In file included from statx01.c:35:
../../../../include/lapi/stat.h:72:8: error: redefinition of ‘struct statx’
struct statx {
^~~~~
In file included from /usr/include/bits/statx.h:31,
from /usr/include/sys/stat.h:446,
from ../../../../include/tst_device.h:11,
from ../../../../include/tst_test.h:23,
from statx01.c:33:
/usr/include/linux/stat.h:99:8: note: originally defined here
struct statx {
^~~~~
In file included from statx01.c:35:
../../../../include/lapi/stat.h:113:19: error: conflicting types for ‘statx’
static inline int statx(int dirfd, const char *pathname, unsigned int
flags,
^~~~~
In file included from /usr/include/bits/statx.h:39,
from /usr/include/sys/stat.h:446,
from ../../../../include/tst_device.h:11,
from ../../../../include/tst_test.h:23,
from statx01.c:33:
/usr/include/bits/statx-generic.h:56:5: note: previous declaration of
‘statx’ was here
int statx (int __dirfd, const char *__restrict __path, int __flags,
^~~~~
statx01.c:96:2: error: #endif without #if
#endif
^~~~~
IMO, to change ltp owner header to avoid use <sys/mount.h> seems
difficulty.
Best Regards
Yang Xu
>
>>>> + 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
More information about the ltp
mailing list