| Message ID | 20230104042801.217898-1-bxue@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] src/stat_test.c: add STATX_DIOALIGN support | expand |
Hi Boyang, On Wed, Jan 04, 2023 at 12:28:01PM +0800, bxue@redhat.com wrote: > From: Boyang Xue <bxue@redhat.com> > > Signed-off-by: Boyang Xue <bxue@redhat.com> > --- > Hi, > > The latest kernel has support for exposing direct I/O alignment > information via statx() by > > 825cf206ed51 statx: add direct I/O alignment information > > I'm trying to enhance xfstests/src/stat_test.c to support this > functionality, and the final goal is enhancing generic/423 to test it. > > I think I have made all the necessary change here, but it always prints > stx_dio_mem_align and stx_dio_offset_align as 0 (should be 512) > > [root@localhost repo_xfstests-dev]# src/stat_test -v > ../testfile stx_dio_offset_align=222 > - call statx ../testfile > - call stat ../testfile > - compare statx and stat > - begin time 0.000000000 > - btime 1672804449.041990601 > - atime 1672804449.041990601 > - mtime 1672804449.127990601 > - ctime 1672804449.127990601 > - check stx_dio_offset_align=222 > [!] stx_dio_offset_align differs, 0 != 222 > Failed > > src/stat_test.c | 10 ++++++++++ > src/statx.h | 10 ++++++++-- > 2 files changed, 18 insertions(+), 2 deletions(-) > > The kernel version in test is kernel-6.2.0-0.rc1. > > Could you suggest how to fix it please? > > Thanks, > Boyang Thanks for working on this! One of the challenges with testing STATX_DIOALIGN is that it can only be usefully tested if DIO is supported, yet STATX_DIOALIGN is itself the way to check whether DIO is supported. Another challenge is that without something to compare the alignments against, it's hard to know whether the correct values are being reported. (A test could try to validate the values by attempting DIO, but the filesystem might fall back to buffered I/O for unsupported or misaligned DIO, which would be hard to distinguish from true DIO. Maybe something clever could be done with mincore() detect buffered I/O.) Is there a specific test that you're planning to add? A test that would be at least somewhat useful would be to test that if STATX_DIOALIGN gives nonzero stx_dio_mem_align and stx_dio_offset_align, then DIO aligned to *only* those alignments doesn't return an error. Another possible test would to find a specific case where the DIO support and alignments can be determined by another method and compared to what STATX_DIOALIGN reports. For example, if testing XFS, the XFS_IOC_DIOINFO ioctl can be used. Or if the test just creates a filesystem with the default options and mounts it with the default options, it might just "know" that DIO is supported with logical_block_size alignment. Anyway, as for why your patch to stat_test.c doesn't work, it's because there's a bug in it. Try the following: diff --git a/src/stat_test.c b/src/stat_test.c index cd38a54a..85d703a0 100644 --- a/src/stat_test.c +++ b/src/stat_test.c @@ -570,6 +570,8 @@ static void check_field(const struct statx *stx, char *arg) case stx_rdev_minor: case stx_dev_major: case stx_dev_minor: + case stx_dio_mem_align: + case stx_dio_offset_align: ucheck = strtoull(val, &p, 0); if (*p) bad_arg("Field '%s' requires unsigned integer\n", key); @@ -577,8 +579,6 @@ static void check_field(const struct statx *stx, char *arg) "%s differs, %llu != %llu\n", key, uval, ucheck); break; - case stx_dio_mem_align: - case stx_dio_offset_align: case stx_atime_tv_sec: case stx_atime_tv_nsec: case stx_btime_tv_sec:
Hi Eric,
Thanks for your suggestion! From the kernel source (
https://github.com/torvalds/linux/blob/1f5abbd77e2c1787e74b7c2caffac97def78ba52/block/bdev.c#L1089
)
stat->dio_mem_align = bdev_dma_alignment(bdev) + 1;
stat->dio_offset_align = bdev_logical_block_size(bdev);
I think that
stx_dio_mem_align=$(cat /sys/block/<dev>/queue/logical_block_size)
stx_dio_offset_align=$(($(cat /sys/block/vda/queue/dma_alignment)+1))
The output on my system is like
[root@localhost ~]# cat /sys/block/vda/queue/logical_block_size
512
[root@localhost ~]# echo $(($(cat /sys/block/<dev>/queue/dma_alignment)+1))
512
I have hacked <kernel source>/samples/vfs/test-statx.c like
[root@localhost ~]# git diff test-statx.c test-statx-mod.c
diff --git a/test-statx.c b/test-statx-mod.c
index 49c7a46..713d7b6 100644
--- a/test-statx.c
+++ b/test-statx-mod.c
@@ -107,6 +107,9 @@ static void dump_statx(struct statx *stx)
printf("Device: %-15s", buffer);
if (stx->stx_mask & STATX_INO)
printf(" Inode: %-11llu", (unsigned long long) stx->stx_ino);
+ if (stx->stx_mask & STATX_DIOALIGN)
+ printf(" stx_dio_mem_align: %u", stx->stx_dio_mem_align);
+ printf(" stx_dio_offset_align: %u", stx->stx_dio_offset_align);
if (stx->stx_mask & STATX_NLINK)
printf(" Links: %-5u", stx->stx_nlink);
if (stx->stx_mask & STATX_TYPE) {
@@ -218,7 +221,7 @@ int main(int argc, char **argv)
struct statx stx;
int ret, raw = 0, atflag = AT_SYMLINK_NOFOLLOW;
- unsigned int mask = STATX_BASIC_STATS | STATX_BTIME;
+ unsigned int mask = STATX_BASIC_STATS | STATX_BTIME | STATX_DIOALIGN;
for (argv++; *argv; argv++) {
if (strcmp(*argv, "-F") == 0) {
and the output on my system is
[root@localhost ~]# ./test-statx-mod testfile
statx(testfile) = 0
results=3fff
Size: 8388608 Blocks: 16384 IO Block: 4096 regular file
Device: fc:02 Inode: 586 stx_dio_mem_align: 512
stx_dio_offset_align: 512 Links: 1
Access: (0644/-rw-r--r--) Uid: 0 Gid: 0
Access: 2023-01-06 04:59:46.967816162-0500
Modify: 2023-01-06 04:59:47.099816162-0500
Change: 2023-01-06 04:59:47.099816162-0500
Birth: 2023-01-06 04:59:46.967816162-0500
Attributes: 0000000000000000 (........ ........ ........ ........
........ ..--.... ..---... .---.-..)
Notice stx_dio_mem_align and stx_dio_offset_align both are 512, so I
guess it shows STATX_DIOALIGN on my system works correctly.
Unfortunately, I am still unable to get my patch working, even with
your suggested fix, the stx_dio_mem_align and stx_dio_offset_align all
printed as 0.
I planned to test this functionality by hacking generic/423, which I
think is a good framework for doing basic statx() validation, like
ref_dio_mem_align=$(cat /sys/block/<dev>/queue/logical_block_size)
ref_dio_offset_align=$(($(cat /sys/block/<dev>/queue/dma_alignment)+1))
...
check_stat $TEST_DIR/$seq-file \
stx_dio_mem_align=$ref_dio_mem_align \
stx_dio_offset_align=$ref_dio_offset_align
I think this is adequate for a basic correctness test?
Thanks,
Boyang
On Fri, Jan 6, 2023 at 4:01 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> Hi Boyang,
>
> On Wed, Jan 04, 2023 at 12:28:01PM +0800, bxue@redhat.com wrote:
> > From: Boyang Xue <bxue@redhat.com>
> >
> > Signed-off-by: Boyang Xue <bxue@redhat.com>
> > ---
> > Hi,
> >
> > The latest kernel has support for exposing direct I/O alignment
> > information via statx() by
> >
> > 825cf206ed51 statx: add direct I/O alignment information
> >
> > I'm trying to enhance xfstests/src/stat_test.c to support this
> > functionality, and the final goal is enhancing generic/423 to test it.
> >
> > I think I have made all the necessary change here, but it always prints
> > stx_dio_mem_align and stx_dio_offset_align as 0 (should be 512)
> >
> > [root@localhost repo_xfstests-dev]# src/stat_test -v
> > ../testfile stx_dio_offset_align=222
> > - call statx ../testfile
> > - call stat ../testfile
> > - compare statx and stat
> > - begin time 0.000000000
> > - btime 1672804449.041990601
> > - atime 1672804449.041990601
> > - mtime 1672804449.127990601
> > - ctime 1672804449.127990601
> > - check stx_dio_offset_align=222
> > [!] stx_dio_offset_align differs, 0 != 222
> > Failed
> >
> > src/stat_test.c | 10 ++++++++++
> > src/statx.h | 10 ++++++++--
> > 2 files changed, 18 insertions(+), 2 deletions(-)
> >
> > The kernel version in test is kernel-6.2.0-0.rc1.
> >
> > Could you suggest how to fix it please?
> >
> > Thanks,
> > Boyang
>
> Thanks for working on this! One of the challenges with testing STATX_DIOALIGN
> is that it can only be usefully tested if DIO is supported, yet STATX_DIOALIGN
> is itself the way to check whether DIO is supported. Another challenge is that
> without something to compare the alignments against, it's hard to know whether
> the correct values are being reported. (A test could try to validate the values
> by attempting DIO, but the filesystem might fall back to buffered I/O for
> unsupported or misaligned DIO, which would be hard to distinguish from true DIO.
> Maybe something clever could be done with mincore() detect buffered I/O.)
>
> Is there a specific test that you're planning to add? A test that would be at
> least somewhat useful would be to test that if STATX_DIOALIGN gives nonzero
> stx_dio_mem_align and stx_dio_offset_align, then DIO aligned to *only* those
> alignments doesn't return an error.
>
> Another possible test would to find a specific case where the DIO support and
> alignments can be determined by another method and compared to what
> STATX_DIOALIGN reports. For example, if testing XFS, the XFS_IOC_DIOINFO ioctl
> can be used. Or if the test just creates a filesystem with the default options
> and mounts it with the default options, it might just "know" that DIO is
> supported with logical_block_size alignment.
>
> Anyway, as for why your patch to stat_test.c doesn't work, it's because there's
> a bug in it. Try the following:
>
> diff --git a/src/stat_test.c b/src/stat_test.c
> index cd38a54a..85d703a0 100644
> --- a/src/stat_test.c
> +++ b/src/stat_test.c
> @@ -570,6 +570,8 @@ static void check_field(const struct statx *stx, char *arg)
> case stx_rdev_minor:
> case stx_dev_major:
> case stx_dev_minor:
> + case stx_dio_mem_align:
> + case stx_dio_offset_align:
> ucheck = strtoull(val, &p, 0);
> if (*p)
> bad_arg("Field '%s' requires unsigned integer\n", key);
> @@ -577,8 +579,6 @@ static void check_field(const struct statx *stx, char *arg)
> "%s differs, %llu != %llu\n", key, uval, ucheck);
> break;
>
> - case stx_dio_mem_align:
> - case stx_dio_offset_align:
> case stx_atime_tv_sec:
> case stx_atime_tv_nsec:
> case stx_btime_tv_sec:
>
On Fri, Jan 06, 2023 at 07:10:04PM +0800, Boyang Xue wrote: > I planned to test this functionality by hacking generic/423, which I > think is a good framework for doing basic statx() validation, like > > ref_dio_mem_align=$(cat /sys/block/<dev>/queue/logical_block_size) > ref_dio_offset_align=$(($(cat /sys/block/<dev>/queue/dma_alignment)+1)) > ... > check_stat $TEST_DIR/$seq-file \ > stx_dio_mem_align=$ref_dio_mem_align \ > stx_dio_offset_align=$ref_dio_offset_align > > I think this is adequate for a basic correctness test? Not in general. The logical_block_size and dma_alignment+1 of the block device (assuming the filesystem has a block device, and only one of them...) are only the *typical* values for stx_dio_mem_align and stx_dio_offset_align. They are *not* the guaranteed values, since the DIO support and alignment restrictions are filesystem-specific. They depend on the filesystem type, mkfs options, mount options, kernel version, block device, and other things. So if you add the above, it will make generic/423 fail in various cases. This problem is the whole reason that STATX_DIOALIGN was added: it provides a way to query DIO support and alignment restrictions. If there was already another way to *reliably* query DIO support and alignment restrictions, then there would have been no need to add STATX_DIOALIGN. Nonetheless, see my previous email for some ideas about tests of STATX_DIOALIGN that might be possible. Another idea is to test STATX_DIOALIGN on a block device node, not a regular file. The results from block devices are more predictable; currently STATX_DIOALIGN on a block device always reports logical_block_size and dma_alignment+1. Though, that could still change in future kernel versions. - Eric
diff --git a/src/stat_test.c b/src/stat_test.c index 95cfdc4f..cd38a54a 100644 --- a/src/stat_test.c +++ b/src/stat_test.c @@ -42,6 +42,8 @@ enum fields { stx_ctime_tv_sec, stx_dev_major, stx_dev_minor, + stx_dio_mem_align, + stx_dio_offset_align, stx_gid, stx_ino, stx_mask, @@ -77,6 +79,8 @@ static const struct field field_list[nr__fields] = { [stx_ctime_tv_sec] = { "stx_ctime.tv_sec", STATX_CTIME }, [stx_dev_major] = { "stx_dev_major", 0 }, [stx_dev_minor] = { "stx_dev_minor", 0 }, + [stx_dio_mem_align] = { "stx_dio_mem_align", STATX_DIOALIGN }, + [stx_dio_offset_align] = { "stx_dio_offset_align", STATX_DIOALIGN }, [stx_gid] = { "stx_gid", STATX_GID }, [stx_ino] = { "stx_ino", STATX_INO }, [stx_mask] = { "stx_mask", 0 }, @@ -460,6 +464,8 @@ static void cmp_ref(const struct statx *stx, unsigned int mask) cmp("%llu", stx_rdev_minor); cmp("%llu", stx_dev_major); cmp("%llu", stx_dev_minor); + cmp("%llu", stx_dio_mem_align); + cmp("%llu", stx_dio_offset_align); } /* @@ -506,6 +512,8 @@ static void check_field(const struct statx *stx, char *arg) case stx_rdev_minor: uval = stx->stx_rdev_minor; break; case stx_dev_major: uval = stx->stx_dev_major; break; case stx_dev_minor: uval = stx->stx_dev_minor; break; + case stx_dio_mem_align: uval = stx->stx_dio_mem_align; break; + case stx_dio_offset_align: uval = stx->stx_dio_offset_align; break; case stx_atime_tv_sec: sval = stx->stx_atime.tv_sec; break; case stx_atime_tv_nsec: sval = stx->stx_atime.tv_nsec; break; @@ -569,6 +577,8 @@ static void check_field(const struct statx *stx, char *arg) "%s differs, %llu != %llu\n", key, uval, ucheck); break; + case stx_dio_mem_align: + case stx_dio_offset_align: case stx_atime_tv_sec: case stx_atime_tv_nsec: case stx_btime_tv_sec: diff --git a/src/statx.h b/src/statx.h index 3f239d79..116d1f43 100644 --- a/src/statx.h +++ b/src/statx.h @@ -114,7 +114,11 @@ struct statx { __u32 stx_dev_major; /* ID of device containing file [uncond] */ __u32 stx_dev_minor; /* 0x90 */ - __u64 __spare2[14]; /* Spare space for future expansion */ + __u64 stx_mnt_id; + __u32 stx_dio_mem_align; /* Memory buffer alignment for direct I/O */ + __u32 stx_dio_offset_align; /* File offset alignment for direct I/O */ + /* 0xa0 */ + __u64 __spare3[12]; /* Spare space for future expansion */ /* 0x100 */ }; @@ -139,7 +143,9 @@ struct statx { #define STATX_BLOCKS 0x00000400U /* Want/got stx_blocks */ #define STATX_BASIC_STATS 0x000007ffU /* The stuff in the normal stat struct */ #define STATX_BTIME 0x00000800U /* Want/got stx_btime */ -#define STATX_ALL 0x00000fffU /* All currently supported flags */ +#define STATX_MNT_ID 0x00001000U /* Got stx_mnt_id */ +#define STATX_DIOALIGN 0x00002000U /* Want/got direct I/O alignment info */ +#define STATX_ALL 0x00003fffU /* All currently supported flags */ /* * Attributes to be found in stx_attributes