[Qemu-devel] [PATCH v5 0/4] Introduction of l2-cache-full option for qcow2 images

[Qemu-devel] [PATCH v5 0/4] Introduction of l2-cache-full option for qcow2 images

[Leonid Bloch]

This series introduces an option to calculate and allocate
automatically enough qcow2 L2 cache to cover the entire image.
Using cache that covers the entire image can benefit performance,
while having only a small memory overhead (just 1 MB for every 8 GB
of virtual image size with the default cluster size).

-------------------------
Differences from v1:

1) Documentation fixes in qapi/block-core.json and qemu-options.hx
2) Removal of the patch which was made to fix the default sizes in
   docs/qcow2-cache.txt - it is not needed, as the default sizes imply
   also default cluster sizes.
3) Documentation fixes in docs/qcow2-cache.txt, mentioning mutual
   exclusivity of the options.
4) Squashing the iotests patch into the main feature addition patch

-------------------------
Differences from v2:

1) A separate patch for the grammar fix for 3.0
2) A separate patch for existing documentation fixes for 3.0
3) Separated back the iotests patch, because the grammar fix is separate now

-------------------------
Differences from v3:

1) Grammar change commit message fix
2) Rewording the documentation more concisely
3) Squashing the l2-cache-full docs commit to the one that introduces this
   feature

-------------------------
Differences from v4:

1) l2-cache-full is now a proper boolean option.
2) tests/qemu-iotests/103.out fixed: the grammar fix is now applied to the
   second relevant occurrence.
3) Error messages fixed in iotests for the l2-cache-full tests.

Leonid Bloch (4):
  qcow2: A grammar fix in conflicting cache sizing error message
  qcow2: Options' documentation fixes
  qcow2: Introduce an option for sufficient L2 cache for the entire
    image
  iotests: Add tests for the new l2-cache-full option

 block/qcow2.c              | 37 +++++++++++++++++++++++++++++--------
 block/qcow2.h              |  1 +
 docs/qcow2-cache.txt       | 18 ++++++++++++++----
 qapi/block-core.json       |  8 +++++++-
 qemu-options.hx            | 14 ++++++++++----
 tests/qemu-iotests/103     |  6 ++++++
 tests/qemu-iotests/103.out |  6 ++++--
 tests/qemu-iotests/137     |  2 ++
 tests/qemu-iotests/137.out |  4 +++-
 9 files changed, 76 insertions(+), 20 deletions(-)

-- 
2.14.1

[Qemu-devel] [PATCH v5 1/4 for-3.0] qcow2: A grammar fix in conflicting cache sizing error message

[Leonid Bloch]

Signed-off-by: Leonid Bloch <lbloch@janustech.com>
---
 block/qcow2.c              | 2 +-
 tests/qemu-iotests/103.out | 4 ++--
 tests/qemu-iotests/137.out | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 6162ed8be2..ec9e6238a0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -797,7 +797,7 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
         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");
+                       "at the same time");
             return;
         } else if (*l2_cache_size > combined_cache_size) {
             error_setg(errp, QCOW2_OPT_L2_CACHE_SIZE " may not exceed "
diff --git a/tests/qemu-iotests/103.out b/tests/qemu-iotests/103.out
index bd45d3875a..bd9eec3250 100644
--- a/tests/qemu-iotests/103.out
+++ b/tests/qemu-iotests/103.out
@@ -5,10 +5,10 @@ wrote 65536/65536 bytes at offset 0
 
 === Testing invalid option combinations ===
 
-can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and refcount-cache-size may not be set the same time
+can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and refcount-cache-size may not be set at the same time
 can't open device TEST_DIR/t.IMGFMT: l2-cache-size may not exceed cache-size
 can't open device TEST_DIR/t.IMGFMT: refcount-cache-size may not exceed cache-size
-can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and refcount-cache-size may not be set the same time
+can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and refcount-cache-size may not be set at the same time
 can't open device TEST_DIR/t.IMGFMT: L2 cache entry size must be a power of two between 512 and the cluster size (65536)
 can't open device TEST_DIR/t.IMGFMT: L2 cache entry size must be a power of two between 512 and the cluster size (65536)
 can't open device TEST_DIR/t.IMGFMT: L2 cache entry size must be a power of two between 512 and the cluster size (65536)
diff --git a/tests/qemu-iotests/137.out b/tests/qemu-iotests/137.out
index 96724a6c33..6a2ffc71fd 100644
--- a/tests/qemu-iotests/137.out
+++ b/tests/qemu-iotests/137.out
@@ -16,7 +16,7 @@ read 33554432/33554432 bytes at offset 0
 === Try setting some invalid values ===
 
 Parameter 'lazy-refcounts' expects 'on' or 'off'
-cache-size, l2-cache-size and refcount-cache-size may not be set the same time
+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
-- 
2.14.1

[Qemu-devel] [PATCH v5 2/4 for-3.0] qcow2: Options' documentation fixes

[Leonid Bloch]

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

diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
index 8a09a5cc5f..3673f2be0e 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..13ece21cb6 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -752,15 +752,17 @@ 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)
 
 @item l2-cache-size
-The maximum size of the L2 table cache in bytes
-(default: 4/5 of the total cache size)
+The maximum size of the L2 table cache.
+(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 any portion of the cache-size, if it is
+specified and large enough, left over after allocating the full L2 cache)
 
 @item cache-clean-interval
 Clean unused entries in the L2 and refcount caches. The interval is in seconds.
-- 
2.14.1

[Qemu-devel] [PATCH v5 3/4] qcow2: Introduce an option for sufficient L2 cache for the entire image

[Leonid Bloch]

An option "l2-cache-full" is introduced to automatically set the qcow2
L2 cache to a sufficient value for covering the entire image. The memory
overhead when using this option is not big (1 MB for each 8 GB of
virtual image size with the default cluster size) and it can noticeably
improve performance when using large images with frequent I/O.
Previously, for this functionality the correct L2 cache size needed to
be calculated manually or with a script, and then this size needed to be
passed to the "l2-cache-size" option. Now it is sufficient to just pass
the boolean "l2-cache-full" option.

Signed-off-by: Leonid Bloch <lbloch@janustech.com>
---
 block/qcow2.c        | 35 ++++++++++++++++++++++++++++-------
 block/qcow2.h        |  1 +
 docs/qcow2-cache.txt | 15 +++++++++++----
 qapi/block-core.json |  8 +++++++-
 qemu-options.hx      |  6 +++++-
 5 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index ec9e6238a0..d8de9be3cd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -695,6 +695,11 @@ static QemuOptsList qcow2_runtime_opts = {
             .type = QEMU_OPT_SIZE,
             .help = "Maximum L2 table cache size",
         },
+        {
+            .name = QCOW2_OPT_L2_CACHE_FULL,
+            .type = QEMU_OPT_BOOL,
+            .help = "Create full coverage of the image with the L2 cache",
+        },
         {
             .name = QCOW2_OPT_L2_CACHE_ENTRY_SIZE,
             .type = QEMU_OPT_SIZE,
@@ -779,10 +784,12 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
     BDRVQcow2State *s = bs->opaque;
     uint64_t combined_cache_size;
     bool l2_cache_size_set, refcount_cache_size_set, combined_cache_size_set;
+    bool l2_cache_full_set;
     int 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);
+    l2_cache_full_set = qemu_opt_get_bool(opts, QCOW2_OPT_L2_CACHE_FULL, 0);
     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);
@@ -793,6 +800,17 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
     *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);
+
+    if (l2_cache_size_set && l2_cache_full_set) {
+        error_setg(errp, QCOW2_OPT_L2_CACHE_SIZE " and "
+                   QCOW2_OPT_L2_CACHE_FULL " may not be set at the same time");
+        return;
+    } else if (l2_cache_full_set) {
+        *l2_cache_size = max_l2_cache;
+    }
+
     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
