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

[John Snow]

On 12/24/2015 05:00 AM, Vladimir Sementsov-Ogievskiy wrote:
> On 24.12.2015 02:41, Eric Blake wrote:
>> 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 don't understand.. No. Bitmap data occupies bitmap_table_size
> clusters. The last one may have some meaningless remaining bits. If
> bitmap_table_size = 1, than bitmap data is stored in "exactly 1"
> cluster. Bitmap table is like page table.
> 

Eric is referring to earlier in the spec where you state:

"bitmap_table_size" "Number of entries in the Bitmap Table of the bitmap."

But later on, it appears as if "bitmap_table_size" refers to a number of
clusters:

"As noted above, bitmap data is stored in several (or may be one,
exactly bitmap_table_size) separate clusters"

Here, one may read "bitmap_table_size" to be referring to a cluster
count -- which is only indirectly true.


I think what is meant is this:

- bitmap_table_size refers to the number of bitmap table entries.
- each bitmap table entry indicates a cluster's worth of data.
- "bitmap_table_size := 0x01" implies eight bytes for the header, but an
entire cluster for data.

So "indirectly," bitmap_table_size refers to the number of clusters that
contain bitmap data, but to be accurately precise, it actually refers to
the number of bitmap table entries.

Correct?

>>
>> 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?
>>
> 
>