[Qemu-devel] [PATCH v3 0/5] qcow2: Make the L2 cache cover the whole image by default

[Qemu-devel] [PATCH v3 0/5] qcow2: Make the L2 cache cover the whole image by default

[Leonid Bloch]

This series makes the qcow2 L2 cache cover the entire image by default.
The importance of this change is in noticeable performance improvement,
especially with heavy random I/O. The memory overhead is not big in most
cases, as only 1 MB of cache for every 8 GB of image size is used.
For cases with very large images and/or small cluster sizes, or systems
with limited RAM resources, there is an upper limit on the default L2
cache: 32 MB. To modify this limit one can use the already existing
'l2-cache-size' and 'cache-size' options. Moreover, this fixes the
behavior of 'l2-cache-size', as it was documented as the *maximum* L2
cache size, but in practice behaved as the absolute size.

To compensate the memory overhead which may be increased following this
behavior, the default cache-clean-interval is set to 30 seconds by
default (was disabled by default before).

The L2 cache is also resized accordingly, by default, if the image is
resized.

Additionally, few minor changes are made (refactoring and documentation
fixes).

Differences from v1:
* .gitignore modification patch removed (unneeded).
* The grammar fix in conflicting cache sizing patch removed (merged).
* The update total_sectors when resizing patch squashed with the
  resizing patch.
* L2 cache is now capped at 32 MB.
* The default cache-clean-interval is set to 30 seconds.

Differences from v2:
* Made it clear in the documentation that setting cache-clean-interval
  to 0 disables this feature.

Leonid Bloch (5):
  qcow2: Options' documentation fixes
  qcow2: Make the default L2 cache sufficient to cover the entire image
  qcow2: Resize the cache upon image resizing
  qcow2: Set the default cache-clean-interval to 30 seconds
  qcow2: Explicit number replaced by a constant

 block/qcow2.c              | 47 +++++++++++++++++++-------------------
 block/qcow2.h              |  5 ++--
 docs/qcow2-cache.txt       | 31 +++++++++++++++----------
 qapi/block-core.json       |  3 ++-
 qemu-options.hx            | 11 +++++----
 tests/qemu-iotests/137     |  1 -
 tests/qemu-iotests/137.out |  1 -
 7 files changed, 54 insertions(+), 45 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v3 1/5] qcow2: Options' documentation fixes

[Leonid Bloch]

Signed-off-by: Leonid Bloch <lbloch@janustech.com>
---
 docs/qcow2-cache.txt | 3 +++
 qemu-options.hx      | 9 ++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
index 8a09a5cc5f..5bf2a8ad29 100644
--- a/docs/qcow2-cache.txt
+++ b/docs/qcow2-cache.txt
@@ -130,6 +130,9 @@ There are a few things that need to be taken into account:
    memory as possible to the L2 cache before increasing the refcount
    cache size.
 
+ - At most two of "l2-cache-size", "refcount-cache-size", and "cache-size"
+   can be set simultaneously.
+
 Unlike L2 tables, refcount blocks are not used during normal I/O but
 only during allocations and internal snapshots. In most cases they are
 accessed sequentially (even during random guest I/O) so increasing the
diff --git a/qemu-options.hx b/qemu-options.hx
index b1bf0f485f..f6804758d3 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -752,15 +752,18 @@ image file)
 
 @item cache-size
 The maximum total size of the L2 table and refcount block caches in bytes
-(default: 1048576 bytes or 8 clusters, whichever is larger)
+(default: the sum of l2-cache-size and refcount-cache-size)
 
 @item l2-cache-size
 The maximum size of the L2 table cache in bytes
-(default: 4/5 of the total cache size)
+(default: if cache-size is not defined - 1048576 bytes or 8 clusters, whichever
+is larger; otherwise, as large as possible or needed within the cache-size,
+while permitting the requested or the minimal refcount cache size)
 
 @item refcount-cache-size
 The maximum size of the refcount block cache in bytes
-(default: 1/5 of the total cache size)
+(default: 4 times the cluster size; or if cache-size is specified, the part of
+it which is not used for the L2 cache)
 
 @item cache-clean-interval
 Clean unused entries in the L2 and refcount caches. The interval is in seconds.
-- 
2.17.1

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

[Leonid Bloch]

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). For cases with very
large images and/or small cluster sizes, there is an upper limit on the
default L2 cache: 32 MB. To modify this limit one can use the already
existing 'l2-cache-size' option. This option was previously documented
as the *maximum* L2 cache size, and this patch makes it behave as such,
instead of a constant size. Also, the existing option 'cache-size' can
limit the sum of both L2 and refcount caches, as previously.

