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

[Fam Zheng]

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.

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

Fam