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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5671 bytes --]

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 <vsementsov@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?
> 
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]