| Message ID | 20220622045844.3219390-5-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] btrfs: fix the_btrfs_get_physical invocation in btrfs-map-logical | expand |
So while this test properly documents the current behavior, it failed to grasp how broken that behavior ist: the current read repair code writes back the uncompressed data to disk even for a compressed extent, and this test verified the behavior. Below is a correct test that fails on current mainline. I'll send fixes but right now they depend on a lot of prep work. --- From 6b6c505f75c6c7cc15359f14053b1db43e3d3091 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig <hch@lst.de> Date: Wed, 22 Jun 2022 06:55:36 +0200 Subject: btrfs: test read repair on a corrupted compressed extent Exercise read repair on a corrupted compressed sector. Signed-off-by: Christoph Hellwig <hch@lst.de> --- tests/btrfs/270 | 82 +++++++++++++++++++++++++++++++++++++++++++++ tests/btrfs/270.out | 7 ++++ 2 files changed, 89 insertions(+) create mode 100755 tests/btrfs/270 create mode 100644 tests/btrfs/270.out diff --git a/tests/btrfs/270 b/tests/btrfs/270 new file mode 100755 index 00000000..5b73fb15 --- /dev/null +++ b/tests/btrfs/270 @@ -0,0 +1,82 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2017 Liu Bo. All Rights Reserved. +# +# FS QA Test 270 +# +# Regression test for btrfs buffered read repair of compressed data. +# +. ./common/preamble +_begin_fstest auto quick read_repair compress + +. ./common/filter + +_supported_fs btrfs +_require_btrfs_command inspect-internal dump-tree +_require_non_zoned_device "${SCRATCH_DEV}" # no overwrites on zoned devices +_require_scratch_dev_pool 2 +_scratch_dev_pool_get 2 + +get_physical() +{ + local logical=$1 + local stripe=$2 + $BTRFS_UTIL_PROG inspect-internal dump-tree -t 3 $SCRATCH_DEV | \ + grep $logical -A 6 | \ + $AWK_PROG "(\$1 ~ /stripe/ && \$3 ~ /devid/ && \$2 ~ /$stripe/) { print \$6 }" +} + +get_devid() +{ + local logical=$1 + local stripe=$2 + $BTRFS_UTIL_PROG inspect-internal dump-tree -t 3 $SCRATCH_DEV | \ + grep $logical -A 6 | \ + $AWK_PROG "(\$1 ~ /stripe/ && \$3 ~ /devid/ && \$2 ~ /$stripe/) { print \$4 }" +} + +get_device_path() +{ + local devid=$1 + echo "$SCRATCH_DEV_POOL" | $AWK_PROG "{print \$$devid}" +} + + +echo "step 1......mkfs.btrfs" +_check_minimal_fs_size $(( 1024 * 1024 * 1024 )) +_scratch_pool_mkfs "-d raid1 -b 1G" >>$seqres.full 2>&1 +_scratch_mount -ocompress + +# Create a file with all data being compressed +$XFS_IO_PROG -f -c "pwrite -S 0xaa -W -b 128K 0 128K" \ + "$SCRATCH_MNT/foobar" | _filter_xfs_io_offset + +logical_in_btrfs=$(_btrfs_get_first_logical $SCRATCH_MNT/foobar) +physical=$(get_physical ${logical_in_btrfs} 1) +devid=$(get_devid ${logical_in_btrfs} 1) +devpath=$(get_device_path ${devid}) + +_scratch_unmount +echo "step 2......corrupt file extent" +echo " corrupt stripe #1, devid $devid devpath $devpath physical $physical" \ + >> $seqres.full +dd if=$devpath of=$TEST_DIR/$seq.dump.good skip=$physical bs=1 count=4096 \ + 2>/dev/null +$XFS_IO_PROG -c "pwrite -S 0xbb -b 4K $physical 4K" $devpath > /dev/null + +_scratch_mount + +echo "step 3......repair the bad copy" +_btrfs_buffered_read_on_mirror 1 2 "$SCRATCH_MNT/foobar" 0 128K + +_scratch_unmount + +echo "step 4......check if the repair worked" +dd if=$devpath of=$TEST_DIR/$seq.dump skip=$physical bs=1 count=4096 \ + 2>/dev/null +cmp -bl $TEST_DIR/$seq.dump.good $TEST_DIR/$seq.dump + +_scratch_dev_pool_put +# success, all done +status=0 +exit diff --git a/tests/btrfs/270.out b/tests/btrfs/270.out new file mode 100644 index 00000000..6d744c02 --- /dev/null +++ b/tests/btrfs/270.out @@ -0,0 +1,7 @@ +QA output created by 270 +step 1......mkfs.btrfs +wrote 131072/131072 bytes +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +step 2......corrupt file extent +step 3......repair the bad copy +step 4......check if the repair worked
On Wed, Jun 22, 2022 at 11:21:40AM +0200, Christoph Hellwig wrote: > So while this test properly documents the current behavior, it failed > to grasp how broken that behavior ist: the current read repair code > writes back the uncompressed data to disk even for a compressed extent, > and this test verified the behavior. > > Below is a correct test that fails on current mainline. I'll send fixes > but right now they depend on a lot of prep work. > > --- > From 6b6c505f75c6c7cc15359f14053b1db43e3d3091 Mon Sep 17 00:00:00 2001 > From: Christoph Hellwig <hch@lst.de> > Date: Wed, 22 Jun 2022 06:55:36 +0200 > Subject: btrfs: test read repair on a corrupted compressed extent > > Exercise read repair on a corrupted compressed sector. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > tests/btrfs/270 | 82 +++++++++++++++++++++++++++++++++++++++++++++ > tests/btrfs/270.out | 7 ++++ > 2 files changed, 89 insertions(+) > create mode 100755 tests/btrfs/270 > create mode 100644 tests/btrfs/270.out > > diff --git a/tests/btrfs/270 b/tests/btrfs/270 > new file mode 100755 > index 00000000..5b73fb15 > --- /dev/null > +++ b/tests/btrfs/270 > @@ -0,0 +1,82 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2017 Liu Bo. All Rights Reserved. ^^^^^^^^^^^ Is it a wrong copy&paste ? > +# > +# FS QA Test 270 > +# > +# Regression test for btrfs buffered read repair of compressed data. If this's a regression test, I'd like to see the fix be reviewed/acked at first :) Thanks, Zorro > +# > +. ./common/preamble > +_begin_fstest auto quick read_repair compress > + > +. ./common/filter > + > +_supported_fs btrfs > +_require_btrfs_command inspect-internal dump-tree > +_require_non_zoned_device "${SCRATCH_DEV}" # no overwrites on zoned devices > +_require_scratch_dev_pool 2 > +_scratch_dev_pool_get 2 > + > +get_physical() > +{ > + local logical=$1 > + local stripe=$2 > + $BTRFS_UTIL_PROG inspect-internal dump-tree -t 3 $SCRATCH_DEV | \ > + grep $logical -A 6 | \ > + $AWK_PROG "(\$1 ~ /stripe/ && \$3 ~ /devid/ && \$2 ~ /$stripe/) { print \$6 }" > +} > + > +get_devid() > +{ > + local logical=$1 > + local stripe=$2 > + $BTRFS_UTIL_PROG inspect-internal dump-tree -t 3 $SCRATCH_DEV | \ > + grep $logical -A 6 | \ > + $AWK_PROG "(\$1 ~ /stripe/ && \$3 ~ /devid/ && \$2 ~ /$stripe/) { print \$4 }" > +} > + > +get_device_path() > +{ > + local devid=$1 > + echo "$SCRATCH_DEV_POOL" | $AWK_PROG "{print \$$devid}" > +} > + > + > +echo "step 1......mkfs.btrfs" > +_check_minimal_fs_size $(( 1024 * 1024 * 1024 )) > +_scratch_pool_mkfs "-d raid1 -b 1G" >>$seqres.full 2>&1 > +_scratch_mount -ocompress > + > +# Create a file with all data being compressed > +$XFS_IO_PROG -f -c "pwrite -S 0xaa -W -b 128K 0 128K" \ > + "$SCRATCH_MNT/foobar" | _filter_xfs_io_offset > + > +logical_in_btrfs=$(_btrfs_get_first_logical $SCRATCH_MNT/foobar) > +physical=$(get_physical ${logical_in_btrfs} 1) > +devid=$(get_devid ${logical_in_btrfs} 1) > +devpath=$(get_device_path ${devid}) > + > +_scratch_unmount > +echo "step 2......corrupt file extent" > +echo " corrupt stripe #1, devid $devid devpath $devpath physical $physical" \ > + >> $seqres.full > +dd if=$devpath of=$TEST_DIR/$seq.dump.good skip=$physical bs=1 count=4096 \ > + 2>/dev/null > +$XFS_IO_PROG -c "pwrite -S 0xbb -b 4K $physical 4K" $devpath > /dev/null > + > +_scratch_mount > + > +echo "step 3......repair the bad copy" > +_btrfs_buffered_read_on_mirror 1 2 "$SCRATCH_MNT/foobar" 0 128K > + > +_scratch_unmount > + > +echo "step 4......check if the repair worked" > +dd if=$devpath of=$TEST_DIR/$seq.dump skip=$physical bs=1 count=4096 \ > + 2>/dev/null > +cmp -bl $TEST_DIR/$seq.dump.good $TEST_DIR/$seq.dump > + > +_scratch_dev_pool_put > +# success, all done > +status=0 > +exit > diff --git a/tests/btrfs/270.out b/tests/btrfs/270.out > new file mode 100644 > index 00000000..6d744c02 > --- /dev/null > +++ b/tests/btrfs/270.out > @@ -0,0 +1,7 @@ > +QA output created by 270 > +step 1......mkfs.btrfs > +wrote 131072/131072 bytes > +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > +step 2......corrupt file extent > +step 3......repair the bad copy > +step 4......check if the repair worked > -- > 2.30.2 >
On Wed, Jun 22, 2022 at 08:41:18PM +0800, Zorro Lang wrote: > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright (c) 2017 Liu Bo. All Rights Reserved. > ^^^^^^^^^^^ > Is it a wrong copy&paste ? A lot of the test is copied from btrfs/141, so I wanted to keep the copyright intact. > > +# > > +# FS QA Test 270 > > +# > > +# Regression test for btrfs buffered read repair of compressed data. > > If this's a regression test, I'd like to see the fix be reviewed/acked > at first :) Heh. Actually I'm not sure it is a regression, this was copy and pasted as well. I think it actually has been broken since day 1.
On Wed, Jun 22, 2022 at 03:07:45PM +0200, Christoph Hellwig wrote: > On Wed, Jun 22, 2022 at 08:41:18PM +0800, Zorro Lang wrote: > > > +# SPDX-License-Identifier: GPL-2.0 > > > +# Copyright (c) 2017 Liu Bo. All Rights Reserved. > > ^^^^^^^^^^^ > > Is it a wrong copy&paste ? > > A lot of the test is copied from btrfs/141, so I wanted to keep the > copyright intact. Oh, there're some cases copy from old cases in fstests, but they are changed a little to have different coverage. Generally they metioned they copy from which one case (number). I don't mind if you'd like to keep original Copyright name, but the copyright time is old for a new test case, I don't know if we'd better to update it. Hmm... sorry, not good at these copyright things... > > > > +# > > > +# FS QA Test 270 > > > +# > > > +# Regression test for btrfs buffered read repair of compressed data. > > > > If this's a regression test, I'd like to see the fix be reviewed/acked > > at first :) > > Heh. Actually I'm not sure it is a regression, this was copy and > pasted as well. I think it actually has been broken since day 1. >
diff --git a/tests/btrfs/270 b/tests/btrfs/270 new file mode 100755 index 00000000..4229a02c --- /dev/null +++ b/tests/btrfs/270 @@ -0,0 +1,79 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2017 Liu Bo. All Rights Reserved. +# +# FS QA Test 270 +# +# Regression test for btrfs buffered read repair of compressed data. +# +. ./common/preamble +_begin_fstest auto quick read_repair + +. ./common/filter + +_supported_fs btrfs +_require_btrfs_command inspect-internal dump-tree +_require_non_zoned_device "${SCRATCH_DEV}" # no overwrites on zoned devices +_require_scratch_dev_pool 2 +_scratch_dev_pool_get 2 + +get_physical() +{ + local logical=$1 + local stripe=$2 + $BTRFS_UTIL_PROG inspect-internal dump-tree -t 3 $SCRATCH_DEV | \ + grep $logical -A 6 | \ + $AWK_PROG "(\$1 ~ /stripe/ && \$3 ~ /devid/ && \$2 ~ /$stripe/) { print \$6 }" +} + +get_devid() +{ + local logical=$1 + local stripe=$2 + $BTRFS_UTIL_PROG inspect-internal dump-tree -t 3 $SCRATCH_DEV | \ + grep $logical -A 6 | \ + $AWK_PROG "(\$1 ~ /stripe/ && \$3 ~ /devid/ && \$2 ~ /$stripe/) { print \$4 }" +} + +get_device_path() +{ + local devid=$1 + echo "$SCRATCH_DEV_POOL" | $AWK_PROG "{print \$$devid}" +} + + +echo "step 1......mkfs.btrfs" +_check_minimal_fs_size $(( 1024 * 1024 * 1024 )) +_scratch_pool_mkfs "-d raid1 -b 1G" >>$seqres.full 2>&1 +_scratch_mount -ocompress + +# Create a file with all data being compressed +$XFS_IO_PROG -f -c "pwrite -S 0xaa -W -b 128K 0 128K" \ + "$SCRATCH_MNT/foobar" | _filter_xfs_io_offset + +logical_in_btrfs=$(_btrfs_get_first_logical $SCRATCH_MNT/foobar) +physical=$(get_physical ${logical_in_btrfs} 1) +devid=$(get_devid ${logical_in_btrfs} 1) +devpath=$(get_device_path ${devid}) + +_scratch_unmount +echo "step 2......corrupt file extent" +echo " corrupt stripe #1, devid $devid devpath $devpath physical $physical" \ + >> $seqres.full +$XFS_IO_PROG -d -c "pwrite -S 0xbb -b 64K $physical 64K" $devpath > /dev/null + +_scratch_mount + +echo "step 3......repair the bad copy" +_btrfs_buffered_read_on_mirror 1 2 "$SCRATCH_MNT/foobar" 0 128K + +_scratch_unmount + +echo "step 4......check if the repair worked" +$XFS_IO_PROG -c "pread -v -b 512 $physical 512" $devpath |\ + _filter_xfs_io_offset + +_scratch_dev_pool_put +# success, all done +status=0 +exit diff --git a/tests/btrfs/270.out b/tests/btrfs/270.out new file mode 100644 index 00000000..53a80692 --- /dev/null +++ b/tests/btrfs/270.out @@ -0,0 +1,41 @@ +QA output created by 270 +step 1......mkfs.btrfs +wrote 131072/131072 bytes +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +step 2......corrupt file extent +step 3......repair the bad copy +step 4......check if the repair worked +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +XXXXXXXX: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................ +read 512/512 bytes +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
Exercise read repair on a corrupted compressed sector. Signed-off-by: Christoph Hellwig <hch@lst.de> --- tests/btrfs/270 | 79 +++++++++++++++++++++++++++++++++++++++++++++ tests/btrfs/270.out | 41 +++++++++++++++++++++++ 2 files changed, 120 insertions(+) create mode 100755 tests/btrfs/270 create mode 100644 tests/btrfs/270.out