Signed-off-by: Leonid Bloch <lbloch@janustech.com>
---
 block/qcow2.c              | 33 +++++++++++++--------------------
 block/qcow2.h              |  4 +---
 docs/qcow2-cache.txt       | 24 ++++++++++++++----------
 qemu-options.hx            |  6 +++---
 tests/qemu-iotests/137     |  1 -
 tests/qemu-iotests/137.out |  1 -
 6 files changed, 31 insertions(+), 38 deletions(-)

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;
 
     combined_cache_size_set = qemu_opt_get(opts, QCOW2_OPT_CACHE_SIZE);
     l2_cache_size_set = qemu_opt_get(opts, QCOW2_OPT_L2_CACHE_SIZE);
     refcount_cache_size_set = qemu_opt_get(opts, QCOW2_OPT_REFCOUNT_CACHE_SIZE);
 
     combined_cache_size = qemu_opt_get_size(opts, QCOW2_OPT_CACHE_SIZE, 0);
-    *l2_cache_size = qemu_opt_get_size(opts, QCOW2_OPT_L2_CACHE_SIZE, 0);
+    l2_cache_max_setting = qemu_opt_get_size(opts, QCOW2_OPT_L2_CACHE_SIZE,
+                                             DEFAULT_L2_CACHE_MAX_SIZE);
     *refcount_cache_size = qemu_opt_get_size(opts,
                                              QCOW2_OPT_REFCOUNT_CACHE_SIZE, 0);
 
     *l2_cache_entry_size = qemu_opt_get_size(
         opts, QCOW2_OPT_L2_CACHE_ENTRY_SIZE, s->cluster_size);
 