@@ -800,8 +818,14 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
                        "at 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);
+            if (l2_cache_full_set) {
+                error_setg(errp, QCOW2_OPT_CACHE_SIZE " must be greater than "
+                           "the full L2 cache if " QCOW2_OPT_L2_CACHE_FULL
+                           " is used");
+            } else {
+                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 "
@@ -809,14 +833,11 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
             return;
         }
 
-        if (l2_cache_size_set) {
+        if (l2_cache_size_set || l2_cache_full_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 {
-            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) {
@@ -829,7 +850,7 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
             }
         }
     } else {
-        if (!l2_cache_size_set) {
+        if (!l2_cache_size_set && !l2_cache_full_set) {
             *l2_cache_size = MAX(DEFAULT_L2_CACHE_BYTE_SIZE,
                                  (uint64_t)DEFAULT_L2_CACHE_CLUSTERS
                                  * s->cluster_size);
diff --git a/block/qcow2.h b/block/qcow2.h
index 81b844e936..151e014bd8 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -97,6 +97,7 @@
 #define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY "overlap-check.bitmap-directory"
 #define QCOW2_OPT_CACHE_SIZE "cache-size"
 #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
+#define QCOW2_OPT_L2_CACHE_FULL "l2-cache-full"
 #define QCOW2_OPT_L2_CACHE_ENTRY_SIZE "l2-cache-entry-size"
 #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
 #define QCOW2_OPT_CACHE_CLEAN_INTERVAL "cache-clean-interval"
diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
index 3673f2be0e..bae21061f3 100644
--- a/docs/qcow2-cache.txt
+++ b/docs/qcow2-cache.txt
@@ -110,11 +110,12 @@ How to configure the cache sizes
 Cache sizes can be configured using the -drive option in the
 command-line, or the 'blockdev-add' QMP command.
 
-There are three options available, and all of them take bytes:
+There are four options available:
 
-"l2-cache-size":         maximum size of the L2 table cache
-"refcount-cache-size":   maximum size of the refcount block cache
-"cache-size":            maximum size of both caches combined
+"l2-cache-size":         maximum size of the L2 table cache (bytes, K, M)
+"refcount-cache-size":   maximum size of the refcount block cache (bytes, K, M)
+"cache-size":            maximum size of both caches combined (bytes, K, M)
+"l2-cache-full":         make the L2 cache cover the full image (boolean)
 
 There are a few things that need to be taken into account:
 
@@ -130,6 +131,12 @@ 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.
 
+- If "l2-cache-full" is specified, QEMU will assign enough memory
+  to the L2 cache to cover the entire size of the image.
+
+- "l2-cache-size" and "l2-cache-full" can not be set simultaneously, as
+  setting "l2-cache-full" already implies a specific size for the L2 cache.
+
 - At most two of "l2-cache-size", "refcount-cache-size", and "cache-size"
   can be set simultaneously.
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d40d5ecc3b..c584059e23 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2812,7 +2812,12 @@
 #                         refcount block caches in bytes (since 2.2)
 #
 # @l2-cache-size:         the maximum size of the L2 table cache in
-#                         bytes (since 2.2)
+#                         bytes (mutually exclusive with l2-cache-full)
+#                         (since 2.2)
+#
+# @l2-cache-full:         make the L2 table cache large enough to cover the
+#                         entire image (mutually exclusive with l2-cache-size)
+#                         (since 3.1)
 #
 # @l2-cache-entry-size:   the size of each entry in the L2 cache in
 #                         bytes. It must be a power of two between 512
@@ -2840,6 +2845,7 @@
             '*overlap-check': 'Qcow2OverlapChecks',
             '*cache-size': 'int',
             '*l2-cache-size': 'int',
+            '*l2-cache-full': 'bool',
             '*l2-cache-entry-size': 'int',
             '*refcount-cache-size': 'int',
             '*cache-clean-interval': 'int',
diff --git a/qemu-options.hx b/qemu-options.hx
index 13ece21cb6..b493371704 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -754,11 +754,15 @@ image file)
 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.
+The maximum size of the L2 table cache. (Mutually exclusive with l2-cache-full)
 (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 l2-cache-full
+Make the L2 table cache large enough to cover the entire image (mutually
+exclusive with l2-cache-size) (on/off; default: off)
+
 @item refcount-cache-size
 The maximum size of the refcount block cache in bytes
 (default: 4 times the cluster size, or any portion of the cache-size, if it is
-- 
2.14.1

[Qemu-devel] [PATCH v5 4/4] iotests: Add tests for the new l2-cache-full option

[Leonid Bloch]

Signed-off-by: Leonid Bloch <lbloch@janustech.com>
---
 tests/qemu-iotests/103     | 6 ++++++
 tests/qemu-iotests/103.out | 2 ++
 tests/qemu-iotests/137     | 2 ++
 tests/qemu-iotests/137.out | 2 ++
 4 files changed, 12 insertions(+)

diff --git a/tests/qemu-iotests/103 b/tests/qemu-iotests/103
index 2841318492..85010c6a6d 100755
--- a/tests/qemu-iotests/103
+++ b/tests/qemu-iotests/103
@@ -52,9 +52,15 @@ echo
 echo '=== Testing invalid option combinations ==='
 echo
 
+# l2-cache-size and l2-cache-full at the same time
+$QEMU_IO -c "open -o l2-cache-full,l2-cache-size=1M $TEST_IMG" 2>&1 |
+    _filter_testdir | _filter_imgfmt
 # 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
+# cache-size may not be smaller than the full L2 size if l2-cache-full is used
+$QEMU_IO -c "open -o l2-cache-full,cache-size=6 $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
diff --git a/tests/qemu-iotests/103.out b/tests/qemu-iotests/103.out
index bd9eec3250..f4bac5371b 100644
--- a/tests/qemu-iotests/103.out
+++ b/tests/qemu-iotests/103.out
@@ -5,7 +5,9 @@ wrote 65536/65536 bytes at offset 0
 
 === Testing invalid option combinations ===
 
+can't open device TEST_DIR/t.IMGFMT: l2-cache-size and l2-cache-full may not be set at the same time
 can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and refcount-cache-size may not be set at the same time
+can't open device TEST_DIR/t.IMGFMT: cache-size must be greater than the full L2 cache if l2-cache-full is used
 can't open device TEST_DIR/t.IMGFMT: l2-cache-size may not exceed cache-size
 can't open device TEST_DIR/t.IMGFMT: refcount-cache-size may not exceed cache-size
 can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and refcount-cache-size may not be set at the same time
diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
index 87965625d8..f460b5bfe1 100755
--- a/tests/qemu-iotests/137
+++ b/tests/qemu-iotests/137
@@ -106,7 +106,9 @@ echo
 
 $QEMU_IO \
     -c "reopen -o lazy-refcounts=42" \
+    -c "reopen -o l2-cache-full,l2-cache-size=64k" \
     -c "reopen -o cache-size=1M,l2-cache-size=64k,refcount-cache-size=64k" \
+    -c "reopen -o l2-cache-full,cache-size=6K" \
     -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" \
diff --git a/tests/qemu-iotests/137.out b/tests/qemu-iotests/137.out
index 6a2ffc71fd..1ef10ac554 100644
--- a/tests/qemu-iotests/137.out
+++ b/tests/qemu-iotests/137.out
@@ -16,7 +16,9 @@ read 33554432/33554432 bytes at offset 0
 === Try setting some invalid values ===
 
 Parameter 'lazy-refcounts' expects 'on' or 'off'
+l2-cache-size and l2-cache-full may not be set at the same time
 cache-size, l2-cache-size and refcount-cache-size may not be set at the same time
+cache-size must be greater than the full L2 cache if l2-cache-full is used
 l2-cache-size may not exceed cache-size
 refcount-cache-size may not exceed cache-size
 L2 cache size too big
-- 
2.14.1

Re: [Qemu-devel] [Qemu-block] [PATCH v5 1/4 for-3.0] qcow2: A grammar fix in conflicting cache sizing error message

[John Snow]

On 07/25/2018 10:27 AM, Leonid Bloch wrote:
> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
> ---
>  block/qcow2.c              | 2 +-
>  tests/qemu-iotests/103.out | 4 ++--
>  tests/qemu-iotests/137.out | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6162ed8be2..ec9e6238a0 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -797,7 +797,7 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
>          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");
> +                       "at the same time");
>              return;
>          } else if (*l2_cache_size > combined_cache_size) {
>              error_setg(errp, QCOW2_OPT_L2_CACHE_SIZE " may not exceed "
> diff --git a/tests/qemu-iotests/103.out b/tests/qemu-iotests/103.out
> index bd45d3875a..bd9eec3250 100644
> --- a/tests/qemu-iotests/103.out
> +++ b/tests/qemu-iotests/103.out
> @@ -5,10 +5,10 @@ wrote 65536/65536 bytes at offset 0
>  
>  === Testing invalid option combinations ===
>  
> -can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and refcount-cache-size may not be set the same time
> +can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and refcount-cache-size may not be set at the same time
>  can't open device TEST_DIR/t.IMGFMT: l2-cache-size may not exceed cache-size
>  can't open device TEST_DIR/t.IMGFMT: refcount-cache-size may not exceed cache-size
> -can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and refcount-cache-size may not be set the same time
> +can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and refcount-cache-size may not be set at the same time
>  can't open device TEST_DIR/t.IMGFMT: L2 cache entry size must be a power of two between 512 and the cluster size (65536)
>  can't open device TEST_DIR/t.IMGFMT: L2 cache entry size must be a power of two between 512 and the cluster size (65536)
>  can't open device TEST_DIR/t.IMGFMT: L2 cache entry size must be a power of two between 512 and the cluster size (65536)
> diff --git a/tests/qemu-iotests/137.out b/tests/qemu-iotests/137.out
> index 96724a6c33..6a2ffc71fd 100644
> --- a/tests/qemu-iotests/137.out
> +++ b/tests/qemu-iotests/137.out
> @@ -16,7 +16,7 @@ read 33554432/33554432 bytes at offset 0
>  === Try setting some invalid values ===
>  
>  Parameter 'lazy-refcounts' expects 'on' or 'off'
> -cache-size, l2-cache-size and refcount-cache-size may not be set the same time
> +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
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH v5 1/4 for-3.0] qcow2: A grammar fix in conflicting cache sizing error message

[Kevin Wolf]

Am 26.07.2018 um 00:19 hat John Snow geschrieben:
> On 07/25/2018 10:27 AM, Leonid Bloch wrote:
> > Signed-off-by: Leonid Bloch <lbloch@janustech.com>
> 
> Reviewed-by: John Snow <jsnow@redhat.com>

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH v5 2/4 for-3.0] qcow2: Options' documentation fixes

[Kevin Wolf]

Am 25.07.2018 um 16:27 hat Leonid Bloch geschrieben:
> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
> ---
>  docs/qcow2-cache.txt |  3 +++
>  qemu-options.hx      | 10 ++++++----
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
> index 8a09a5cc5f..3673f2be0e 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.

The indentation is off here, the other list items have one space more.

>  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..13ece21cb6 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -752,15 +752,17 @@ 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)

I think it would be good to still say something about the default.
Maybe something like "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)
> +The maximum size of the L2 table cache.

Why did you remove "in bytes" and add a period which the other options
don't have? I prefer the old version of this line.

> +(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 any portion of the cache-size, if it is
> +specified and large enough, left over after allocating the full L2 cache)

I found the second part hard to understand. Maybe "4 times the cluster
size; or if both cache-size and l2-cache-size are given, the part of
the cache-size that is not used for the L2 cache yet."?

Kevin

Re: [Qemu-devel] [PATCH v5 2/4 for-3.0] qcow2: Options' documentation fixes

[Eric Blake]

On 07/26/2018 05:02 AM, Kevin Wolf wrote:
> Am 25.07.2018 um 16:27 hat Leonid Bloch geschrieben:
>> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
>> ---
>>   docs/qcow2-cache.txt |  3 +++
>>   qemu-options.hx      | 10 ++++++----
>>   2 files changed, 9 insertions(+), 4 deletions(-)
>>

>> +++ b/qemu-options.hx
>> @@ -752,15 +752,17 @@ 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)
> 
> I think it would be good to still say something about the default.
> Maybe something like "default: the sum of l2-cache-size and
> refcount-cache-size"?

