[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 05/22] qcow2-bitmap: structs and consts
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH 05/22] qcow2-bitmap: structs and consts |
Date: |
Wed, 12 Oct 2016 20:20:39 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 |
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.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?
>
> Actualy BME_MAX_PHYS_SIZE is this limit:
> in check_constraints we have
>
> uint64_t phys_bitmap_bytes =
> (uint64_t)h->bitmap_table_size * s->cluster_size;
>
> ...
>
> (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
signature.asc
Description: OpenPGP digital signature