Re: [PATCH v2] generic: test page fault during direct IO write with O_APPEND

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



On Mon, Aug 05, 2024 at 04:09:06PM +0100, Filipe Manana wrote:
> On Sun, Jul 28, 2024 at 5:59 PM Zorro Lang <zlang@xxxxxxxxxx> wrote:
> >
> > On Sun, Jul 28, 2024 at 04:14:42PM +0100, Filipe Manana wrote:
> > > On Sun, Jul 28, 2024 at 3:28 PM Zorro Lang <zlang@xxxxxxxxxx> wrote:
> > > >
> > > > On Sat, Jul 27, 2024 at 12:28:16PM +0100, fdmanana@xxxxxxxxxx wrote:
> > > > > From: Filipe Manana <fdmanana@xxxxxxxx>
> > > > >
> > > > > Test that doing a direct IO append write to a file when the input buffer
> > > > > was not yet faulted in, does not result in an incorrect file size.
> > > > >
> > > > > This exercises a bug on btrfs reported by users and which is fixed by
> > > > > the following kernel patch:
> > > > >
> > > > >    "btrfs: fix corruption after buffer fault in during direct IO append write"
> > > > >
> > > > > Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
> > > > > ---
> > > > >
> > > > > V2: Deal with partial writes by looping and writing any remaining data.
> > > > >     Don't exit when the first test fails, instead let the second test
> > > > >     run as well.
> > > >
> > > > With this change I got two error lines this time [1]. Last time (V1) I
> > > > only got "Wrong file size after first write, got 8192 expected 4096".
> > >
> > > Yes, it's expected.
> > > As the changelog for v2 says, now the second test is run even if the
> > > first one failed.
> >
> > Thanks, I'd like to merge this patch:
> >
> > Reviewed-by: Zorro Lang <zlang@xxxxxxxxxx>
> 
> The kernel patch landed in Linus' tree, with a commit ID of 939b656bc8ab20.
> Do you want me to send a new version replacing the xxxxxxxxxxx with
> 939b656bc8ab20, or can you do that when you pick the patch?

Sorry for this late reply, don't worry, I'll merge it with that ID.

Thanks,
Zorro

