[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 05/22] qcow2-bitmap: structs and consts
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH 05/22] qcow2-bitmap: structs and consts |
Date: |
Sat, 1 Oct 2016 16:56:46 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 |
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 <address@hidden>
>> ---
>> 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