[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH 1/2] qcow2: Give the refcount cache the minimum

From: Eric Blake
Subject: Re: [Qemu-block] [PATCH 1/2] qcow2: Give the refcount cache the minimum possible size by default
Date: Tue, 13 Mar 2018 13:23:36 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

On 03/13/2018 10:02 AM, Alberto Garcia wrote:
The L2 and refcount caches have default sizes that can be overriden
using the l2-cache-size and refcount-cache-size (an additional
parameter named cache-size sets the combined size of both caches).

Unless forced by one of the aforementioned parameters, QEMU will set
the unspecified sizes so that the L2 cache is 4 times larger than the
refcount cache.

However this patch takes a completely different approach and instead
of keeping a ratio between both cache sizes it assigns as much as
possible to the L2 cache and the remainder to the refcount cache.

The reason is that L2 tables are used for every single I/O request
from the guest and the effect of increasing the cache is significant
and clearly measurable. Refcount blocks are however only used for
cluster allocation and internal snapshots and in practice are accessed
sequentially in most cases, so the effect of increasing the cache is
negligible (even when doing random writes from the guest).

So, make the refcount cache as small as possible unless the user
explicitly asks for a larger one.

I like the reasoning given here.

I'd count this as a bugfix, safe even during freeze (but it's ultimately the maintainer's call)

Signed-off-by: Alberto Garcia <address@hidden>
  block/qcow2.c              | 31 +++++++++++++++++++------------
  block/qcow2.h              |  4 ----
  tests/qemu-iotests/137.out |  2 +-
  3 files changed, 20 insertions(+), 17 deletions(-)

+++ b/block/qcow2.c
@@ -802,23 +802,30 @@ static void read_cache_sizes(BlockDriverState *bs, 
QemuOpts *opts,
          } else if (refcount_cache_size_set) {
              *l2_cache_size = combined_cache_size - *refcount_cache_size;
          } else {
-            *refcount_cache_size = combined_cache_size
-                                 / (DEFAULT_L2_REFCOUNT_SIZE_RATIO + 1);
-            *l2_cache_size = combined_cache_size - *refcount_cache_size;

In the old code, refcount_cache_size and l2_cache_size are both set to fractions of the combined size, so both are positive (even if combined_cache_size is too small for the minimums required)

+            uint64_t virtual_disk_size = bs->total_sectors * BDRV_SECTOR_SIZE;
+            uint64_t max_l2_cache = virtual_disk_size / (s->cluster_size / 8);
+            uint64_t min_refcount_cache =
+                (uint64_t) MIN_REFCOUNT_CACHE_SIZE * s->cluster_size;
+            /* Assign as much memory as possible to the L2 cache, and
+             * use the remainder for the refcount cache */
+            if (combined_cache_size >= max_l2_cache + min_refcount_cache) {
+                *l2_cache_size = max_l2_cache;
+                *refcount_cache_size = combined_cache_size - *l2_cache_size;
+            } else {
+                *refcount_cache_size =
+                    MIN(combined_cache_size, min_refcount_cache);

but here, if combined_cache_size is smaller than min_refcount_cache,

+                *l2_cache_size = combined_cache_size - *refcount_cache_size;

then l2_cache_size is set to a negative value.

I think you're missing bounds validations that the combined cache size is large enough for the minimums required. Or maybe a slight tweak, if it is okay for one of the two caches to be sized at 0 (that is, if combined_cache_size is too small for refcount, can it instead be given 100% to the l2 cache and let refcount be uncached)?

+            }
      } else {
-        if (!l2_cache_size_set && !refcount_cache_size_set) {
+        if (!l2_cache_size_set) {
              *l2_cache_size = MAX(DEFAULT_L2_CACHE_BYTE_SIZE,
                                   * s->cluster_size);
-            *refcount_cache_size = *l2_cache_size
-                                 / DEFAULT_L2_REFCOUNT_SIZE_RATIO;
-        } else if (!l2_cache_size_set) {
-            *l2_cache_size = *refcount_cache_size
-                           * DEFAULT_L2_REFCOUNT_SIZE_RATIO;
-        } else if (!refcount_cache_size_set) {
-            *refcount_cache_size = *l2_cache_size
-                                 / DEFAULT_L2_REFCOUNT_SIZE_RATIO;
+        }
+        if (!refcount_cache_size_set) {
+            *refcount_cache_size = MIN_REFCOUNT_CACHE_SIZE * s->cluster_size;
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

reply via email to

[Prev in Thread] Current Thread [Next in Thread]