[v23] implement zstd cluster compression method

[PATCH v23 0/4] implement zstd cluster compression method

Posted by Denis Plotnikov 5 years, 6 months ago

v23:
   Undecided: whether to add zstd(zlib) compression
              details to the qcow2 spec
   03: tighten assertion on zstd decompression [Eric]
   04: use _rm_test_img appropriately [Max]

v22:
   03: remove assignemnt in if condition

v21:
   03:
       * remove the loop on compression [Max]
       * use designated initializers [Max]
   04:
       * don't erase user's options [Max]
       * use _rm_test_img [Max]
       * add unsupported qcow2 options [Max]

v20:
   04: fix a number of flaws [Vladimir]
       * don't use $RAND_FILE passing to qemu-io,
         so check $TEST_DIR is redundant
       * re-arrage $RAND_FILE writing
       * fix a typo

v19:
   04: fix a number of flaws [Eric]
       * remove rudundant test case descriptions
       * fix stdout redirect
       * don't use (())
       * use peek_file_be instead of od
       * check $TEST_DIR for spaces and other before using
       * use $RAND_FILE safer

v18:
   * 04: add quotes to all file name variables [Vladimir] 
   * 04: add Vladimir's comment according to "qemu-io write -s"
         option issue.

v17:
   * 03: remove incorrect comment in zstd decompress [Vladimir]
   * 03: remove "paraniod" and rewrite the comment on decompress [Vladimir]
   * 03: fix dead assignment [Vladimir]
   * 04: add and remove quotes [Vladimir]
   * 04: replace long offset form with the short one [Vladimir]

v16:
   * 03: ssize_t for ret, size_t for zstd_ret [Vladimir]
   * 04: small fixes according to the comments [Vladimir] 

v15:
   * 01: aiming qemu 5.1 [Eric]
   * 03: change zstd_res definition place [Vladimir]
   * 04: add two new test cases [Eric]
         1. test adjacent cluster compression with zstd
         2. test incompressible cluster processing
   * 03, 04: many rewording and gramma fixing [Eric]

v14:
   * fix bug on compression - looping until compress == 0 [Me]
   * apply reworked Vladimir's suggestions:
      1. not mixing ssize_t with size_t
      2. safe check for ENOMEM in compression part - avoid overflow
      3. tolerate sanity check allow zstd to make progress only
         on one of the buffers
v13:
   * 03: add progress sanity check to decompression loop [Vladimir]
     03: add successful decompression check [Me]

v12:
   * 03: again, rework compression and decompression loops
         to make them more correct [Vladimir]
     03: move assert in compression to more appropriate place
             [Vladimir]
v11:
   * 03: the loops don't need "do{}while" form anymore and
         the they were buggy (missed "do" in the beginning)
         replace them with usual "while(){}" loops [Vladimir]
v10:
   * 03: fix zstd (de)compressed loops for multi-frame
         cases [Vladimir]
v9:
   * 01: fix error checking and reporting in qcow2_amend compression type part [Vladimir]
   * 03: replace asserts with -EIO in qcow2_zstd_decompression [Vladimir, Alberto]
   * 03: reword/amend/add comments, fix typos [Vladimir]

v8:
   * 03: switch zstd API from simple to stream [Eric]
         No need to state a special cluster layout for zstd
         compressed clusters.
v7:
   * use qapi_enum_parse instead of the open-coding [Eric]
   * fix wording, typos and spelling [Eric]

v6:
   * "block/qcow2-threads: fix qcow2_decompress" is removed from the series
      since it has been accepted by Max already
   * add compile time checking for Qcow2Header to be a multiple of 8 [Max, Alberto]
   * report error on qcow2 amending when the compression type is actually chnged [Max]
   * remove the extra space and the extra new line [Max]
   * re-arrange acks and signed-off-s [Vladimir]

v5:
   * replace -ENOTSUP with abort in qcow2_co_decompress [Vladimir]
   * set cluster size for all test cases in the beginning of the 287 test

v4:
   * the series is rebased on top of 01 "block/qcow2-threads: fix qcow2_decompress"
   * 01 is just a no-change resend to avoid extra dependencies. Still, it may be merged in separate

v3:
   * remove redundant max compression type value check [Vladimir, Eric]
     (the switch below checks everything)
   * prevent compression type changing on "qemu-img amend" [Vladimir]
   * remove zstd config setting, since it has been added already by
     "migration" patches [Vladimir]
   * change the compression type error message [Vladimir] 
   * fix alignment and 80-chars exceeding [Vladimir]

