Re: [PATCH v2] fstests: btrfs/048: amend property validation cases

[Nikolay Borisov <nborisov@suse.com>]

On 7.04.19 г. 14:54 ч., Anand Jain wrote:
> On 6/4/19 8:02 pm, Eryu Guan wrote:
>> On Fri, Apr 05, 2019 at 04:21:10PM +0300, Nikolay Borisov wrote:
>>>
>>>
>>> On 3.04.19 г. 20:04 ч., Anand Jain wrote:
>>>> Add more property validation cases which are fixed by the patches [1]
>>>>   [1]
>>>>    btrfs: fix vanished compression property after failed set
>>>>    btrfs: fix zstd compression parameter
>>>>
>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>
>>> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>>
>> Thanks for the test and the review!
>>
>> But this looks like a targeted regression test that may fail an existing
>> test. It's better to write a new test for this.
> 
> Regression is only when fstests is upgraded. This
> test case mentions the prerequisite kernel patches [1].
> So that should suffice the concern?

I agree with Anand here, this is an extension to an existing test, which
covers specific feature. IMO it's not good to always introduce new tests
because every invocation of a test comes with an overhead of spawning
processes and whatnot. THis is not a problem for 10 tests, but currently
for btrfs we execute around 600 tests each one "wasting" some cycles to
spawn bash processes to execute the actual test.

> 
> Thanks, Anand
> 
>>
>> Thanks,
>> Eryu
>>
>>>
>>>> ---
>>>> v2: correct kernel patch titles in the test header and change log
>>>>
>>>>   tests/btrfs/048     | 23 +++++++++++++++++++++++
>>>>   tests/btrfs/048.out | 16 ++++++++++++++++
>>>>   2 files changed, 39 insertions(+)
>>>>
>>>> diff --git a/tests/btrfs/048 b/tests/btrfs/048
>>>> index 588219579cc6..f6de0b8ca8b1 100755
>>>> --- a/tests/btrfs/048
>>>> +++ b/tests/btrfs/048
>>>> @@ -6,6 +6,9 @@
>>>>   #
>>>>   # Btrfs properties test. The btrfs properties feature was
>>>> introduced in the
>>>>   # linux kernel 3.14.
> 
> 
> [1]
> 
>>>> +# Fails without the kernel patches:
>>>> +#  btrfs: fix vanished compression property after failed set
>>>> +#  btrfs: fix zstd compression parameter
>>>>   #
>>>>   seq=`basename $0`
>>>>   seqres=$RESULT_DIR/$seq
>>>> @@ -34,6 +37,7 @@ _supported_os Linux
>>>>   _require_test
>>>>   _require_scratch
>>>>   _require_btrfs_command "property"
>>>> +_require_btrfs_command inspect-internal dump-super
>>>>     send_files_dir=$TEST_DIR/btrfs-test-$seq
>>>>   @@ -203,5 +207,24 @@ $BTRFS_UTIL_PROG property get
>>>> $SCRATCH_MNT/sv1 compression
>>>>   touch $SCRATCH_MNT/sv1/file2
>>>>   $BTRFS_UTIL_PROG property get $SCRATCH_MNT/sv1/file2 compression
>>>>   +echo -e "\nTesting argument validation, should fail"
>>>> +$BTRFS_UTIL_PROG property set $SCRATCH_MNT compression 'lz' |
>>>> _filter_scratch
>>>> +echo "***"
>>>> +$BTRFS_UTIL_PROG property set $SCRATCH_MNT compression 'zli' |
>>>> _filter_scratch
>>>> +echo "***"
>>>> +$BTRFS_UTIL_PROG property set $SCRATCH_MNT compression 'zst' |
>>>> _filter_scratch
>>>> +
>>>> +echo -e "\nTesting if property is persistent across failed validation"
>>>> +$BTRFS_UTIL_PROG property set $SCRATCH_MNT compression 'lzo'
>>>> +$BTRFS_UTIL_PROG property set $SCRATCH_MNT compression 'zli' |
>>>> _filter_scratch
>>>> +$BTRFS_UTIL_PROG property get $SCRATCH_MNT compression
>>>> +
>>>> +echo -e "\nTesting generation is unchanged after failed validation"
>>>> +$BTRFS_UTIL_PROG filesystem sync $SCRATCH_MNT
>>>> +$BTRFS_UTIL_PROG inspect-internal dump-super $SCRATCH_DEV | grep
>>>> '^generation'
>>>> +$BTRFS_UTIL_PROG property set $SCRATCH_MNT compression 'lz' |
>>>> _filter_scratch
>>>> +$BTRFS_UTIL_PROG filesystem sync $SCRATCH_MNT
>>>> +$BTRFS_UTIL_PROG inspect-internal dump-super $SCRATCH_DEV | grep
>>>> '^generation'
>>>> +
>>>>   status=0
>>>>   exit
>>>> diff --git a/tests/btrfs/048.out b/tests/btrfs/048.out
>>>> index 3e4e3d28950a..00f39bc01227 100644
>>>> --- a/tests/btrfs/048.out
>>>> +++ b/tests/btrfs/048.out
>>>> @@ -76,3 +76,19 @@ compression=zlib
>>>>   Testing subvolume property inheritance
>>>>   compression=lzo
>>>>   compression=lzo
>>>> +
>>>> +Testing argument validation, should fail
>>>> +ERROR: failed to set compression for /mnt/scratch: Invalid argument
>>>> +***
>>>> +ERROR: failed to set compression for /mnt/scratch: Invalid argument
>>>> +***
>>>> +ERROR: failed to set compression for /mnt/scratch: Invalid argument
>>>> +
>>>> +Testing if property is persistent across failed validation
>>>> +ERROR: failed to set compression for /mnt/scratch: Invalid argument
>>>> +compression=lzo
>>>> +
>>>> +Testing generation is unchanged after failed validation
>>>> +generation        7
>>>> +ERROR: failed to set compression for /mnt/scratch: Invalid argument
>>>> +generation        7
>>>>
> 
>