Re: [PATCH] generic/646: Test page faults during read and write

[Andreas Gruenbacher <agruenba@redhat.com>]

On Fri, Aug 27, 2021 at 6:17 PM Darrick J. Wong <djwong@kernel.org> wrote:
> On Fri, Aug 27, 2021 at 05:06:03PM +0200, Andreas Gruenbacher wrote:
> > Here's an update of a test case I'm trying to get merged since May.
> > Please have a look; I'm tired of dragging this along.
> >
> > Thanks,
> > Andreas
> >
> > --
> >
> > Some filesystems have problems when the buffer passed to read or write
> > is memory-mapped to the file being read from or written to, and the
> > buffer needs to be faulted in during the read or write.  That's not
> > common, but filesystems are still required to cope with it, and if they
> > fail this test, then they will also fail more complex scenarios that
> > involve multiple files.
> >
> > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> > ---
> >  src/Makefile          |   2 +-
> >  src/mmap-rw-fault.c   | 176 ++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/646     |  27 +++++++
> >  tests/generic/646.out |   2 +
> >  4 files changed, 206 insertions(+), 1 deletion(-)
> >  create mode 100644 src/mmap-rw-fault.c
> >  create mode 100755 tests/generic/646
> >  create mode 100644 tests/generic/646.out
> >
> > diff --git a/src/Makefile b/src/Makefile
> > index 884bd86a..25ab061d 100644
> > --- a/src/Makefile
> > +++ b/src/Makefile
> > @@ -18,7 +18,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
> >       t_ext4_dax_journal_corruption t_ext4_dax_inline_corruption \
> >       t_ofd_locks t_mmap_collision mmap-write-concurrent \
> >       t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \
> > -     t_mmap_writev_overlap checkpoint_journal
> > +     t_mmap_writev_overlap checkpoint_journal mmap-rw-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/mmap-rw-fault.c b/src/mmap-rw-fault.c
> > new file mode 100644
> > index 00000000..244d2cb6
> > --- /dev/null
> > +++ b/src/mmap-rw-fault.c
>
> The generated binary needs a gitignore entry.
>
> > @@ -0,0 +1,176 @@
> > +/* Trigger mmap page faults in the same file during pread and pwrite. */
>
> Needs a SPDX header and a copyright statement.
>
> > +
> > +#ifndef _GNU_SOURCE
> > +#define _GNU_SOURCE /* to get definition of O_DIRECT flag. */
> > +#endif
> > +
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <sys/mman.h>
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +#include <err.h>
>
> <now we skip to main>
>
> > +int main(int argc, char *argv[])
> > +{
> > +     if (argc != 2)
> > +             errx(1, "no test filename argument given");
> > +     filename = argv[1];
> > +
> > +     page_size = ret = sysconf(_SC_PAGE_SIZE);
> > +     if (ret == -1)
> > +             err(1, NULL);
> > +
> > +     ret = posix_memalign(&page, page_size, page_size);
> > +     if (ret) {
> > +             errno = ENOMEM;
> > +             err(1, NULL);
> > +     }
> > +
> > +     /*
> > +      * Make sure page faults during pread are handled correctly.
> > +      */
>
> These test reading from page 1 into (mmap) page 0, right?

Yes. I've added more comments.

> > +     init('a', O_RDWR);
> > +     ret = do_read(fd, addr, page_size, page_size);
> > +     if (ret != page_size)
> > +             err(1, "pread %s: %ld != %u", filename, ret, page_size);
> > +     if (memcmp(addr, page, page_size))
> > +             errx(1, "pread is broken");
> > +     done();
> > +
> > +     init('b', O_RDWR | O_DIRECT);
> > +     ret = do_read(fd, addr, page_size, page_size);
> > +     if (ret != page_size)
> > +             err(1, "pread %s (O_DIRECT): %ld != %u", filename, ret, page_size);
> > +     if (memcmp(addr, page, page_size))
> > +             errx(1, "pread (D_DIRECT) is broken");
> > +     done();
> > +
> > +     /*
> > +      * Make sure page faults during pwrite are handled correctly.
> > +      */
>
> ...and writing from (mmap) page 1 into page 0?

Yes.

> > +     init('c', O_RDWR);
> > +     ret = do_write(fd, addr + page_size, page_size, 0);
> > +     if (ret != page_size)
> > +             err(1, "pwrite %s: %ld != %u", filename, ret, page_size);
> > +     if (memcmp(addr, page, page_size))
> > +             errx(1, "pwrite is broken");
> > +     done();
> > +
> > +     init('d', O_RDWR | O_DIRECT);
> > +     ret = do_write(fd, addr + page_size, page_size, 0);
> > +     if (ret != page_size)
> > +             err(1, "pwrite %s (O_DIRECT): %ld != %u", filename, ret, page_size);
> > +     if (memcmp(addr, page, page_size))
> > +             errx(1, "pwrite (O_DIRECT) is broken");
> > +     done();
> > +
> > +     /*
> > +      * Reading from a hole takes a different code path in the kernel.
> > +      */
>
> ...and finally we test direct reading from page 0 into (mmap) page 0?
>
> If that's correct, then this looks good to me, modulo the license nit at
> the top.

Yes.

> > +     init('e', O_RDWR | O_DIRECT);
> > +     ret = do_read(fd, addr, page_size, 0);
> > +     if (ret != page_size)
> > +             err(1, "pread %s (O_DIRECT) from hole: %ld != %u", filename, ret, page_size);
> > +     memset(page, 0, page_size);
> > +     if (memcmp(addr, page, page_size))
> > +             errx(1, "pread (D_DIRECT) from hole is broken");
> > +     done();
> > +
> > +     if (unlink(filename))
> > +             err(1, "unlink %s", filename);
> > +
> > +     return 0;
> > +}
> > diff --git a/tests/generic/646 b/tests/generic/646
> > new file mode 100755
> > index 00000000..57d8c13d
> > --- /dev/null
> > +++ b/tests/generic/646
> > @@ -0,0 +1,27 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2021 Red Hat, Inc.  All Rights Reserved.
> > +#
> > +# FS QA Test 646
> > +#
> > +# Trigger mmap page faults in the same file during pread and pwrite
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +
> > +# real QA test starts here
> > +
> > +_supported_fs generic
> > +_require_test
> > +_require_test_program mmap-rw-fault
> > +
> > +echo "Silence is golden"
> > +
> > +$here/src/mmap-rw-fault $TEST_DIR/mmap-rw-fault.tmp
>
> _cleanup should clean up the tempfile if the C program crashes, right?
>
> Or use O_TMPFILE and let the kernel clean up the file for you.

Yep, I'll go for the _cleanup approach.

Thanks,
Andreas