[Qemu-devel] [PATCH v2 0/4] qcow2: Allow runtime specification of cache sizes

[Qemu-devel] [PATCH v2 0/4] qcow2: Allow runtime specification of cache sizes

[Max Reitz]

Currently, the metadata cache size is only tunable on compile time
through macros. However, some users may want to use the minimal cache
size (for whatever reason) and others may want to increase the cache
size because they have enough memory and want to increase performance.

This series adds runtime options for setting the cache size in bytes
(which is an easily comprehensible unit) in various ways (by setting
each cache explicitly or the total size).


This series (patch 2) depends on Markus' series
"[PATCH v2 0/4] block: Use g_new() & friends more".


v2:
 - Patch 2: c->entries may be NULL in the fail path; respect that case


git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/4:[----] [--] 'qcow2: Constant cache size in bytes'
002/4:[0006] [FC] 'qcow2: Use g_try_new0() for cache array'
003/4:[----] [--] 'qcow2: Add runtime options for cache sizes'
004/4:[----] [--] 'iotests: Add test for qcow2's cache options'


Max Reitz (4):
  qcow2: Constant cache size in bytes
  qcow2: Use g_try_new0() for cache array
  qcow2: Add runtime options for cache sizes
  iotests: Add test for qcow2's cache options

 block/qcow2-cache.c        |  13 +++--
 block/qcow2.c              | 120 +++++++++++++++++++++++++++++++++++++++++----
 block/qcow2.h              |  13 ++++-
 tests/qemu-iotests/103     |  99 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/103.out |  29 +++++++++++
 tests/qemu-iotests/group   |   1 +
 6 files changed, 259 insertions(+), 16 deletions(-)
 create mode 100755 tests/qemu-iotests/103
 create mode 100644 tests/qemu-iotests/103.out

-- 
2.0.4

[Qemu-devel] [PATCH v2 1/4] qcow2: Constant cache size in bytes

[Max Reitz]

Specifying the metadata cache sizes in clusters results in less clusters
(and much less bytes) covered for small cluster sizes and vice versa.
Using a constant byte size reduces this difference, and makes it
possible to manually specify the cache size in an easily comprehensible
unit.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 16 ++++++++++++++--
 block/qcow2.h | 10 ++++++++--
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 435e0e1..910d9cf 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -470,6 +470,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     uint64_t l1_vm_state_index;
     const char *opt_overlap_check;
     int overlap_check_template = 0;
