On 9/2/20 11:18 AM, Filipe Manana wrote:
On Wed, Sep 2, 2020 at 4:03 PM Josef Bacik <josef@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
---
.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
