[PATCH v7 0/2] qcow2: add zstd cluster compression

[PATCH v7 0/2] qcow2: add zstd cluster compression

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is my proposal, about how to correctly update qcow2 specification
to introduce new field, keeping in mind currently existing images and
downstream Qemu instances.

Let's consider discussing from this.

I called it v7, as it's a continuation of
"[PATCH v6 0/3] qcow2: add zstd cluster compression". Still there are
actually only two preparing patches, so, if we like them (may be after
some resends) Denis's series should be rebased on these two patches.

Vladimir Sementsov-Ogievskiy (2):
  docs: improve qcow2 spec about extending image header
  docs: qcow2: introduce compression type feature

 docs/interop/qcow2.txt | 43 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

-- 
2.21.0

[PATCH v7 1/2] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

Make it more obvious how to add new fields to the version 3 header and
how to interpret them.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 docs/interop/qcow2.txt | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index af5711e533..3f2855593f 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -79,9 +79,9 @@ The first cluster of a qcow2 image contains the file header:
                     Offset into the image file at which the snapshot table
                     starts. Must be aligned to a cluster boundary.
 
-If the version is 3 or higher, the header has the following additional fields.
-For version 2, the values are assumed to be zero, unless specified otherwise
-in the description of a field.
+For version 2, header is always 72 bytes length and finishes here.
+For version 3 or higher the header length is at least 104 bytes and has at
+least next five fields, up to the @header_length field.
 
          72 -  79:  incompatible_features
                     Bitmask of incompatible features. An implementation must
@@ -165,6 +165,26 @@ in the description of a field.
                     Length of the header structure in bytes. For version 2
                     images, the length is always assumed to be 72 bytes.
 
+Additional fields (version 3 and higher)
+
+The following fields of the header are optional: if software don't know how to
+interpret the field, it may safely ignore it. Still the field must be kept as is
+when rewriting the image. @header_length must be bound to the end of one of
+these fields (or to @header_length field end itself, to be 104 bytes).
+This definition implies the following:
+1. Software may support some of these optional fields and ignore the others,
+   which means that features may be backported to downstream Qemu independently.
+2. Software may check @header_length, if it knows optional fields specification
+   enough (knows about the field which exceeds @header_length).
+3. If @header_length is higher than the highest field end that software knows,
+   it should assume that additional fields are correct, @header_length is
+   correct and keep @header_length and additional unknown fields as is on
+   rewriting the image.
+3. If we want to add incompatible field (or a field, for which some its values
+   would be incompatible), it must be accompanied by incompatible feature bit.
+
+        < ... No additional fields in the header currently ... >
+
 Directly after the image header, optional sections called header extensions can
 be stored. Each extension has a structure like the following:
 
-- 
2.21.0

[PATCH v7 2/2] docs: qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

The patch add new additional field to qcow2 header: compression_type,
which specifies compression type. If field is absent or zero, default
compression type is set: ZLIB, which corresponds to current behavior.

New compression type (ZSTD) is to be added in further commit.

Suggested-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 docs/interop/qcow2.txt | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 3f2855593f..82ec0845d8 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -109,6 +109,12 @@ least next five fields, up to the @header_length field.
                                 An External Data File Name header extension may
                                 be present if this bit is set.
 
+                    Bit 3:      Compression type bit.  If this bit is set,
+                                non-default compression is used for compressed
+                                clusters. In this case, @header_length must
+                                be at least 108 and @compression_type field
+                                must be non-zero.
+
                     Bits 3-63:  Reserved (set to 0)
 
          80 -  87:  compatible_features
@@ -183,7 +189,18 @@ This definition implies the following:
 3. If we want to add incompatible field (or a field, for which some its values
    would be incompatible), it must be accompanied by incompatible feature bit.
 
-        < ... No additional fields in the header currently ... >
+              104:  compression_type
+                    Defines the compression method used for compressed clusters.
+                    A single compression type is applied to all compressed image
+                    clusters.
+                    If incompatible compression type bit is set: the field must
+                    exist (i.e. @header_length >= 105) and must be non-zero (
+                    which means non-zlib compression type)
+                    If incompatible compression type bit is unset: the field
+                    may not exist (if @header_length < 105) or it must be zero
+                    (which means zlib).
+                    Available compression type values:
+                        0: zlib <https://www.zlib.net/>
 
 Directly after the image header, optional sections called header extensions can
 be stored. Each extension has a structure like the following:
