On Tue, Jun 6, 2023 at 11:34 AM Qu Wenruo <quwenruo.btrfs@xxxxxxx> wrote:
>
>
>
> On 2023/6/6 17:35, Filipe Manana wrote:
> > On Tue, Jun 6, 2023 at 4:20 AM Qu Wenruo <wqu@xxxxxxxx> wrote:
> >>
> >> There is a regression on btrfs read-only scrub behavior.
> >>
> >> The commit e02ee89baa66 ("btrfs: scrub: switch scrub_simple_mirror() to
> >> scrub_stripe infrastructure") makes btrfs scrub to ignore the read-only
> >> flag completely, causing scrub to always fix the corruption.
> >>
> >> This test case would create an fs with repairable corruptions, then run
> >> a read-only scrub, and finally to make sure the corruption is not
> >> repaired.
> >>
> >> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
> >> ---
> >> tests/btrfs/288 | 65 +++++++++++++++++++++++++++++++++++++++++++++
> >> tests/btrfs/288.out | 39 +++++++++++++++++++++++++++
> >> 2 files changed, 104 insertions(+)
> >> create mode 100755 tests/btrfs/288
> >> create mode 100644 tests/btrfs/288.out
> >>
> >> diff --git a/tests/btrfs/288 b/tests/btrfs/288
> >> new file mode 100755
> >> index 00000000..7795bdd9
> >> --- /dev/null
> >> +++ b/tests/btrfs/288
> >> @@ -0,0 +1,65 @@
> >> +#! /bin/bash
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +# Copyright (C) 2023 SUSE Linux Products GmbH. All Rights Reserved.
> >> +#
> >> +# FS QA Test 288
> >> +#
> >> +# Make sure btrfs-scrub respects the read-only flag.
> >> +#
> >> +. ./common/preamble
> >> +_begin_fstest auto repair quick volume scrub
> >> +
> >> +# For filedefrag and all the filters
> >
> > This comment is a bit confusing. File defrag? The test doesn't exercise defrag.
> > Probably just leaving the comment out is better, it's obvious since we
> > are using filters.
> >
> >> +. ./common/filter
> >> +
> >> +# real QA test starts here
> >> +
> >> +# Modify as appropriate.
> >> +_supported_fs btrfs
> >> +_require_scratch_dev_pool 2
> >> +
> >> +_require_odirect
> >> +# Overwriting data is forbidden on a zoned block device
> >> +_require_non_zoned_device "${SCRATCH_DEV}"
> >
> > Can we please get a _fixed_by_kernel_commit call to identify the patch
> > that fixes the regression?
>
> Unfortunately it's not yet merged, we only have the offending commit.
Yes, I know. But you can still use it, and we normally do it.
Just use "xxxxxxxxxxxx" as the commit id and leave the patch's subject.
>
> >
> >> +
> >> +_scratch_dev_pool_get 2
> >> +
> >> +# Step 1, create a raid btrfs with one 128K file
> >> +echo "step 1......mkfs.btrfs"
> >> +_scratch_pool_mkfs -d raid1 -b 1G >> $seqres.full 2>&1
> >> +_scratch_mount
> >> +
> >> +$XFS_IO_PROG -f -d -c "pwrite -S 0xaa -b 128K 0 128K" "$SCRATCH_MNT/foobar" |\
> >> + _filter_xfs_io_offset
> >
> > So why do we filter offsets?
> > Why not just a plain _filter_xfs_io as we most commonly do?
>
> Oh yep, that's better.
>
> Thanks for the review,
> Qu
>
> >
> > Thanks.
> >
> >> +
> >> +# Step 2, corrupt one mirror so we can still repair the fs.
> >> +echo "step 2......corrupt one mirror"
> >> +# ensure btrfs-map-logical sees the tree updates
> >> +sync
> >> +
> >> +logical=$(_btrfs_get_first_logical $SCRATCH_MNT/foobar)
> >> +
> >> +physical1=$(_btrfs_get_physical ${logical} 1)
> >> +devpath1=$(_btrfs_get_device_path ${logical} 1)
> >> +
> >> +_scratch_unmount
> >> +
> >> +echo " corrupt stripe #1, devpath $devpath1 physical $physical1" \
> >> + >> $seqres.full
> >> +$XFS_IO_PROG -d -c "pwrite -S 0xf1 -b 64K $physical1 64K" $devpath1 \
> >> + > /dev/null
> >> +
> >> +
> >> +# Step 3, do a read-only scrub, which should not fix the corruption.
> >> +_scratch_mount -o ro
> >> +$BTRFS_UTIL_PROG scrub start -BRrd $SCRATCH_MNT >> $seqres.full 2>&1
> >> +_scratch_unmount
> >> +
> >> +# Step 4, make sure the corruption is still there
> >> +$XFS_IO_PROG -d -c "pread -v -b 512 $physical1 512" $devpath1 |\
> >> + _filter_xfs_io_offset
> >> +
> >> +_scratch_dev_pool_put
> >> +# success, all done
> >> +status=0
> >> +exit
> >> diff --git a/tests/btrfs/288.out b/tests/btrfs/288.out
> >> new file mode 100644
> >> index 00000000..c6c8e886
> >> --- /dev/null
> >> +++ b/tests/btrfs/288.out
> >> @@ -0,0 +1,39 @@
> >> +QA output created by 288
> >> +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 one mirror
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +XXXXXXXX: f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 f1 ................
> >> +read 512/512 bytes
> >> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >> --
> >> 2.39.0
> >>