+    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);
+
     if (combined_cache_size_set) {
         if (l2_cache_size_set && refcount_cache_size_set) {
             error_setg(errp, QCOW2_OPT_CACHE_SIZE ", " QCOW2_OPT_L2_CACHE_SIZE
                        " and " QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not be set "
                        "at the same time");
             return;
-        } else if (*l2_cache_size > combined_cache_size) {
+        } else if (l2_cache_size_set &&
+                   (l2_cache_max_setting > combined_cache_size)) {
             error_setg(errp, QCOW2_OPT_L2_CACHE_SIZE " may not exceed "
                        QCOW2_OPT_CACHE_SIZE);
             return;
@@ -814,29 +820,16 @@ 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 {
-            uint64_t virtual_disk_size = bs->total_sectors * BDRV_SECTOR_SIZE;
-            uint64_t max_l2_cache = virtual_disk_size / (s->cluster_size / 8);
-
             /* 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;
+            if (combined_cache_size >= *l2_cache_size + min_refcount_cache) {
                 *refcount_cache_size = combined_cache_size - *l2_cache_size;
             } else {
-                *refcount_cache_size =
-                    MIN(combined_cache_size, min_refcount_cache);
+                *refcount_cache_size = MIN(combined_cache_size,
+                                           min_refcount_cache);
                 *l2_cache_size = combined_cache_size - *refcount_cache_size;
             }
         }
-    } 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;
-        }
     }
 
     if (*l2_cache_entry_size < (1 << MIN_CLUSTER_BITS) ||
diff --git a/block/qcow2.h b/block/qcow2.h
index 81b844e936..d77a31d932 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -73,9 +73,7 @@
 /* Must be at least 4 to cover all cases of refcount table growth */
 #define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */
 
-/* Whichever is more */
-#define DEFAULT_L2_CACHE_CLUSTERS 8 /* clusters */
-#define DEFAULT_L2_CACHE_BYTE_SIZE 1048576 /* bytes */
+#define DEFAULT_L2_CACHE_MAX_SIZE 0x2000000U /* bytes */
 
 #define DEFAULT_CLUSTER_SIZE 65536
 
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:
 
-   1048576 / 131072 = 8 GB of virtual disk covered by that cache
-    262144 /  32768 = 8 GB
+   65536 / 8 = 8192 = 8 GB / 1 MB
+
+A default refcount cache is 4 times the cluster size, which defaults to
+256 KB (262144 bytes).
 
 
 How to configure the cache sizes
@@ -121,8 +123,11 @@ There are a few things that need to be taken into account:
  - Both caches must have a size that is a multiple of the cluster size
    (or the cache entry size: see "Using smaller cache sizes" below).
 
- - The default L2 cache size is 8 clusters or 1MB (whichever is more),
-   and the minimum is 2 clusters (or 2 cache entries, see below).
+ - 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). This maximum value can be reduced or
+   enlarged using the "l2-cache-size" option. The minimum is 2 clusters
+   (or 2 cache entries, see below).
 
  - The default (and minimum) refcount cache size is 4 clusters.
 
@@ -180,9 +185,8 @@ Some things to take into account:
    always uses the cluster size as the entry size.
 
  - 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.
 
 
 Reducing the memory usage
diff --git a/qemu-options.hx b/qemu-options.hx
index f6804758d3..d6e15b2f06 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -756,9 +756,9 @@ The maximum total size of the L2 table and refcount block caches in bytes
 
 @item l2-cache-size
 The maximum size of the L2 table cache in bytes
-(default: if cache-size is not defined - 1048576 bytes or 8 clusters, whichever
-is larger; otherwise, as large as possible or needed within the cache-size,
-while permitting the requested or the minimal refcount cache size)
+(default: if cache-size is not defined - 32M; otherwise, as large as possible
+or needed within the cache-size, while permitting the requested or the minimal
+refcount cache size)
 
 @item refcount-cache-size
 The maximum size of the refcount block cache in bytes
diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
index 87965625d8..e3fb078588 100755
--- a/tests/qemu-iotests/137
+++ b/tests/qemu-iotests/137
@@ -109,7 +109,6 @@ $QEMU_IO \
     -c "reopen -o cache-size=1M,l2-cache-size=64k,refcount-cache-size=64k" \
     -c "reopen -o cache-size=1M,l2-cache-size=2M" \
     -c "reopen -o cache-size=1M,refcount-cache-size=2M" \
-    -c "reopen -o l2-cache-size=256T" \
     -c "reopen -o l2-cache-entry-size=33k" \
     -c "reopen -o l2-cache-entry-size=128k" \
     -c "reopen -o refcount-cache-size=256T" \
diff --git a/tests/qemu-iotests/137.out b/tests/qemu-iotests/137.out
index 6a2ffc71fd..70f245ae7a 100644
--- a/tests/qemu-iotests/137.out
+++ b/tests/qemu-iotests/137.out
@@ -19,7 +19,6 @@ Parameter 'lazy-refcounts' expects 'on' or 'off'
 cache-size, l2-cache-size and refcount-cache-size may not be set at the same time
 l2-cache-size may not exceed cache-size
 refcount-cache-size may not exceed cache-size
-L2 cache size too big
 L2 cache entry size must be a power of two between 512 and the cluster size (65536)
 L2 cache entry size must be a power of two between 512 and the cluster size (65536)
 Refcount cache size too big
-- 
2.17.1

[Qemu-devel] [PATCH v3 3/5] qcow2: Resize the cache upon image resizing

[Leonid Bloch]

The caches are now recalculated upon image resizing. This is done
because the new default behavior of assigning a sufficient L2 cache to
cover the entire image implies that the cache will still be sufficient
after an image resizing.

Signed-off-by: Leonid Bloch <lbloch@janustech.com>
---
 block/qcow2.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 98cb96aaca..f60cb92169 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3639,6 +3639,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
         }
     }
 
+    bs->total_sectors = offset / BDRV_SECTOR_SIZE;
+
     /* write updated header.size */
     offset = cpu_to_be64(offset);
     ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
@@ -3649,6 +3651,12 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
     }
 
     s->l1_vm_state_index = new_l1_size;
+    /* Update cache sizes */
+    QDict *options = qdict_clone_shallow(bs->options);
+    ret = qcow2_update_options(bs, options, s->flags, errp);
+    if (ret < 0) {
+        goto fail;
+    }
     ret = 0;
 fail:
     qemu_co_mutex_unlock(&s->lock);
-- 
2.17.1

[Qemu-devel] [PATCH v3 4/5] qcow2: Set the default cache-clean-interval to 30 seconds

[Leonid Bloch]

The default cache-clean-interval is set to 30 seconds, in order to lower
the overhead of the qcow2 caches (before the default was 0, i.e.
disabled).

Signed-off-by: Leonid Bloch <lbloch@janustech.com>
---
 block/qcow2.c        | 2 +-
 block/qcow2.h        | 1 +
 docs/qcow2-cache.txt | 4 ++--
 qapi/block-core.json | 3 ++-
 qemu-options.hx      | 2 +-
 5 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index f60cb92169..453a6377ac 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -941,7 +941,7 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
     /* New interval for cache cleanup timer */
     r->cache_clean_interval =
         qemu_opt_get_number(opts, QCOW2_OPT_CACHE_CLEAN_INTERVAL,
-                            s->cache_clean_interval);
+                            DEFAULT_CACHE_CLEAN_INTERVAL);
 #ifndef CONFIG_LINUX
     if (r->cache_clean_interval != 0) {
         error_setg(errp, QCOW2_OPT_CACHE_CLEAN_INTERVAL
diff --git a/block/qcow2.h b/block/qcow2.h
index d77a31d932..96a2808685 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -77,6 +77,7 @@
 
 #define DEFAULT_CLUSTER_SIZE 65536
 
+#define DEFAULT_CACHE_CLEAN_INTERVAL 30  /* seconds */
 
 #define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts"
 #define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request"
diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
index c7625cdeb3..c795febbdc 100644
--- a/docs/qcow2-cache.txt
+++ b/docs/qcow2-cache.txt
@@ -202,8 +202,8 @@ This example removes all unused cache entries every 15 minutes:
 
    -drive file=hd.qcow2,cache-clean-interval=900
 
-If unset, the default value for this parameter is 0 and it disables
-this feature.
+If unset, the default value for this parameter is 30. Setting it to 0
+disables this feature.
 
 Note that this functionality currently relies on the MADV_DONTNEED
 argument for madvise() to actually free the memory. This is a
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5b9084a394..f98cd14740 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2830,7 +2830,8 @@
 #
 # @cache-clean-interval:  clean unused entries in the L2 and refcount
 #                         caches. The interval is in seconds. The default value
-#                         is 0 and it disables this feature (since 2.5)
+#                         is 30. Setting 0 disables this feature. (since 2.5)
+#
 # @encrypt:               Image decryption options. Mandatory for
 #                         encrypted images, except when doing a metadata-only
 #                         probe of the image. (since 2.10)
diff --git a/qemu-options.hx b/qemu-options.hx
index d6e15b2f06..a0e0763f71 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -767,7 +767,7 @@ it which is not used for the L2 cache)
 
 @item cache-clean-interval
 Clean unused entries in the L2 and refcount caches. The interval is in seconds.
-The default value is 0 and it disables this feature.
+The default value is 30. Setting it to 0 disables this feature.
 
 @item pass-discard-request
 Whether discard requests to the qcow2 device should be forwarded to the data
-- 
2.17.1

[Qemu-devel] [PATCH v3 5/5] qcow2: Explicit number replaced by a constant

[Leonid Bloch]

Signed-off-by: Leonid Bloch <lbloch@janustech.com>
---
 block/qcow2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 453a6377ac..a54e20402b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1321,7 +1321,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
     /* 2^(s->refcount_order - 3) is the refcount width in bytes */
     s->refcount_block_bits = s->cluster_bits - (s->refcount_order - 3);
     s->refcount_block_size = 1 << s->refcount_block_bits;
-    bs->total_sectors = header.size / 512;
+    bs->total_sectors = header.size / BDRV_SECTOR_SIZE;
     s->csize_shift = (62 - (s->cluster_bits - 8));
     s->csize_mask = (1 << (s->cluster_bits - 8)) - 1;
     s->cluster_offset_mask = (1LL << s->csize_shift) - 1;
@@ -3446,7 +3446,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
         goto fail;
     }
 
-    old_length = bs->total_sectors * 512;
+    old_length = bs->total_sectors * BDRV_SECTOR_SIZE;
     new_l1_size = size_to_l1(s, offset);
 
     if (offset < old_length) {
-- 
2.17.1

Re: [Qemu-devel] [PATCH v3 1/5] qcow2: Options' documentation fixes

[Alberto Garcia]

On Wed 08 Aug 2018 09:10:47 AM CEST, Leonid Bloch wrote:
> Signed-off-by: Leonid Bloch <lbloch@janustech.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH v3 5/5] qcow2: Explicit number replaced by a constant

[Alberto Garcia]

On Wed 08 Aug 2018 09:10:51 AM CEST, Leonid Bloch wrote:
> Signed-off-by: Leonid Bloch <lbloch@janustech.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH v3 4/5] qcow2: Set the default cache-clean-interval to 30 seconds

[Alberto Garcia]