-- 
2.21.0

Re: [PATCH v7 1/2] docs: improve qcow2 spec about extending image header

[Eric Blake]

On 10/7/19 11:04 AM, Vladimir Sementsov-Ogievskiy wrote:
> Make it more obvious how to add new fields to the version 3 header and
> how to interpret them.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   docs/interop/qcow2.txt | 26 +++++++++++++++++++++++---
>   1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index af5711e533..3f2855593f 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -79,9 +79,9 @@ The first cluster of a qcow2 image contains the file header:
>                       Offset into the image file at which the snapshot table
>                       starts. Must be aligned to a cluster boundary.
>   
> -If the version is 3 or higher, the header has the following additional fields.
> -For version 2, the values are assumed to be zero, unless specified otherwise
> -in the description of a field.
> +For version 2, header is always 72 bytes length and finishes here.
> +For version 3 or higher the header length is at least 104 bytes and has at
> +least next five fields, up to the @header_length field.

This hunk seems okay.

>   
>            72 -  79:  incompatible_features
>                       Bitmask of incompatible features. An implementation must
> @@ -165,6 +165,26 @@ in the description of a field.
>                       Length of the header structure in bytes. For version 2
>                       images, the length is always assumed to be 72 bytes.
>   
> +Additional fields (version 3 and higher)
> +
> +The following fields of the header are optional: if software don't know how to
> +interpret the field, it may safely ignore it. Still the field must be kept as is
> +when rewriting the image.

if software doesn't know how to interpret the field, it may be safely 
ignored, other than preserving the field unchanged when rewriting the 
image header.

Missing:

If header_length excludes an optional field, the value of 0 should be 
used for that field.

> @header_length must be bound to the end of one of
> +these fields (or to @header_length field end itself, to be 104 bytes).

We don't use the @header_length markup anywhere else in this file, 
starting to do so here is odd.

I would suggest a stronger requirement:

header_length must be a multiple of 4, and must not land in the middle 
of any optional 8-byte field.

Or maybe even add our compression type extension with 4 bytes of 
padding, so that we could go even stronger:

header_length must be a multiple of 8.

> +This definition implies the following:
> +1. Software may support some of these optional fields and ignore the others,
> +   which means that features may be backported to downstream Qemu independently.

I don't think this belongs in the spec.  Ideally, we add fields so 
infrequently that backporting doesn't have to worry about backporting 
field 2 while skipping field 1.

> +2. Software may check @header_length, if it knows optional fields specification
> +   enough (knows about the field which exceeds @header_length).

Again, I don't think this adds anything.  Since we already documented 
fields are optional, and that if header_length is too short, the missing 
field is treated as 0, software that knows about a longer header_length 
will already handle it correctly.

> +3. If @header_length is higher than the highest field end that software knows,
> +   it should assume that additional fields are correct, @header_length is
> +   correct and keep @header_length and additional unknown fields as is on
> +   rewriting the image.
> +3. If we want to add incompatible field (or a field, for which some its values
> +   would be incompatible), it must be accompanied by incompatible feature bit.
> +
> +        < ... No additional fields in the header currently ... >
> +

I'm still not seeing the value in adding any of this paragraph to the 
spec.  Maybe in the commit message that accompanies the spec change, but 
the spec is clear enough if it documents how optional header fields are 
to be managed (treat as 0 if missing, preserve on write if unknown, and 
with a mandated alignment to avoid having to worry about other issues).

>   Directly after the image header, optional sections called header extensions can
>   be stored. Each extension has a structure like the following:
>   
> 

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

Re: [PATCH v7 1/2] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

