Re: [Qemu-block] [PATCH v3 2/5] qcow2: Make the default L2 cache sufficient to cover the entire image

[Alberto Garcia]

On Wed 08 Aug 2018 04:35:19 PM CEST, Leonid Bloch wrote:
>>>> The way I see it: there are two simple changes from the user's point of
>>>> view (they can even be two separate patches).
>>>>
>>>> 1) The default l2-cache-size is now 32MB. DEFAULT_L2_CACHE_CLUSTERS is
>>>>      useless now and disappears.
>>> I don't think that it can be a separate patch, because unless the other
>>> logic is changed, the cache will occupy 32 MB *always*, regardless of
>>> the image size, and that's quite a big and unneeded overhead.
>> 
>> Change the order of both patches then :-)
>
> Do you really think it's necessary? The increase of the default max
> size is directly tied to the functionality change: it will be harmful
> to increase the maximum before the new functionality is implemented,
> and there is no need to change the functionality if the default max is
> not increased.

I think that we're looking at this from two different perspectives.

a) If I understand you correctly, you see this as a way to make the user
   forget about the L2 cache: we guarantee that it's going to be big
   enough for the entire image, so simply forget about it. Exception: if
   you're using very large images you will have to set its size
   manually, but for the vast majority of cases you'll be alright with
   the default (32MB).

b) The way I see it: setting the right L2 cache size is not trivial, it
   depends on the image and cluster sizes, and it involves a trade-off
   between how much memory you want to use and how much performance
   you're willing to sacrifice. QEMU has many use cases and there's no
   good default, you need to make the numbers yourself if you want to
   fine-tune it. Don't blindly trust the new default size (32MB) because
   it won't be enough for many cases. But we can promise you this: make
   l2-cache-size the maximum amount of memory you're willing to spend on
   this disk image's cache, and we guarantee that we'll only use the
   amount that we need to give you the best performance.

I hope (a) was a fair description of what you're trying to achieve with
these patches. But I also hope that you can see why making l2_cache_size
= MIN(l2_cache_size, virtual_disk_size / (s->cluster_size / 8)) is a
worthwhile change on its own, even if we didn't increase the default
cache size to 32MB.

Berto