Re: [Qemu-devel] [PATCH 05/22] qcow2-bitmap: structs and consts

[Max Reitz]

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