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 <[email protected]> >> --- >> 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
