Re: [PATCH v1] src/stat_test.c: add STATX_DIOALIGN support

[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]



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@xxxxxxxxxx> wrote:
>
> Hi Boyang,
>
> On Wed, Jan 04, 2023 at 12:28:01PM +0800, bxue@xxxxxxxxxx wrote:
> > From: Boyang Xue <bxue@xxxxxxxxxx>
> >
> > Signed-off-by: Boyang Xue <bxue@xxxxxxxxxx>
> > ---
> > 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:
>




[Index of Archives]     [Linux Filesystems Development]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux