diff mbox series

[fstests] btrfs: add regression test for fsync vs. size-extending direct I/O into prealloc crash

Message ID 297da2b53ce9b697d82d89afd322b2cc0d0f392d.1716492850.git.osandov@osandov.com (mailing list archive)
State New, archived
Headers show
Series [fstests] btrfs: add regression test for fsync vs. size-extending direct I/O into prealloc crash | expand

Commit Message

Omar Sandoval May 23, 2024, 7:34 p.m. UTC
From: Omar Sandoval <osandov@fb.com>

Since this is a race, we just try to make the race happen in a loop and
pass if it doesn't crash after all of our attempts.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 tests/btrfs/312     | 66 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/312.out |  2 ++
 2 files changed, 68 insertions(+)
 create mode 100755 tests/btrfs/312
 create mode 100644 tests/btrfs/312.out

Comments

Filipe Manana May 24, 2024, 1:24 p.m. UTC | #1
On Thu, May 23, 2024 at 9:43 PM Omar Sandoval <osandov@osandov.com> wrote:
>
> From: Omar Sandoval <osandov@fb.com>
>
> Since this is a race, we just try to make the race happen in a loop and
> pass if it doesn't crash after all of our attempts.
>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  tests/btrfs/312     | 66 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/312.out |  2 ++
>  2 files changed, 68 insertions(+)
>  create mode 100755 tests/btrfs/312
>  create mode 100644 tests/btrfs/312.out
>
> diff --git a/tests/btrfs/312 b/tests/btrfs/312
> new file mode 100755
> index 00000000..aaca0e3e
> --- /dev/null
> +++ b/tests/btrfs/312
> @@ -0,0 +1,66 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) Meta Platforms, Inc. and affiliates.
> +#
> +# FS QA Test 312
> +#
> +# Repeatedly fsync after size-extending direct I/O into a preallocated extent.
> +#
> +. ./common/preamble
> +_begin_fstest dangerous log prealloc

Can also add in the "quick group".
A since this is writing into a prealloc extent, the correct group is
"preallocrw".

> +
> +_supported_fs btrfs
> +_require_scratch
> +_require_btrfs_command inspect-internal dump-tree
> +_require_btrfs_command inspect-internal inode-resolve

Missing a _require_attrs because $SETFATTR_PROG is needed/used.

Also missing a:

_require_xfs_io_command falloc -k

> +_fixed_by_kernel_commit XXXXXXXXXXXX \
> +       "btrfs: fix crash on racing fsync and size-extending direct I/O into prealloc"
> +
> +_scratch_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"
> +_scratch_mount
> +
> +sectorsize=$(_scratch_btrfs_sectorsize)
> +
> +# Create a bunch of files so that we hopefully get one whose items are at the
> +# end of a leaf.
> +for ((i = 0; i < 1000; i++)); do
> +       $XFS_IO_PROG -c "open -f -d $SCRATCH_MNT/$i" -c "falloc -k 0 $((sectorsize * 3))" -c "pwrite -q 0 $sectorsize"

You can pass -d to $XFS_IO_PROG and make this a bit shorter:

$XFS_IO_PROG -f -d -c "falloc -k 0 $((sectorsize * 3))" -c "pwrite -q
0 $sectorsize"  "$SCRATCH_MNT/$i"

> +       $SETFATTR_PROG -n user.a -v a "$SCRATCH_MNT/$i"
> +done
> +touch "$SCRATCH_MNT/$i"

Why is this touch needed here?
I can trigger the bug without it - it's confusing, if it's really
needed then add a comment explaining why please.

So now this works with the default leaf size of 16K on x86.
But what about in case we run with MKFS_OPTIONS="-n 64K", or on a PPC
machine where default leaf/node size is 64K?

The 1000 iterations aren't enough, so I would suggest making the test
always create a fs with a 64K node size and adjusting the iterations
to be enough to trigger the bug.

> +
> +_scratch_unmount
> +
> +ino=$($BTRFS_UTIL_PROG inspect-internal dump-tree "$SCRATCH_DEV" -t 5 |
> +      $AWK_PROG -v sectorsize="$sectorsize" '
> +match($0, /^leaf [0-9]+ items ([0-9]+)/, arr) {
> +       nritems = arr[1]
> +}
> +match($0, /item ([0-9]+) key \(([0-9]+) EXTENT_DATA ([0-9]+)\)/, arr) {
> +       if (arr[1] == nritems - 1 && arr[3] == sectorsize) {
> +               print arr[2]
> +               exit
> +       }
> +}
> +')
> +
> +if [ -z "$ino" ]; then
> +       _fail "Extent at end of leaf not found"
> +fi
> +
> +_scratch_mount
> +path=$($BTRFS_UTIL_PROG inspect-internal inode-resolve "$ino" "$SCRATCH_MNT")
> +
> +# Try repeatedly to reproduce the race of an ordered extent finishing while
> +# we're logging prealloc extents beyond i_size.
> +for ((i = 0; i < 1000; i++)); do
> +       $XFS_IO_PROG -c "open -t -d $path" -c "falloc -k 0 $((sectorsize * 3))" -c "pwrite -q -w 0 $sectorsize"
> +       $SETFATTR_PROG -n user.a -v a "$path"
> +       $XFS_IO_PROG -c "open -d $path" -c "pwrite -q -w $sectorsize $sectorsize" || exit 1

the || exit 1 is odd here.
Normally we do || _fail "some reason" or just don't do anything at
all, as a golden output mismatch due to an error will make the test
fail.

Also don't forget to cc the fstests mailing list.

Thanks.

> +done
> +
> +# If it didn't crash, we're good.
> +
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/btrfs/312.out b/tests/btrfs/312.out
> new file mode 100644
> index 00000000..6e72aa94
> --- /dev/null
> +++ b/tests/btrfs/312.out
> @@ -0,0 +1,2 @@
> +QA output created by 312
> +Silence is golden
> --
> 2.45.1
>
>
diff mbox series

Patch

diff --git a/tests/btrfs/312 b/tests/btrfs/312
new file mode 100755
index 00000000..aaca0e3e
--- /dev/null
+++ b/tests/btrfs/312
@@ -0,0 +1,66 @@ 
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) Meta Platforms, Inc. and affiliates.
+#
+# FS QA Test 312
+#
+# Repeatedly fsync after size-extending direct I/O into a preallocated extent.
+#
+. ./common/preamble
+_begin_fstest dangerous log prealloc
+
+_supported_fs btrfs
+_require_scratch
+_require_btrfs_command inspect-internal dump-tree
+_require_btrfs_command inspect-internal inode-resolve
+_fixed_by_kernel_commit XXXXXXXXXXXX \
+	"btrfs: fix crash on racing fsync and size-extending direct I/O into prealloc"
+
+_scratch_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"
+_scratch_mount
+
+sectorsize=$(_scratch_btrfs_sectorsize)
+
+# Create a bunch of files so that we hopefully get one whose items are at the
+# end of a leaf.
+for ((i = 0; i < 1000; i++)); do
+	$XFS_IO_PROG -c "open -f -d $SCRATCH_MNT/$i" -c "falloc -k 0 $((sectorsize * 3))" -c "pwrite -q 0 $sectorsize"
+	$SETFATTR_PROG -n user.a -v a "$SCRATCH_MNT/$i"
+done
+touch "$SCRATCH_MNT/$i"
+
+_scratch_unmount
+
+ino=$($BTRFS_UTIL_PROG inspect-internal dump-tree "$SCRATCH_DEV" -t 5 |
+      $AWK_PROG -v sectorsize="$sectorsize" '
+match($0, /^leaf [0-9]+ items ([0-9]+)/, arr) {
+	nritems = arr[1]
+}
+match($0, /item ([0-9]+) key \(([0-9]+) EXTENT_DATA ([0-9]+)\)/, arr) {
+	if (arr[1] == nritems - 1 && arr[3] == sectorsize) {
+		print arr[2]
+		exit
+	}
+}
+')
+
+if [ -z "$ino" ]; then
+	_fail "Extent at end of leaf not found"
+fi
+
+_scratch_mount
+path=$($BTRFS_UTIL_PROG inspect-internal inode-resolve "$ino" "$SCRATCH_MNT")
+
+# Try repeatedly to reproduce the race of an ordered extent finishing while
+# we're logging prealloc extents beyond i_size.
+for ((i = 0; i < 1000; i++)); do
+	$XFS_IO_PROG -c "open -t -d $path" -c "falloc -k 0 $((sectorsize * 3))" -c "pwrite -q -w 0 $sectorsize"
+	$SETFATTR_PROG -n user.a -v a "$path"
+	$XFS_IO_PROG -c "open -d $path" -c "pwrite -q -w $sectorsize $sectorsize" || exit 1
+done
+
+# If it didn't crash, we're good.
+
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/btrfs/312.out b/tests/btrfs/312.out
new file mode 100644
index 00000000..6e72aa94
--- /dev/null
+++ b/tests/btrfs/312.out
@@ -0,0 +1,2 @@ 
+QA output created by 312
+Silence is golden