Except what happens if you specify only one of l2-cache-size or 
refcount-cache-size? Is the defaulted cache-size then just that one size 
you specified (and the other cache ignored), or is the total cache size 
still defaulted to the 1M/8-cluster size (assuming its larger than the 
other size specified)?

> 
>>   @item l2-cache-size
>> -The maximum size of the L2 table cache in bytes
>> -(default: 4/5 of the total cache size)
>> +The maximum size of the L2 table cache.
> 
> Why did you remove "in bytes" and add a period which the other options
> don't have? I prefer the old version of this line.
> 
>> +(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 any portion of the cache-size, if it is
>> +specified and large enough, left over after allocating the full L2 cache)
> 
> I found the second part hard to understand. Maybe "4 times the cluster
> size; or if both cache-size and l2-cache-size are given, the part of
> the cache-size that is not used for the L2 cache yet."?
> 
> Kevin
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v5 2/4 for-3.0] qcow2: Options' documentation fixes

[Leonid Bloch]

On 07/26/2018 01:02 PM, Kevin Wolf wrote:
> Am 25.07.2018 um 16:27 hat Leonid Bloch geschrieben:
>> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
>> ---
>>   docs/qcow2-cache.txt |  3 +++
>>   qemu-options.hx      | 10 ++++++----
>>   2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
>> index 8a09a5cc5f..3673f2be0e 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.
> 
> The indentation is off here, the other list items have one space more.
> 
>>   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..13ece21cb6 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -752,15 +752,17 @@ 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)
> 
> I think it would be good to still say something about the default.
> Maybe something like "default: the sum of l2-cache-size and
> refcount-cache-size"?

