Re: [PATCH v19 4/4] iotests: 287: add qcow2 compression type test

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

20.04.2020 17:30, Denis Plotnikov wrote:
> 
> 
> On 20.04.2020 15:56, Vladimir Sementsov-Ogievskiy wrote:
>> 20.04.2020 12:17, Denis Plotnikov wrote:
>>> The test checks fulfilling qcow2 requirements for the compression
>>> type feature and zstd compression type operability.
>>>
>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>>> ---
>>>   tests/qemu-iotests/287     | 153 +++++++++++++++++++++++++++++++++++++
>>>   tests/qemu-iotests/287.out |  67 ++++++++++++++++
>>>   tests/qemu-iotests/group   |   1 +
>>>   3 files changed, 221 insertions(+)
>>>   create mode 100755 tests/qemu-iotests/287
>>>   create mode 100644 tests/qemu-iotests/287.out
>>>
>>> diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
>>> new file mode 100755
>>> index 0000000000..44988bc14b
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/287
>>> @@ -0,0 +1,153 @@
>>> +#!/usr/bin/env bash
>>> +#
>>> +# Test case for an image using zstd compression
>>> +#
>>> +# Copyright (c) 2020 Virtuozzo International GmbH
>>> +#
>>> +# This program is free software; you can redistribute it and/or modify
>>> +# it under the terms of the GNU General Public License as published by
>>> +# the Free Software Foundation; either version 2 of the License, or
>>> +# (at your option) any later version.
>>> +#
>>> +# This program is distributed in the hope that it will be useful,
>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +# GNU General Public License for more details.
>>> +#
>>> +# You should have received a copy of the GNU General Public License
>>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +#
>>> +
>>> +# creator
>>> +owner=dplotnikov@virtuozzo.com
>>> +
>>> +seq="$(basename $0)"
>>> +echo "QA output created by $seq"
>>> +
>>> +status=1    # failure is the default!
>>> +
>>> +# standard environment
>>> +. ./common.rc
>>> +. ./common.filter
>>> +
>>> +# This tests qocw2-specific low-level functionality
>>> +_supported_fmt qcow2
>>> +_supported_proto file
>>> +_supported_os Linux
>>> +
>>> +# Sanity check: our use of $RAND_FILE fails if $TEST_DIR contains spaces
>>> +# or other problems
>>
>> Hmm, but I see, you now don't use writing from file, but instead, rewrite rand_file by 0xFA pattern. So, we don't have now any restrictions on RAND_FILE now, and than, we should drop this check.
>>
>>> +case "$TEST_DIR" in
>>> +    *[^-_a-zA-Z0-9/]*)
>>> +        _notrun "Suspicious TEST_DIR='$TEST_DIR', cowardly refusing to run" ;;
>>> +esac
>>> +
>>> +COMPR_IMG="$TEST_IMG.compressed"
>>> +RAND_FILE="$TEST_DIR/rand_data"
>>> +
>>> +_cleanup()
>>> +{
>>> +    _cleanup_test_img
>>> +    rm -f "$COMPR_IMG"
>>> +    rm -f "$RAND_FILE"
>>> +}
>>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>>> +
>>> +# for all the cases
>>> +CLUSTER_SIZE=65536
>>> +
>>> +# Check if we can run this test.
>>> +if IMGOPTS='compression_type=zstd' _make_test_img 64M |
>>> +    grep "Invalid parameter 'zstd'"; then
>>> +    _notrun "ZSTD is disabled"
>>> +fi
>>> +
>>> +echo
>>> +echo "=== Testing compression type incompatible bit setting for zstd ==="
>>> +echo
>>> +# use the zstd image created on the test runnable check above.
>>
>> Hmm, but you don't? Should you also drop the following line?
>>
>>> +IMGOPTS='compression_type=zstd' _make_test_img 64M
>>> +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
>>> +
>>> +echo
>>> +echo "=== Testing compression type incompatible bit setting for zlib ==="
>>> +echo
>>> +IMGOPTS='compression_type=zlib' _make_test_img 64M
>>> +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
>>> +
>>> +echo
>>> +echo "=== Testing zlib with incompatible bit set ==="
>>> +echo
>>> +IMGOPTS='compression_type=zlib' _make_test_img 64M
>>> +$PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 3
>>> +# to make sure the bit was actually set
>>> +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
>>> +
>>> +if $QEMU_IMG info "$TEST_IMG" >/dev/null 2>&1 ; then
>>> +    echo "Error: The image opened successfully. The image must not be opened."
>>> +fi
>>> +
>>> +echo
>>> +echo "=== Testing zstd with incompatible bit unset ==="
>>> +echo
>>> +IMGOPTS='compression_type=zstd' _make_test_img 64M
>>> +$PYTHON qcow2.py "$TEST_IMG" set-header incompatible_features 0
>>> +# to make sure the bit was actually unset
>>> +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
>>> +
>>> +if $QEMU_IMG info "$TEST_IMG" >/dev/null 2>&1 ; then
>>> +    echo "Error: The image opened successfully. The image must not be opened."
>>> +fi
>>> +
>>> +echo
>>> +echo "=== Testing compression type values ==="
>>> +echo
>>> +# zlib=0
>>> +IMGOPTS='compression_type=zlib' _make_test_img 64M
>>> +peek_file_be "$TEST_IMG" 104 1
>>> +echo
>>> +
>>> +# zstd=1
>>> +IMGOPTS='compression_type=zstd' _make_test_img 64M
>>> +peek_file_be "$TEST_IMG" 104 1
>>> +echo
>>> +
>>> +echo
>>> +echo "=== Testing simple reading and writing with zstd ==="
>>> +echo
>>> +IMGOPTS='compression_type=zstd' _make_test_img 64M
>>> +$QEMU_IO -c "write -c -P 0xAC 64K 64K " "$TEST_IMG" | _filter_qemu_io
>>> +$QEMU_IO -c "read -P 0xAC 64K 64K " "$TEST_IMG" | _filter_qemu_io
>>> +# read on the cluster boundaries
>>> +$QEMU_IO -c "read -v 131070 8 " "$TEST_IMG" | _filter_qemu_io
>>> +$QEMU_IO -c "read -v 65534 8" "$TEST_IMG" | _filter_qemu_io
>>> +
>>> +echo
>>> +echo "=== Testing adjacent clusters reading and writing with zstd ==="
>>> +echo
>>> +IMGOPTS='compression_type=zstd' _make_test_img 64M
>>> +$QEMU_IO -c "write -c -P 0xAB 0 64K " "$TEST_IMG" | _filter_qemu_io
>>> +$QEMU_IO -c "write -c -P 0xAC 64K 64K " "$TEST_IMG" | _filter_qemu_io
>>> +$QEMU_IO -c "write -c -P 0xAD 128K 64K " "$TEST_IMG" | _filter_qemu_io
>>> +
>>> +$QEMU_IO -c "read -P 0xAB 0 64k " "$TEST_IMG" | _filter_qemu_io
>>> +$QEMU_IO -c "read -P 0xAC 64K 64k " "$TEST_IMG" | _filter_qemu_io
>>> +$QEMU_IO -c "read -P 0xAD 128K 64k " "$TEST_IMG" | _filter_qemu_io
>>> +
>>> +echo
>>> +echo "=== Testing incompressible cluster processing with zstd ==="
>>> +echo
>>> +# create a 2M imgae and fill it with 1M likely incompressible data
>>
>> image
>>
>>> +# and 1M compressible data
>>> +dd if=/dev/urandom of="$RAND_FILE" bs=2M count=1
>>> +# rewrite the last 1M with compressible data
>>> +$QEMU_IO -f raw -c "write -P 0xFA 1M 1M" "$RAND_FILE" | _filter_qemu_io
>>
>> So, we avoid problems with whitespaces  in RAND_FILE.
>>
>> Still, you may improve it a bit, by not generating extra 1M of random data, for example,
>> create 2M raw image, then write to it b dd only 1M, then the other megabyte by pattern.
> Would it be any significant improvement? This example 2 lines of code. Your example is 3 lines (create-dd-write).
> You example demand more output in test.out file also because of 3 command instead of 2.
> We won't win anything significant in performance by reducing 2M rand data to 1M.
> So, I don't really know

No objections, I'm OK with it as is.

If you want, you may instead write random data to the second megabyte, and pattern to the first, keeping two lines of code:

dd if=/dev/urandom of="$RAND_FILE" bs=1M count=1 seek=1
QEMU_IO_OPTIONS="$QEMU_IO_OPTIONS_NO_FMT" $QEMU_IO -f raw -c "write -P 0xFA 0 1M" "$RAND_FILE" | _filter_qemu_io

but, it's nitpicking anyway.

>>
>> Also, I believe, that it works as is, but passing two -f options to qemu-io is not good
>> thing, I think, actually, we'd better forbid it in qemu-io option parsing (Haha, I'm
>> sure, a lot of iotests will need a fix:).. So, more correct is something like
>>
>> QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT $QEMU_IO -f raw [...]
>>
>>
>>
> 


-- 
Best regards,
Vladimir