On Wed 08 Aug 2018 09:10:50 AM CEST, Leonid Bloch wrote:
> The default cache-clean-interval is set to 30 seconds, in order to lower
> the overhead of the qcow2 caches (before the default was 0, i.e.
> disabled).
>
> Signed-off-by: Leonid Bloch <lbloch@janustech.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

> +#define DEFAULT_CACHE_CLEAN_INTERVAL 30  /* seconds */

I wonder if it shouldn't be a bit higher, though. Isn't 30 seconds too
low for a default?

Berto

Re: [Qemu-devel] [PATCH v3 4/5] qcow2: Set the default cache-clean-interval to 30 seconds

[Leonid Bloch]

On 08/08/2018 02:47 PM, Alberto Garcia wrote:
> On Wed 08 Aug 2018 09:10:50 AM CEST, Leonid Bloch wrote:
>> The default cache-clean-interval is set to 30 seconds, in order to lower
>> the overhead of the qcow2 caches (before the default was 0, i.e.
>> disabled).
>>
>> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
> 
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> 
>> +#define DEFAULT_CACHE_CLEAN_INTERVAL 30  /* seconds */
> 
> I wonder if it shouldn't be a bit higher, though. Isn't 30 seconds too
> low for a default?

I also had this thought. What would you suggest?

Leonid.

> 
> Berto
> 

Re: [Qemu-devel] [PATCH v3 4/5] qcow2: Set the default cache-clean-interval to 30 seconds

[Alberto Garcia]

On Wed 08 Aug 2018 01:49:08 PM CEST, Leonid Bloch wrote:
> On 08/08/2018 02:47 PM, Alberto Garcia wrote:
>> On Wed 08 Aug 2018 09:10:50 AM CEST, Leonid Bloch wrote:
>>> The default cache-clean-interval is set to 30 seconds, in order to lower
>>> the overhead of the qcow2 caches (before the default was 0, i.e.
>>> disabled).
>>>
>>> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
>> 
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> 
>>> +#define DEFAULT_CACHE_CLEAN_INTERVAL 30  /* seconds */
>> 
>> I wonder if it shouldn't be a bit higher, though. Isn't 30 seconds too
>> low for a default?
>
> I also had this thought. What would you suggest?

I don't know, 5 minutes at least. It should discard cache entries that
haven't been used in a while.

It my opinion the default should be for saving memory in VMs that are
mostly idle. If the VM is more or less active I wouldn't want to clean
cache entries that are used frequently.

Berto

Re: [Qemu-devel] [PATCH v3 4/5] qcow2: Set the default cache-clean-interval to 30 seconds

[Leonid Bloch]

On 08/08/2018 02:57 PM, Alberto Garcia wrote:
> On Wed 08 Aug 2018 01:49:08 PM CEST, Leonid Bloch wrote:
>> On 08/08/2018 02:47 PM, Alberto Garcia wrote:
>>> On Wed 08 Aug 2018 09:10:50 AM CEST, Leonid Bloch wrote:
>>>> The default cache-clean-interval is set to 30 seconds, in order to lower
>>>> the overhead of the qcow2 caches (before the default was 0, i.e.
>>>> disabled).
>>>>
>>>> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
>>>
>>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>>>
>>>> +#define DEFAULT_CACHE_CLEAN_INTERVAL 30  /* seconds */
>>>
>>> I wonder if it shouldn't be a bit higher, though. Isn't 30 seconds too
>>> low for a default?
>>
>> I also had this thought. What would you suggest?
> 
> I don't know, 5 minutes at least. It should discard cache entries that
> haven't been used in a while.
> 
> It my opinion the default should be for saving memory in VMs that are
> mostly idle. If the VM is more or less active I wouldn't want to clean
> cache entries that are used frequently.

How about 10 minutes?

Leonid.

> 
> Berto
> 

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

[Alberto Garcia]

On Wed 08 Aug 2018 09:10:48 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). For cases with very
> large images and/or small cluster sizes, there is an upper limit on the
> default L2 cache: 32 MB.

I find this description a bit confusing.

First of all, because it's not true that the default will cover the
whole image: we're just increasing it, but any image > 256GB is going to
need more than 32MB (with 64KB clusters, that is).

Second, because it's not clear what happens if you increase that
maximum. Do you have to calculate the new value? Can you make it larger
than what you actually need?

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.

2) QEMU will only use as much memory from l2-cache-size as it can use to
   cover the whole image. Increasing the value of l2-cache-size will not
   use any additional memory, so it's safe to do.

