Re: [PATCH 1/2] common/filter.btrfs: add a new _filter_snapshot

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

在 2024/4/6 03:16, Darrick J. Wong 写道:
> On Fri, Apr 05, 2024 at 08:20:59PM +1030, Qu Wenruo wrote:
>>
>>
>> 在 2024/4/5 19:55, Anand Jain 写道:
>>>
>>>
>>> On 4/5/24 16:52, Qu Wenruo wrote:
>>>>
>>>>
>>>> 在 2024/4/5 19:15, Anand Jain 写道:
>>>>> As the newer btrfs-progs have changed the output of the command
>>>>> "btrfs subvolume snapshot," which is part of the golden output,
>>>>> create a helper filter to ensure the test cases pass on older
>>>>> btrfs-progs.
>>>>>
>>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>>
>>>> Can we stop the golden output filter game?
>>>>
>>>>   From day one I'm not a big fan of the golden output idea.
>>>> For snapshot/subvolume creation, we don't really care about what the
>>>> output is, we only care if there is any error (which would come from
>>>> stderr).
>>>>
>>>> In that case, why not just redirect the stdout to null?
>>>>
>>>> To me, if we really care something from the stdout, we can still save it
>>>> and let bash/awk/grep to process it, like what we did for various test
>>>> cases, and then save the result to seqres.full.
>
> That sums up what output filters do; I don't understand the objection
> here...

Well, I have seen quite some cases where we need to verify the speed of
scrub to be inside a reasonable range.

In that case, I do not thing any simple filter is good enough.

And for a lot of cases, we just want to make sure the IO finishes, in
that case simply redirect to seqres.full looks more reasonable other
than call _xfs_io_filter.

But I guess this is to personal tastes.

>
>>>>
>>>
>>> This is a bug-fix patch; it's not a good idea to change the concept of
>>> fstests' golden output. Perhaps an RFC patch about your idea can help
>>> to discuss and achieve consensus.
>>
>> Even as bug-fix, a simple redirect to seqres.full and remove the
>> corresponding line from golden output is very valid to me.
>>
>> In fact, introducing a filter looks very over-engineered in this
>> particular case.
>
> ...but having said that , I also dislike overfixation on golden output.
> Patches welcome. ;)

Sure, one simple patch for evaluation.

