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

[Denis Plotnikov]

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.

May be I'm missing something. Please correct my if so.

Denis
> 
> Kevin
> 

-- 
Best,
Denis