Re: [PATCH] fstests: add generic/609 to test O_DIRECT|O_DSYNC

[Josef Bacik <josef@toxicpanda.com>]

On 9/2/20 11:18 AM, Filipe Manana wrote:
> On Wed, Sep 2, 2020 at 4:03 PM Josef Bacik <josef@toxicpanda.com> wrote:
>>
>> We had a problem recently where btrfs would deadlock with
>> O_DIRECT|O_DSYNC because of an unexpected dependency on ->fsync in
>> iomap.  This was only caught by chance with aiostress, because weirdly
>> we don't actually test this particular configuration anywhere in
>> xfstests.  Fix this by adding a basic test that just does
>> O_DIRECT|O_DSYNC writes.  With this test the box deadlocks right away
>> with Btrfs, which would have been helpful in finding this issue before
>> the patches were merged.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   .gitignore                      |  1 +
>>   src/aio-dio-regress/dio-dsync.c | 61 +++++++++++++++++++++++++++++++++
>>   tests/generic/609               | 44 ++++++++++++++++++++++++
>>   tests/generic/group             |  1 +
>>   4 files changed, 107 insertions(+)
>>   create mode 100644 src/aio-dio-regress/dio-dsync.c
>>   create mode 100755 tests/generic/609
>>
>> diff --git a/.gitignore b/.gitignore
>> index 5f5c4a0f..07c8014b 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -175,6 +175,7 @@
>>   /src/aio-dio-regress/aio-last-ref-held-by-io
>>   /src/aio-dio-regress/aiocp
>>   /src/aio-dio-regress/aiodio_sparse2
>> +/src/aio-dio-regress/dio-dsync
>>   /src/log-writes/replay-log
>>   /src/perf/*.pyc
>>
>> diff --git a/src/aio-dio-regress/dio-dsync.c b/src/aio-dio-regress/dio-dsync.c
>> new file mode 100644
>> index 00000000..53cda9ac
>> --- /dev/null
>> +++ b/src/aio-dio-regress/dio-dsync.c
>> @@ -0,0 +1,61 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-newer
>> +/*
>> + * Copyright (c) 2020 Facebook.
>> + * All Rights Reserved.
>> + *
>> + * Do some O_DIRECT writes with O_DSYNC to exercise this path.
>> + */
>> +#include <stdio.h>
>> +#include <sys/types.h>
>> +#include <sys/stat.h>
>> +#include <fcntl.h>
>> +#include <unistd.h>
>> +#include <stdlib.h>
>> +#include <errno.h>
>> +#include <string.h>
>> +
>> +int main(int argc, char **argv)
>> +{
>> +       struct stat st;
>> +       char *buf;
>> +       ssize_t ret;
>> +       int fd, i;
>> +       int bufsize;
>> +
>> +       if (argc != 2) {
>> +               printf("Usage: %s filename\n", argv[0]);
>> +               return 1;
>> +       }
>> +
>> +       fd = open(argv[1], O_DIRECT | O_RDWR | O_TRUNC | O_CREAT | O_DSYNC,
>> +                 0644);
>> +       if (fd < 0) {
>> +               perror(argv[1]);
>> +               return 1;
>> +       }
>> +
>> +       if (fstat(fd, &st)) {
>> +               perror(argv[1]);
>> +               return 1;
>> +       }
>> +       bufsize = st.st_blksize * 10;
>> +
>> +       ret = posix_memalign((void **)&buf, st.st_blksize, bufsize);
>> +       if (ret) {
>> +               errno = ret;
>> +               perror("allocating buffer");
>> +               return 1;
>> +       }
>> +
>> +       memset(buf, 'a', bufsize);
>> +       for (i = 0; i < 10; i++) {
>> +               ret = write(fd, buf, bufsize);
>> +               if (ret < 0) {
>> +                       perror("writing");
>> +                       return 1;
>> +               }
>> +       }
>> +       free(buf);
>> +       close(fd);
>> +       return 0;
>> +}
>> diff --git a/tests/generic/609 b/tests/generic/609
>> new file mode 100755
>> index 00000000..8a888eb9
>> --- /dev/null
>> +++ b/tests/generic/609
>> @@ -0,0 +1,44 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2020 Josef Bacik.  All Rights Reserved.
>> +#
>> +# FS QA Test 609
>> +#
>> +# iomap can call generic_write_sync() if we're O_DSYNC, so write a basic test to
>> +# exercise O_DSYNC so any unsuspecting file systems will get lockdep warnings if
>> +# they're locking isn't compatible.
>> +#
>> +seq=`basename $0`
>> +seqres=$RESULT_DIR/$seq
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=1       # failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +_cleanup()
>> +{
>> +       cd /
>> +       rm -f $tmp.*
>> +       rm -rf $TEST_DIR/file
>> +}
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +. ./common/filter
>> +
>> +# remove previous $seqres.full before test
>> +rm -f $seqres.full
>> +
>> +# Modify as appropriate.
>> +_supported_fs generic
>> +_supported_os Linux
>> +_require_test
>> +_require_aiodio dio-dsync
>> +
>> +$AIO_TEST $TEST_DIR/file
> 
> This can be triggered with xfs_io and without adding a new test program:
> 
> #!/bin/bash
> 
> mkfs.btrfs -f /dev/sdj
> mount /dev/sdj /mnt/sdj
> 
> xfs_io -f -d -c "pwrite -D -V 1 0 4K" /mnt/sdj/foobar
> 
>

Ah that's how you do it, I didn't realize I could pass the open flags on the 
command line, so I had done something wonkey like

xfs_io -c "open -fds FILE" -c "pwrite"

and it hadn't worked, I really didn't want to write a bunch of code.  Thanks, 
I'll fix that,

Josef