Berto

Re: [Qemu-devel] [PATCH v3 4/5] qcow2: Set the default cache-clean-interval to 30 seconds

[Alberto Garcia]

On Wed 08 Aug 2018 02:17:47 PM CEST, Leonid Bloch wrote:
> On 08/08/2018 02:57 PM, Alberto Garcia wrote:
>> On Wed 08 Aug 2018 01:49:08 PM CEST, Leonid Bloch wrote:
>>> On 08/08/2018 02:47 PM, Alberto Garcia wrote:
>>>> On Wed 08 Aug 2018 09:10:50 AM CEST, Leonid Bloch wrote:
>>>>> The default cache-clean-interval is set to 30 seconds, in order to lower
>>>>> the overhead of the qcow2 caches (before the default was 0, i.e.
>>>>> disabled).
>>>>>
>>>>> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
>>>>
>>>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>>>>
>>>>> +#define DEFAULT_CACHE_CLEAN_INTERVAL 30  /* seconds */
>>>>
>>>> I wonder if it shouldn't be a bit higher, though. Isn't 30 seconds too
>>>> low for a default?
>>>
>>> I also had this thought. What would you suggest?
>> 
>> I don't know, 5 minutes at least. It should discard cache entries that
>> haven't been used in a while.
>> 
>> It my opinion the default should be for saving memory in VMs that are
>> mostly idle. If the VM is more or less active I wouldn't want to clean
>> cache entries that are used frequently.
>
> How about 10 minutes?

Sounds good to me.

Berto

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

[Leonid Bloch]

On 08/08/2018 03:39 PM, Alberto Garcia wrote:
> On Wed 08 Aug 2018 09:10:48 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). For cases with very
>> large images and/or small cluster sizes, there is an upper limit on the
>> default L2 cache: 32 MB.
> 
> I find this description a bit confusing.
> 
> First of all, because it's not true that the default will cover the
> whole image: we're just increasing it, but any image > 256GB is going to
> need more than 32MB (with 64KB clusters, that is).

How about the following:

qcow2: Make the default L2 cache try to cover the entire image

> 
> Second, because it's not clear what happens if you increase that
> maximum. Do you have to calculate the new value? Can you make it larger
> than what you actually need?

What if I add after the full text of the comment (which already says 
about the 'l2-cache-size' option) something like: "In any case, the L2 
cache will be set to fit the virtual image size, unless it will require 
more space than the allowed maximum, in which case it will occupy the 
allowed maximum only."

> 
> 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.

> 2) QEMU will only use as much memory from l2-cache-size as it can use to
>     cover the whole image. Increasing the value of l2-cache-size will not
>     use any additional memory, so it's safe to do.

I'll add the clarification that it's safe to increase the l2-cache-size 
value to the commit message. It is reasonably safe, except of some 
corner cases with very large image sizes.

> 
> Berto
> 

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

[Alberto Garcia]

On Wed 08 Aug 2018 03:07:10 PM CEST, Leonid Bloch wrote:
> On 08/08/2018 03:39 PM, Alberto Garcia wrote:
>> On Wed 08 Aug 2018 09:10:48 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). For cases with very
>>> large images and/or small cluster sizes, there is an upper limit on the
>>> default L2 cache: 32 MB.
>> 
>> I find this description a bit confusing.
>> 
>> First of all, because it's not true that the default will cover the
>> whole image: we're just increasing it, but any image > 256GB is going to
>> need more than 32MB (with 64KB clusters, that is).
>
> How about the following:
>
> qcow2: Make the default L2 cache try to cover the entire image

That's what I think it's misleading, the patch only increases the
default cache size value. The current 1MB cache also tries to cover the
entire image, if the entire image is <= 8GB. But there's no new feature
that extends the cache size to cover the entire image. There's no "we
used to cover 8GB images by default, but now we cover all images".

In other words, the message that I think the user should get is: set
l2-cache-size as high as the maximum amount of memory you're willing to
use for the cache, and QEMU will ensure that it will only use as much as
it needs and no extra memory will be wasted. If you're working with a
100GB image it doesn't matter if you set l2-cache-size to 32MB or
1GB. It will use the exact same amount of RAM.