07.10.2019 23:21, Eric Blake wrote:
> On 10/7/19 11:04 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Make it more obvious how to add new fields to the version 3 header and
>> how to interpret them.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   docs/interop/qcow2.txt | 26 +++++++++++++++++++++++---
>>   1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index af5711e533..3f2855593f 100644
>> --- a/docs/interop/qcow2.txt
>> +++ b/docs/interop/qcow2.txt
>> @@ -79,9 +79,9 @@ The first cluster of a qcow2 image contains the file header:
>>                       Offset into the image file at which the snapshot table
>>                       starts. Must be aligned to a cluster boundary.
>> -If the version is 3 or higher, the header has the following additional fields.
>> -For version 2, the values are assumed to be zero, unless specified otherwise
>> -in the description of a field.
>> +For version 2, header is always 72 bytes length and finishes here.
>> +For version 3 or higher the header length is at least 104 bytes and has at
>> +least next five fields, up to the @header_length field.
> 
> This hunk seems okay.
> 
>>            72 -  79:  incompatible_features
>>                       Bitmask of incompatible features. An implementation must
>> @@ -165,6 +165,26 @@ in the description of a field.
>>                       Length of the header structure in bytes. For version 2
>>                       images, the length is always assumed to be 72 bytes.
>> +Additional fields (version 3 and higher)
>> +
>> +The following fields of the header are optional: if software don't know how to
>> +interpret the field, it may safely ignore it. Still the field must be kept as is
>> +when rewriting the image.
> 
> if software doesn't know how to interpret the field, it may be safely ignored, other than preserving the field unchanged when rewriting the image header.
> 
> Missing:
> 
> If header_length excludes an optional field, the value of 0 should be used for that field.

This is what I dislike in old wording. Why do we need this default-zero thing[*]? What is the default?

Default is absence of the feature, we don't have these future features now and don't care of them.
What is this default 0 for us now? Nothing.

Consider some future version: if it sees that header_length excludes some fields, it understands,
that there is no such feature here. That's all. Work without it. The feature itself should declare
behavior without this feature, which should correspond to behavior before this feature introduction..

So at least, I don't like "the value of 0 should be used for that field", as instances of Qemu which
don't know about the feature will ignore this requirement, as they don't need any value of that
field at all.

What you actually mean, IMHO, is: for all optional field 0 value must be equal to absence of the feature,
like when header_length excludes this field. I don't see, do we really need this requirement, but
seems it was mentioned before this patch and we'd better keep it.. I just don't like concept of
"default" value keeping in mind valid Qemu instances which don't know about field at all.

> 
>> @header_length must be bound to the end of one of
>> +these fields (or to @header_length field end itself, to be 104 bytes).
> 
> We don't use the @header_length markup anywhere else in this file, starting to do so here is odd.
> 
> I would suggest a stronger requirement:
> 
> header_length must be a multiple of 4, and must not land in the middle of any optional 8-byte field.
> 
> Or maybe even add our compression type extension with 4 bytes of padding, so that we could go even stronger:
> 
> header_length must be a multiple of 8.

Hmm, if we imply that software will have to add some padding, than requirement above about zero === feature-absence
becomes necessary. [*]

Still I have two questions:
1. Do we really need all fields to be 4 or 8 bytes? Why not use 1 byte for compression?
2. What is the benefit of padding, which you propose?

> 
>> +This definition implies the following:
>> +1. Software may support some of these optional fields and ignore the others,
>> +   which means that features may be backported to downstream Qemu independently.
> 
> I don't think this belongs in the spec.

Me too. But at least I noted what I try to achieve, so consider it a bit like RFC. And of course I hoped for your rewordings )

>  Ideally, we add fields so infrequently that backporting doesn't have to worry about backporting field 2 while skipping field 1.

Who knows.. Even having only two fields A and B, when we need B which actually needs 10 patches to backport and A needs 100 would be
a problem, if we can't backport B in separate.

I remember similar thing about NBD: I needed BLOCK_STATUS, but because of specification I had to implement
structured read first, which wasn't interesting to me at that moment.

> 
>> +2. Software may check @header_length, if it knows optional fields specification
>> +   enough (knows about the field which exceeds @header_length).
> 
> Again, I don't think this adds anything.  Since we already documented fields are optional, and that if header_length is too short, the missing field is treated as 0, software that knows about a longer header_length will already handle it correctly.

I think, I'll move these points to commit message, to keep them somehow.

> 
>> +3. If @header_length is higher than the highest field end that software knows,
>> +   it should assume that additional fields are correct, @header_length is
>> +   correct and keep @header_length and additional unknown fields as is on
>> +   rewriting the image.
>> +3. If we want to add incompatible field (or a field, for which some its values
>> +   would be incompatible), it must be accompanied by incompatible feature bit.
>> +
>> +        < ... No additional fields in the header currently ... >
>> +
> 
> I'm still not seeing the value in adding any of this paragraph to the spec.  Maybe in the commit message that accompanies the spec change, but the spec is clear enough if it documents how optional header fields are to be managed (treat as 0 if missing, preserve on write if unknown, and with a mandated alignment to avoid having to worry about other issues).
> 
>>   Directly after the image header, optional sections called header extensions can
>>   be stored. Each extension has a structure like the following:
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH v7 1/2] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