Yes, that sounds good!

> 
>>   @item l2-cache-size
>> -The maximum size of the L2 table cache in bytes
>> -(default: 4/5 of the total cache size)
>> +The maximum size of the L2 table cache.
> 
> Why did you remove "in bytes" and add a period which the other options
> don't have? I prefer the old version of this line.

I removed "in bytes" because it also accepts k, M, G, ... but on a 
second thought, these are amounts of bytes as well, so changing back to 
the old version.

>> +(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 any portion of the cache-size, if it is
>> +specified and large enough, left over after allocating the full L2 cache)
> 
> I found the second part hard to understand. Maybe "4 times the cluster
> size; or if both cache-size and l2-cache-size are given, the part of
> the cache-size that is not used for the L2 cache yet."?

That is not accurate. Because even if l2-cache-size is not given and 
cache-size is large enough to accommodate enough L2 cache to cover the 
entire image, plus the minimal amount of refcount cache with room to 
spare - refcount cache will use all the rest of the cache-size. How about:

"default: 4 times the cluster size; or if cache-size is specified, the 
part of it which is not used for the L2 cache"

Leonid.

> Kevin
>

Re: [Qemu-devel] [PATCH v5 2/4 for-3.0] qcow2: Options' documentation fixes

[Leonid Bloch]

