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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8537 bytes --]

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 <dplotnikov@virtuozzo.com>
>>>>>>>> ---
>>>>>>>>     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.”

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]