[Top][All Lists]

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

Re: [Qemu-block] [PATCH v4 2/5] qcow2: Assign the L2 cache relatively to

From: Leonid Bloch
Subject: Re: [Qemu-block] [PATCH v4 2/5] qcow2: Assign the L2 cache relatively to image size
Date: Thu, 9 Aug 2018 17:04:23 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0

On 8/9/18 3:14 PM, Alberto Garcia wrote:
On Thu 09 Aug 2018 12:11:35 AM CEST, Leonid Bloch wrote:
Sufficient L2 cache can noticeably improve the performance when using
large images with frequent I/O. The memory overhead is not significant
in most cases, as the cache size is only 1 MB for each 8 GB of virtual
image size (with the default cluster size of 64 KB).

Previously, the L2 cache was allocated without considering the image
size, and an option existed to manually determine this size. Thus to
achieve full coverage of the image by the L2 cache (i.e. use more than
the default value of MAX(1 MB, 8 clusters)), a user needed to calculate
the required size manually or using a script, and passs this value to
the 'l2-cache-size' option.


Thanks! fixed.

diff --git a/block/qcow2.c b/block/qcow2.c
index ec9e6238a0..98cb96aaca 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -777,29 +777,35 @@ static void read_cache_sizes(BlockDriverState *bs, 
QemuOpts *opts,
                               uint64_t *refcount_cache_size, Error **errp)
      BDRVQcow2State *s = bs->opaque;
-    uint64_t combined_cache_size;
+    uint64_t combined_cache_size, l2_cache_max_setting;
      bool l2_cache_size_set, refcount_cache_size_set, combined_cache_size_set;
-    int min_refcount_cache = MIN_REFCOUNT_CACHE_SIZE * s->cluster_size;
+    uint64_t min_refcount_cache = MIN_REFCOUNT_CACHE_SIZE * s->cluster_size;

Why do you need to change this data type here? min_refcount_cache is
guaranteed to fit in an int.

No necessity here, just it participates in arithmetics with other uint64_t's afterwards, so it might as well be uint64_t from the get-go.

+    uint64_t virtual_disk_size = bs->total_sectors * BDRV_SECTOR_SIZE;
+    uint64_t max_l2_cache = virtual_disk_size / (s->cluster_size / 8);
+    *l2_cache_size = MIN(max_l2_cache, l2_cache_max_setting);

You need to declare virtual_disk_size and max_l2_cache at the beginning.

Sure, done, thanks.

              } else {
-                *refcount_cache_size =
-                    MIN(combined_cache_size, min_refcount_cache);
+                *refcount_cache_size = MIN(combined_cache_size,
+                                           min_refcount_cache);

There are no functional changes, why do you need to change the
indentation here?

It's in the "immediate area (few lines) of the lines [I'm] changing".

-    } else {
-        if (!l2_cache_size_set) {
-            *l2_cache_size = MAX(DEFAULT_L2_CACHE_BYTE_SIZE,
-                                 (uint64_t)DEFAULT_L2_CACHE_CLUSTERS
-                                 * s->cluster_size);
-        }
-        if (!refcount_cache_size_set) {
-            *refcount_cache_size = min_refcount_cache;
-        }

It's not obvious why you are removing the *refcount_cache_size
assignment here. I see that if you leave this out then the caller
qcow2_update_options_prepare() ensures that refcount_cache_size has the
minimum size, but that's not directly related to the rest of the changes
in this patch.

So either mention in explicitly in the commit message or remove those
lines in a separate patch.

OK, I'll mention it.

diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
index 5bf2a8ad29..c7625cdeb3 100644
--- a/docs/qcow2-cache.txt
+++ b/docs/qcow2-cache.txt
@@ -97,12 +97,14 @@ need:
     l2_cache_size = disk_size_GB * 131072
     refcount_cache_size = disk_size_GB * 32768
-QEMU has a default L2 cache of 1MB (1048576 bytes) and a refcount
-cache of 256KB (262144 bytes), so using the formulas we've just seen
-we have
+QEMU will use a default L2 cache sufficient to cover the entire virtual
+size of an image, which with the default cluster size will result in 1 MB
+of cache for every 8 GB of virtual image size:

This is not true. QEMU will use a default size of 32MB, which may or may
not cover the entire image.

But no, QEMU will not use a default size of 32MB. It will use a default size which is just enough to cover the image, unless the needed size is larger than 32 MB. Anyway, this is a section which deals with explanations of the cache coverage, and not with the defaults, so I have removed the mention of the defaults here, as it is mentioned in the relevant section below.

-   1048576 / 131072 = 8 GB of virtual disk covered by that cache
-    262144 /  32768 = 8 GB
+   65536 / 8 = 8192 = 8 GB / 1 MB

And those formulas still apply, but with the new values.

They apply for the coverage calculation, yes. Made a bit of clarification in v5.

+ - The default L2 cache size will cover the entire virtual size of an
+   image, but is capped at 32 MB (enough for image sizes of up to 256 GB
+   with the default cluster size).

Again, this is misleading. A constant in this series is that "The L2
cache now covers the entire image", but that's not the case at all :-)

I would prefer if you would say "we increased the default cache size so
now we cover larger images" instead of "the default cache size will now
cover the entire image", because the latter is not true.

But it's not correct: we did not increase the default size, we made the default size fit the image size, and set a maximum. It's not the same, do you agree?
In any case, I have reworded this part in v5.

   - If the L2 cache is big enough to hold all of the image's L2 tables
-   (as explained in the "Choosing the right cache sizes" section
-   earlier in this document) then none of this is necessary and you
-   can omit the "l2-cache-entry-size" parameter altogether.
+   (the default behavior) then none of this is necessary and you can
+   omit the "l2-cache-entry-size" parameter altogether.

And once more. In the previous paragraph you say that the default is
enough for images <= 256GB, and in this one you say that it's enough to
hold all L2 tables.

Reworded here as well.

Thanks for your review!

The previous text was accurate, you don't need to change this paragraph.


reply via email to

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