Thanks,
Qu
>
> --D
>
>>
>>>
>>> Thanks, Anand
>>>
>>>
>>>> Thanks,
>>>> Qu
>>>>> ---
>>>>>    common/filter.btrfs | 9 +++++++++
>>>>>    tests/btrfs/001     | 3 ++-
>>>>>    tests/btrfs/152     | 6 +++---
>>>>>    tests/btrfs/168     | 6 +++---
>>>>>    tests/btrfs/202     | 4 ++--
>>>>>    tests/btrfs/302     | 4 ++--
>>>>>    6 files changed, 21 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/common/filter.btrfs b/common/filter.btrfs
>>>>> index 9ef9676175c9..415ed6dfd088 100644
>>>>> --- a/common/filter.btrfs
>>>>> +++ b/common/filter.btrfs
>>>>> @@ -156,5 +156,14 @@ _filter_device_add()
>>>>>
>>>>>    }
>>>>>
>>>>> +_filter_snapshot()
>>>>> +{
>>>>> +    # btrfs-progs commit 5f87b467a9e7 ("btrfs-progs: subvolume:
>>>>> output the
>>>>> +    # prompt line only when the ioctl succeeded") changed the output
>>>>> for
>>>>> +    # btrfs subvolume snapshot, ensure that the latest fstests
>>>>> continue to
>>>>> +    # work on older btrfs-progs without the above commit.
>>>>> +    _filter_scratch | sed -e "s/Create a/Create/g"
>>>>> +}
>>>>> +
>>>>>    # make sure this script returns success
>>>>>    /bin/true
>>>>> diff --git a/tests/btrfs/001 b/tests/btrfs/001
>>>>> index 6c2639990373..cfcf2ade4590 100755
>>>>> --- a/tests/btrfs/001
>>>>> +++ b/tests/btrfs/001
>>>>> @@ -26,7 +26,8 @@ dd if=/dev/zero of=$SCRATCH_MNT/foo bs=1M count=1
>>>>> &> /dev/null
>>>>>    echo "List root dir"
>>>>>    ls $SCRATCH_MNT
>>>>>    echo "Creating snapshot of root dir"
>>>>> -$BTRFS_UTIL_PROG subvolume snapshot $SCRATCH_MNT $SCRATCH_MNT/snap |
>>>>> _filter_scratch
>>>>> +$BTRFS_UTIL_PROG subvolume snapshot $SCRATCH_MNT $SCRATCH_MNT/snap | \
>>>>> +                            _filter_snapshot
>>>>>    echo "List root dir after snapshot"
>>>>>    ls $SCRATCH_MNT
>>>>>    echo "List snapshot dir"
>>>>> diff --git a/tests/btrfs/152 b/tests/btrfs/152
>>>>> index 75f576c3cfca..b89fe361e84e 100755
>>>>> --- a/tests/btrfs/152
>>>>> +++ b/tests/btrfs/152
>>>>> @@ -11,7 +11,7 @@
>>>>>    _begin_fstest auto quick metadata qgroup send
>>>>>
>>>>>    # Import common functions.
>>>>> -. ./common/filter
>>>>> +. ./common/filter.btrfs
>>>>>
>>>>>    # real QA test starts here
>>>>>    _supported_fs btrfs
>>>>> @@ -32,9 +32,9 @@ touch $SCRATCH_MNT/subvol{1,2}/foo
>>>>>
>>>>>    # Create base snapshots and send them
>>>>>    $BTRFS_UTIL_PROG subvolume snapshot -r $SCRATCH_MNT/subvol1 \
>>>>> -    $SCRATCH_MNT/subvol1/.snapshots/1 | _filter_scratch
>>>>> +    $SCRATCH_MNT/subvol1/.snapshots/1 | _filter_snapshot
>>>>>    $BTRFS_UTIL_PROG subvolume snapshot -r $SCRATCH_MNT/subvol2 \
>>>>> -    $SCRATCH_MNT/subvol2/.snapshots/1 | _filter_scratch
>>>>> +    $SCRATCH_MNT/subvol2/.snapshots/1 | _filter_snapshot
>>>>>    for recv in recv1_1 recv1_2 recv2_1 recv2_2; do
>>>>>        $BTRFS_UTIL_PROG send $SCRATCH_MNT/subvol1/.snapshots/1 2>
>>>>> /dev/null | \
>>>>>            $BTRFS_UTIL_PROG receive $SCRATCH_MNT/${recv} |
>>>>> _filter_scratch
>>>>> diff --git a/tests/btrfs/168 b/tests/btrfs/168
>>>>> index acc58b51ee39..78bc9b8f81bb 100755
>>>>> --- a/tests/btrfs/168
>>>>> +++ b/tests/btrfs/168
>>>>> @@ -20,7 +20,7 @@ _cleanup()
>>>>>    }
>>>>>
>>>>>    # Import common functions.
>>>>> -. ./common/filter
>>>>> +. ./common/filter.btrfs
>>>>>
>>>>>    # real QA test starts here
>>>>>    _supported_fs btrfs
>>>>> @@ -74,7 +74,7 @@ $BTRFS_UTIL_PROG property set $SCRATCH_MNT/sv1 ro
>>>>> false
>>>>>    # Create a snapshot of the subvolume, to be used later as the
>>>>> parent snapshot
>>>>>    # for an incremental send operation.
>>>>>    $BTRFS_UTIL_PROG subvolume snapshot -r $SCRATCH_MNT/sv1
>>>>> $SCRATCH_MNT/snap1 \
>>>>> -    | _filter_scratch
>>>>> +                            | _filter_snapshot
>>>>>
>>>>>    # First do a full send of this snapshot.
>>>>>    $FSSUM_PROG -A -f -w $send_files_dir/snap1.fssum $SCRATCH_MNT/snap1
>>>>> @@ -88,7 +88,7 @@ $XFS_IO_PROG -c "pwrite -S 0x19 4K 8K"
>>>>> $SCRATCH_MNT/sv1/baz >>$seqres.full
>>>>>    # Create a second snapshot of the subvolume, to be used later as
>>>>> the send
>>>>>    # snapshot of an incremental send operation.
>>>>>    $BTRFS_UTIL_PROG subvolume snapshot -r $SCRATCH_MNT/sv1
>>>>> $SCRATCH_MNT/snap2 \
>>>>> -    | _filter_scratch
>>>>> +                            | _filter_snapshot
>>>>>
>>>>>    # Temporarily turn the second snapshot to read-write mode and then
>>>>> open a file
>>>>>    # descriptor on its foo file.
>>>>> diff --git a/tests/btrfs/202 b/tests/btrfs/202
>>>>> index 5f0429f18bf9..57ecbe47c0bb 100755
>>>>> --- a/tests/btrfs/202
>>>>> +++ b/tests/btrfs/202
>>>>> @@ -8,7 +8,7 @@
>>>>>    . ./common/preamble
>>>>>    _begin_fstest auto quick subvol snapshot
>>>>>
>>>>> -. ./common/filter
>>>>> +. ./common/filter.btrfs
>>>>>
>>>>>    _supported_fs btrfs
>>>>>    _require_scratch
>>>>> @@ -28,7 +28,7 @@ _scratch_mount
>>>>>    $BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/a | _filter_scratch
>>>>>    $BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/a/b | _filter_scratch
>>>>>    $BTRFS_UTIL_PROG subvolume snapshot $SCRATCH_MNT/a $SCRATCH_MNT/c \
>>>>> -    | _filter_scratch
>>>>> +                            | _filter_snapshot
>>>>>
>>>>>    # Need the dummy entry created so that we get the invalid removal
>>>>> when we rmdir
>>>>>    ls $SCRATCH_MNT/c/b
>>>>> diff --git a/tests/btrfs/302 b/tests/btrfs/302
>>>>> index f3e6044b5251..52d712ac50de 100755
>>>>> --- a/tests/btrfs/302
>>>>> +++ b/tests/btrfs/302
>>>>> @@ -15,7 +15,7 @@
>>>>>    . ./common/preamble
>>>>>    _begin_fstest auto quick snapshot subvol
>>>>>
>>>>> -. ./common/filter
>>>>> +. ./common/filter.btrfs
>>>>>
>>>>>    _supported_fs btrfs
>>>>>    _require_scratch
>>>>> @@ -46,7 +46,7 @@ $FSSUM_PROG -A -f -w $fssum_file $SCRATCH_MNT/subvol
>>>>>    # Now create a snapshot of the subvolume and make it accessible
>>>>> from within the
>>>>>    # subvolume.
>>>>>    $BTRFS_UTIL_PROG subvolume snapshot -r $SCRATCH_MNT/subvol \
>>>>> -         $SCRATCH_MNT/subvol/snap | _filter_scratch
>>>>> +                 $SCRATCH_MNT/subvol/snap | _filter_snapshot
>>>>>
>>>>>    # Now unmount and mount again the fs. We want to verify we are able
>>>>> to read all
>>>>>    # metadata for the snapshot from disk (no IO failures, etc).
>>>
>>
>