[Qemu-devel] [PATCH] [RFC] qcow2: add compression type feature

Denis Plotnikov posted 1 patch 5 years, 1 month ago
Test docker-mingw@fedora passed
Test asan passed
Test checkpatch passed
Test docker-clang@ubuntu passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190205090825.14059-1-dplotnikov@virtuozzo.com
Maintainers: Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>
There is a newer version of this series
block/qcow2.c | 25 +++++++++++++++++++++++++
block/qcow2.h | 26 ++++++++++++++++++++++----
2 files changed, 47 insertions(+), 4 deletions(-)
[Qemu-devel] [PATCH] [RFC] qcow2: add compression type feature
Posted by Denis Plotnikov 5 years, 1 month ago
The patch adds some preparation parts for incompatible compression type
feature into QCOW2 header that indicates that *all* compressed clusters
must be (de)compressed using a certain compression type.

It is implied that the compression type is set on the image creation and
can be changed only later by image convertion, thus the only compression
algorithm is used for the image.

The plan is to add support for ZSTD and then may be something more effective
in the future.

ZSDT compression algorithm consumes 3-5 times less CPU power with a
comparable comression ratio with zlib. It would be wise to use it for
data compression f.e. for backups.

The default compression is ZLIB.

Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
 block/qcow2.c | 25 +++++++++++++++++++++++++
 block/qcow2.h | 26 ++++++++++++++++++++++----
 2 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 8c91b92865..cb3d6cc1c0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -73,6 +73,7 @@ typedef struct {
 #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
 #define  QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
 #define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
+#define  QCOW2_EXT_MAGIC_COMPRESSION_TYPE 0x434D5052
 
 static int coroutine_fn
 qcow2_co_preadv_compressed(BlockDriverState *bs,
@@ -397,6 +398,9 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
 #endif
             break;
 
+        case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
+            /* Setting compression type to BDRVQcow2State->compression_type */
+            /* from the image header is going to be here */
         default:
             /* unknown magic - save it in case we need to rewrite the header */
             /* If you add a new feature, make sure to also update the fast
@@ -2431,6 +2435,11 @@ int qcow2_update_header(BlockDriverState *bs)
                 .bit  = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
                 .name = "lazy refcounts",
             },
+            {
+                .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
+                .bit  = QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR,
+                .name = "compression type",
+            },
         };
 
         ret = header_ext_add(buf, QCOW2_EXT_MAGIC_FEATURE_TABLE,
@@ -2461,6 +2470,22 @@ int qcow2_update_header(BlockDriverState *bs)
         buflen -= ret;
     }
 
+    /* Compression type extension */
+    if (s->compression_type != 0) {
+        Qcow2CompressionTypeExt comp_header = {
+            .compression_type = cpu_to_be32(s->compression_type),
+        };
+        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_COMPRESSION_TYPE,
+                             &comp_header,
+                             cpu_to_be64(sizeof(comp_header)),
+                             buflen);
+        if (ret < 0) {
+            goto fail;
+        }
+        buf += ret;
+        buflen -= ret;
+    }
+
     /* Keep unknown header extensions */
     QLIST_FOREACH(uext, &s->unknown_header_ext, next) {
         ret = header_ext_add(buf, uext->magic, uext->data, uext->len, buflen);
diff --git a/block/qcow2.h b/block/qcow2.h
index 32cce9eee2..fdde5bbefd 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -112,6 +112,10 @@
 #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
 #define QCOW2_OPT_CACHE_CLEAN_INTERVAL "cache-clean-interval"
 
+/* Compression types */
+#define QCOW2_COMPRESSION_TYPE_ZLIB    0
+#define QCOW2_COMPRESSION_TYPE_ZSTD    1
+
 typedef struct QCowHeader {
     uint32_t magic;
     uint32_t version;
@@ -197,10 +201,13 @@ enum {
 
 /* Incompatible feature bits */
 enum {
-    QCOW2_INCOMPAT_DIRTY_BITNR   = 0,
-    QCOW2_INCOMPAT_CORRUPT_BITNR = 1,
-    QCOW2_INCOMPAT_DIRTY         = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
-    QCOW2_INCOMPAT_CORRUPT       = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
+    QCOW2_INCOMPAT_DIRTY_BITNR            = 0,
+    QCOW2_INCOMPAT_CORRUPT_BITNR          = 1,
+    QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR = 2,
+    QCOW2_INCOMPAT_DIRTY                  = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
+    QCOW2_INCOMPAT_CORRUPT                = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
+    QCOW2_INCOMPAT_COMPRESSION_TYPE       =
+                                    1 << QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR,
 
     QCOW2_INCOMPAT_MASK          = QCOW2_INCOMPAT_DIRTY
                                  | QCOW2_INCOMPAT_CORRUPT,
@@ -256,6 +263,10 @@ typedef struct Qcow2BitmapHeaderExt {
     uint64_t bitmap_directory_offset;
 } QEMU_PACKED Qcow2BitmapHeaderExt;
 
+typedef struct Qcow2CompressionTypeExt {
+    uint32_t compression_type;
+} QEMU_PACKED Qcow2CompressionTypeExt;
+
 typedef struct BDRVQcow2State {
     int cluster_bits;
     int cluster_size;
@@ -340,6 +351,13 @@ typedef struct BDRVQcow2State {
 
     CoQueue compress_wait_queue;
     int nb_compress_threads;
+    /**
+     * Compression type used for the image. Default: 0 - ZLIB
+     * The image compression type is set on image creation.
+     * The only way to change the compression type is to convert the image
+     * with the desired compresion type set
+     */
+    uint32_t compression_type;
 } BDRVQcow2State;
 
 typedef struct Qcow2COWRegion {
-- 
2.17.0


Re: [Qemu-devel] [PATCH] [RFC] qcow2: add compression type feature
Posted by Max Reitz 4 years, 11 months ago
On 05.02.19 10:08, Denis Plotnikov wrote:
> The patch adds some preparation parts for incompatible compression type
> feature into QCOW2 header that indicates that *all* compressed clusters
> must be (de)compressed using a certain compression type.
> 
> It is implied that the compression type is set on the image creation and
> can be changed only later by image convertion, thus the only compression
> algorithm is used for the image.
> 
> The plan is to add support for ZSTD and then may be something more effective
> in the future.
> 
> ZSDT compression algorithm consumes 3-5 times less CPU power with a
> comparable comression ratio with zlib. It would be wise to use it for
> data compression f.e. for backups.
> 
> The default compression is ZLIB.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---
>  block/qcow2.c | 25 +++++++++++++++++++++++++
>  block/qcow2.h | 26 ++++++++++++++++++++++----
>  2 files changed, 47 insertions(+), 4 deletions(-)

Are there plans to pursue this further?

[...]

> diff --git a/block/qcow2.h b/block/qcow2.h
> index 32cce9eee2..fdde5bbefd 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -112,6 +112,10 @@
>  #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
>  #define QCOW2_OPT_CACHE_CLEAN_INTERVAL "cache-clean-interval"
>  
> +/* Compression types */
> +#define QCOW2_COMPRESSION_TYPE_ZLIB    0
> +#define QCOW2_COMPRESSION_TYPE_ZSTD    1

We probably want QAPI types anyway (qemu-img info should report the
compression type), so I think we could use them instead.

>  typedef struct QCowHeader {
>      uint32_t magic;
>      uint32_t version;
> @@ -197,10 +201,13 @@ enum {
>  
>  /* Incompatible feature bits */
>  enum {
> -    QCOW2_INCOMPAT_DIRTY_BITNR   = 0,
> -    QCOW2_INCOMPAT_CORRUPT_BITNR = 1,
> -    QCOW2_INCOMPAT_DIRTY         = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
> -    QCOW2_INCOMPAT_CORRUPT       = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
> +    QCOW2_INCOMPAT_DIRTY_BITNR            = 0,
> +    QCOW2_INCOMPAT_CORRUPT_BITNR          = 1,
> +    QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR = 2,
> +    QCOW2_INCOMPAT_DIRTY                  = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
> +    QCOW2_INCOMPAT_CORRUPT                = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
> +    QCOW2_INCOMPAT_COMPRESSION_TYPE       =
> +                                    1 << QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR,
>  
>      QCOW2_INCOMPAT_MASK          = QCOW2_INCOMPAT_DIRTY
>                                   | QCOW2_INCOMPAT_CORRUPT,

This mask needs to be expanded by QCOW2_INCOMPAT_COMPRESSION_TYPE.

> @@ -256,6 +263,10 @@ typedef struct Qcow2BitmapHeaderExt {
>      uint64_t bitmap_directory_offset;
>  } QEMU_PACKED Qcow2BitmapHeaderExt;
>  
> +typedef struct Qcow2CompressionTypeExt {
> +    uint32_t compression_type;
> +} QEMU_PACKED Qcow2CompressionTypeExt;
> +
>  typedef struct BDRVQcow2State {
>      int cluster_bits;
>      int cluster_size;
> @@ -340,6 +351,13 @@ typedef struct BDRVQcow2State {
>  
>      CoQueue compress_wait_queue;
>      int nb_compress_threads;
> +    /**
> +     * Compression type used for the image. Default: 0 - ZLIB
> +     * The image compression type is set on image creation.
> +     * The only way to change the compression type is to convert the image
> +     * with the desired compresion type set

*compression

And, well, ideally qemu-img amend could perform this operation, too.

Max

> +     */
> +    uint32_t compression_type;
>  } BDRVQcow2State;
>  
>  typedef struct Qcow2COWRegion {
> 


Re: [Qemu-devel] [PATCH] [RFC] qcow2: add compression type feature
Posted by Denis Plotnikov 4 years, 10 months ago

On 29.04.2019 1:32, Max Reitz wrote:
> On 05.02.19 10:08, Denis Plotnikov wrote:
>> The patch adds some preparation parts for incompatible compression type
>> feature into QCOW2 header that indicates that *all* compressed clusters
>> must be (de)compressed using a certain compression type.
>>
>> It is implied that the compression type is set on the image creation and
>> can be changed only later by image convertion, thus the only compression
>> algorithm is used for the image.
>>
>> The plan is to add support for ZSTD and then may be something more effective
>> in the future.
>>
>> ZSDT compression algorithm consumes 3-5 times less CPU power with a
>> comparable comression ratio with zlib. It would be wise to use it for
>> data compression f.e. for backups.
>>
>> The default compression is ZLIB.
>>
>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> ---
>>   block/qcow2.c | 25 +++++++++++++++++++++++++
>>   block/qcow2.h | 26 ++++++++++++++++++++++----
>>   2 files changed, 47 insertions(+), 4 deletions(-)
> 
> Are there plans to pursue this further?
Yes, I'm going to come up with the first version in a few weeks
> 
> [...]
> 
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 32cce9eee2..fdde5bbefd 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -112,6 +112,10 @@
>>   #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
>>   #define QCOW2_OPT_CACHE_CLEAN_INTERVAL "cache-clean-interval"
>>   
>> +/* Compression types */
>> +#define QCOW2_COMPRESSION_TYPE_ZLIB    0
>> +#define QCOW2_COMPRESSION_TYPE_ZSTD    1
> 
> We probably want QAPI types anyway (qemu-img info should report the
> compression type), so I think we could use them instead.
I'm not sure that I understood the idea. Could you please explain what 
is meant here? We don't need those constants and should use the 
constants defined somewhere else (where)?

Denis


> 
>>   typedef struct QCowHeader {
>>       uint32_t magic;
>>       uint32_t version;
>> @@ -197,10 +201,13 @@ enum {
>>   
>>   /* Incompatible feature bits */
>>   enum {
>> -    QCOW2_INCOMPAT_DIRTY_BITNR   = 0,
>> -    QCOW2_INCOMPAT_CORRUPT_BITNR = 1,
>> -    QCOW2_INCOMPAT_DIRTY         = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
>> -    QCOW2_INCOMPAT_CORRUPT       = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
>> +    QCOW2_INCOMPAT_DIRTY_BITNR            = 0,
>> +    QCOW2_INCOMPAT_CORRUPT_BITNR          = 1,
>> +    QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR = 2,
>> +    QCOW2_INCOMPAT_DIRTY                  = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
>> +    QCOW2_INCOMPAT_CORRUPT                = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
>> +    QCOW2_INCOMPAT_COMPRESSION_TYPE       =
>> +                                    1 << QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR,
>>   
>>       QCOW2_INCOMPAT_MASK          = QCOW2_INCOMPAT_DIRTY
>>                                    | QCOW2_INCOMPAT_CORRUPT,
> 
> This mask needs to be expanded by QCOW2_INCOMPAT_COMPRESSION_TYPE.
> 
>> @@ -256,6 +263,10 @@ typedef struct Qcow2BitmapHeaderExt {
>>       uint64_t bitmap_directory_offset;
>>   } QEMU_PACKED Qcow2BitmapHeaderExt;
>>   
>> +typedef struct Qcow2CompressionTypeExt {
>> +    uint32_t compression_type;
>> +} QEMU_PACKED Qcow2CompressionTypeExt;
>> +
>>   typedef struct BDRVQcow2State {
>>       int cluster_bits;
>>       int cluster_size;
>> @@ -340,6 +351,13 @@ typedef struct BDRVQcow2State {
>>   
>>       CoQueue compress_wait_queue;
>>       int nb_compress_threads;
>> +    /**
>> +     * Compression type used for the image. Default: 0 - ZLIB
>> +     * The image compression type is set on image creation.
>> +     * The only way to change the compression type is to convert the image
>> +     * with the desired compresion type set
> 
> *compression
> 
> And, well, ideally qemu-img amend could perform this operation, too.
> 
> Max
> 
>> +     */
>> +    uint32_t compression_type;
>>   } BDRVQcow2State;
>>   
>>   typedef struct Qcow2COWRegion {
>>
> 
> 

-- 
Best,
Denis
Re: [Qemu-devel] [PATCH] [RFC] qcow2: add compression type feature
Posted by Max Reitz 4 years, 10 months ago
On 16.05.19 09:50, Denis Plotnikov wrote:
> 
> 
> On 29.04.2019 1:32, Max Reitz wrote:
>> On 05.02.19 10:08, Denis Plotnikov wrote:
>>> The patch adds some preparation parts for incompatible compression type
>>> feature into QCOW2 header that indicates that *all* compressed clusters
>>> must be (de)compressed using a certain compression type.
>>>
>>> It is implied that the compression type is set on the image creation and
>>> can be changed only later by image convertion, thus the only compression
>>> algorithm is used for the image.
>>>
>>> The plan is to add support for ZSTD and then may be something more effective
>>> in the future.
>>>
>>> ZSDT compression algorithm consumes 3-5 times less CPU power with a
>>> comparable comression ratio with zlib. It would be wise to use it for
>>> data compression f.e. for backups.
>>>
>>> The default compression is ZLIB.
>>>
>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>>> ---
>>>   block/qcow2.c | 25 +++++++++++++++++++++++++
>>>   block/qcow2.h | 26 ++++++++++++++++++++++----
>>>   2 files changed, 47 insertions(+), 4 deletions(-)
>>
>> Are there plans to pursue this further?
> Yes, I'm going to come up with the first version in a few weeks

OK.

>> [...]
>>
>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>> index 32cce9eee2..fdde5bbefd 100644
>>> --- a/block/qcow2.h
>>> +++ b/block/qcow2.h
>>> @@ -112,6 +112,10 @@
>>>   #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
>>>   #define QCOW2_OPT_CACHE_CLEAN_INTERVAL "cache-clean-interval"
>>>   
>>> +/* Compression types */
>>> +#define QCOW2_COMPRESSION_TYPE_ZLIB    0
>>> +#define QCOW2_COMPRESSION_TYPE_ZSTD    1
>>
>> We probably want QAPI types anyway (qemu-img info should report the
>> compression type), so I think we could use them instead.
> I'm not sure that I understood the idea. Could you please explain what 
> is meant here? We don't need those constants and should use the 
> constants defined somewhere else (where)?

qemu-img info can report format-specific information.  I think the
compression type should be listed there as well, once there is more than
one.

Format-specific information is a QAPI type, namely
ImageInfoSpecificQCow2.  Therefore, everything it contains needs to be a
QAPI object, and this includes potential compression information.  We
thus need a QAPI enum like Qcow2CompressionType, and that would
automatically give us these values.

Max

Re: [Qemu-devel] [PATCH] [RFC] qcow2: add compression type feature
Posted by Denis Plotnikov 4 years, 10 months ago

On 16.05.2019 13:40, Max Reitz wrote:
> On 16.05.19 09:50, Denis Plotnikov wrote:
>>
>>
>> On 29.04.2019 1:32, Max Reitz wrote:
>>> On 05.02.19 10:08, Denis Plotnikov wrote:
>>>> The patch adds some preparation parts for incompatible compression type
>>>> feature into QCOW2 header that indicates that *all* compressed clusters
>>>> must be (de)compressed using a certain compression type.
>>>>
>>>> It is implied that the compression type is set on the image creation and
>>>> can be changed only later by image convertion, thus the only compression
>>>> algorithm is used for the image.
>>>>
>>>> The plan is to add support for ZSTD and then may be something more effective
>>>> in the future.
>>>>
>>>> ZSDT compression algorithm consumes 3-5 times less CPU power with a
>>>> comparable comression ratio with zlib. It would be wise to use it for
>>>> data compression f.e. for backups.
>>>>
>>>> The default compression is ZLIB.
>>>>
>>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>>>> ---
>>>>    block/qcow2.c | 25 +++++++++++++++++++++++++
>>>>    block/qcow2.h | 26 ++++++++++++++++++++++----
>>>>    2 files changed, 47 insertions(+), 4 deletions(-)
>>>
>>> Are there plans to pursue this further?
>> Yes, I'm going to come up with the first version in a few weeks
> 
> OK.
> 
>>> [...]
>>>
>>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>>> index 32cce9eee2..fdde5bbefd 100644
>>>> --- a/block/qcow2.h
>>>> +++ b/block/qcow2.h
>>>> @@ -112,6 +112,10 @@
>>>>    #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
>>>>    #define QCOW2_OPT_CACHE_CLEAN_INTERVAL "cache-clean-interval"
>>>>    
>>>> +/* Compression types */
>>>> +#define QCOW2_COMPRESSION_TYPE_ZLIB    0
>>>> +#define QCOW2_COMPRESSION_TYPE_ZSTD    1
>>>
>>> We probably want QAPI types anyway (qemu-img info should report the
>>> compression type), so I think we could use them instead.
>> I'm not sure that I understood the idea. Could you please explain what
>> is meant here? We don't need those constants and should use the
>> constants defined somewhere else (where)?
> 
> qemu-img info can report format-specific information.  I think the
> compression type should be listed there as well, once there is more than
> one.
> 
> Format-specific information is a QAPI type, namely
> ImageInfoSpecificQCow2.  Therefore, everything it contains needs to be a
> QAPI object, and this includes potential compression information.  We
> thus need a QAPI enum like Qcow2CompressionType, and that would
> automatically give us these values.

Thanks for clarification. Will do that way.

Denis
> 
> Max
> 

-- 
Best,
Denis
Re: [Qemu-devel] [PATCH] [RFC] qcow2: add compression type feature
Posted by Stefano Garzarella 4 years, 11 months ago
Hi Denis,
few simple comments below regarding the CODING_STYLE (point 7. Comment style)

On Tue, Feb 05, 2019 at 12:08:25PM +0300, Denis Plotnikov wrote:
> The patch adds some preparation parts for incompatible compression type
> feature into QCOW2 header that indicates that *all* compressed clusters
> must be (de)compressed using a certain compression type.
> 
> It is implied that the compression type is set on the image creation and
> can be changed only later by image convertion, thus the only compression
> algorithm is used for the image.
> 
> The plan is to add support for ZSTD and then may be something more effective
> in the future.
> 
> ZSDT compression algorithm consumes 3-5 times less CPU power with a
> comparable comression ratio with zlib. It would be wise to use it for
> data compression f.e. for backups.
> 
> The default compression is ZLIB.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---
>  block/qcow2.c | 25 +++++++++++++++++++++++++
>  block/qcow2.h | 26 ++++++++++++++++++++++----
>  2 files changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 8c91b92865..cb3d6cc1c0 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -73,6 +73,7 @@ typedef struct {
>  #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>  #define  QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
>  #define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
> +#define  QCOW2_EXT_MAGIC_COMPRESSION_TYPE 0x434D5052
>  
>  static int coroutine_fn
>  qcow2_co_preadv_compressed(BlockDriverState *bs,
> @@ -397,6 +398,9 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>  #endif
>              break;
>  
> +        case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
> +            /* Setting compression type to BDRVQcow2State->compression_type */
> +            /* from the image header is going to be here */

I'd use a multiline comment block here.

>          default:
>              /* unknown magic - save it in case we need to rewrite the header */
>              /* If you add a new feature, make sure to also update the fast
> @@ -2431,6 +2435,11 @@ int qcow2_update_header(BlockDriverState *bs)
>                  .bit  = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
>                  .name = "lazy refcounts",
>              },
> +            {
> +                .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
> +                .bit  = QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR,
> +                .name = "compression type",
> +            },
>          };
>  
>          ret = header_ext_add(buf, QCOW2_EXT_MAGIC_FEATURE_TABLE,
> @@ -2461,6 +2470,22 @@ int qcow2_update_header(BlockDriverState *bs)
>          buflen -= ret;
>      }
>  
> +    /* Compression type extension */
> +    if (s->compression_type != 0) {
> +        Qcow2CompressionTypeExt comp_header = {
> +            .compression_type = cpu_to_be32(s->compression_type),
> +        };
> +        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_COMPRESSION_TYPE,
> +                             &comp_header,
> +                             cpu_to_be64(sizeof(comp_header)),
> +                             buflen);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +        buf += ret;
> +        buflen -= ret;
> +    }
> +
>      /* Keep unknown header extensions */
>      QLIST_FOREACH(uext, &s->unknown_header_ext, next) {
>          ret = header_ext_add(buf, uext->magic, uext->data, uext->len, buflen);
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 32cce9eee2..fdde5bbefd 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -112,6 +112,10 @@
>  #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
>  #define QCOW2_OPT_CACHE_CLEAN_INTERVAL "cache-clean-interval"
>  
> +/* Compression types */
> +#define QCOW2_COMPRESSION_TYPE_ZLIB    0
> +#define QCOW2_COMPRESSION_TYPE_ZSTD    1
> +
>  typedef struct QCowHeader {
>      uint32_t magic;
>      uint32_t version;
> @@ -197,10 +201,13 @@ enum {
>  
>  /* Incompatible feature bits */
>  enum {
> -    QCOW2_INCOMPAT_DIRTY_BITNR   = 0,
> -    QCOW2_INCOMPAT_CORRUPT_BITNR = 1,
> -    QCOW2_INCOMPAT_DIRTY         = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
> -    QCOW2_INCOMPAT_CORRUPT       = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
> +    QCOW2_INCOMPAT_DIRTY_BITNR            = 0,
> +    QCOW2_INCOMPAT_CORRUPT_BITNR          = 1,
> +    QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR = 2,
> +    QCOW2_INCOMPAT_DIRTY                  = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
> +    QCOW2_INCOMPAT_CORRUPT                = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
> +    QCOW2_INCOMPAT_COMPRESSION_TYPE       =
> +                                    1 << QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR,
>  
>      QCOW2_INCOMPAT_MASK          = QCOW2_INCOMPAT_DIRTY
>                                   | QCOW2_INCOMPAT_CORRUPT,
> @@ -256,6 +263,10 @@ typedef struct Qcow2BitmapHeaderExt {
>      uint64_t bitmap_directory_offset;
>  } QEMU_PACKED Qcow2BitmapHeaderExt;
>  
> +typedef struct Qcow2CompressionTypeExt {
> +    uint32_t compression_type;
> +} QEMU_PACKED Qcow2CompressionTypeExt;
> +
>  typedef struct BDRVQcow2State {
>      int cluster_bits;
>      int cluster_size;
> @@ -340,6 +351,13 @@ typedef struct BDRVQcow2State {
>  
>      CoQueue compress_wait_queue;
>      int nb_compress_threads;
> +    /**

I'd remove the second asterisk here.

> +     * Compression type used for the image. Default: 0 - ZLIB
> +     * The image compression type is set on image creation.
> +     * The only way to change the compression type is to convert the image
> +     * with the desired compresion type set
> +     */
> +    uint32_t compression_type;
>  } BDRVQcow2State;
>  
>  typedef struct Qcow2COWRegion {
> -- 
> 2.17.0
> 
> 

Thanks,
Stefano

Re: [Qemu-devel] [PATCH] [RFC] qcow2: add compression type feature
Posted by Eric Blake 4 years, 11 months ago
On 2/5/19 3:08 AM, Denis Plotnikov wrote:
> The patch adds some preparation parts for incompatible compression type
> feature into QCOW2 header that indicates that *all* compressed clusters
> must be (de)compressed using a certain compression type.
> 
> It is implied that the compression type is set on the image creation and
> can be changed only later by image convertion, thus the only compression

s/convertion/conversion/

> algorithm is used for the image.
> 
> The plan is to add support for ZSTD and then may be something more effective
> in the future.
> 
> ZSDT compression algorithm consumes 3-5 times less CPU power with a

s/ZSDT/ZSTD/

> comparable comression ratio with zlib. It would be wise to use it for

s/comression/compression/

> data compression f.e. for backups.
> 
> The default compression is ZLIB.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---
>  block/qcow2.c | 25 +++++++++++++++++++++++++
>  block/qcow2.h | 26 ++++++++++++++++++++++----
>  2 files changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 8c91b92865..cb3d6cc1c0 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -73,6 +73,7 @@ typedef struct {
>  #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>  #define  QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
>  #define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
> +#define  QCOW2_EXT_MAGIC_COMPRESSION_TYPE 0x434D5052

This appears to be adding a new header extension magic number, but
didn't actually modify the specification. I'd expect this patch to touch
docs/interop/qcow2.txt before it can be considered complete.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org