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.j...@oracle.com>
>>>
>>> Reviewed-by: Nikolay Borisov <nbori...@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
>>>>
>
>
