Re: [Qemu-devel] [PATCH v6] spec: add qcow2 bitmaps extension specification

[Eric Blake]

On 12/23/2015 10:49 AM, Vladimir Sementsov-Ogievskiy wrote:
> The new feature for qcow2: storing bitmaps.
> 
> This patch adds new header extension to qcow2 - Bitmaps Extension. It
> provides an ability to store virtual disk related bitmaps in a qcow2
> image. For now there is only one type of such bitmaps: Dirty Tracking
> Bitmap, which just tracks virtual disk changes from some moment.
> 
> Note: Only bitmaps, relative to the virtual disk, stored in qcow2 file,
> should be stored in this qcow2 file. The size of each bitmap
> (considering its granularity) is equal to virtual disk size.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
> 

> @@ -166,6 +179,34 @@ the header extension data. Each entry look like this:
>                      terminated if it has full length)
>  
>  
> +== Bitmaps extension ==
> +
> +Bitmaps extension is an optional header extension. It provides an ability to
> +store virtual disk related bitmaps in a qcow2 image. For now there is only 
> one
> +type of such bitmaps: Dirty Tracking Bitmap, which just tracks virtual disk
> +changes from some moment.

This is already the qcow2 spec, so 'in a qcow2 image' may be redundant.
 Possible idea for nicer grammar:

It provides the ability to store bitmaps related to a virtual disk.  For
now, there is only one bitmap type: Dirty Tracking Bitmap, which tracks
virtual disk changes from some moment.


> +             17:    granularity_bits
> +                    Granularity bits. Valid values are: 0 - 63.

Elsewhere, the file has 'valid values: 0-63'; dropping 'are' would make
this more consistent.

> +
> +                    Note: Qemu currently doesn't support granularity_bits
> +                    greater than 31.
> +
> +                    Granularity is calculated as
> +                        granularity = 1 << granularity_bits
> +
> +                    Granularity of the bitmap is how many bytes of the image
> +                    accounts for one bit of the bitmap.
> +
> +        18 - 19:    name_size
> +                    Size of the bitmap name. Valid values: 1 - 1023.

Should this be more like:
Must be non-zero. Note: Qemu currently doesn't support values greater
than 1023.


> +=== Bitmap Data ===
> +
> +As noted above, bitmap data is stored in several (or may be one, exactly
> +bitmap_table_size) separate clusters, described by Bitmap Table.

bitmap_table_size was documented as "Number of entries in the Bitmap
Table of the bitmap", where each entry is 8 bytes.  But this sounds like
bitmap_table_size == 1 implies that the table is exactly 1 cluster (at
least 512 bytes).  I think you are trying to imply that the bitmap data
occupies ceil(cluster size / 8 / bitmap_tablesize) clusters.

I also wonder if you need more text to cover what happens when the
number of entries does not end on a cluster boundary.  Must the
remaining bits of the cluster containing the tail of the Bitmap be set
to all 0, or is it garbage that must be ignored regardless of content?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature