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.txt >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >> --- >> 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 += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o >> dmg.o >> -block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o >> qcow2-cache.o >> +block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o >> qcow2-cache.o qcow2-bitmap.o >> block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o >> block-obj-y += qed-check.o >> block-obj-$(CONFIG_VHDX) += 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 (512 > MB) divided by the cluster size (>= 512; 512 MB / cluster_size <= 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? > >> +#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 > > ull suffix is missing. > >> + >> +typedef enum BitmapType { >> + BT_DIRTY_TRACKING_BITMAP = 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) >> >> +/* Bitmap header extension constraints */ >> +#define QCOW_MAX_DIRTY_BITMAPS 65535 >> +#define QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE (1024 * QCOW_MAX_DIRTY_BITMAPS) >> + >> /* indicate that the refcount of the referenced cluster is exactly one. */ >> #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; >> >> +/* 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); >> >> +/* Be careful, Qcow2BitmapHeaderExt is not an extension of >> Qcow2BitmapDirEntry, 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; >> >
signature.asc
Description: OpenPGP digital signature