Re: [PATCH] btrfs: add a test case to verify read-only scrub

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/6/6 17:35, Filipe Manana wrote:
> On Tue, Jun 6, 2023 at 4:20 AM Qu Wenruo <wqu@suse.com> 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@suse.com>
>> ---
>>   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.

>
>> +
>> +_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
>>