From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45434) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGDVv-0005Jg-Hl for qemu-devel@nongnu.org; Mon, 04 Jan 2016 17:22:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGDVr-0005UR-D0 for qemu-devel@nongnu.org; Mon, 04 Jan 2016 17:22:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58394) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGDVr-0005UJ-2Z for qemu-devel@nongnu.org; Mon, 04 Jan 2016 17:22:03 -0500 References: <1450892961-6495-1-git-send-email-vsementsov@virtuozzo.com> From: Max Reitz Message-ID: <568AF085.7060905@redhat.com> Date: Mon, 4 Jan 2016 23:21:57 +0100 MIME-Version: 1.0 In-Reply-To: <1450892961-6495-1-git-send-email-vsementsov@virtuozzo.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="t3ENWu14t1opmFLuaNkUhif6VLOxQeeFw" Subject: Re: [Qemu-devel] [PATCH v6] spec: add qcow2 bitmaps extension specification List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, stefanha@redhat.com, den@openvz.org, jsnow@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --t3ENWu14t1opmFLuaNkUhif6VLOxQeeFw Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 23.12.2015 18:49, Vladimir Sementsov-Ogievskiy wrote: > The new feature for qcow2: storing bitmaps. >=20 > 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. >=20 > 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. >=20 > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- 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(-) >=20 > 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. > =20 > - Bits 0-63: Reserved (set to 0) > + Bit 0: Bitmaps extension bit. > + This bit is responsible for Bitmaps ex= tension > + consistency. > + > + If it is set, but there is no Bitmaps > + extension, this should be considered a= s 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 Bitma= ps > + extension, its data should be consider= ed as > + inconsistent. Same here. > + > + Bits 1-63: Reserved (set to 0) > =20 > 96 - 99: refcount_order > Describes the width of a reference count block ent= ry (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 > =20 > @@ -166,6 +179,34 @@ the header extension data. Each entry look like th= is: > terminated if it has full length) > =20 > =20 > +=3D=3D Bitmaps extension =3D=3D > + > +Bitmaps extension is an optional header extension. It provides an abil= ity 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 ab= ove). > + > +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 bitm= aps per > + image. > + > + 4 - 7: bitmap_directory_size > + Size of the Bitmap Directory in bytes. It is a cumu= lative "...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 Dire= ctory > + starts. Must be aligned to a cluster boundary. > + > + > =3D=3D Host cluster management =3D=3D > =20 > qcow2 manages the allocation of host clusters by maintaining a referen= ce count > @@ -360,3 +401,121 @@ Snapshot table entry: > =20 > variable: Padding to round up the snapshot table entry size = to the > next multiple of 8. > + > + > +=3D=3D Bitmaps =3D=3D > + > +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. > + > +=3D=3D=3D Bitmap Directory =3D=3D=3D > + > +Each bitmap saved in the image is described in a Bitmap Directory entr= y. Bitmap > +Directory is a contiguous area in the image file, whose starting offse= t and "... The Bitmap Directory is..." > +length are given by the header extension fields bitmap_directory_offse= t 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 variab= le > +length, depending on the length of the bitmap name and extra data. The= se > +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 Tab= le > + (described below) for the bitmap starts. Must be a= ligned to > + a cluster boundary. > + > + 8 - 11: bitmap_table_size > + Number of entries in the Bitmap Table of the bitma= p. > + > + 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 vi= rtual > + disk by any application that would write to t= his qcow2 > + file (including writes, snapshot switching, e= tc.). The > + type of this bitmap must be 'Dirty Tracking B= itmap'. > + > + 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_b= its > + greater than 31. > + > + Granularity is calculated as > + granularity =3D 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_s= ize 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 e= xtension. > + > + variable: Padding to round up the Bitmap Directory Entry siz= e 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). > + > +=3D=3D=3D Bitmap Table =3D=3D=3D > + > +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 cl= usters. 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 im= age 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-z= ero. > + 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 aligne= d 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. > + > +=3D=3D=3D Bitmap Data =3D=3D=3D 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, exact= ly 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 =3D > + bitmap_table[bitmap_data_offset / cluster_size] + > + (bitmap_data_offset % cluster_size) > + > +Taking into account the granularity of the bitmap, an offset in bits i= nto the > +image file, corresponding to byte number byte_nr of the virtual disk c= an 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 =3D > + image_offset(byte_nr / granularity / 8) * 8 + > + (byte_nr / granularity) % 8 >=20 Max --t3ENWu14t1opmFLuaNkUhif6VLOxQeeFw Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWivCFAAoJEDuxQgLoOKytfBsH/RIIl5sO7Blq3658QZnVm0+r /LXx24BGpKTPhTpS48r3mijw6YPxYRN+Dzkjq6VSmPpCeaJ6xeBP/roLOFsItKoj jld886rgyU2gTMnTePkMVZJPEukT8/aMbG40bpRmog110euSeJ+/A9FuaMXaO2dz 7SYwzmQbu5Q3BiiJWmIQMe9BFn/I4g2aRHYVC5ln2wCpqDQGtajx6PL+qwk8G/5Z iYaN8WgeVpFWsOcxeKtOIGlpTPvlvPF+ZvYY1+6Vc/B5IAldg44prIrpb+mwBYih 8uW0bbCvAQEHXbZTjfzGyS5hOp+yWguBFD1rugj1g0ncs7+hoczCQOAGeHKJfvk= =eZqs -----END PGP SIGNATURE----- --t3ENWu14t1opmFLuaNkUhif6VLOxQeeFw--