+    uint64_t l2_cache_size, refcount_cache_size;
 
     ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
     if (ret < 0) {
@@ -707,8 +708,19 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* alloc L2 table/refcount block cache */
-    s->l2_table_cache = qcow2_cache_create(bs, L2_CACHE_SIZE);
-    s->refcount_block_cache = qcow2_cache_create(bs, REFCOUNT_CACHE_SIZE);
+    l2_cache_size = DEFAULT_L2_CACHE_BYTE_SIZE / s->cluster_size;
+    if (l2_cache_size < MIN_L2_CACHE_SIZE) {
+        l2_cache_size = MIN_L2_CACHE_SIZE;
+    }
+
+    refcount_cache_size = l2_cache_size
+                        / (DEFAULT_L2_REFCOUNT_SIZE_RATIO * s->cluster_size);
+    if (refcount_cache_size < MIN_REFCOUNT_CACHE_SIZE) {
+        refcount_cache_size = MIN_REFCOUNT_CACHE_SIZE;
+    }
+
+    s->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
+    s->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
     if (s->l2_table_cache == NULL || s->refcount_block_cache == NULL) {
         error_setg(errp, "Could not allocate metadata caches");
         ret = -ENOMEM;
diff --git a/block/qcow2.h b/block/qcow2.h
index b49424b..671783d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -64,10 +64,16 @@
 #define MIN_CLUSTER_BITS 9
 #define MAX_CLUSTER_BITS 21
 
-#define L2_CACHE_SIZE 16
+#define MIN_L2_CACHE_SIZE 1 /* cluster */
 
 /* Must be at least 4 to cover all cases of refcount table growth */
-#define REFCOUNT_CACHE_SIZE 4
+#define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */
+
+#define DEFAULT_L2_CACHE_BYTE_SIZE 1048576 /* bytes */
+
+/* The refblock cache needs only a fourth of the L2 cache size to cover as many
+ * clusters */
+#define DEFAULT_L2_REFCOUNT_SIZE_RATIO 4
 
 #define DEFAULT_CLUSTER_SIZE 65536
 
-- 
2.0.4

[Qemu-devel] [PATCH v2 2/4] qcow2: Use g_try_new0() for cache array

[Max Reitz]

With a variable cache size, the number given to qcow2_cache_create() may
be huge. Therefore, use g_try_new0().

While at it, use g_new0() instead of g_malloc0() for allocating the
Qcow2Cache object.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cache.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index fe0615a..904f6b1 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -48,9 +48,12 @@ Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables)
     Qcow2Cache *c;
     int i;
 
-    c = g_malloc0(sizeof(*c));
+    c = g_new0(Qcow2Cache, 1);
     c->size = num_tables;
-    c->entries = g_new0(Qcow2CachedTable, num_tables);
+    c->entries = g_try_new0(Qcow2CachedTable, num_tables);
+    if (!c->entries) {
+        goto fail;
+    }
 
     for (i = 0; i < c->size; i++) {
         c->entries[i].table = qemu_try_blockalign(bs->file, s->cluster_size);
@@ -62,8 +65,10 @@ Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables)
     return c;
 
 fail:
-    for (i = 0; i < c->size; i++) {
-        qemu_vfree(c->entries[i].table);
+    if (c->entries) {
+        for (i = 0; i < c->size; i++) {
+            qemu_vfree(c->entries[i].table);
+        }
     }
     g_free(c->entries);
     g_free(c);
-- 
2.0.4

[Qemu-devel] [PATCH v2 3/4] qcow2: Add runtime options for cache sizes

[Max Reitz]

Add options for specifying the size of the metadata caches. This can
either be done directly for each cache (if only one is given, the other
will be derived according to a default ratio) or combined for both.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
 block/qcow2.h |   3 ++
 2 files changed, 103 insertions(+), 12 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 910d9cf..f9e045f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -442,6 +442,22 @@ static QemuOptsList qcow2_runtime_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "Check for unintended writes into an inactive L2 table",
         },
+        {
+            .name = QCOW2_OPT_CACHE_SIZE,
+            .type = QEMU_OPT_SIZE,
+            .help = "Maximum combined metadata (L2 tables and refcount blocks) "
+                    "cache size",
+        },
+        {
+            .name = QCOW2_OPT_L2_CACHE_SIZE,
+            .type = QEMU_OPT_SIZE,
+            .help = "Maximum L2 table cache size",
+        },
+        {
+            .name = QCOW2_OPT_REFCOUNT_CACHE_SIZE,
+            .type = QEMU_OPT_SIZE,
+            .help = "Maximum refcount block cache size",
+        },
         { /* end of list */ }
     },
 };
@@ -457,6 +473,61 @@ static const char *overlap_bool_option_names[QCOW2_OL_MAX_BITNR] = {
     [QCOW2_OL_INACTIVE_L2_BITNR]    = QCOW2_OPT_OVERLAP_INACTIVE_L2,
 };
 
