From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55704) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bqLim-0006bu-Qg for qemu-devel@nongnu.org; Sat, 01 Oct 2016 10:57:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bqLil-0001ef-EL for qemu-devel@nongnu.org; Sat, 01 Oct 2016 10:57:00 -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: <06d5d5d9-e8e0-2246-cf2b-bc151888ff93@redhat.com> Date: Sat, 1 Oct 2016 16:56:46 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="53xhtikaxhQ8hdOS4D9dU2CF2vIfBOFnT" 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) --53xhtikaxhQ8hdOS4D9dU2CF2vIfBOFnT 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: <06d5d5d9-e8e0-2246-cf2b-bc151888ff93@redhat.com> 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: Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 01.10.2016 16:34, Max Reitz wrote: > 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.tx= t >> >> 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 >> >> 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 = vvfat.o dmg.o >> -block-obj-y +=3D qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snaps= hot.o qcow2-cache.o >> +block-obj-y +=3D qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snaps= hot.o qcow2-cache.o qcow2-bitmap.o >> block-obj-y +=3D qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-clu= ster.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 obtain= ing a copy >> + * of this software and associated documentation files (the "Software= "), to deal >> + * in the Software without restriction, including without limitation = the rights >> + * to use, copy, modify, merge, publish, distribute, sublicense, and/= or 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 inc= luded in >> + * all copies or substantial portions of the Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EX= PRESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTAB= ILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT = SHALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES = OR OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, AR= ISING FROM, >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEAL= INGS IN >> + * THE SOFTWARE. >> + */ >> + >> +/* NOTICE: BME here means Bitmaps Extension and used as a namespace f= or >> + * _internal_ constants. Please do not use this _internal_ abbreviati= on 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 */ >=20 > I suppose BME_MAX_TABLE_SIZE (8M) is greater than BME_MAX_PHYS_SIZE (51= 2 > MB) divided by the cluster size (>=3D 512; 512 MB / cluster_size <=3D 1= MB) > because fully zero or one clusters do not require any physical space? >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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 b= e > completely fine. But imagine you download some qcow2 image to your > laptop. Then, 1 GB may not be fine, actually. >=20 > Maybe it would make sense to use a runtime-adjustable limit here? >=20 >> +#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 Oh, and this should probably get a u suffix. Max >> + >> +/* bits [1, 8] U [56, 63] are reserved */ >> +#define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001fe >=20 > ull suffix is missing. >=20 >> + >> +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_B= ITMAPS) >> + >> /* indicate that the refcount of the referenced cluster is exactly on= e. */ >> #define QCOW_OFLAG_COPIED (1ULL << 63) >> /* indicate that the cluster is compressed (they never have the copie= d flag) */ >> @@ -142,6 +146,22 @@ typedef struct QEMU_PACKED QCowSnapshotHeader { >> /* name follows */ >> } QCowSnapshotHeader; >> =20 >> +/* Qcow2BitmapDirEntry is actually a bitmap directory entry */ >=20 > This comment doesn't make a whole lot of sense now that this structure > is indeed called what it "actually is". ;-) >=20 >> +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 Qcow2Bitma= pDirEntry, it >> + * is Qcow2 header extension */ >=20 > And this makes even less sense now. >=20 > (These comments don't stop me from giving an R-b, but I'm not so sure > about the constants...) >=20 > Max >=20 >> +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 --53xhtikaxhQ8hdOS4D9dU2CF2vIfBOFnT Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEvBAEBCAAZBQJX786uEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRD0B9sAYdXPQCtL CACS8tYc+SuKTacRv0Om073u6rOumQ9ecoGotktMGamFBIkhNLTWcu9cbBOWcFP6 I8kkXDd7BRuXeVsHQhLKR2j2KpDZQt4DGyb4C0Javi8CT9jHwixzH0vdujrVg+qO h6KlJEx4lzSWSBa/2/mmQ1PznmWj5AJwt45ttnwfUUc5hIClJe5iMT54SLYdq169 aihUTajCug8HPmDv1TL2xVTm1BGbvzq6cbNDJqBIsV2LkIqPrcVrJUZyk5QnyFFy YHZ4ag+5sqgCMA7WviBS6IRx2VWzrd2eE+ZRdrdTJVSDuf4zBLmjAYVvADDpltB/ BnyiSMXf2SNqaNLsWr+rM6ur =PcRa -----END PGP SIGNATURE----- --53xhtikaxhQ8hdOS4D9dU2CF2vIfBOFnT--