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 <vsement...@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 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