v2:
   * rework compression type setting [Vladimir]
   * squash iotest changes to the compression type introduction patch [Vladimir, Eric]
   * fix zstd availability checking in zstd iotest [Vladimir]
   * remove unnecessry casting [Eric]
   * remove rudundant checks [Eric]
   * fix compressed cluster layout in qcow2 spec [Vladimir]
   * fix wording [Eric, Vladimir]
   * fix compression type filtering in iotests [Eric]

v1:
   the initial series

Denis Plotnikov (4):
  qcow2: introduce compression type feature
  qcow2: rework the cluster compression routine
  qcow2: add zstd cluster compression
  iotests: 287: add qcow2 compression type test

 docs/interop/qcow2.txt           |   1 +
 configure                        |   2 +-
 qapi/block-core.json             |  23 ++-
 block/qcow2.h                    |  20 ++-
 include/block/block_int.h        |   1 +
 block/qcow2-threads.c            | 240 +++++++++++++++++++++++++++++--
 block/qcow2.c                    | 120 ++++++++++++++++
 tests/qemu-iotests/031.out       |  14 +-
 tests/qemu-iotests/036.out       |   4 +-
 tests/qemu-iotests/049.out       | 102 ++++++-------
 tests/qemu-iotests/060.out       |   1 +
 tests/qemu-iotests/061.out       |  34 +++--
 tests/qemu-iotests/065           |  28 ++--
 tests/qemu-iotests/080           |   2 +-
 tests/qemu-iotests/144.out       |   4 +-
 tests/qemu-iotests/182.out       |   2 +-
 tests/qemu-iotests/242.out       |   5 +
 tests/qemu-iotests/255.out       |   8 +-
 tests/qemu-iotests/287           | 152 ++++++++++++++++++++
 tests/qemu-iotests/287.out       |  67 +++++++++
 tests/qemu-iotests/common.filter |   3 +-
 tests/qemu-iotests/group         |   1 +
 22 files changed, 726 insertions(+), 108 deletions(-)
 create mode 100755 tests/qemu-iotests/287
 create mode 100644 tests/qemu-iotests/287.out

-- 
2.17.0

Re: [PATCH v23 0/4] implement zstd cluster compression method

Posted by Max Reitz 5 years, 6 months ago

On 30.04.20 12:19, Denis Plotnikov wrote:
> v23:
>    Undecided: whether to add zstd(zlib) compression
>               details to the qcow2 spec
>    03: tighten assertion on zstd decompression [Eric]
>    04: use _rm_test_img appropriately [Max]

Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max

Re: [PATCH v23 0/4] implement zstd cluster compression method

Posted by Max Reitz 5 years, 6 months ago

On 05.05.20 12:26, Max Reitz wrote:
> On 30.04.20 12:19, Denis Plotnikov wrote:
>> v23:
>>    Undecided: whether to add zstd(zlib) compression
>>               details to the qcow2 spec
>>    03: tighten assertion on zstd decompression [Eric]
>>    04: use _rm_test_img appropriately [Max]
> 
> Thanks, applied to my block branch:

I’m afraid I have to unqueue this series again, because it makes many
iotests fail due to an additional “compression_type=zlib” output when
images are created, an additional “compression type” line in
qemu-img info output where format-specific information is not
suppressed, and an additional line in qemu-img create -f qcow2 -o help.

Max

Re: [PATCH v23 0/4] implement zstd cluster compression method

Posted by Denis Plotnikov 5 years, 6 months ago


On 05.05.2020 15:03, Max Reitz wrote:
> On 05.05.20 12:26, Max Reitz wrote:
>> On 30.04.20 12:19, Denis Plotnikov wrote:
>>> v23:
>>>     Undecided: whether to add zstd(zlib) compression
>>>                details to the qcow2 spec
>>>     03: tighten assertion on zstd decompression [Eric]
>>>     04: use _rm_test_img appropriately [Max]
>> Thanks, applied to my block branch:
> I’m afraid I have to unqueue this series again, because it makes many
> iotests fail due to an additional “compression_type=zlib” output when
> images are created, an additional “compression type” line in
> qemu-img info output where format-specific information is not
> suppressed, and an additional line in qemu-img create -f qcow2 -o help.
>
> Max
>

