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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 10220 bytes --]

On 21.12.2015 16:25, Vladimir Sementsov-Ogievskiy wrote:
> The new feature for qcow2: storing bitmaps.
> 
> Only bitmaps, relative to the virtual disk, stored in qcow2 file, should
> be stored in this qcow2 file.
> 
> Strings started from +# are RFC-strings, not to be commited of course
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> 
> v5:
> 
> - 'Dirty bitmaps' renamed to 'Bitmaps', as we may have several types of
>   bitmaps.
> - rewordings
> - move upper bounds to "Notes about Qemu limits"
> - s/should/must somewhere. (but not everywhere)
> - move name_size field closer to name itself in bitmap header
> - add extra data area to bitmap header
> - move bitmap data description to separate section
> 
> 
> 
>  docs/specs/qcow2.txt | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 159 insertions(+), 1 deletion(-)

Looks good! :-)

Some comments below, but I think the general design is good now.

> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
> index 121dfc8..3d557ee 100644
> --- a/docs/specs/qcow2.txt
> +++ b/docs/specs/qcow2.txt
> @@ -103,7 +103,19 @@ in the description of a field.
>                      write to an image with unknown auto-clear features if it
>                      clears the respective bits from this field first.
>  
> -                    Bits 0-63:  Reserved (set to 0)
> +                    Bit 0:      Bitmaps extension bit.
> +                                This bit is responsible for Bitmaps extension
> +                                consistency.
> +
> +                                If it is set, but there is no Bitmaps
> +                                extension, this should be considered as an
> +                                error.
> +
> +                                If it is not set, but there is a Bitmaps
> +                                extension, its data should be considered as
> +                                inconsistent.
> +
> +                    Bits 1-63:  Reserved (set to 0)
>  
>           96 -  99:  refcount_order
>                      Describes the width of a reference count block entry (width
> @@ -123,6 +135,7 @@ be stored. Each extension has a structure like the following:
>                          0x00000000 - End of the header extension area
>                          0xE2792ACA - Backing file format name
>                          0x6803f857 - Feature name table
> +                        0x23852875 - Bitmaps extension
>                          other      - Unknown header extension, can be safely
>                                       ignored
>  
> @@ -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.
> +
> +The data of the extension should be considered as consistent only if
> +corresponding auto-clear feature bit is set (see autoclear_features above).
> +
> +The fields of Bitmaps extension are:
> +
> +          0 -  3:  nb_bitmaps
> +                   The number of bitmaps contained in the image. Must be
> +                   greater or equal to 1.
> +
> +                   Note: Qemu currently only supports up to 65535 bitmaps per
> +                   image.
> +
> +          4 -  7:  bitmap_directory_size
> +                   Size of the Bitmap Directory in bytes. It must be equal to
> +                   sum of sizes of all (nb_bitmaps) bitmap headers.

I'd rather write this as: "Size of the Bitmap Directory in bytes, i.e.
the cumulative size of all (nb_bitmaps) bitmap headers."

("It must" sounds like it's an additional restriction while it's
actually just an explanation.)

> +
> +          8 - 15:  bitmap_directory_offset
> +                   Offset into the image file at which the Bitmap Directory
> +                   starts. Must be aligned to a cluster boundary.
> +
> +
>  == Host cluster management ==
>  
>  qcow2 manages the allocation of host clusters by maintaining a reference count
> @@ -360,3 +401,120 @@ Snapshot table entry:
>  
>          variable:   Padding to round up the snapshot table entry size to the
>                      next multiple of 8.
> +
> +
> +== Bitmaps ==
> +
> +The feature supports storing bitmaps in a qcow2 image. All bitmaps are related
> +to the virtual disk, stored in this image.
> +
> +=== Bitmap Directory ===
> +
> +Each bitmap saved in the image is described in a Bitmap Directory entry. Bitmap
> +Directory is a contiguous area in the image file, whose starting offset and
> +length are given by the header extension fields bitmap_directory_offset and
> +bitmap_directory_size. The entries of the bitmap directory have variable
> +length, depending on the length of the bitmap name and extra data. These
> +entries are also called bitmap headers.
> +
> +Bitmap Directory Entry:
> +
> +    Byte 0 -  7:    bitmap_table_offset
> +                    Offset into the image file at which the Bitmap Table
> +                    (described below) for the bitmap starts. Must be aligned to
> +                    a cluster boundary.
> +
> +         8 - 11:    bitmap_table_size
> +                    Number of entries in the Bitmap Table of the bitmap.
> +
> +        12 - 15:    flags
> +                    Bit
> +                      0: in_use
> +                         The bitmap was not saved correctly and may be
> +                         inconsistent.
> +
> +                      1: auto
> +                         The bitmap must reflect all changes of the virtual
> +                         disk by any application that would write to this qcow2
> +                         file (including writes, snapshot switching, etc.). The
> +                         type of this bitmap must be 'Dirty Tracking Bitmap'.
> +
> +                    Bits 2 - 31 are reserved and must be 0.
> +
> +             16:    type
> +                    This field describes the sort of the bitmap.
> +                    Values:
> +                      0: Dirty Tracking Bitmap
> +
> +                    Values 1 - 255 are reserved.
> +# Is there any reason, to make it '1 - Dirty Tracking Bitmap, 0,2-255 - reserved?'

Maybe something arbitrary like "But 0 feels like 'unknown' to me!". But
I can't think of anything substantial.

> +
> +             17:    granularity_bits
> +                    Granularity bits. Valid values are: 0 - 63.
> +
> +                    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.
> +
> +        20 - 21:    extra_data_size
> +                    Size of type-specific extra data.
> +# 2 bytes or 4 bytes ?

I'd make it 4. I don't think those additional two bytes will hurt
anybody, and the simple fact that you're asking that question shows to
me that there is a possibility that 2 won't be enough. ;-)

> +
> +        variable:   The name of the bitmap (not null terminated). Must be
> +                    unique among all bitmap names within the Bitmaps extension.
> +
> +        variable:   Type-specific extra data for the bitmap.

I'd swap name and type-specific data so we know the alignment of the
type-specific data (if they are swapped and sizeof(extra_data_size) ==
4, its alignment will be 4). Maybe that'll come in handy at some point.

Maybe you should make a note that currently, no extra data is defined
for Dirty Tracking Bitmaps, but that this may change in the future.

Max

> +        variable:   Padding to round up the Bitmap Directory Entry size to the
> +                    next multiple of 8.
> +
> +=== Bitmap Table ===
> +
> +Bitmaps are stored using a one-level (not two-level like refcounts and guest
> +clusters mapping) structure for the mapping of bitmaps data to host clusters.
> +It is called 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.
> +
> +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.
> +
> +         1 -  8:    Reserved and must be zero.
> +
> +         9 - 55:    Bits 9 - 55 of host cluster offset. Must be aligned to a
> +                    cluster boundary. If the offset is 0, the cluster is
> +                    unallocated, see bit 0 description.
> +
> +        56 - 63:    Reserved and must be zero.
> +
> +=== 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. Given an
> +offset (in bytes) into the bitmap data, the offset into the image file can be
> +obtained as follows:
> +
> +    image_offset =
> +        bitmap_table[bitmap_data_offset / cluster_size] +
> +            (bitmap_data_offset % cluster_size)
> +
> +Taking into account the granularity of the bitmap, an offset in bits into the
> +image file, corresponding to byte number byte_nr of the virtual disk can be
> +calculated like this:
> +
> +    bit_offset =
> +        image_offset(byte_nr / granularity / 8) * 8 +
> +            (byte_nr / granularity) % 8
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]