* [PATCH] fstests: add generic/609 to test O_DIRECT|O_DSYNC
@ 2020-09-02 15:00 Josef Bacik
2020-09-02 15:03 ` Johannes Thumshirn
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Josef Bacik @ 2020-09-02 15:00 UTC (permalink / raw)
To: linux-btrfs, fstests
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
+
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/generic/group b/tests/generic/group
index aa969bcb..ae2567a0 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -611,3 +611,4 @@
606 auto attr quick dax
607 auto attr quick dax
608 auto attr quick dax
+609 auto quick
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] fstests: add generic/609 to test O_DIRECT|O_DSYNC
2020-09-02 15:00 [PATCH] fstests: add generic/609 to test O_DIRECT|O_DSYNC Josef Bacik
@ 2020-09-02 15:03 ` Johannes Thumshirn
2020-09-02 15:18 ` Filipe Manana
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Johannes Thumshirn @ 2020-09-02 15:03 UTC (permalink / raw)
To: Josef Bacik, linux-btrfs, fstests
On 02/09/2020 17:00, Josef Bacik wrote:
> +# they're locking isn't compatible.
Nit: their isn't it?
Otherwise looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fstests: add generic/609 to test O_DIRECT|O_DSYNC
2020-09-02 15:00 [PATCH] fstests: add generic/609 to test O_DIRECT|O_DSYNC Josef Bacik
2020-09-02 15:03 ` Johannes Thumshirn
@ 2020-09-02 15:18 ` Filipe Manana
2020-09-02 15:29 ` Josef Bacik
2020-09-02 15:37 ` [PATCH][v2] " Josef Bacik
2020-09-02 16:00 ` [PATCH][v3 " Josef Bacik
3 siblings, 1 reply; 7+ messages in thread
From: Filipe Manana @ 2020-09-02 15:18 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, fstests
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
umount /mnt/sdj
It triggers the bug right away:
[22674.919276] BTRFS: device fsid 90a97705-b2ca-46a9-a686-87a131f91786
devid 1 transid 5 /dev/sdj scanned by mkfs.btrfs (1325449)
[22674.960097] BTRFS info (device sdj): disk space caching is enabled
[22674.960101] BTRFS info (device sdj): has skinny extents
[22674.960103] BTRFS info (device sdj): flagging fs with big metadata feature
[22674.965914] BTRFS info (device sdj): checking UUID tree
[22675.034231] ============================================
[22675.034835] WARNING: possible recursive locking detected
[22675.035444] 5.9.0-rc3-btrfs-next-67 #1 Not tainted
[22675.036049] --------------------------------------------
[22675.036657] xfs_io/1325480 is trying to acquire lock:
[22675.037273] ffff9cd0b4aea658
(&sb->s_type->i_mutex_key#15){++++}-{3:3}, at:
btrfs_sync_file+0x179/0x600 [btrfs]
[22675.037936]
but task is already holding lock:
[22675.039143] ffff9cd0b4aea658
(&sb->s_type->i_mutex_key#15){++++}-{3:3}, at:
btrfs_file_write_iter+0x86/0x5f0 [btrfs]
[22675.039791]
other info that might help us debug this:
[22675.041007] Possible unsafe locking scenario:
[22675.042234] CPU0
[22675.042838] ----
[22675.043433] lock(&sb->s_type->i_mutex_key#15);
[22675.044025] lock(&sb->s_type->i_mutex_key#15);
[22675.044624]
*** DEADLOCK ***
[22675.046238] May be due to missing lock nesting notation
[22675.047326] 3 locks held by xfs_io/1325480:
[22675.047855] #0: ffff9cd0ad8ac470 (sb_writers#14){.+.+}-{0:0}, at:
vfs_writev+0xd8/0xf0
[22675.048439] #1: ffff9cd0b4aea658
(&sb->s_type->i_mutex_key#15){++++}-{3:3}, at:
btrfs_file_write_iter+0x86/0x5f0 [btrfs]
[22675.049038] #2: ffff9cd0b4aea4c8 (&ei->dio_sem){++++}-{3:3}, at:
btrfs_direct_IO+0x113/0x160 [btrfs]
[22675.049633]
stack backtrace:
[22675.050727] CPU: 0 PID: 1325480 Comm: xfs_io Not tainted
5.9.0-rc3-btrfs-next-67 #1
[22675.051275] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
[22675.052399] Call Trace:
[22675.052976] dump_stack+0x8d/0xc0
[22675.053559] __lock_acquire.cold+0x210/0x2e9
[22675.054145] ? rcu_read_lock_sched_held+0x5d/0x90
[22675.054777] lock_acquire+0xb1/0x470
[22675.055352] ? btrfs_sync_file+0x179/0x600 [btrfs]
[22675.055923] down_write+0x40/0x130
[22675.056496] ? btrfs_sync_file+0x179/0x600 [btrfs]
[22675.057044] btrfs_sync_file+0x179/0x600 [btrfs]
[22675.057593] ? __do_sys_kcmp+0x980/0xce0
[22675.058143] iomap_dio_complete+0x112/0x130
[22675.058679] ? iomap_dio_rw+0x2c4/0x620
[22675.059199] iomap_dio_rw+0x494/0x620
[22675.059723] ? btrfs_direct_IO+0xd5/0x160 [btrfs]
[22675.060238] btrfs_direct_IO+0xd5/0x160 [btrfs]
[22675.060738] btrfs_file_write_iter+0x215/0x5f0 [btrfs]
[22675.061221] do_iter_readv_writev+0x169/0x1f0
[22675.061698] do_iter_write+0x80/0x1b0
[22675.062170] vfs_writev+0xa6/0xf0
[22675.062643] ? syscall_enter_from_user_mode+0xb4/0x270
[22675.063114] ? find_held_lock+0x32/0x90
[22675.063581] ? syscall_enter_from_user_mode+0xb4/0x270
[22675.064045] do_pwritev+0x8f/0xd0
[22675.064489] do_syscall_64+0x33/0x80
[22675.064934] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[22675.065372] RIP: 0033:0x7fac51c898b9
[22675.065792] Code: 89 fd 53 44 89 c3 48 83 ec 18 64 8b 04 25 18 00
00 00 85 c0 0f 85 97 00 00 00 45 89 c1 49 89 ca 45 31 c0 b8 48 01 00
00 0f 05 <48> 3d 00 f0 ff ff 0f 87 cb 00 00 00 48 85 c0 79 44 4c 8b 2d
9f 75
[22675.066692] RSP: 002b:00007ffdae24ecd0 EFLAGS: 00000246 ORIG_RAX:
0000000000000148
[22675.067155] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007fac51c898b9
[22675.067628] RDX: 0000000000000001 RSI: 000055990bc5bf90 RDI: 0000000000000003
[22675.068113] RBP: 0000000000000003 R08: 0000000000000000 R09: 0000000000000002
[22675.068592] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
[22675.069074] R13: 0000000000000000 R14: 0000000000000001 R15: 0000000000000002
Or is there anything I missed?
Thanks.
> +
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/generic/group b/tests/generic/group
> index aa969bcb..ae2567a0 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -611,3 +611,4 @@
> 606 auto attr quick dax
> 607 auto attr quick dax
> 608 auto attr quick dax
> +609 auto quick
> --
> 2.26.2
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fstests: add generic/609 to test O_DIRECT|O_DSYNC
2020-09-02 15:18 ` Filipe Manana
@ 2020-09-02 15:29 ` Josef Bacik
0 siblings, 0 replies; 7+ messages in thread
From: Josef Bacik @ 2020-09-02 15:29 UTC (permalink / raw)
To: fdmanana; +Cc: linux-btrfs, fstests
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH][v2] fstests: add generic/609 to test O_DIRECT|O_DSYNC
2020-09-02 15:00 [PATCH] fstests: add generic/609 to test O_DIRECT|O_DSYNC Josef Bacik
2020-09-02 15:03 ` Johannes Thumshirn
2020-09-02 15:18 ` Filipe Manana
@ 2020-09-02 15:37 ` Josef Bacik
2020-09-02 16:00 ` [PATCH][v3 " Josef Bacik
3 siblings, 0 replies; 7+ messages in thread
From: Josef Bacik @ 2020-09-02 15:37 UTC (permalink / raw)
To: linux-btrfs, fstests; +Cc: Johannes Thumshirn
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.
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
v1->v2:
- Fix the spelling thing Johannes spotted.
- Use xfs_io like a normal person.
tests/generic/609 | 44 ++++++++++++++++++++++++++++++++++++++++++++
tests/generic/group | 1 +
2 files changed, 45 insertions(+)
create mode 100755 tests/generic/609
diff --git a/tests/generic/609 b/tests/generic/609
new file mode 100755
index 00000000..6f3ab055
--- /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
+# their 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_xfs_io_command "pwrite" "-DV"
+
+$XFS_IO_PROG -f -d -c "pwrite -D -V 1 0 4k" $TEST_DIR/file
+
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/generic/group b/tests/generic/group
index aa969bcb..ae2567a0 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -611,3 +611,4 @@
606 auto attr quick dax
607 auto attr quick dax
608 auto attr quick dax
+609 auto quick
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH][v3 fstests: add generic/609 to test O_DIRECT|O_DSYNC
2020-09-02 15:00 [PATCH] fstests: add generic/609 to test O_DIRECT|O_DSYNC Josef Bacik
` (2 preceding siblings ...)
2020-09-02 15:37 ` [PATCH][v2] " Josef Bacik
@ 2020-09-02 16:00 ` Josef Bacik
2020-09-02 16:50 ` Darrick J. Wong
3 siblings, 1 reply; 7+ messages in thread
From: Josef Bacik @ 2020-09-02 16:00 UTC (permalink / raw)
To: linux-btrfs, fstests; +Cc: Johannes Thumshirn
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.
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
v2->v3:
- This time with 609.out added, verified it passed with xfs.
tests/generic/609 | 43 +++++++++++++++++++++++++++++++++++++++++++
tests/generic/609.out | 3 +++
tests/generic/group | 1 +
3 files changed, 47 insertions(+)
create mode 100755 tests/generic/609
create mode 100644 tests/generic/609.out
diff --git a/tests/generic/609 b/tests/generic/609
new file mode 100755
index 00000000..3d1c97b2
--- /dev/null
+++ b/tests/generic/609
@@ -0,0 +1,43 @@
+#! /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
+# their 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_xfs_io_command "pwrite" "-DV"
+
+$XFS_IO_PROG -f -d -c "pwrite -D -V 1 0 4k" $TEST_DIR/file | _filter_xfs_io
+
+status=0
+exit
diff --git a/tests/generic/609.out b/tests/generic/609.out
new file mode 100644
index 00000000..db3242cb
--- /dev/null
+++ b/tests/generic/609.out
@@ -0,0 +1,3 @@
+QA output created by 609
+wrote 4096/4096 bytes at offset 0
+XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
diff --git a/tests/generic/group b/tests/generic/group
index aa969bcb..ae2567a0 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -611,3 +611,4 @@
606 auto attr quick dax
607 auto attr quick dax
608 auto attr quick dax
+609 auto quick
--
2.28.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH][v3 fstests: add generic/609 to test O_DIRECT|O_DSYNC
2020-09-02 16:00 ` [PATCH][v3 " Josef Bacik
@ 2020-09-02 16:50 ` Darrick J. Wong
0 siblings, 0 replies; 7+ messages in thread
From: Darrick J. Wong @ 2020-09-02 16:50 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, fstests, Johannes Thumshirn
On Wed, Sep 02, 2020 at 12:00:44PM -0400, Josef Bacik 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.
>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> v2->v3:
> - This time with 609.out added, verified it passed with xfs.
>
> tests/generic/609 | 43 +++++++++++++++++++++++++++++++++++++++++++
> tests/generic/609.out | 3 +++
> tests/generic/group | 1 +
> 3 files changed, 47 insertions(+)
> create mode 100755 tests/generic/609
> create mode 100644 tests/generic/609.out
>
> diff --git a/tests/generic/609 b/tests/generic/609
> new file mode 100755
> index 00000000..3d1c97b2
> --- /dev/null
> +++ b/tests/generic/609
> @@ -0,0 +1,43 @@
> +#! /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
> +# their 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_xfs_io_command "pwrite" "-DV"
> +
> +$XFS_IO_PROG -f -d -c "pwrite -D -V 1 0 4k" $TEST_DIR/file | _filter_xfs_io
I wonder, does this also work if you did:
$XFS_IO_PROG -f -d -s -c 'pwrite 0 4k' $TEST_DIR/file
In other words, can you reproduce the problem with good old pwrite() and
a file descriptor opened O_SYNC? Or do you specifically have to have
pwritev2 with RWF_DSYNC?
(I might also write 64k to future proof this testcase for the day when
someone builds an fs that can only do 64k direct writes, but maybe
that's crazy...)
> +
> +status=0
> +exit
> diff --git a/tests/generic/609.out b/tests/generic/609.out
> new file mode 100644
> index 00000000..db3242cb
> --- /dev/null
> +++ b/tests/generic/609.out
> @@ -0,0 +1,3 @@
> +QA output created by 609
> +wrote 4096/4096 bytes at offset 0
> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> diff --git a/tests/generic/group b/tests/generic/group
> index aa969bcb..ae2567a0 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -611,3 +611,4 @@
> 606 auto attr quick dax
> 607 auto attr quick dax
> 608 auto attr quick dax
> +609 auto quick
This probably ought to be 'auto quick rw' since it's a write test.
The rest of the logic looks sound to me though.
--D
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-09-02 16:52 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-02 15:00 [PATCH] fstests: add generic/609 to test O_DIRECT|O_DSYNC Josef Bacik
2020-09-02 15:03 ` Johannes Thumshirn
2020-09-02 15:18 ` Filipe Manana
2020-09-02 15:29 ` Josef Bacik
2020-09-02 15:37 ` [PATCH][v2] " Josef Bacik
2020-09-02 16:00 ` [PATCH][v3 " Josef Bacik
2020-09-02 16:50 ` Darrick J. Wong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).