Hmm, this is strange. I made some modifications for the tests
in 0001 of the series (qcow2: introduce compression type feature).

Among the other test related changes, the patch contains the hunk:

+++ b/tests/qemu-iotests/common.filter
@@ -152,7 +152,8 @@ _filter_img_create()
          -e "s# refcount_bits=[0-9]\\+##g" \
          -e "s# key-secret=[a-zA-Z0-9]\\+##g" \
          -e "s# iter-time=[0-9]\\+##g" \
-        -e "s# force_size=\\(on\\|off\\)##g"
+        -e "s# force_size=\\(on\\|off\\)##g" \
+        -e "s# compression_type=[a-zA-Z0-9]\\+##g"
  }

which has to filter out "compression_type" on image creation.

But you say that you can see the "compression_type" on the image creation.
May be the patch wasn't fully applied? Or the test related modification 
were omitted?

I've just re-based the series on top of:

681b07f4e76dbb700c16918d (vanilla/master, mainstream)
Merge: a2261b2754 714eb0dbc5
Author: Peter Maydell <peter.maydell@linaro.org>
Date:   Tue May 5 15:47:44 2020 +0100

and run the tests with 'make check-block'

and got the following:

Not run: 071 099 184 220 259 267
Some cases not run in: 030 040 041
Passed all 113 iotests

May be I do something wrong?

Denis

Re: [PATCH v23 0/4] implement zstd cluster compression method

Posted by Vladimir Sementsov-Ogievskiy 5 years, 6 months ago

06.05.2020 11:01, Denis Plotnikov wrote:
> 
> 
> On 05.05.2020 15:03, Max Reitz wrote:
>> On 05.05.20 12:26, Max Reitz wrote:
>>> On 30.04.20 12:19, Denis Plotnikov wrote:
>>>> v23:
>>>>     Undecided: whether to add zstd(zlib) compression
>>>>                details to the qcow2 spec
>>>>     03: tighten assertion on zstd decompression [Eric]
>>>>     04: use _rm_test_img appropriately [Max]
>>> Thanks, applied to my block branch:
>> I’m afraid I have to unqueue this series again, because it makes many
>> iotests fail due to an additional “compression_type=zlib” output when
>> images are created, an additional “compression type” line in
>> qemu-img info output where format-specific information is not
>> suppressed, and an additional line in qemu-img create -f qcow2 -o help.
>>
>> Max
>>
> 
> Hmm, this is strange. I made some modifications for the tests
> in 0001 of the series (qcow2: introduce compression type feature).
> 
> Among the other test related changes, the patch contains the hunk:
> 
> +++ b/tests/qemu-iotests/common.filter
> @@ -152,7 +152,8 @@ _filter_img_create()
>           -e "s# refcount_bits=[0-9]\\+##g" \
>           -e "s# key-secret=[a-zA-Z0-9]\\+##g" \
>           -e "s# iter-time=[0-9]\\+##g" \
> -        -e "s# force_size=\\(on\\|off\\)##g"
> +        -e "s# force_size=\\(on\\|off\\)##g" \
> +        -e "s# compression_type=[a-zA-Z0-9]\\+##g"
>   }
> 
> which has to filter out "compression_type" on image creation.
> 
> But you say that you can see the "compression_type" on the image creation.
> May be the patch wasn't fully applied? Or the test related modification were omitted?
> 
> I've just re-based the series on top of:
> 
> 681b07f4e76dbb700c16918d (vanilla/master, mainstream)
> Merge: a2261b2754 714eb0dbc5
> Author: Peter Maydell <peter.maydell@linaro.org>
> Date:   Tue May 5 15:47:44 2020 +0100
> 
> and run the tests with 'make check-block'
> 
> and got the following:
> 
> Not run: 071 099 184 220 259 267
> Some cases not run in: 030 040 041
> Passed all 113 iotests
> 

Hmm, we definitely have a lot more iotests. So, I assume make check-block doesn't run all of them.

To run all:

cd tests/qemu-iotests
./check -qcow2
./check -raw
... and so for any format you want to test

I also recommend to do
   export TEST_DIR=/something
before running tests, where something is tmpfs or ssd, which will decrease testing time a lot. Still, some (very small subset) of tests doesn't run on tmpfs, you can rerun them with TEST_DIR=/normal/hdd/directory

-- 
Best regards,
Vladimir