From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43854) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buO99-0003FV-QI for qemu-devel@nongnu.org; Wed, 12 Oct 2016 14:20:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1buO98-0006G7-8q for qemu-devel@nongnu.org; Wed, 12 Oct 2016 14:20:55 -0400 References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-6-git-send-email-vsementsov@virtuozzo.com> <57FCD216.60405@virtuozzo.com> From: Max Reitz Message-ID: <98ecdb71-80cb-f3bd-40e0-e08e63f171db@redhat.com> Date: Wed, 12 Oct 2016 20:20:39 +0200 MIME-Version: 1.0 In-Reply-To: <57FCD216.60405@virtuozzo.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="NVFdKWRDwObcsLoFv32fOfVwWF4KvNHcn" 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) --NVFdKWRDwObcsLoFv32fOfVwWF4KvNHcn 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: <98ecdb71-80cb-f3bd-40e0-e08e63f171db@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> <57FCD216.60405@virtuozzo.com> In-Reply-To: <57FCD216.60405@virtuozzo.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 11.10.2016 13:50, Vladimir Sementsov-Ogievskiy wrote: > On 01.10.2016 17: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.t= xt >>> >>> 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-snapshot.o qcow2-cache.o >>> +block-obj-y +=3D qcow2.o qcow2-refcount.o qcow2-cluster.o >>> qcow2-snapshot.o qcow2-cache.o qcow2-bitmap.o >>> block-obj-y +=3D qed.o qed-gencb.o qed-l2-cache.o qed-table.o >>> qed-cluster.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 >>> obtaining 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 >>> included in >>> + * all copies or substantial portions of the Software. >>> + * >>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, >>> EXPRESS OR >>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >>> MERCHANTABILITY, >>> + * 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, >>> ARISING FROM, >>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER >>> DEALINGS IN >>> + * THE SOFTWARE. >>> + */ >>> + >>> +/* NOTICE: BME here means Bitmaps Extension and used as a namespace = for >>> + * _internal_ constants. Please do not use this _internal_ >>> abbreviation 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 (5= 12 >> 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? >> >> 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? >=20 > Actualy BME_MAX_PHYS_SIZE is this limit: > in check_constraints we have >=20 > uint64_t phys_bitmap_bytes =3D > (uint64_t)h->bitmap_table_size * s->cluster_size; >=20 > ... >=20 > (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) || OK, so BME_MAX_PHYS_SIZE is actually supposed to be the limit of the size of the bitmaps in RAM? And I suppose it is going to be calculated differently in the future once qemu has sparse bitmap support? My fault, then, I thought BME_MAX_PHYS_SIZE was supposed to be the limit of the size on disk. OK, makes sense then, but the question whether a runtime-adjustable limit would make sense still remains. OTOH, this is something that can always be added later on. Max --NVFdKWRDwObcsLoFv32fOfVwWF4KvNHcn Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEvBAEBCAAZBQJX/n73EhxtcmVpdHpAcmVkaGF0LmNvbQAKCRD0B9sAYdXPQJyc B/9vo8VrotwuRFb9ALIHzqLbKmn0/BT60N/kLnoLR7XA2uEiQ8z5qCvgxkt5/VlL +VDukaVx4HycP+p97IWtWm+x5vjv+Spe+XnoB7Bnm712zhUJbq8ENwy7ALUdTnNp vKiwfuAaF7y7D3p/hs+5i/EhPOjdgHsF51kBVnVryxk3eIMd+lxBAzh4zQAFWo/B vmq9/zSxUK+T6Kds7Q6kA3+o3Jw8BF/p8a0pNAdFWAkDzyTM+q3xxxW4wgJ9/PZE lvNeiKUpJ6OkDopHkied5Arypec/Cw8/P8vBoPDYEWn8bbTLLtVY2vIkhE3X6TU1 dnYCPYT8EmCn5k6wWbB3ZwW7 =Y/ql -----END PGP SIGNATURE----- --NVFdKWRDwObcsLoFv32fOfVwWF4KvNHcn--