Message ID | f5ba8625d6277035b69e466f6ea87f19620f7fcb.1599058822.git.josef@toxicpanda.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | fstests: add generic/609 to test O_DIRECT|O_DSYNC | expand |
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>
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 >
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
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
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