>> 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 :-)

 1) If l2-cache-size > l2_metadata_size, then make l2-cache-size =
    l2_metadata_size. This is already useful on its own, even with the
    current default of 1MB.

 2) Increase the default to 32MB. This won't waste additional memory for
    small images because of the previous patch, and will cover images up
    to 256GB. If you have larger images you would need to increase
    l2-cache-size manually if you want to cache all the L2 metadata.

The way I see it is that the important change is (1): it's safe to
increase l2-cache-size, it won't waste memory[*]. (2) is nice too
because it will cover larger images by default, but if you're running
VMs with disk images larger than 256GB (which is not an extreme
scenario) you still need to increase the cache.

[*] In practice it's not wasting too much memory even now, because even
if you allocate a 32MB cache but only need 1MB, those extra 31MB are
going to be uninitialized and unused, and therefore not taking up any
physical memory. But you still need to have Qcow2CachedTable structures
for those entries, and any loop that needs to walk Qcow2Cache.entries is
going to be slower. So it's a good idea not to allocate unneeded memory
and to make this an explicit promise to the user.

Berto

Re: [Qemu-devel] [PATCH v3 3/5] qcow2: Resize the cache upon image resizing

[Alberto Garcia]

On Wed 08 Aug 2018 09:10:49 AM CEST, Leonid Bloch wrote:
> The caches are now recalculated upon image resizing. This is done
> because the new default behavior of assigning a sufficient L2 cache to
> cover the entire image implies that the cache will still be sufficient
> after an image resizing.

This is related to what I mentioned in the previous patch. The default
behavior doesn't make the cache try to cover the entire image (or at
least it doesn't *extend* the cache, which is what I understand from
this paragraph). What it does is *reduce* the cache if the smaller
version is enough for the entire image.

> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
> ---
>  block/qcow2.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 98cb96aaca..f60cb92169 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3639,6 +3639,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>          }
>      }
>  
> +    bs->total_sectors = offset / BDRV_SECTOR_SIZE;
> +
>      /* write updated header.size */
>      offset = cpu_to_be64(offset);
>      ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
> @@ -3649,6 +3651,12 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>      }
>  
>      s->l1_vm_state_index = new_l1_size;

You could add an empty line here for readability.

> +    /* Update cache sizes */
> +    QDict *options = qdict_clone_shallow(bs->options);

C99 allows variable declarations in the middle of a block, but we're
still doing it at the beginning (I don't know if there's a good reason
for this?).

Otherwise the patch looks good to me. Thanks!

Berto

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

[Leonid Bloch]

On 08/08/2018 04:58 PM, Alberto Garcia wrote:
> On Wed 08 Aug 2018 03:07:10 PM CEST, Leonid Bloch wrote:
>> On 08/08/2018 03:39 PM, Alberto Garcia wrote:
>>> On Wed 08 Aug 2018 09:10:48 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). For cases with very
>>>> large images and/or small cluster sizes, there is an upper limit on the
>>>> default L2 cache: 32 MB.
>>>
>>> I find this description a bit confusing.
>>>
>>> First of all, because it's not true that the default will cover the
>>> whole image: we're just increasing it, but any image > 256GB is going to
>>> need more than 32MB (with 64KB clusters, that is).
>>
>> How about the following:
>>
>> qcow2: Make the default L2 cache try to cover the entire image
> 
> That's what I think it's misleading, the patch only increases the
> default cache size value. The current 1MB cache also tries to cover the
> entire image, if the entire image is <= 8GB. But there's no new feature
> that extends the cache size to cover the entire image. There's no "we
> used to cover 8GB images by default, but now we cover all images".
> 
> In other words, the message that I think the user should get is: set
> l2-cache-size as high as the maximum amount of memory you're willing to
> use for the cache, and QEMU will ensure that it will only use as much as
> it needs and no extra memory will be wasted. If you're working with a
> 100GB image it doesn't matter if you set l2-cache-size to 32MB or
> 1GB. It will use the exact same amount of RAM.

Then:

qcow2: Make the L2 cache size relative to the size of the image

[Something like] Now the L2 cache will be only as large as needed to 
cover the entire image, until a preset maximum (which is 32 MB by default).
?

> 
>>> 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.

Leonid.

