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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

On 03.02.2016 17:41, Fam Zheng wrote:
> On Wed, 02/03 16:45, Vladimir Sementsov-Ogievskiy wrote:
>> Also current scheme is made like one for snapshots.
> Okay, then I'll be fine with being consistent.
>
>
>>>> +
>>>> +
>>>> +=== Bitmap table ===
>>>> +
>>>> +Bitmaps are stored using a one-level structure (as opposed to two-level
>>>> +structure like for refcounts and guest clusters mapping) for the mapping of
>>> s/structure/structures/
>>>
>>>> +bitmap data to host clusters. This structure is called the bitmap table.
>>>> +
>>>> +Each bitmap table has a variable size (stored in the bitmap directory entry)
>>>> +and may use multiple clusters, however, it must be contiguous in the image
>>>> +file.
>>>> +
>>>> +Structure of a bitmap table entry:
>>>> +
>>>> +    Bit       0:    Reserved and must be zero if bits 9 - 55 are non-zero.
>>>> +                    If bits 9 - 55 are zero:
>>>> +                      0: Cluster should be read as all zeros.
>>>> +                      1: Cluster should be read as all ones.
>>> Once bits 9 - 55 are non-zero, this bit goes useless? That doesn't make much
>>> sense to me. In which case bit 0 is set but 9-55 are zero?
>> In case "1: Cluster should be read as all ones.".
> I cannot think of a use case leading to this.

Why not? It is the dirty bitmap. It may be very dirty, it even may be 
all-ones.

>
>>>> +
>>>> +If the size of the bitmap data is not a multiple of the cluster size then the
>>>> +last cluster of the bitmap data contains some unused tail bits. These bits must
>>>> +be zero.
>>> What defines the size of the bitmap data?
>> bitmap size === virtual disk size.
> okay.
>
>>>> +
>>>> +
>>>> +=== Dirty tracking bitmaps ===
>>>> +
>>>> +Bitmaps with 'type' field equal to one are dirty tracking bitmaps.
>>>> +
>>>> +When the virtual disk is in use dirty tracking bitmap may be 'enabled' or
>>>> +'disabled'.
>>>> While the bitmap is 'enabled', all writes to the virtual disk
>>>> +should be reflected in the bitmap. A set bit in the bitmap means that the
>>>> +corresponding range of the virtual disk (see above) was written to while the
>>>> +bitmap was 'enabled'. An unset bit means that this range was not written to.
>>>> +
>>>> +The software should not sync the bitmap in the image file with its
>>>> +representation in RAM after each write. Flag 'in_use' should be set while the
>>>> +bitmap is not synced.
>>> I think this is an implementation detail. IMO a software *can* keep the bitmap
>>> synced, "should not" is an obsecure and unnecessary constraint.
>> s/should not/doesn't have to/, ok?
> yes, that's fine.
>
>>>> +
>>>> +In the image file the 'enabled' state is reflected by the 'auto' flag. If this
>>>> +flag is set, the software must consider the bitmap as 'enabled' and start
>>>> +tracking virtual disk changes to this bitmap from the first write to the
>>>> +virtual disk. If this flag is not set then the bitmap is disabled.
>>>> +
>>>> +To maintain bitmap consistency, the only software which is allowed to change
>>>> +the value of the 'auto' flag is the one which has created the bitmap.
>>> How does one software know if the image is created by it or not?
>> I understand that this is not very good point for spec.. I can drop
>> it. The idea is that "change this flag, do some writes, change it
>> back" may bring great damage to backup tool, which was created that
>> bitmap.
> I think the only reason to switch the 'auto' flag is discarding the bitmap
> data, no?

Hmm, may be.. Ok lets drop this paranoic last paragraph. With the same 
logic I can add something like "to maintain bitmap consistency, the only 
software which is allowed to clear bits in it..."..

>
> Fam


-- 
Best regards,
Vladimir