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

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

On 05.01.2016 02:16, John Snow wrote:
> Since Max didn't offer a grammatical review, here's my attempt at some
> suggestions.
>
> On 12/23/2015 12:49 PM, 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 <vsementsov@virtuozzo.com>
>> ---
>>
>> v6:
>>
>> - reword bitmap_directory_size description
>> - bitmap type: make 0 reserved
>> - extra_data_size: resize to 4bytes
>>    Also, I've marked this field as "must be zero". We can always change
>>    it, if we decide allowing managing app to specify any extra data, by
>>    defining some magic value as a top of user extra data.. So, for now
>>    non zeor extra_data_size should be considered as an error.
>> - swap name and extra_data to give good alignment to extra_data.
>>
>>
>> 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 | 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.
> For consistency, no period after this.
>
>> +                                This bit is responsible for Bitmaps extension
>> +                                consistency.
>> +
> I might phrase it as: "This bit indicates consistency for the Bitmaps
> extension data."
>
>> +                                If it is set, but there is no Bitmaps
>> +                                extension, this should be considered as an
>> +                                error.
>> +
> "This should be considered as an error" can be shortened to just "This
> is an error." This makes the sentence top-heavy though, so how about:
>
> "It is an error if this bit is set without the Bitmaps extension present."
>
>> +                                If it is not set, but there is a Bitmaps
>> +                                extension, its data should be considered as
>> +                                inconsistent.
>> +
> Let's remove "considered" here. The data *is* inconsistent if this has
> happened.
>
> "If the Bitmaps extension is present but this bit is unset, the Bitmaps
> extension data is 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.
>> +
> I think "Bitmaps extension" is awkward without "The" leading it, so:
>
> "The Bitmaps extension is an optional header extension."
>
> And from Eric's suggestion:
>
> "It provides the ability to store bitmaps related to a virtual disk.
> For now, there is only one bitmap type: Dirty Tracking Bitmap, which
> 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).
>> +
> I might remove the parenthetical.
>
> "The data in this extension should be considered consistent only if the
> corresponding auto-clear feature bit is set, see autoclear_features above."
>
>> +The fields of Bitmaps extension are:
>> +
> Adding 'the' again:
>
> "The fields of the Bitmaps extension are:"
>
>> +          0 -  3:  nb_bitmaps
>> +                   The number of bitmaps contained in the image. Must be
>> +                   greater or equal to 1.
>> +
> "Greater than or equal to" is a common phrasing -- adding the 'than'.
>
>> +                   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
>> +                   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
>> +to the virtual disk, stored in this image.
>> +
> Maybe like Eric's earlier suggestion, we can avoid "in a qcow2 image"
> here. We can probably just repeat some of the earlier description here.
>
>> +=== Bitmap Directory ===
>> +
>> +Each bitmap saved in the image is described in a Bitmap Directory entry. Bitmap
> "The Bitmap Directory"
>
>> +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:
>> +                      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.
>> +
>> +        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.
>> +
> And I guess we don't have any definitions for the "Dirty Tracking" type,
> so this is zero.
>
> Should we add a table of the bitmap types and their corresponding
> definitions for the "Type-specific extra data"?
>
> e.g.
>
> === Bitmap Types ===
>
> 0: Reserved
>     extra_data_size: 0
>
>     This bitmap type is invalid and must not be used.
>
> 1: Dirty Tracking Bitmap
>     extra_data_size: 0

Are you sure? What about creation\last change dates, file links, user 
data, etc?
For now, formally, current "For now, as no extra data is defined, 
extra_data_size is reserved and must be zero." is equal to such table, 
but provides more flexibility for future..

>
>     This bitmap type represents data changed since a point in time
>     and has no extra data.
>
> 2: Lorem Ipsum Bitmap
>     extra_data_size: 4
>
>     Lorem ipsum, dolor sit amet...
>     This bitmap type has four byte of extra data:
>
>     0-1: Identifier
>     2-3: Padding
>
>> +        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.
>> +
>> +=== 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.
>> +
> I might add "the" before Bitmap Table above.
>
>> +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
>>
> That's all the language commentary I have on this. Hopefully not long
> before consensus.
>
> Thank you,
> --John


-- 
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.