From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53017) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bqLMy-0001Q3-Gr for qemu-devel@nongnu.org; Sat, 01 Oct 2016 10:34:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bqLMv-000721-TS for qemu-devel@nongnu.org; Sat, 01 Oct 2016 10:34:27 -0400 References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-6-git-send-email-vsementsov@virtuozzo.com> From: Max Reitz Message-ID: Date: Sat, 1 Oct 2016 16:34:13 +0200 MIME-Version: 1.0 In-Reply-To: <1475232808-4852-6-git-send-email-vsementsov@virtuozzo.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ABV3hPkeFgTEcLnud1khRGWOEkDSPn4CT" Subject: Re: [Qemu-devel] [PATCH 05/22] qcow2-bitmap: structs and consts List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --ABV3hPkeFgTEcLnud1khRGWOEkDSPn4CT From: Max Reitz To: Vladimir Sementsov-Ogievskiy , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com Message-ID: Subject: Re: [PATCH 05/22] qcow2-bitmap: structs and consts References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-6-git-send-email-vsementsov@virtuozzo.com> In-Reply-To: <1475232808-4852-6-git-send-email-vsementsov@virtuozzo.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: > Create block/qcow2-bitmap.c > Add data structures and constraints accordingly to docs/specs/qcow2.txt= >=20 > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/Makefile.objs | 2 +- > block/qcow2-bitmap.c | 47 ++++++++++++++++++++++++++++++++++++++++++++= +++ > block/qcow2.h | 29 +++++++++++++++++++++++++++++ > 3 files changed, 77 insertions(+), 1 deletion(-) > create mode 100644 block/qcow2-bitmap.c >=20 > diff --git a/block/Makefile.objs b/block/Makefile.objs > index fa4d8b8..0f661bb 100644 > --- a/block/Makefile.objs > +++ b/block/Makefile.objs > @@ -1,5 +1,5 @@ > block-obj-y +=3D raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o v= vfat.o dmg.o > -block-obj-y +=3D qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapsh= ot.o qcow2-cache.o > +block-obj-y +=3D qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapsh= ot.o qcow2-cache.o qcow2-bitmap.o > block-obj-y +=3D qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-clus= ter.o > block-obj-y +=3D qed-check.o > block-obj-$(CONFIG_VHDX) +=3D vhdx.o vhdx-endian.o vhdx-log.o > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > new file mode 100644 > index 0000000..cd18b07 > --- /dev/null > +++ b/block/qcow2-bitmap.c > @@ -0,0 +1,47 @@ > +/* > + * Bitmaps for the QCOW version 2 format > + * > + * Copyright (c) 2014-2016 Vladimir Sementsov-Ogievskiy > + * > + * This file is derived from qcow2-snapshot.c, original copyright: > + * Copyright (c) 2004-2006 Fabrice Bellard > + * > + * Permission is hereby granted, free of charge, to any person obtaini= ng a copy > + * of this software and associated documentation files (the "Software"= ), to deal > + * in the Software without restriction, including without limitation t= he rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/o= r sell > + * copies of the Software, and to permit persons to whom the Software = is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be incl= uded in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXP= RESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABI= LITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT S= HALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES O= R OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARI= SING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALI= NGS IN > + * THE SOFTWARE. > + */ > + > +/* NOTICE: BME here means Bitmaps Extension and used as a namespace fo= r > + * _internal_ constants. Please do not use this _internal_ abbreviatio= n for > + * other needs and/or outside of this file. */ > + > +/* Bitmap directory entry constraints */ > +#define BME_MAX_TABLE_SIZE 0x8000000 > +#define BME_MAX_PHYS_SIZE 0x20000000 /* 512 mb */ I suppose BME_MAX_TABLE_SIZE (8M) is greater than BME_MAX_PHYS_SIZE (512 MB) divided by the cluster size (>=3D 512; 512 MB / cluster_size <=3D 1 M= B) because fully zero or one clusters do not require any physical space? Makes some sense, but I can see that this might make give some trouble when trying to serialize overly large bitmaps. But I guess that comes later in this series, so I'll wait for that point. Another thing is that 512 MB is rather big. It gets worse: The bitmap may only require 512 MB on disk, but with a maximum table size of 8 MB, it can require up to 8M * cluster_size in memory (with just 64 MB of disk space!) by using the "read as all zeroes" or "read as all ones" flags. With the default cluster size of 64 kB, this would be 512 GB in RAM. That sounds bad to me. Well, it is probably fine as long as the bitmap is not auto-loaded... But we do have a flag for exactly that. So it seems to me that a manipulated image can easily consume huge amounts of RAM on the host. So I think we also need some sane limitation on the in-RAM size of a bitmap (which is BME_MAX_TABLE_SIZE * cluster_size, as far as I understand). The question of course is, what is sane? For a server system with no image manipulation possible from the outside, 1 GB may be completely fine. But imagine you download some qcow2 image to your laptop. Then, 1 GB may not be fine, actually. Maybe it would make sense to use a runtime-adjustable limit here? > +#define BME_MAX_GRANULARITY_BITS 31 > +#define BME_MIN_GRANULARITY_BITS 9 > +#define BME_MAX_NAME_SIZE 1023 > + > +/* Bitmap directory entry flags */ > +#define BME_RESERVED_FLAGS 0xffffffff > + > +/* bits [1, 8] U [56, 63] are reserved */ > +#define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001fe ull suffix is missing. > + > +typedef enum BitmapType { > + BT_DIRTY_TRACKING_BITMAP =3D 1 > +} BitmapType; > diff --git a/block/qcow2.h b/block/qcow2.h > index b36a7bf..0480b8b 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -52,6 +52,10 @@ > * space for snapshot names and IDs */ > #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS) > =20 > +/* Bitmap header extension constraints */ > +#define QCOW_MAX_DIRTY_BITMAPS 65535 > +#define QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE (1024 * QCOW_MAX_DIRTY_BI= TMAPS) > + > /* indicate that the refcount of the referenced cluster is exactly one= =2E */ > #define QCOW_OFLAG_COPIED (1ULL << 63) > /* indicate that the cluster is compressed (they never have the copied= flag) */ > @@ -142,6 +146,22 @@ typedef struct QEMU_PACKED QCowSnapshotHeader { > /* name follows */ > } QCowSnapshotHeader; > =20 > +/* Qcow2BitmapDirEntry is actually a bitmap directory entry */ This comment doesn't make a whole lot of sense now that this structure is indeed called what it "actually is". ;-) > +typedef struct QEMU_PACKED Qcow2BitmapDirEntry { > + /* header is 8 byte aligned */ > + uint64_t bitmap_table_offset; > + > + uint32_t bitmap_table_size; > + uint32_t flags; > + > + uint8_t type; > + uint8_t granularity_bits; > + uint16_t name_size; > + uint32_t extra_data_size; > + /* extra data follows */ > + /* name follows */ > +} Qcow2BitmapDirEntry; > + > typedef struct QEMU_PACKED QCowSnapshotExtraData { > uint64_t vm_state_size_large; > uint64_t disk_size; > @@ -222,6 +242,15 @@ typedef uint64_t Qcow2GetRefcountFunc(const void *= refcount_array, > typedef void Qcow2SetRefcountFunc(void *refcount_array, > uint64_t index, uint64_t value); > =20 > +/* Be careful, Qcow2BitmapHeaderExt is not an extension of Qcow2Bitmap= DirEntry, it > + * is Qcow2 header extension */ And this makes even less sense now. (These comments don't stop me from giving an R-b, but I'm not so sure about the constants...) Max > +typedef struct Qcow2BitmapHeaderExt { > + uint32_t nb_bitmaps; > + uint32_t reserved32; > + uint64_t bitmap_directory_size; > + uint64_t bitmap_directory_offset; > +} QEMU_PACKED Qcow2BitmapHeaderExt; > + > typedef struct BDRVQcow2State { > int cluster_bits; > int cluster_size; >=20 --ABV3hPkeFgTEcLnud1khRGWOEkDSPn4CT Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEvBAEBCAAZBQJX78llEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRD0B9sAYdXPQLvV CAC8zlR7+i43OdqgQzv+2qWXto4eSJVf6JN6V/SZxdaHYF3SuG+ZBnLvAr23KsHV EIVBMb9CR45cJQtfbnqfEQ2POarv6A4wKpzxTVH6KlNhRaYXA51UgPPD9ide7Hwn ALG/7660etlHypq52Mw2g+vvfKFL/g8wc96vFUqXM2tD2It12LJdt4+nEwfoYENj jOw2fH1og4hRhp409KPj20Mz5Y4qnfNwk+UkA6kaJIdEZUevtZcDZU4D2r4TuRdp vicGLh2GcNqEuQBmXkDA0k2jmtrYpe7QxgPGylnMFnOGFHKO7fCoQf7fy8sicBpr UtADxX3FJz0o0INJLTPLJonj =MyXa -----END PGP SIGNATURE----- --ABV3hPkeFgTEcLnud1khRGWOEkDSPn4CT--