08.10.2019 12:05, Vladimir Sementsov-Ogievskiy wrote:
> 07.10.2019 23:21, Eric Blake wrote:
>> On 10/7/19 11:04 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Make it more obvious how to add new fields to the version 3 header and
>>> how to interpret them.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   docs/interop/qcow2.txt | 26 +++++++++++++++++++++++---
>>>   1 file changed, 23 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>>> index af5711e533..3f2855593f 100644
>>> --- a/docs/interop/qcow2.txt
>>> +++ b/docs/interop/qcow2.txt
>>> @@ -79,9 +79,9 @@ The first cluster of a qcow2 image contains the file header:
>>>                       Offset into the image file at which the snapshot table
>>>                       starts. Must be aligned to a cluster boundary.
>>> -If the version is 3 or higher, the header has the following additional fields.
>>> -For version 2, the values are assumed to be zero, unless specified otherwise
>>> -in the description of a field.
>>> +For version 2, header is always 72 bytes length and finishes here.
>>> +For version 3 or higher the header length is at least 104 bytes and has at
>>> +least next five fields, up to the @header_length field.
>>
>> This hunk seems okay.
>>
>>>            72 -  79:  incompatible_features
>>>                       Bitmask of incompatible features. An implementation must
>>> @@ -165,6 +165,26 @@ in the description of a field.
>>>                       Length of the header structure in bytes. For version 2
>>>                       images, the length is always assumed to be 72 bytes.
>>> +Additional fields (version 3 and higher)
>>> +
>>> +The following fields of the header are optional: if software don't know how to
>>> +interpret the field, it may safely ignore it. Still the field must be kept as is
>>> +when rewriting the image.
>>
>> if software doesn't know how to interpret the field, it may be safely ignored, other than preserving the field unchanged when rewriting the image header.
>>
>> Missing:
>>
>> If header_length excludes an optional field, the value of 0 should be used for that field.
> 
> This is what I dislike in old wording. Why do we need this default-zero thing[*]? What is the default?
> 
> Default is absence of the feature, we don't have these future features now and don't care of them.
> What is this default 0 for us now? Nothing.
> 
> Consider some future version: if it sees that header_length excludes some fields, it understands,
> that there is no such feature here. That's all. Work without it. The feature itself should declare
> behavior without this feature, which should correspond to behavior before this feature introduction..
> 
> So at least, I don't like "the value of 0 should be used for that field", as instances of Qemu which
> don't know about the feature will ignore this requirement, as they don't need any value of that
> field at all.
> 
> What you actually mean, IMHO, is: for all optional field 0 value must be equal to absence of the feature,
> like when header_length excludes this field. I don't see, do we really need this requirement, but
> seems it was mentioned before this patch and we'd better keep it.. I just don't like concept of
> "default" value keeping in mind valid Qemu instances which don't know about field at all.
> 
>>
>>> @header_length must be bound to the end of one of
>>> +these fields (or to @header_length field end itself, to be 104 bytes).
>>
>> We don't use the @header_length markup anywhere else in this file, starting to do so here is odd.
>>
>> I would suggest a stronger requirement:
>>
>> header_length must be a multiple of 4, and must not land in the middle of any optional 8-byte field.
>>
>> Or maybe even add our compression type extension with 4 bytes of padding, so that we could go even stronger:
>>
>> header_length must be a multiple of 8.
> 
> Hmm, if we imply that software will have to add some padding, than requirement above about zero === feature-absence
> becomes necessary. [*]
> 
> Still I have two questions:
> 1. Do we really need all fields to be 4 or 8 bytes? Why not use 1 byte for compression?
> 2. What is the benefit of padding, which you propose?

Hmm, now I think, that we should align header to multiply of 8, as header extensions are already have
"""
Directly after the image header, optional sections called header extensions can
be stored. Each extension has a structure like the following:

[...]

           n -  m:   Padding to round up the header extension size to the next
                     multiple of 8.
"""

So, it looks inconsistent, if we pad all header extensions to  8 bytes except for the start of the first extension.

I'll resend with padding soon.