> 
> Thanks.
> 
> >
> > >
> > > > Does this mean this v2 change help this case to be better?
> > >
> > > I prefer it like that.
> > > It's common in fstests to let all steps of a test run if possible
> > > (i.e. we don't exit, call _fail, or anything equivalent, everywhere
> > > unless the test can't proceed anymore).
> > >
> > > >
> > > > Thanks,
> > > > Zorro
> > > >
> > > > [1]
> > > > [root@dell-per750-41 xfstests]# ./check -s default generic/362
> > > > SECTION       -- default
> > > > FSTYP         -- btrfs
> > > > PLATFORM      -- Linux/x86_64 dell-xx-xxxxxx 6.10.0-0.rc7.20240712git43db1e03c086.62.fc41.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Jul 12 22:31:14 UTC 2024
> > > > MKFS_OPTIONS  -- /dev/sda6
> > > > MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/sda6 /mnt/scratch
> > > >
> > > > generic/362 0s ... - output mismatch (see /root/git/xfstests/results//default/generic/362.out.bad)
> > > >     --- tests/generic/362.out   2024-07-28 22:22:06.098982182 +0800
> > > >     +++ /root/git/xfstests/results//default/generic/362.out.bad 2024-07-28 22:23:16.622577397 +0800
> > > >     @@ -1,2 +1,4 @@
> > > >      QA output created by 362
> > > >     +Wrong file size after first write, got 8192 expected 4096
> > > >     +Wrong file size after second write, got 12288 expected 8192
> > > >      Silence is golden
> > > >
> > > >
> > > > >
> > > > >  .gitignore                 |   1 +
> > > > >  src/Makefile               |   2 +-
> > > > >  src/dio-append-buf-fault.c | 145 +++++++++++++++++++++++++++++++++++++
> > > > >  tests/generic/362          |  28 +++++++
> > > > >  tests/generic/362.out      |   2 +
> > > > >  5 files changed, 177 insertions(+), 1 deletion(-)
> > > > >  create mode 100644 src/dio-append-buf-fault.c
> > > > >  create mode 100755 tests/generic/362
> > > > >  create mode 100644 tests/generic/362.out
> > > > >
> > > > > diff --git a/.gitignore b/.gitignore
> > > > > index b5f15162..97c7e001 100644
> > > > > --- a/.gitignore
> > > > > +++ b/.gitignore
> > > > > @@ -72,6 +72,7 @@ tags
> > > > >  /src/deduperace
> > > > >  /src/detached_mounts_propagation
> > > > >  /src/devzero
> > > > > +/src/dio-append-buf-fault
> > > > >  /src/dio-buf-fault
> > > > >  /src/dio-interleaved
> > > > >  /src/dio-invalidate-cache
> > > > > diff --git a/src/Makefile b/src/Makefile
> > > > > index 99796137..559209be 100644
> > > > > --- a/src/Makefile
> > > > > +++ b/src/Makefile
> > > > > @@ -20,7 +20,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
> > > > >       t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \
> > > > >       t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \
> > > > >       t_mmap_cow_memory_failure fake-dump-rootino dio-buf-fault rewinddir-test \
> > > > > -     readdir-while-renames
> > > > > +     readdir-while-renames dio-append-buf-fault
> > > > >
> > > > >  LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
> > > > >       preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
> > > > > diff --git a/src/dio-append-buf-fault.c b/src/dio-append-buf-fault.c
> > > > > new file mode 100644
> > > > > index 00000000..72c23265
> > > > > --- /dev/null
> > > > > +++ b/src/dio-append-buf-fault.c
> > > > > @@ -0,0 +1,145 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright (c) 2024 SUSE Linux Products GmbH.  All Rights Reserved.
> > > > > + */
> > > > > +
> > > > > +/*
> > > > > + * Test a direct IO write in append mode with a buffer that was not faulted in
> > > > > + * (or just partially) before the write.
> > > > > + */
> > > > > +
> > > > > +/* Get the O_DIRECT definition. */
> > > > > +#ifndef _GNU_SOURCE
> > > > > +#define _GNU_SOURCE
> > > > > +#endif
> > > > > +
> > > > > +#include <stdio.h>
> > > > > +#include <stdlib.h>
> > > > > +#include <unistd.h>
> > > > > +#include <stdint.h>
> > > > > +#include <fcntl.h>
> > > > > +#include <errno.h>
> > > > > +#include <string.h>
> > > > > +#include <sys/mman.h>
> > > > > +#include <sys/stat.h>
> > > > > +
> > > > > +static ssize_t do_write(int fd, const void *buf, size_t count)
> > > > > +{
> > > > > +        while (count > 0) {
> > > > > +             ssize_t ret;
> > > > > +
> > > > > +             ret = write(fd, buf, count);
> > > > > +             if (ret < 0) {
> > > > > +                     if (errno == EINTR)
> > > > > +                             continue;
> > > > > +                     return ret;
> > > > > +             }
> > > > > +             count -= ret;
> > > > > +             buf += ret;
> > > > > +     }
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +int main(int argc, char *argv[])
> > > > > +{
> > > > > +     struct stat stbuf;
> > > > > +     int fd;
> > > > > +     long pagesize;
> > > > > +     void *buf;
> > > > > +     ssize_t ret;
> > > > > +
> > > > > +     if (argc != 2) {
> > > > > +             fprintf(stderr, "Use: %s <file path>\n", argv[0]);
> > > > > +             return 1;
> > > > > +     }
> > > > > +
> > > > > +     /*
> > > > > +      * First try an append write against an empty file of a buffer with a
> > > > > +      * size matching the page size. The buffer is not faulted in before
> > > > > +      * attempting the write.
> > > > > +      */
> > > > > +
> > > > > +     fd = open(argv[1], O_WRONLY | O_CREAT | O_TRUNC | O_DIRECT | O_APPEND, 0666);
> > > > > +     if (fd == -1) {
> > > > > +             perror("Failed to open/create file");
> > > > > +             return 2;
> > > > > +     }
> > > > > +
> > > > > +     pagesize = sysconf(_SC_PAGE_SIZE);
> > > > > +     if (pagesize == -1) {
> > > > > +             perror("Failed to get page size");
> > > > > +             return 3;
> > > > > +     }
> > > > > +
> > > > > +     buf = mmap(NULL, pagesize, PROT_READ | PROT_WRITE,
> > > > > +                MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > > > > +     if (buf == MAP_FAILED) {
> > > > > +             perror("Failed to allocate first buffer");
> > > > > +             return 4;
> > > > > +     }
> > > > > +
> > > > > +     ret = do_write(fd, buf, pagesize);
> > > > > +     if (ret < 0) {
> > > > > +             perror("First write failed");
> > > > > +             return 5;
> > > > > +     }
> > > > > +
> > > > > +     ret = fstat(fd, &stbuf);
> > > > > +     if (ret < 0) {
> > > > > +             perror("First stat failed");
> > > > > +             return 6;
> > > > > +     }
> > > > > +
> > > > > +     /* Don't exit on failure so that we run the second test below too. */
> > > > > +     if (stbuf.st_size != pagesize)
> > > > > +             fprintf(stderr,
> > > > > +                     "Wrong file size after first write, got %jd expected %ld\n",
> > > > > +                     (intmax_t)stbuf.st_size, pagesize);
> > > > > +
> > > > > +     munmap(buf, pagesize);
> > > > > +     close(fd);
> > > > > +
> > > > > +     /*
> > > > > +      * Now try an append write against an empty file of a buffer with a
> > > > > +      * size matching twice the page size. Only the first page of the buffer
> > > > > +      * is faulted in before attempting the write, so that the second page
> > > > > +      * should be faulted in during the write.
> > > > > +      */
> > > > > +     fd = open(argv[1], O_WRONLY | O_CREAT | O_TRUNC | O_DIRECT | O_APPEND, 0666);
> > > > > +     if (fd == -1) {
> > > > > +             perror("Failed to open/create file");
> > > > > +             return 7;
> > > > > +     }
> > > > > +
> > > > > +     buf = mmap(NULL, pagesize * 2, PROT_READ | PROT_WRITE,
> > > > > +                MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > > > > +     if (buf == MAP_FAILED) {
> > > > > +             perror("Failed to allocate second buffer");
> > > > > +             return 8;
> > > > > +     }
> > > > > +
> > > > > +     /* Fault in first page of the buffer before the write. */
> > > > > +     memset(buf, 0, 1);
> > > > > +
> > > > > +     ret = do_write(fd, buf, pagesize * 2);
> > > > > +     if (ret < 0) {
> > > > > +             perror("Second write failed");
> > > > > +             return 9;
> > > > > +     }
> > > > > +
> > > > > +     ret = fstat(fd, &stbuf);
> > > > > +     if (ret < 0) {
> > > > > +             perror("Second stat failed");
> > > > > +             return 10;
> > > > > +     }
> > > > > +
> > > > > +     if (stbuf.st_size != pagesize * 2)
> > > > > +             fprintf(stderr,
> > > > > +                     "Wrong file size after second write, got %jd expected %ld\n",
> > > > > +                     (intmax_t)stbuf.st_size, pagesize * 2);
> > > > > +
> > > > > +     munmap(buf, pagesize * 2);
> > > > > +     close(fd);
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > diff --git a/tests/generic/362 b/tests/generic/362
> > > > > new file mode 100755
> > > > > index 00000000..2c127347
> > > > > --- /dev/null
> > > > > +++ b/tests/generic/362
> > > > > @@ -0,0 +1,28 @@
> > > > > +#! /bin/bash
> > > > > +# SPDX-License-Identifier: GPL-2.0
> > > > > +# Copyright (C) 2024 SUSE Linux Products GmbH. All Rights Reserved.
> > > > > +#
> > > > > +# FS QA Test 362
> > > > > +#
> > > > > +# Test that doing a direct IO append write to a file when the input buffer was
> > > > > +# not yet faulted in, does not result in an incorrect file size.
> > > > > +#
> > > > > +. ./common/preamble
> > > > > +_begin_fstest auto quick
> > > > > +
> > > > > +_require_test
> > > > > +_require_odirect
> > > > > +_require_test_program dio-append-buf-fault
> > > > > +
> > > > > +[ $FSTYP == "btrfs" ] && \
> > > > > +     _fixed_by_kernel_commit xxxxxxxxxxxx \
> > > > > +     "btrfs: fix corruption after buffer fault in during direct IO append write"
> > > > > +
> > > > > +# On error the test program writes messages to stderr, causing a golden output
> > > > > +# mismatch and making the test fail.
> > > > > +$here/src/dio-append-buf-fault $TEST_DIR/dio-append-buf-fault
> > > > > +
> > > > > +# success, all done
> > > > > +echo "Silence is golden"
> > > > > +status=0
> > > > > +exit
> > > > > diff --git a/tests/generic/362.out b/tests/generic/362.out
> > > > > new file mode 100644
> > > > > index 00000000..0ff40905
> > > > > --- /dev/null
> > > > > +++ b/tests/generic/362.out
> > > > > @@ -0,0 +1,2 @@
> > > > > +QA output created by 362
> > > > > +Silence is golden
> > > > > --
> > > > > 2.43.0
> > > > >
> > > > >
> > > >
> > >
> >
> 





[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