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

[Denis Plotnikov]

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?

Denis
> 
> Kevin
> 

-- 
Best,
Denis