On 07/26/2018 05:20 PM, Eric Blake wrote:
> On 07/26/2018 05:02 AM, Kevin Wolf wrote:
>> Am 25.07.2018 um 16:27 hat Leonid Bloch geschrieben:
>>> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
>>> ---
>>>   docs/qcow2-cache.txt |  3 +++
>>>   qemu-options.hx      | 10 ++++++----
>>>   2 files changed, 9 insertions(+), 4 deletions(-)
>>>
> 
>>> +++ b/qemu-options.hx
>>> @@ -752,15 +752,17 @@ 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)
>>
>> I think it would be good to still say something about the default.
>> Maybe something like "default: the sum of l2-cache-size and
>> refcount-cache-size"?
> 
> Except what happens if you specify only one of l2-cache-size or 
> refcount-cache-size? Is the defaulted cache-size then just that one size 
> you specified (and the other cache ignored), or is the total cache size 
> still defaulted to the 1M/8-cluster size (assuming its larger than the 
> other size specified)?

I think that the meaning here is the sum of the actual l2-cache-size and 
refcount-cache-size. Not the specified options, but the actual caches.

Leonid.

Re: [Qemu-devel] [PATCH v5 2/4 for-3.0] qcow2: Options' documentation fixes

[Kevin Wolf]

Am 26.07.2018 um 16:27 hat Leonid Bloch geschrieben:
> On 07/26/2018 01:02 PM, Kevin Wolf wrote:
> > Am 25.07.2018 um 16:27 hat Leonid Bloch geschrieben:
> > > Signed-off-by: Leonid Bloch <lbloch@janustech.com>
> > > ---
> > >   docs/qcow2-cache.txt |  3 +++
> > >   qemu-options.hx      | 10 ++++++----
> > >   2 files changed, 9 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
> > > index 8a09a5cc5f..3673f2be0e 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.
> > 
> > The indentation is off here, the other list items have one space more.
> > 
> > >   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..13ece21cb6 100644
> > > --- a/qemu-options.hx
> > > +++ b/qemu-options.hx
> > > @@ -752,15 +752,17 @@ 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)
> > 
> > I think it would be good to still say something about the default.
> > Maybe something like "default: the sum of l2-cache-size and
> > refcount-cache-size"?
> 
> Yes, that sounds good!
> 
> > 
> > >   @item l2-cache-size
> > > -The maximum size of the L2 table cache in bytes
> > > -(default: 4/5 of the total cache size)
> > > +The maximum size of the L2 table cache.
> > 
> > Why did you remove "in bytes" and add a period which the other options
> > don't have? I prefer the old version of this line.
> 
> I removed "in bytes" because it also accepts k, M, G, ... but on a second
> thought, these are amounts of bytes as well, so changing back to the old
> version.
> 
> > > +(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 any portion of the cache-size, if it is
> > > +specified and large enough, left over after allocating the full L2 cache)
> > 
> > I found the second part hard to understand. Maybe "4 times the cluster
> > size; or if both cache-size and l2-cache-size are given, the part of
> > the cache-size that is not used for the L2 cache yet."?
> 
> That is not accurate. Because even if l2-cache-size is not given and
> cache-size is large enough to accommodate enough L2 cache to cover the
> entire image, plus the minimal amount of refcount cache with room to spare -
> refcount cache will use all the rest of the cache-size.

Oh, you're right! I wasn't aware that we consider the image size there.

> How about:
> 
> "default: 4 times the cluster size; or if cache-size is specified, the part
> of it which is not used for the L2 cache"

Even simpler, easier to understand and more accurate. I like it.

Kevin

Re: [Qemu-devel] [PATCH v5 2/4 for-3.0] qcow2: Options' documentation fixes

[Leonid Bloch]

>> How about:
>>
>> "default: 4 times the cluster size; or if cache-size is specified, the part
>> of it which is not used for the L2 cache"
> 
> Even simpler, easier to understand and more accurate. I like it.

Thanks!

> 
> Kevin
>