Re: [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing

[Denis Plotnikov]

On 28.06.2019 18:03, Max Reitz wrote:
> On 28.06.19 16:54, Kevin Wolf wrote:
>> Am 28.06.2019 um 16:40 hat Denis Plotnikov geschrieben:
>>>
>>>
>>> On 28.06.2019 17:24, Kevin Wolf wrote:
>>>> Am 28.06.2019 um 14:56 hat Denis Plotnikov geschrieben:
>>>>>
>>>>>
>>>>> On 28.06.2019 15:06, Kevin Wolf wrote:
>>>>>> Am 28.06.2019 um 13:24 hat Denis Plotnikov geschrieben:
>>>>>>>
>>>>>>>
>>>>>>> On 28.06.2019 13:23, Kevin Wolf wrote:
>>>>>>>> Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
>>>>>>>>> With the patch, qcow2 is able to process image compression type
>>>>>>>>> defined in the image header and choose the corresponding method
>>>>>>>>> for clusters compressing.
>>>>>>>>>
>>>>>>>>> Also, it rework the cluster compression code for adding more
>>>>>>>>> compression types.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Denis Plotnikov <address@hidden>
>>>>>>>>> ---
>>>>>>>>>      block/qcow2.c | 103 
>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++------
>>>>>>>>>      1 file changed, 92 insertions(+), 11 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>>>>> index c4b5b93408..90f15cc3c9 100644
>>>>>>>>> --- a/block/qcow2.c
>>>>>>>>> +++ b/block/qcow2.c
>>>>>>>>> @@ -400,11 +400,39 @@ static int 
>>>>>>>>> qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>>>>>>>>                  break;
>>>>>>>>>      
>>>>>>>>>              case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
>>>>>>>>> +            /* Compression type always goes with the compression 
>>>>>>>>> type bit set */
>>>>>>>>> +            if (!(s->incompatible_features & 
>>>>>>>>> QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
>>>>>>>>> +                error_setg(errp,
>>>>>>>>> +                           "compression_type_ext: "
>>>>>>>>> +                           "expect compression type bit set");
>>>>>>>>> +                return -EINVAL;
>>>>>>>>> +            }
>>>>>>>>> +
>>>>>>>>> +            ret = bdrv_pread(bs->file, offset, &s->compression_type, 
>>>>>>>>> ext.len);
>>>>>>>>> +            s->compression_type = be32_to_cpu(s->compression_type);
>>>>>>>>> +
>>>>>>>>> +            if (ret < 0) {
>>>>>>>>> +                error_setg_errno(errp, -ret,
>>>>>>>>> +                                 "ERROR: Could not read compression 
>>>>>>>>> type");
>>>>>>>>> +                return ret;
>>>>>>>>> +            }
>>>>>>>>> +
>>>>>>>>>                  /*
>>>>>>>>> -             * Setting compression type to 
>>>>>>>>> BDRVQcow2State->compression_type
>>>>>>>>> -             * from the image header is going to be here
>>>>>>>>> +             * The default compression type is not allowed when the 
>>>>>>>>> extension
>>>>>>>>> +             * is present. ZLIB is used as the default compression 
>>>>>>>>> type.
>>>>>>>>> +             * When compression type extension header is present then
>>>>>>>>> +             * compression_type should have a value different from 
>>>>>>>>> the default.
>>>>>>>>>                   */
>>>>>>>>> -             break;
>>>>>>>>> +            if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
>>>>>>>>> +                error_setg(errp,
>>>>>>>>> +                           "compression_type_ext:"
>>>>>>>>> +                           "invalid compression type %d",
>>>>>>>>> +                           QCOW2_COMPRESSION_TYPE_ZLIB);
>>>>>>>>> +            }
>>>>>>>>
>>>>>>>> This is a restriction that the spec doesn't make, so strictly speaking
>>>>>>>> this implementation wouldn't be compliant to the spec.
>>>>>>> The idea is that ZLIB shouldn't appear in the compression type
>>>>>>> extension. This allows image backward compatibility with an older qemu
>>>>>>> if zlib is used.
>>>>>>>
>>>>>>> There is no reason to set ZLIB in the extension because an older qemu
>>>>>>> knows how to tread ZLIB compressed clusters.
>>>>>>>
>>>>>>> The restriction aims to guarantee that.
>>>>>>>
>>>>>>> I tried to describe this case in the specification:
>>>>>>> ...
>>>>>>> When the compression type bit is not set, and the compression type
>>>>>>> header extension is absent, ZLIB compression is used for compressed
>>>>>>> clusters.
>>>>>>>
>>>>>>> Qemu versions older than 4.1 can use images created with compression
>>>>>>> type ZLIB without any additional preparations and cannot use images
>>>>>>> created with compression types != ZLIB.
>>>>>>> ...
>>>>>>>
>>>>>>> Does it makes sense?
>>>>>>
>>>>>> This text says that using zlib in the extension is not necessary because
>>>>>> it's the default. But it doesn't say that using zlib in the extension is
>>>>>> illegal.
>>>>>>
>>>>>> I agree that there is no good reason to create a compression type
>>>>>> extension if you have zlib. But is there a good reason to forbid it?
>>>>> I think yes, if we create image with the extension set to zlib we
>>>>> prevent an older qemu from using that image. Furthermore, to allow older
>>>>> qemu using such images we need to create special conversion procedure
>>>>> which has to remove the extension header.
>>>>>
>>>>> If zlib is a "special compression type" which is always set by default
>>>>> without the extension header we'll get rid of such image conversion
>>>>> procedure and an older qemu could use it "as is"
>>>>>
>>>>> Might it work as a good reason?
>>>>>
>>>>>> It
>>>>>> only requires us to add artificial restrictions to code that would work
>>>>>> fine without them.
>>>>>>
>>>>>> Either way, if we want to reject such extensions, the spec needs to say
>>>>>> that it's illegal. And if the spec allows such images, we must accept
>>>>>> them.
>>>>> Yes, it's true
>>>>>
>>>>> The only reasons that zlib compression type even exists in the
>>>>> enumeration is to avoid ambiguity for users.
>>>>> For them it may be hard to understand why they can set zstd and cannot
>>>>> set zlib as compression type and to really set zlib they have to set no
>>>>> compression type to make the default zlib to apply.
>>>>>
>>>>> When a user set zlib as compression type the image is created as before
>>>>> the extension header were introduced.
>>>>>
>>>>> Reasonable?
>>>>>>
>>>>>>>> We can discuss whether the code or the spec should be changed. At the
>>>>>>>> moment, I don't see a good reason to make the restriction
>>>>>>>>
>>>>>>>>> +#ifdef DEBUG_EXT
>>>>>>>>> +            printf("Qcow2: image compression type %s\n", 
>>>>>>>>> s->compression_type);
>>>>>>>>> +#endif
>>>>>>>>> +            break;
>>>>>>>>>      
>>>>>>>>>              case QCOW2_EXT_MAGIC_DATA_FILE:
>>>>>>>>>              {
>>>>>>>>
>>>>>>>> We would save most of this code if we added a new field to the header
>>>>>>>> instead of adding a header extension. Not saying that we should
>>>>>>>> definitely do this, but let's discuss it at least.
>>>>>>>
>>>>>>> If we add the new field to the header will the older qemu be able to use
>>>>>>> it. Or we will add the header only if needed, i.e. if compression_type
>>>>>>> != zlib
>>>>>>
>>>>>> Increasing the header size is backwards compatible. Older qemu versions
>>>>>> should handle such images correctly. They would store the unknown part
>>>>>> of the header in s->unknown_header_fields and keep it unmodified when
>>>>>> updating the image header.
>>>>>>
>>>>>> We would still add the incompatible feature flag for non-zlib, of
>>>>>> course.
>>>>> so, we basically need to do the same: store compression type and forbid
>>>>> to use because of flag if not zlib.
>>>>>
>>>>> Sounds like it doesn't differ that much from the extension header 
>>>>> approach.
>>>>
>>>> It provides more or less the same functionality, but would probably make
>>>> this patch half the size because all of the code related to reading and
>>>> checking the header extension would go away. It also saves a few bytes
>>>> in the header cluster (4 bytes vs. 16 bytes).
>>> ok, will re-do it that way.
>>>
>>> Do you agree in general with how zlib compression type is treated?
>>
>> As I said, I think both ways are justifiable as long as we stay
>> consistent between qemu and spec.
>>
>> I'd prefer to allow zlib in the extension, you'd prefer to forbid it.
>> So I'd like to hear opinions from some more people on which way they
>> prefer.
> 
> I don’t think it’s any better to completely forbid it than to just
> recommend in the spec that software should not set this field to zlib to
> ensure backwards compatibility.
> 
> I see the point of forbidding it, but if I were to know nothing of qcow2
> and read the spec, I guess I’d find it a bit weird to read “If this
> field is not present, the compression type is zlib; if it is, it is not
> zlib, but the specified value.”  I’d ask myself why it isn’t simply “The
> compression type is given by this field, it defaults to zlib.”
> 
As I understood, if we go with the header extending we'll add a new 
field in the header anyway which always hold some value. By, default it 
will be zlib.
So, the only question now is whether we allow zlib value with the 
incompatible feature flag set. My point is if compression type is zlib 
the corresponding incompatible feature flag should NOT be set.

In that case an older qemu will put the compression type to 
s->unknown_header_fields and run with zlib, otherwise it couldn't open 
the file.

Denis
> Max
> 

-- 
Best,
Denis