> 
>>
>>> +This definition implies the following:
>>> +1. Software may support some of these optional fields and ignore the others,
>>> +   which means that features may be backported to downstream Qemu independently.
>>
>> I don't think this belongs in the spec.
> 
> Me too. But at least I noted what I try to achieve, so consider it a bit like RFC. And of course I hoped for your rewordings )
> 
>>   Ideally, we add fields so infrequently that backporting doesn't have to worry about backporting field 2 while skipping field 1.
> 
> Who knows.. Even having only two fields A and B, when we need B which actually needs 10 patches to backport and A needs 100 would be
> a problem, if we can't backport B in separate.
> 
> I remember similar thing about NBD: I needed BLOCK_STATUS, but because of specification I had to implement
> structured read first, which wasn't interesting to me at that moment.
> 
>>
>>> +2. Software may check @header_length, if it knows optional fields specification
>>> +   enough (knows about the field which exceeds @header_length).
>>
>> Again, I don't think this adds anything.  Since we already documented fields are optional, and that if header_length is too short, the missing field is treated as 0, software that knows about a longer header_length will already handle it correctly.
> 
> I think, I'll move these points to commit message, to keep them somehow.
> 
>>
>>> +3. If @header_length is higher than the highest field end that software knows,
>>> +   it should assume that additional fields are correct, @header_length is
>>> +   correct and keep @header_length and additional unknown fields as is on
>>> +   rewriting the image.
>>> +3. If we want to add incompatible field (or a field, for which some its values
>>> +   would be incompatible), it must be accompanied by incompatible feature bit.
>>> +
>>> +        < ... No additional fields in the header currently ... >
>>> +
>>
>> I'm still not seeing the value in adding any of this paragraph to the spec.  Maybe in the commit message that accompanies the spec change, but the spec is clear enough if it documents how optional header fields are to be managed (treat as 0 if missing, preserve on write if unknown, and with a mandated alignment to avoid having to worry about other issues).
>>
>>>   Directly after the image header, optional sections called header extensions can
>>>   be stored. Each extension has a structure like the following:
>>>
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [PATCH v7 1/2] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

18.10.2019 11:29, Vladimir Sementsov-Ogievskiy wrote:
> 08.10.2019 12:05, Vladimir Sementsov-Ogievskiy wrote:
>> 07.10.2019 23:21, Eric Blake wrote:
>>> On 10/7/19 11:04 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Make it more obvious how to add new fields to the version 3 header and
>>>> how to interpret them.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>   docs/interop/qcow2.txt | 26 +++++++++++++++++++++++---
>>>>   1 file changed, 23 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>>>> index af5711e533..3f2855593f 100644
>>>> --- a/docs/interop/qcow2.txt
>>>> +++ b/docs/interop/qcow2.txt
>>>> @@ -79,9 +79,9 @@ The first cluster of a qcow2 image contains the file header:
>>>>                       Offset into the image file at which the snapshot table
>>>>                       starts. Must be aligned to a cluster boundary.
>>>> -If the version is 3 or higher, the header has the following additional fields.
>>>> -For version 2, the values are assumed to be zero, unless specified otherwise
>>>> -in the description of a field.
>>>> +For version 2, header is always 72 bytes length and finishes here.
>>>> +For version 3 or higher the header length is at least 104 bytes and has at
>>>> +least next five fields, up to the @header_length field.
>>>
>>> This hunk seems okay.
>>>
>>>>            72 -  79:  incompatible_features
>>>>                       Bitmask of incompatible features. An implementation must
>>>> @@ -165,6 +165,26 @@ in the description of a field.
>>>>                       Length of the header structure in bytes. For version 2
>>>>                       images, the length is always assumed to be 72 bytes.
>>>> +Additional fields (version 3 and higher)
>>>> +
>>>> +The following fields of the header are optional: if software don't know how to
>>>> +interpret the field, it may safely ignore it. Still the field must be kept as is
>>>> +when rewriting the image.
>>>
>>> if software doesn't know how to interpret the field, it may be safely ignored, other than preserving the field unchanged when rewriting the image header.
>>>
>>> Missing:
>>>
>>> If header_length excludes an optional field, the value of 0 should be used for that field.
>>
>> This is what I dislike in old wording. Why do we need this default-zero thing[*]? What is the default?
>>
>> Default is absence of the feature, we don't have these future features now and don't care of them.
>> What is this default 0 for us now? Nothing.
>>
>> Consider some future version: if it sees that header_length excludes some fields, it understands,
>> that there is no such feature here. That's all. Work without it. The feature itself should declare
>> behavior without this feature, which should correspond to behavior before this feature introduction..
>>
>> So at least, I don't like "the value of 0 should be used for that field", as instances of Qemu which
>> don't know about the feature will ignore this requirement, as they don't need any value of that
>> field at all.
>>
>> What you actually mean, IMHO, is: for all optional field 0 value must be equal to absence of the feature,
>> like when header_length excludes this field. I don't see, do we really need this requirement, but
>> seems it was mentioned before this patch and we'd better keep it.. I just don't like concept of
>> "default" value keeping in mind valid Qemu instances which don't know about field at all.
>>
>>>
>>>> @header_length must be bound to the end of one of
>>>> +these fields (or to @header_length field end itself, to be 104 bytes).
>>>
>>> We don't use the @header_length markup anywhere else in this file, starting to do so here is odd.
>>>
>>> I would suggest a stronger requirement:
>>>
>>> header_length must be a multiple of 4, and must not land in the middle of any optional 8-byte field.
>>>
>>> Or maybe even add our compression type extension with 4 bytes of padding, so that we could go even stronger:
>>>
>>> header_length must be a multiple of 8.
>>
>> Hmm, if we imply that software will have to add some padding, than requirement above about zero === feature-absence
>> becomes necessary. [*]
>>
>> Still I have two questions:
>> 1. Do we really need all fields to be 4 or 8 bytes? Why not use 1 byte for compression?
>> 2. What is the benefit of padding, which you propose?
> 
> Hmm, now I think, that we should align header to multiply of 8, as header extensions are already have
> """
> Directly after the image header, optional sections called header extensions can
> be stored. Each extension has a structure like the following:
> 
> [...]
> 
>            n -  m:   Padding to round up the header extension size to the next
>                      multiple of 8.
> """
> 
> So, it looks inconsistent, if we pad all header extensions to  8 bytes except for the start of the first extension.
> 
> I'll resend with padding soon.


