Re: [Qemu-devel] [PATCH v4 16/17] qapi: Expose new qcow2 overlap check options

[Max Reitz <mreitz@redhat.com>]

On 04.05.2015 21:39, Eric Blake wrote:
> On 05/04/2015 01:15 PM, Max Reitz wrote:
>> Expose the two new options for controlling the memory usage of the
>> overlap check implementation via QAPI.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   qapi/block-core.json | 37 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 1c17224..99456e6 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1509,6 +1509,39 @@
>>               'mode':  'Qcow2OverlapCheckMode' } }
>>   
>>   ##
>> +# @Qcow2OverlapStructures
>> +#
>> +# Contains options for controlling the behavior of the metadata overlap
>> +# prevention structures.
>> +#
>> +# The primary structure used for overlap check and prevention is a bitmap
>> +# (actually a bytemap) which has one entry per cluster designating the type(s)
>> +# of metadata it contains. In order to save memory, there is an RLE-like
>> +# representation of this bitmap, too.
>> +#
>> +# The whole array of clusters is chunked. The RLE representation of one chunk
>> +# is converted to the equivalent bitmap whenever a cluster in the chunk is
>> +# accessed. The bitmaps are kept for a limited number of chunks (as a cache). On
>> +# cache eviction, the bitmap is converted back to RLE again.
> Feels a bit verbose at describing the internal representation,
> especially if we change the internals at some point in the future.  I'm
> more interested that there are two limits at play, and that tuning them
> larger may consume more memory but offer better protection on large images.

Well, that works for total-size-limit, but it won't work as an 
explanation for bitmap-size. I could rename it to cache-size and just 
say that it's the size of some (opaque) internal cache, though.

>> +#
>> +# @bitmap-size:      #optional size (in bytes) of the bitmap cache, defaults to
>> +#                    64 kB
>> +#
> Any requirement on it being a power of 2, or even a recommendation on
> how large or small to make it in relation to an image size?

No requirements and I can't give recommendations. We do have a fixed 
default for the metadata caches (refcount blocks and L2 tables), too, 
and this is basically the same thing when it comes to choosing the right 
size. For a large image, you have multiple e.g. L2 tables and also 
multiple windows concerning the overlap checks; therefore, if you have a 
lot of totally random accesses, both caches will fail. Considering that 
we seem to have accepted this for the metadata caches, I think it should 
be fine here, too.

>> +# @total-size-limit: #optional maximum total size (in bytes) of all the metadata
> What happens if bitmap-size is larger than total-size-limit?

Interesting question. Well, what does happen is that at some point it 
tries to create a bitmap for the cache, and that will fail due to the 
size limit, so the event will get thrown. The problem is that accessing 
any area of the qcow2 file which does not yet have an entry in the 
bitmap cache will result in hitting the limit, so there is no protection 
for these areas.

The problem is that it's difficult to give a limit for the ratio of 
bitmap-size to total-size-limit which always works. A fragments list 
(the RLE representation) will never exceed the bitmap in size, so using 
0.5 as the maximum ratio seems to make sense. However, there are other 
structures as well, such as the array of windows which depends on the 
size of the qcow2 file, so depending on the file, the maximum ratio may 
get infinitely small.

So I guess the best thing to do would be to detect the limit excess 
condition when creating a new bitmap, and then instead of trying to 
occupy a previously empty cache entry, evict a non-empty one (repeat 
until there is enough memory available, or the cache is empty, in which 
case it is justified to complain about hitting the memory limit).

>
>> +#                    overlap prevention data structures combined; if this limit
>> +#                    is exceeded, a QCOW2_OVERLAP_CHECK_MEMORY_LIMIT_REACHED
>> +#                    event will be emitted and some parts of the image may no
>> +#                    longer be protected against erroneous overwriting of
>> +#                    metadata by (meta)data of a different kind. Defaults to
>> +#                    SIZE_MAX.
>> +#
>> +# Since: 2.4
>> +##
>> +{ 'type': 'Qcow2OverlapStructures',
> Rebase conflict with my series that does s/type/struct/

Will fix.

Thanks for reviewing,

Max

>> +  'data': { '*bitmap-size':      'int',
>> +            '*total-size-limit': 'int' } }
>> +
>> +##
>>   # @BlockdevOptionsQcow2
>>   #
>>   # Driver specific block device options for qcow2.
>> @@ -1530,6 +1563,9 @@
>>   # @overlap-check:         #optional which overlap checks to perform for writes
>>   #                         to the image, defaults to 'cached' (since 2.2)
>>   #
>> +# @overlap-structures:    #optional options for controlling the behavior of the
>> +#                         metadata overlap prevention structures (since 2.4)
>> +#
>>   # @cache-size:            #optional the maximum total size of the L2 table and
>>   #                         refcount block caches in bytes (since 2.2)
>>   #
>> @@ -1548,6 +1584,7 @@
>>               '*pass-discard-snapshot': 'bool',
>>               '*pass-discard-other': 'bool',
>>               '*overlap-check': 'Qcow2OverlapChecks',
>> +            '*overlap-structures': 'Qcow2OverlapStructures',
>>               '*cache-size': 'int',
>>               '*l2-cache-size': 'int',
>>               '*refcount-cache-size': 'int' } }
>>