Re: [Qemu-devel] [PATCH 1/8] spec: add qcow2-dirty-bitmaps specification

[John Snow]

On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> From: Vladimir Sementsov-Ogievskiy <address@hidden>
> 
> Persistent dirty bitmaps will be saved into qcow2 files. It may be used
> as 'internal' bitmaps (for qcow2 drives) or as 'external' bitmaps for
> other drives (there may be qcow2 file with zero disk size but with
> several dirty bitmaps for other drives).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  docs/specs/qcow2.txt | 66 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
> index 121dfc8..0fffba2 100644
> --- a/docs/specs/qcow2.txt
> +++ b/docs/specs/qcow2.txt
> @@ -123,6 +123,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 - Dirty bitmaps
>                          other      - Unknown header extension, can be safely
>                                       ignored
>  
> @@ -166,6 +167,19 @@ the header extension data. Each entry look like this:
>                      terminated if it has full length)
>  
>  
> +== Dirty bitmaps ==
> +
> +Dirty bitmaps is an optional header extension. It provides a possibility of
> +storing dirty bitmaps in qcow2 image. The fields are:
> +

I would say "It provides the ability to store dirty bitmaps in a qcow2
image." We have more than the possibility now :)

> +          0 -  3:  nb_dirty_bitmaps
> +                   Number of dirty bitmaps contained in the image
> +

Most fields seem to be documented as complete sentences, so maybe "The
number of dirty bitmaps contained in the image." and add the period.

> +          4 - 11:  dirty_bitmaps_offset
> +                   Offset into the image file at which the dirty bitmaps 
> table
> +                   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 +374,55 @@ Snapshot table entry:
>  
>          variable:   Padding to round up the snapshot table entry size to the
>                      next multiple of 8.
> +
> +
> +== Dirty bitmaps ==
> +
> +The feature supports storing several dirty bitmaps in the qcow2 file.

You could drop "several" from this sentence, we support an arbitrary number.

> +
> +=== Cluster mapping ===
> +
> +Dirty bitmaps are stored using a ONE-level structure for the mapping of
> +bitmaps to host clusters. There is only an L1 table.
> +
> +The L1 table has a variable size (stored in the Bitmap table entry) and may
> +use multiple clusters, however it must be contiguous in the image file.
> +
> +Given an offset into the bitmap, the offset into the image file can be
> +obtained as follows:
> +
> +    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
> +
> +L1 table entry:
> +
> +    Bit  0 -  61:   Standard cluster descriptor
> +
> +        62 -  63:   Reserved
> +
> +=== Bitmap table ===
> +
> +A directory of all bitmaps is stored in the bitmap table, a contiguous area 
> in
> +the image file, whose starting offset and length are given by the header 
> fields
> +dirty_bitmaps_offset and nb_dirty_bitmaps. The entries of the bitmap table 
> have
> +variable length, depending on the length of name and extra data.
> +
> +Bitmap table entry:
> +
> +    Byte 0 -  7:    Offset into the image file at which the L1 table for the
> +                    bitmap starts. Must be aligned to a cluster boundary.
> +
> +         8 - 11:    Number of entries in the L1 table of the bitmap
> +
> +        12 - 15:    Bitmap granularity in bytes
> +
> +        16 - 23:    Bitmap size in sectors
> +
> +        24 - 25:    Size of the bitmap name
> +

This is, believe it or not, the first place in code that I am aware of
that actually places a limit on how big a bitmap name can be (64K!) --
we should probably clamp this value to something even lower (1024 is
probably graciously sufficient) and enforce that in the various bitmap
add/create routines.

> +        variable:   The name of the bitmap (not null terminated)
> +
> +        variable:   Padding to round up the bitmap table entry size to the
> +                    next multiple of 8.
> +
> +The fields "size", "granularity" and "name" are corresponding with the fields
> +in struct BdrvDirtyBitmap.
> 

Not yet being intricately familiar with qcow2, this looks good to me.

--js