Still, we have to make an exception at least for header_length = 104, which is not a multiply of 8.

Also, is requiring alignment is an incompatible change of specification?

> 
>>
>>>
>>>> +This definition implies the following:
>>>> +1. Software may support some of these optional fields and ignore the others,
>>>> +   which means that features may be backported to downstream Qemu independently.
>>>
>>> I don't think this belongs in the spec.
>>
>> Me too. But at least I noted what I try to achieve, so consider it a bit like RFC. And of course I hoped for your rewordings )
>>
>>>   Ideally, we add fields so infrequently that backporting doesn't have to worry about backporting field 2 while skipping field 1.
>>
>> Who knows.. Even having only two fields A and B, when we need B which actually needs 10 patches to backport and A needs 100 would be
>> a problem, if we can't backport B in separate.
>>
>> I remember similar thing about NBD: I needed BLOCK_STATUS, but because of specification I had to implement
>> structured read first, which wasn't interesting to me at that moment.
>>
>>>
>>>> +2. Software may check @header_length, if it knows optional fields specification
>>>> +   enough (knows about the field which exceeds @header_length).
>>>
>>> Again, I don't think this adds anything.  Since we already documented fields are optional, and that if header_length is too short, the missing field is treated as 0, software that knows about a longer header_length will already handle it correctly.
>>
>> I think, I'll move these points to commit message, to keep them somehow.
>>
>>>
>>>> +3. If @header_length is higher than the highest field end that software knows,
>>>> +   it should assume that additional fields are correct, @header_length is
>>>> +   correct and keep @header_length and additional unknown fields as is on
>>>> +   rewriting the image.
>>>> +3. If we want to add incompatible field (or a field, for which some its values
>>>> +   would be incompatible), it must be accompanied by incompatible feature bit.
>>>> +
>>>> +        < ... No additional fields in the header currently ... >
>>>> +
>>>
>>> I'm still not seeing the value in adding any of this paragraph to the spec.  Maybe in the commit message that accompanies the spec change, but the spec is clear enough if it documents how optional header fields are to be managed (treat as 0 if missing, preserve on write if unknown, and with a mandated alignment to avoid having to worry about other issues).
>>>
>>>>   Directly after the image header, optional sections called header extensions can
>>>>   be stored. Each extension has a structure like the following:
>>>>
>>>
>>
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [PATCH v7 1/2] docs: improve qcow2 spec about extending image header

[Eric Blake]