> 
>   1) If l2-cache-size > l2_metadata_size, then make l2-cache-size =
>      l2_metadata_size. This is already useful on its own, even with the
>      current default of 1MB.
> 
>   2) Increase the default to 32MB. This won't waste additional memory for
>      small images because of the previous patch, and will cover images up
>      to 256GB. If you have larger images you would need to increase
>      l2-cache-size manually if you want to cache all the L2 metadata.
> 
> The way I see it is that the important change is (1): it's safe to
> increase l2-cache-size, it won't waste memory[*]. (2) is nice too
> because it will cover larger images by default, but if you're running
> VMs with disk images larger than 256GB (which is not an extreme
> scenario) you still need to increase the cache.
> 
> [*] In practice it's not wasting too much memory even now, because even
> if you allocate a 32MB cache but only need 1MB, those extra 31MB are
> going to be uninitialized and unused, and therefore not taking up any
> physical memory. But you still need to have Qcow2CachedTable structures
> for those entries, and any loop that needs to walk Qcow2Cache.entries is
> going to be slower. So it's a good idea not to allocate unneeded memory
> and to make this an explicit promise to the user.
> 
> Berto
> 

Re: [Qemu-devel] [PATCH v3 3/5] qcow2: Resize the cache upon image resizing

[Leonid Bloch]

On 08/08/2018 05:13 PM, Alberto Garcia wrote:
> On Wed 08 Aug 2018 09:10:49 AM CEST, Leonid Bloch wrote:
>> The caches are now recalculated upon image resizing. This is done
>> because the new default behavior of assigning a sufficient L2 cache to
>> cover the entire image implies that the cache will still be sufficient
>> after an image resizing.
> 
> This is related to what I mentioned in the previous patch. The default
> behavior doesn't make the cache try to cover the entire image (or at
> least it doesn't *extend* the cache, which is what I understand from
> this paragraph). What it does is *reduce* the cache if the smaller
> version is enough for the entire image.

But it doesn't say that it extends the cache. It says that it *adapts* 
the cache to the image size, and therefore it should be resized when the 
image is resized. At least I understand it this way. That said, I'd 
mention the limit there, instead of just "sufficient".

> 
>> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
>> ---
>>   block/qcow2.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 98cb96aaca..f60cb92169 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -3639,6 +3639,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>           }
>>       }
>>   
>> +    bs->total_sectors = offset / BDRV_SECTOR_SIZE;
>> +
>>       /* write updated header.size */
>>       offset = cpu_to_be64(offset);
>>       ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
>> @@ -3649,6 +3651,12 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>       }
>>   
>>       s->l1_vm_state_index = new_l1_size;
> 
> You could add an empty line here for readability.

Yes, definitely, I will. Thanks.

> 
>> +    /* Update cache sizes */
>> +    QDict *options = qdict_clone_shallow(bs->options);
> 
> C99 allows variable declarations in the middle of a block, but we're
> still doing it at the beginning (I don't know if there's a good reason
> for this?).

I did it for readability, and I didn't see a style directive for this. 
But if the style requires it - no problem. :)

> 
> Otherwise the patch looks good to me. Thanks!

Thanks!

Leonid.

> 
> Berto
> 

Re: [Qemu-devel] [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

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

[Alberto Garcia]

On Wed 08 Aug 2018 03:58:02 PM CEST, Alberto Garcia wrote:
>  1) If l2-cache-size > l2_metadata_size, then make l2-cache-size =
>     l2_metadata_size. This is already useful on its own, even with the
>     current default of 1MB.
>
>  2) Increase the default to 32MB. This won't waste additional memory for
>     small images because of the previous patch, and will cover images up
>     to 256GB. If you have larger images you would need to increase
>     l2-cache-size manually if you want to cache all the L2 metadata.

Let's try this way: we had many users requesting us to add a new option
to set l2-cache-size=100%, but we never agreed on a good API. Patch (1)
would do precisely that (l2-cache-size=1T). Patch (2) changes the
default, which may be better and probably enough for many users, but
it's not what solves the problem.

Berto

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

[Leonid Bloch]

On 08/08/2018 06:16 PM, Alberto Garcia wrote:
> 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).

Yes, just with a small fix: my aim is not to make the user forget about 
the L2 cache, my aim is to make it as large as needed to cover the 
entire image in order to increase the performance. This implies 
increasing its size. Because for images smaller than 8 GB the 
performance will stay the same, and the memory usage will not be that 
different: less than 1 MB difference, while the overall QEMU memory 
overhead is about 600 MB. That's why I think that increasing the max 
size is an integral part of this patch, because just changing the 
behavior, without changing the max size, will not cause a noticeable 
improvement. But it will cause some complications, like changing the 
code for the current maximal value, and then changing to 32 MB in a 
separate patch. This doesn't look necessary to me.

Leonid.

> 
> 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
>