+static void read_cache_sizes(QemuOpts *opts, uint64_t *l2_cache_size,
+                             uint64_t *refcount_cache_size, Error **errp)
+{
+    uint64_t combined_cache_size;
+    bool l2_cache_size_set, refcount_cache_size_set, combined_cache_size_set;
+
+    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);
+    *refcount_cache_size = qemu_opt_get_size(opts,
+                                             QCOW2_OPT_REFCOUNT_CACHE_SIZE, 0);
+
+    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 "
+                       "the same time");
+            return;
+        } else if (*l2_cache_size > combined_cache_size) {
+            error_setg(errp, QCOW2_OPT_L2_CACHE_SIZE " may not exceed "
+                       QCOW2_OPT_CACHE_SIZE);
+            return;
+        } else if (*refcount_cache_size > combined_cache_size) {
+            error_setg(errp, QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not exceed "
+                       QCOW2_OPT_CACHE_SIZE);
+            return;
+        }
+
+        if (l2_cache_size_set) {
+            *refcount_cache_size = combined_cache_size - *l2_cache_size;
+        } 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;
+        }
+    } else {
+        if (!l2_cache_size_set && !refcount_cache_size_set) {
+            *l2_cache_size = DEFAULT_L2_CACHE_BYTE_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;
+        }
+    }
+}
+
 static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
                       Error **errp)
 {
@@ -707,18 +778,43 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
-    /* alloc L2 table/refcount block cache */
-    l2_cache_size = DEFAULT_L2_CACHE_BYTE_SIZE / s->cluster_size;
+    /* get L2 table/refcount block cache size from command line options */
+    opts = qemu_opts_create(&qcow2_runtime_opts, NULL, 0, &error_abort);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    read_cache_sizes(opts, &l2_cache_size, &refcount_cache_size, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    l2_cache_size /= s->cluster_size;
     if (l2_cache_size < MIN_L2_CACHE_SIZE) {
         l2_cache_size = MIN_L2_CACHE_SIZE;
     }
+    if (l2_cache_size > INT_MAX) {
+        error_setg(errp, "L2 cache size too big");
+        ret = -EINVAL;
+        goto fail;
+    }
 
-    refcount_cache_size = l2_cache_size
-                        / (DEFAULT_L2_REFCOUNT_SIZE_RATIO * s->cluster_size);
+    refcount_cache_size /= s->cluster_size;
     if (refcount_cache_size < MIN_REFCOUNT_CACHE_SIZE) {
         refcount_cache_size = MIN_REFCOUNT_CACHE_SIZE;
     }
+    if (refcount_cache_size > INT_MAX) {
+        error_setg(errp, "Refcount cache size too big");
+        ret = -EINVAL;
+        goto fail;
+    }
 
+    /* alloc L2 table/refcount block cache */
     s->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
     s->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
     if (s->l2_table_cache == NULL || s->refcount_block_cache == NULL) {
@@ -810,14 +906,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* Enable lazy_refcounts according to image and command line options */
-    opts = qemu_opts_create(&qcow2_runtime_opts, NULL, 0, &error_abort);
-    qemu_opts_absorb_qdict(opts, options, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        ret = -EINVAL;
-        goto fail;
-    }
-
     s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
         (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
 
diff --git a/block/qcow2.h b/block/qcow2.h
index 671783d..6aeb7ea 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -91,6 +91,9 @@
 #define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE "overlap-check.snapshot-table"
 #define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
 #define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
+#define QCOW2_OPT_CACHE_SIZE "cache-size"
+#define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
+#define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
 
 typedef struct QCowHeader {
     uint32_t magic;
-- 
2.0.4

[Qemu-devel] [PATCH v2 4/4] iotests: Add test for qcow2's cache options

[Max Reitz]

Add a test which tests various combinations of qcow2's cache options
(some of which are valid, some of which are not).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/103     | 99 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/103.out | 29 ++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 129 insertions(+)
 create mode 100755 tests/qemu-iotests/103
 create mode 100644 tests/qemu-iotests/103.out

diff --git a/tests/qemu-iotests/103 b/tests/qemu-iotests/103
new file mode 100755
index 0000000..0f1dc9f
--- /dev/null
+++ b/tests/qemu-iotests/103
@@ -0,0 +1,99 @@
+#!/bin/bash
+#
+# Test case for qcow2 metadata cache size specification
+#
+# Copyright (C) 2014 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq=$(basename $0)
+echo "QA output created by $seq"
+
+here=$PWD
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+IMG_SIZE=64K
+
+_make_test_img $IMG_SIZE
+$QEMU_IO -c 'write -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo '=== Testing invalid option combinations ==='
+echo
+
+# all sizes set at the same time
+$QEMU_IO -c "open -o cache-size=1.25M,l2-cache-size=1M,refcount-cache-size=0.25M $TEST_IMG" \
+    2>&1 | _filter_testdir | _filter_imgfmt
+# l2-cache-size may not exceed cache-size
+$QEMU_IO -c "open -o cache-size=1M,l2-cache-size=2M $TEST_IMG" 2>&1 \
+    | _filter_testdir | _filter_imgfmt
+# refcount-cache-size may not exceed cache-size
+$QEMU_IO -c "open -o cache-size=1M,refcount-cache-size=2M $TEST_IMG" 2>&1 \
+    | _filter_testdir | _filter_imgfmt
+# 0 should be a valid size (e.g. for enforcing the minimum), so this should not
+# work
+$QEMU_IO -c "open -o cache-size=0,l2-cache-size=0,refcount-cache-size=0 $TEST_IMG" \
+    2>&1 | _filter_testdir | _filter_imgfmt
+
+echo
+echo '=== Testing valid option combinations ==='
+echo
+
+# There should be a reasonable and working minimum
+$QEMU_IO -c "open -o cache-size=0 $TEST_IMG" -c 'read -P 42 0 64k' \
+    | _filter_qemu_io
+$QEMU_IO -c "open -o l2-cache-size=0 $TEST_IMG" -c 'read -P 42 0 64k' \
+    | _filter_qemu_io
+$QEMU_IO -c "open -o refcount-cache-size=0 $TEST_IMG" -c 'read -P 42 0 64k' \
+    | _filter_qemu_io
+
+# Derive cache sizes from combined size (with a reasonable ratio, but we cannot
+# test that)
+$QEMU_IO -c "open -o cache-size=2M $TEST_IMG" -c 'read -P 42 0 64k' \
+    | _filter_qemu_io
+# Fix one cache, derive the other
+$QEMU_IO -c "open -o cache-size=2M,l2-cache-size=1M $TEST_IMG" \
+         -c 'read -P 42 0 64k' \
+    | _filter_qemu_io
+$QEMU_IO -c "open -o cache-size=2M,refcount-cache-size=1M $TEST_IMG" \
+         -c 'read -P 42 0 64k' \
+    | _filter_qemu_io
+# Directly set both caches
+$QEMU_IO -c "open -o l2-cache-size=1M,refcount-cache-size=0.25M $TEST_IMG" \
+         -c 'read -P 42 0 64k' \
+    | _filter_qemu_io
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/103.out b/tests/qemu-iotests/103.out
new file mode 100644
index 0000000..ddf6b5a
--- /dev/null
+++ b/tests/qemu-iotests/103.out
@@ -0,0 +1,29 @@
+QA output created by 103
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536 
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing invalid option combinations ===
+
+qemu-io: can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and refcount-cache-size may not be set the same time
+qemu-io: can't open device TEST_DIR/t.IMGFMT: l2-cache-size may not exceed cache-size
+qemu-io: can't open device TEST_DIR/t.IMGFMT: refcount-cache-size may not exceed cache-size
+qemu-io: can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and refcount-cache-size may not be set the same time
+
+=== Testing valid option combinations ===
+
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 6e67f61..1c8d453 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -100,3 +100,4 @@
 091 rw auto quick
 092 rw auto quick
 095 rw auto quick
+103 rw auto quick
-- 
2.0.4

Re: [Qemu-devel] [PATCH v2 1/4] qcow2: Constant cache size in bytes

[Kevin Wolf]

Am 18.08.2014 um 22:07 hat Max Reitz geschrieben:
> Specifying the metadata cache sizes in clusters results in less clusters
> (and much less bytes) covered for small cluster sizes and vice versa.
> Using a constant byte size reduces this difference, and makes it
> possible to manually specify the cache size in an easily comprehensible
> unit.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.c | 16 ++++++++++++++--
>  block/qcow2.h | 10 ++++++++--
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 435e0e1..910d9cf 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -470,6 +470,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>      uint64_t l1_vm_state_index;
>      const char *opt_overlap_check;
>      int overlap_check_template = 0;
> +    uint64_t l2_cache_size, refcount_cache_size;
>  
>      ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
>      if (ret < 0) {
> @@ -707,8 +708,19 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>  
>      /* alloc L2 table/refcount block cache */
> -    s->l2_table_cache = qcow2_cache_create(bs, L2_CACHE_SIZE);
> -    s->refcount_block_cache = qcow2_cache_create(bs, REFCOUNT_CACHE_SIZE);
> +    l2_cache_size = DEFAULT_L2_CACHE_BYTE_SIZE / s->cluster_size;
> +    if (l2_cache_size < MIN_L2_CACHE_SIZE) {
> +        l2_cache_size = MIN_L2_CACHE_SIZE;
> +    }
> +
> +    refcount_cache_size = l2_cache_size
> +                        / (DEFAULT_L2_REFCOUNT_SIZE_RATIO * s->cluster_size);

The factor s->cluster_size is too much, l2_cache_size is already in
clusters. I think one of the other patches in the series fixes this
again, at least I can't see the bug in the final applied version.

If you agree, I'll change this patch while applying to make the series
bisectable.

Kevin

> +    if (refcount_cache_size < MIN_REFCOUNT_CACHE_SIZE) {
> +        refcount_cache_size = MIN_REFCOUNT_CACHE_SIZE;
> +    }
> +
> +    s->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
> +    s->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
>      if (s->l2_table_cache == NULL || s->refcount_block_cache == NULL) {
>          error_setg(errp, "Could not allocate metadata caches");
>          ret = -ENOMEM;
> diff --git a/block/qcow2.h b/block/qcow2.h
> index b49424b..671783d 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -64,10 +64,16 @@
>  #define MIN_CLUSTER_BITS 9
>  #define MAX_CLUSTER_BITS 21
>  
> -#define L2_CACHE_SIZE 16
> +#define MIN_L2_CACHE_SIZE 1 /* cluster */
>  
>  /* Must be at least 4 to cover all cases of refcount table growth */
> -#define REFCOUNT_CACHE_SIZE 4
> +#define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */
> +
> +#define DEFAULT_L2_CACHE_BYTE_SIZE 1048576 /* bytes */
> +
> +/* The refblock cache needs only a fourth of the L2 cache size to cover as many
> + * clusters */
> +#define DEFAULT_L2_REFCOUNT_SIZE_RATIO 4
>  
>  #define DEFAULT_CLUSTER_SIZE 65536
>  
> -- 
> 2.0.4
> 

Re: [Qemu-devel] [PATCH v2 0/4] qcow2: Allow runtime specification of cache sizes

[Kevin Wolf]

Am 18.08.2014 um 22:07 hat Max Reitz geschrieben:
> Currently, the metadata cache size is only tunable on compile time
> through macros. However, some users may want to use the minimal cache
> size (for whatever reason) and others may want to increase the cache
> size because they have enough memory and want to increase performance.
> 
> This series adds runtime options for setting the cache size in bytes
> (which is an easily comprehensible unit) in various ways (by setting
> each cache explicitly or the total size).
> 
> 
> This series (patch 2) depends on Markus' series
> "[PATCH v2 0/4] block: Use g_new() & friends more".
> 
> 
> v2:
>  - Patch 2: c->entries may be NULL in the fail path; respect that case

Thanks, applied all to the block branch (with patch 1 changed as
commented there).

Kevin

Re: [Qemu-devel] [PATCH v2 0/4] qcow2: Allow runtime specification of cache sizes

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

On 08/19/2014 08:00 AM, Kevin Wolf wrote:
> Am 18.08.2014 um 22:07 hat Max Reitz geschrieben:
>> Currently, the metadata cache size is only tunable on compile time
>> through macros. However, some users may want to use the minimal cache
>> size (for whatever reason) and others may want to increase the cache
>> size because they have enough memory and want to increase performance.
>>
>> This series adds runtime options for setting the cache size in bytes
>> (which is an easily comprehensible unit) in various ways (by setting
>> each cache explicitly or the total size).
>>
>>
>> This series (patch 2) depends on Markus' series
>> "[PATCH v2 0/4] block: Use g_new() & friends more".
>>
>>
>> v2:
>>  - Patch 2: c->entries may be NULL in the fail path; respect that case
> 
> Thanks, applied all to the block branch (with patch 1 changed as
> commented there).

I'd still like to see the changes to qapi/block-core.json that expose
these for hot-plugging before we call this series finished (I mentioned
that in my review of v1, without realizing that v2 had already been sent).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH v2 0/4] qcow2: Allow runtime specification of cache sizes

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1448 bytes --]

Am 19.08.2014 um 16:18 hat Eric Blake geschrieben:
> On 08/19/2014 08:00 AM, Kevin Wolf wrote:
> > Am 18.08.2014 um 22:07 hat Max Reitz geschrieben:
> >> Currently, the metadata cache size is only tunable on compile time
> >> through macros. However, some users may want to use the minimal cache
> >> size (for whatever reason) and others may want to increase the cache
> >> size because they have enough memory and want to increase performance.
> >>
> >> This series adds runtime options for setting the cache size in bytes
> >> (which is an easily comprehensible unit) in various ways (by setting
> >> each cache explicitly or the total size).
> >>
> >>
> >> This series (patch 2) depends on Markus' series
> >> "[PATCH v2 0/4] block: Use g_new() & friends more".
> >>
> >>
> >> v2:
> >>  - Patch 2: c->entries may be NULL in the fail path; respect that case
> > 
> > Thanks, applied all to the block branch (with patch 1 changed as
> > commented there).
> 
> I'd still like to see the changes to qapi/block-core.json that expose
> these for hot-plugging before we call this series finished (I mentioned
> that in my review of v1, without realizing that v2 had already been sent).

Yes, we need this. But Max replied there that he'd send a follow-up and
that was good enough for me because my experience is that he is one of
the few people who don't only promise follow-up patches, but actually
send them.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]