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

[Max Reitz]

On 23.12.2015 18:49, 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>
> ---

OK, keeping my eyes open for grammar etc.; here goes nothing:

Generally, most of the qcow2 specification does not consider structure
names to be proper nouns, that is, they are generally written starting
with lower letters (e.g. "refcount table", "L2 table", "image header").

I'd prefer this spelling for all the structures presented herein (e.g.
"bitmaps extension", "bitmap directory", "dirty tracking bitmap", ...),
too, but that is probably a personal preference.

>  docs/specs/qcow2.txt | 161 
> ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 160 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
> index 121dfc8..b23966a 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.

Maybe correct (this is why a native speaker should be doing this...),
but I'd omit the "as" (i.e. just "this should be considered an error").

> +
> +                                If it is not set, but there is a Bitmaps
> +                                extension, its data should be considered as
> +                                inconsistent.

Same here.

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

"The Bitmaps extension..."

Also, while it may be correct as it is, "provides the ability" sounds
better to me.

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

I'd prefer "The Dirty Tracking Bitmap" or "Dirty Tracking Bitmaps".

> +changes from some moment.

I think, "moment" is rather used for a time span in English. So this
should be "point in time" instead.

(Maybe even "a certain point in time" rather than "some point in time",
but that would be a semantic change.)

> +
> +The data of the extension should be considered as consistent only if

Again, the "as" can be dropped (and maybe it should be).

Also, it should be "...only if the corresponding..." ("the" missing).

> +corresponding auto-clear feature bit is set (see autoclear_features above).
> +
> +The fields of Bitmaps extension are:

"...of the Bitmaps..."

> +
> +          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 is a cumulative

"...It is the cumulative size..."

> +                   size of all (nb_bitmaps) bitmap headers.
> +
> +          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,121 @@ 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

Maybe "This feature supports...".

> +to the virtual disk, stored in this image.

The comma should be omitted.

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

"... The Bitmap Directory is..."

> +length are given by the header extension fields bitmap_directory_offset and

"bitmap header extension fields" may be clearer, but that can be parsed
both as "(bitmap (header extension)) fields" and "((bitmap header)
extension) fields"; the first is what we want (the qcow2 header
extension for bitmaps), the second would be an extension for a bitmap
directory entry (which are "bitmap headers"), sooo... Probably it'll be
best leaving this as it is.

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

"entry" should be written starting with a lower letter, and a more
verbose "Structure of a Bitmap Directory entry" may be nicer.

> +
> +    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:
> +                      1: Dirty Tracking Bitmap
> +
> +                    Values 0, 2 - 255 are reserved.
> +
> +             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.

I'd prefer "A bitmap's granularity" or at least "The granularity of the
bitmap".

> +
> +        18 - 19:    name_size
> +                    Size of the bitmap name. Valid values: 1 - 1023.
> +
> +        20 - 23:    extra_data_size
> +                    Size of type-specific extra data.
> +
> +                    For now, as no extra data is defined, extra_data_size is
> +                    reserved and must be zero.
> +
> +        variable:   Type-specific extra data for the bitmap.
> +
> +        variable:   The name of the bitmap (not null terminated). Must be
> +                    unique among all bitmap names within the Bitmaps 
> extension.
> +
> +        variable:   Padding to round up the Bitmap Directory Entry size to 
> the
> +                    next multiple of 8.

"...to round up this entry's size..." would be shorter. In any case,
"Entry" should be written with a lower letter, I think (but frankly, I
think that even "Bitmap Directory" should be written in lowercase, so
make of that what you wish).

> +
> +=== 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.

The sentence in the parantheses may be correct, but it sounds weird.
Maybe "as opposed to two-level like for refcounts and..." would be
better (but the plain "two-level" still sounds strange).

Also, probably s/bitmaps data/bitmap data/.

> +It is called Bitmap Table.

I'd prefer either "...to host clusters, which is called a Bitmap Table."
or "This structure is called a 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.

I'd put a comma after the "however", too.

> +
> +Bitmap Table entry:

Again, a more verbose "Structure of a Bitmap Table entry" might be
nicer, but this is completely optional.

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

"...of the host cluster offset."

> +                    cluster boundary. If the offset is 0, the cluster is
> +                    unallocated, see bit 0 description.

"..., the custer is unallocated; in that case, see the description
of/for bit 0.",

or:

"..., the cluster is unallocated; in that case, bit 0 determines how
this cluster should be treated when read from."

> +
> +        56 - 63:    Reserved and must be zero.
> +
> +=== Bitmap Data ===

As you never use this as a proper noun anywhere, I think it's safe to
make this a "Bitmap data".

> +
> +As noted above, bitmap data is stored in several (or may be one, exactly

s/may be/maybe/

Alternatively, the common phrasing is: "...is stored in one or more
(bitmap_table_size) separate clusters...".

> +bitmap_table_size) separate clusters, described by Bitmap Table. Given an

"..., as described by the Bitmap Table."

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

Probably the following is better:

Given an offset byte_nr into the virtual disk and the bitmap's
granularity, the bit offset into the bitmap can be calculated like this:

> +
> +    bit_offset =
> +        image_offset(byte_nr / granularity / 8) * 8 +
> +            (byte_nr / granularity) % 8
> 

Max

signature.asc
Description: OpenPGP digital signature