On 10/18/19 4:27 AM, Vladimir Sementsov-Ogievskiy wrote:

>>>> I would suggest a stronger requirement:
>>>>
>>>> header_length must be a multiple of 4, and must not land in the middle of any optional 8-byte field.
>>>>
>>>> Or maybe even add our compression type extension with 4 bytes of padding, so that we could go even stronger:
>>>>
>>>> header_length must be a multiple of 8.
>>>
>>> Hmm, if we imply that software will have to add some padding, than requirement above about zero === feature-absence
>>> becomes necessary. [*]
>>>
>>> Still I have two questions:
>>> 1. Do we really need all fields to be 4 or 8 bytes? Why not use 1 byte for compression?

No, fields can be smaller if that makes sense; but I still think it's 
important that fields don't straddle natural alignment boundaries. When 
adding just one field at a time, it's easier to add a wide field and 
less padding than a narrow field and lots of padding, if we're still 
striving for alignment.

>>> 2. What is the benefit of padding, which you propose?

The biggest benefit to keeping large fields from straddling alignment 
boundaries is that you can mmap() a qcow2 file and do naturally-aligned 
reads of those large fields, rather than byte-by-byte reads, without 
risking SIGBUS on some architectures.  (You still have to worry about 
endianness, but that's true regardless of alignment)


>> So, it looks inconsistent, if we pad all header extensions to  8 bytes except for the start of the first extension.
>>
>> I'll resend with padding soon.
> 
> 
> Still, we have to make an exception at least for header_length = 104, which is not a multiply of 8.

Huh?  104 == 8 * 13, so our current v3 header size is 8-byte aligned. 
Likewise for 72 == 8 * 9 for v2 header size.

> 
> Also, is requiring alignment is an incompatible change of specification?

No. I think it is just clarifying what was implicitly already  the case. 
  Remember, immediately after the header comes header extensions, and 
THOSE are explicitly required to be multiple-of-8 in size.  That 
requirement makes no sense unless the header itself ends on an 8-byte 
alignment (which it does for all existing v2 and v3 images prior to our 
first official v3 header extension).

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

Re: [PATCH v7 1/2] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

18.10.2019 16:39, Eric Blake wrote:
> On 10/18/19 4:27 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>>>> I would suggest a stronger requirement:
>>>>>
>>>>> header_length must be a multiple of 4, and must not land in the middle of any optional 8-byte field.
>>>>>
>>>>> Or maybe even add our compression type extension with 4 bytes of padding, so that we could go even stronger:
>>>>>
>>>>> header_length must be a multiple of 8.
>>>>
>>>> Hmm, if we imply that software will have to add some padding, than requirement above about zero === feature-absence
>>>> becomes necessary. [*]
>>>>
>>>> Still I have two questions:
>>>> 1. Do we really need all fields to be 4 or 8 bytes? Why not use 1 byte for compression?
> 
> No, fields can be smaller if that makes sense; but I still think it's important that fields don't straddle natural alignment boundaries. When adding just one field at a time, it's easier to add a wide field and less padding than a narrow field and lots of padding, if we're still striving for alignment.
> 
>>>> 2. What is the benefit of padding, which you propose?
> 
> The biggest benefit to keeping large fields from straddling alignment boundaries is that you can mmap() a qcow2 file and do naturally-aligned reads of those large fields, rather than byte-by-byte reads, without risking SIGBUS on some architectures.  (You still have to worry about endianness, but that's true regardless of alignment)
> 
> 
>>> So, it looks inconsistent, if we pad all header extensions to  8 bytes except for the start of the first extension.
>>>
>>> I'll resend with padding soon.
>>
>>
>> Still, we have to make an exception at least for header_length = 104, which is not a multiply of 8.
> 
> Huh?  104 == 8 * 13, so our current v3 header size is 8-byte aligned. Likewise for 72 == 8 * 9 for v2 header size.

Ahahahaha, shame on my head:) I should go to school again.

> 
>>
>> Also, is requiring alignment is an incompatible change of specification?
> 
> No. I think it is just clarifying what was implicitly already  the case.  Remember, immediately after the header comes header extensions, and THOSE are explicitly required to be multiple-of-8 in size.  That requirement makes no sense unless the header itself ends on an 8-byte alignment (which it does for all existing v2 and v3 images prior to our first official v3 header extension).
> 

OK, let's go with it, I'll resend again.


-- 
Best regards,
Vladimir