Re: [PATCH v2 03/12] block/vmdk: Change extent info type

[Kevin Wolf]

Am 28.01.2023 um 00:06 hat Eric Blake geschrieben:
> On Thu, Jan 19, 2023 at 04:20:16PM +0100, Kevin Wolf wrote:
> > Am 20.06.2022 um 18:26 hat Hanna Reitz geschrieben:
> > > VMDK's implementation of .bdrv_get_specific_info() returns information
> > > about its extent files, ostensibly in the form of ImageInfo objects.
> > > However, it does not get this information through
> > > bdrv_query_image_info(), but fills only a select few fields with custom
> > > information that does not always match the fields' purposes.
> > > 
> > > For example, @format, which is supposed to be a block driver name, is
> > > filled with the extent type, e.g. SPARSE or FLAT.
> > > 
> > > In ImageInfo, @compressed shows whether the data that can be seen in the
> > > image is stored in compressed form or not.  For example, a compressed
> > > qcow2 image will store compressed data in its data file, but when
> > > accessing the qcow2 node, you will see normal data.  This is not how
> > > VMDK uses the @compressed field for its extent files: Instead, it
> > > signifies whether accessing the extent file will yield compressed data
> > > (which the VMDK driver then (de-)compresses).
> > 
> > Actually, VMDK was the only user of the field in ImageInfo. qcow2
> > doesn't set the field at all because it would have to parse all L2
> > tables in order to set it.
> > 
> > So I suppose @compressed can now be removed from ImageInfo?
> 
> I think you are okay for VMDK (the new struct has the same field
> names, but better meanings, and the on-the-wire representation for
> someone querying a known-VMDK image doesn't change).  For non-VMDK,
> any code that was querying @compressed will break, but arguably no one
> was doing that since it would have always been false.  If we want to
> be super-conservative, we deprecate the field now and only remove it
> from ImageInfo in a later release, but I'd rather trust Markus on this
> decision.

It is an optional field and VMDK is the only driver that ever provided
it, and only in the context of extent information. In this case, the
information on the wire stays the same after this patch.

So I don't think there is any visible difference for a client, apart
from schema introspection?

> On a side note - would it be worth adding a bit to the qcow2 header
> (one of the compatible_feature bits seems best) which we set when
> writing a compressed cluster, so that on newer images, it is an O(1)
> probe of whether the image contains (or at least has contained in the
> past) a compressed cluster?  Or is that going to add needless overhead
> for something we really don't need to know all that often?

I think the only use for it would be displaying it in 'qemu-img info'.

Would you combine two bits then? One for "compressed bit is valid" and
one for "contains compressed clusters"? Because with one bit you can
only distinguish "compressed" from "don't know", but there wouldn't be a
way to say for certain that an image is uncompressed.

Kevin