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

[Qemu-devel] [PATCH v3 0/5] 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

Leonid Bloch (5):
  A grammar fix
  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
  docs: Document the 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            | 19 +++++++++++++++----
 tests/qemu-iotests/103     |  6 ++++++
 tests/qemu-iotests/103.out |  4 +++-
 tests/qemu-iotests/137     |  2 ++
 tests/qemu-iotests/137.out |  4 +++-
 9 files changed, 80 insertions(+), 19 deletions(-)

-- 
2.14.1

[Qemu-devel] [PATCH v3 1/5 for-3.0] A grammar fix

[Leonid Bloch]

Signed-off-by: Leonid Bloch <lbloch@janustech.com>
---
 block/qcow2.c              | 2 +-
 tests/qemu-iotests/103.out | 2 +-
 tests/qemu-iotests/137.out | 2 +-
 3 files changed, 3 insertions(+), 3 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..ab56f03a00 100644
--- a/tests/qemu-iotests/103.out
+++ b/tests/qemu-iotests/103.out
@@ -5,7 +5,7 @@ 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
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 v3 2/5 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      | 15 +++++++++++----
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
index 8a09a5cc5f..9d261b7da9 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.
 
+- All three "l2-cache-size", "refcount-cache-size", and "cache-size" options
+  can not 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..ef0706c359 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -752,15 +752,22 @@ 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; if cache-size is defined and is large enough to
+accommodate enough L2 cache to cover the entire virtual size of the image plus
+the minimal amount of refcount cache - enough to cover the entire image;
+if cache-size is defined and is not large enough - as much as possible while
+leaving space for the needed refcount cache)
 
 @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 defined and is large
+enough to accommodate enough L2 cache to cover the entire virtual size of the
+image plus the minimal amount of refcount cache - the part of cache-size which
+is left 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 v3 3/5] 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 +
 qapi/block-core.json |  8 +++++++-
 qemu-options.hx      |  6 +++++-
 4 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index ec9e6238a0..101b8b474b 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(opts, QCOW2_OPT_L2_CACHE_FULL);
     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/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 ef0706c359..6d417cb267 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -754,7 +754,7 @@ 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; if cache-size is defined and is large enough to
 accommodate enough L2 cache to cover the entire virtual size of the image plus
@@ -762,6 +762,10 @@ the minimal amount of refcount cache - enough to cover the entire image;
 if cache-size is defined and is not large enough - as much as possible while
 leaving space for the needed refcount cache)
 
+@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 if cache-size is defined and is large
-- 
2.14.1

[Qemu-devel] [PATCH v3 4/5] 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..a2886e8569 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=6K $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 ab56f03a00..92afbff024 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-full and l2-cache-size 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 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..b15dfc391a 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-full and l2-cache-size 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

[Qemu-devel] [PATCH v3 5/5] docs: Document the l2-cache-full option

[Leonid Bloch]

Signed-off-by: Leonid Bloch <lbloch@janustech.com>
---
 docs/qcow2-cache.txt | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
index 9d261b7da9..ea61585a4b 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.
+
 - All three "l2-cache-size", "refcount-cache-size", and "cache-size" options
   can not be set simultaneously.
 
-- 
2.14.1

Re: [Qemu-devel] [PATCH v3 1/5 for-3.0] A grammar fix

[Eric Blake]

On 07/24/2018 03:03 PM, Leonid Bloch wrote:

Subject line is rather vague; at a bare minimum, mentioning 'qcow2:' 
might be helpful. But the maintainer can improve that. My suggestion:

qcow2: grammar fix for conflicting cache sizing

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

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> 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..ab56f03a00 100644
> --- a/tests/qemu-iotests/103.out
> +++ b/tests/qemu-iotests/103.out
> @@ -5,7 +5,7 @@ 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
> 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
> 

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

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

[Eric Blake]

On 07/24/2018 03:03 PM, Leonid Bloch wrote:
> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
> ---
>   docs/qcow2-cache.txt |  3 +++
>   qemu-options.hx      | 15 +++++++++++----
>   2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
> index 8a09a5cc5f..9d261b7da9 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.
>   
> +- All three "l2-cache-size", "refcount-cache-size", and "cache-size" options
> +  can not be set simultaneously.

Reads awkwardly; maybe:

At most two of "l2-cache-size", "refcount-cache-size", and "cache-size" 
can be set together on the command line (the omitted options are 
calculated accordingly).

> +++ b/qemu-options.hx
> @@ -752,15 +752,22 @@ 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; if cache-size is defined and is large enough to
> +accommodate enough L2 cache to cover the entire virtual size of the image plus
> +the minimal amount of refcount cache - enough to cover the entire image;
> +if cache-size is defined and is not large enough - as much as possible while
> +leaving space for the needed refcount cache)

Wordy; maybe:

(default: if cache-size is not defined - 1048576 bytes or 8 clusters, 
whichever is larger; otherwise, as large as possible within cache-size 
while still permitting the requested or minimum 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 defined and is large
> +enough to accommodate enough L2 cache to cover the entire virtual size of the
> +image plus the minimal amount of refcount cache - the part of cache-size which
> +is left after allocating the full L2 cache)

Maybe:

(default: 4 times the cluster size, plus any portion of a specified 
cache-size left over after sizing the L2 cache large enough to cover the 
entire image)

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

Re: [Qemu-devel] [PATCH v3 5/5] docs: Document the l2-cache-full option

[Eric Blake]

On 07/24/2018 03:03 PM, Leonid Bloch wrote:
> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
> ---
>   docs/qcow2-cache.txt | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)

I'd probably squash this with 3/5 introducing the option.

> 
> diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
> index 9d261b7da9..ea61585a4b 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.
> +
>   - All three "l2-cache-size", "refcount-cache-size", and "cache-size" options
>     can not be set simultaneously.
>   

Might be a rebase conflict here once you polish the wording in 2/5.

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

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

[Kevin Wolf]

Am 24.07.2018 um 22:03 hat Leonid Bloch geschrieben:
> 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>

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

Only looking at the external interface for now, I wonder whether it
would be nicer not to have two mutually exclusive options, but to make
l2-cache-size an alternate that can take either an int like before
(meaning the number of bytes) or a string/enum (with the only accepted
value "full" for now).

Another interesting question is whether 'full' shouldn't keep meaning
full throughout the lifetime of the BlockDriverState, i.e. should it
keep adapting to the new size when the image size changes?

Kevin

Re: [Qemu-devel] [PATCH v3 1/5 for-3.0] A grammar fix

[Kevin Wolf]

Am 24.07.2018 um 23:00 hat Eric Blake geschrieben:
> On 07/24/2018 03:03 PM, Leonid Bloch wrote:
> 
> Subject line is rather vague; at a bare minimum, mentioning 'qcow2:' might
> be helpful. But the maintainer can improve that. My suggestion:
> 
> qcow2: grammar fix for conflicting cache sizing

How about "qcow2: Grammar fix for conflicting cache size options"?

> > Signed-off-by: Leonid Bloch <lbloch@janustech.com>
> > ---
> >   block/qcow2.c              | 2 +-
> >   tests/qemu-iotests/103.out | 2 +-

You replaced only one of the two places in the reference output, so 103
fails for me now.

Kevin

Re: [Qemu-devel] [PATCH v3 1/5 for-3.0] A grammar fix

[Leonid Bloch]

   On 07/25/2018 11:49 AM, Kevin Wolf wrote:

Am 24.07.2018 um 23:00 hat Eric Blake geschrieben:

On 07/24/2018 03:03 PM, Leonid Bloch wrote:

Subject line is rather vague; at a bare minimum, mentioning 'qcow2:' might
be helpful. But the maintainer can improve that. My suggestion:

qcow2: grammar fix for conflicting cache sizing

How about "qcow2: Grammar fix for conflicting cache size options"?

   Already fixed in v4.



Signed-off-by: Leonid Bloch [1]<lbloch@janustech.com>
---
  block/qcow2.c              | 2 +-
  tests/qemu-iotests/103.out | 2 +-

You replaced only one of the two places in the reference output, so 103
fails for me now.

   Thanks! Didn't add the second place by mistake. Will fix in v5.
   Leonid.

Kevin

References

   1. mailto:lbloch@janustech.com

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

[Eric Blake]

On 07/25/2018 03:26 AM, Kevin Wolf wrote:

>> @@ -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',
> 
> Only looking at the external interface for now, I wonder whether it
> would be nicer not to have two mutually exclusive options, but to make
> l2-cache-size an alternate that can take either an int like before
> (meaning the number of bytes) or a string/enum (with the only accepted
> value "full" for now).

That does sound interesting.

> 
> Another interesting question is whether 'full' shouldn't keep meaning
> full throughout the lifetime of the BlockDriverState, i.e. should it
> keep adapting to the new size when the image size changes?

Do we even resize the cache now for image size changes? If we use an 
enum, we could have two different values depending on whether the chosen 
cache size remains fixed or also tries to resize when the image grows.

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

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

[Leonid Bloch]

   On 07/25/2018 03:22 PM, Eric Blake wrote:

     On 07/25/2018 03:26 AM, Kevin Wolf wrote:

     Only looking at the external interface for now, I wonder whether it
     would be nicer not to have two mutually exclusive options, but to
     make
     l2-cache-size an alternate that can take either an int like before
     (meaning the number of bytes) or a string/enum (with the only
     accepted
     value "full" for now).

     That does sound interesting.

   This does, but currently QEMU supports QEMU_OPT_STRING, QEMU_OPT_BOOL,
   QEMU_OPT_NUMBER, and QEMU_OPT_SIZE. Looks like it will require a more
   fundamental change to accept an option that can be either a string or a
   size.

     Another interesting question is whether 'full' shouldn't keep
     meaning
     full throughout the lifetime of the BlockDriverState, i.e. should it
     keep adapting to the new size when the image size changes?

     Do we even resize the cache now for image size changes? If we use an
     enum, we could have two different values depending on whether the
     chosen cache size remains fixed or also tries to resize when the
     image grows.

   Is it even possible to change the virtual disk image size online?
   Found a problem with my previous patch: the property was not actually
   set as a proper boolean option. Also, fixing the error output in iotest
   103 (thanks Kevin for the catch!). V5 is on the way.

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

[Kevin Wolf]

Am 25.07.2018 um 14:32 hat Leonid Bloch geschrieben:
> On 07/25/2018 03:22 PM, Eric Blake wrote:
> 
>     On 07/25/2018 03:26 AM, Kevin Wolf wrote:
> 
>         Only looking at the external interface for now, I wonder whether it
>         would be nicer not to have two mutually exclusive options, but to make
>         l2-cache-size an alternate that can take either an int like before
>         (meaning the number of bytes) or a string/enum (with the only accepted
>         value "full" for now).
> 
>     That does sound interesting.
> 
> This does, but currently QEMU supports QEMU_OPT_STRING, QEMU_OPT_BOOL,
> QEMU_OPT_NUMBER, and QEMU_OPT_SIZE. Looks like it will require a more
> fundamental change to accept an option that can be either a string or a size.

Hm, yes, good point. We wouldn't be able to parse the options purely
with QemuOpts any more. So we would have to manually check for 'full' in
the QDict before calling qemu_opts_absorb_qdict(). If it's there, we
would have to process it and then delete it from the QDict before we
feed the QDict to qemu_opts_absorb_qdict(), which would only accept a
number there. A bit ugly, but should be workable.

Maybe this is really the time that we should convert qcow2 to use the
QAPI types anyway, like some of the protocol drivers do internally now.
Obviously, this is out of scope for this series, but it gives a
perspective for how to get rid of the ugliness again.

>         Another interesting question is whether 'full' shouldn't keep meaning
>         full throughout the lifetime of the BlockDriverState, i.e. should it
>         keep adapting to the new size when the image size changes?
> 
> 
>     Do we even resize the cache now for image size changes? If we use an enum,
>     we could have two different values depending on whether the chosen cache
>     size remains fixed or also tries to resize when the image grows.

We don't because we only support absolute cache sizes today. 'full'
would be the first one that is relative to the image size.

> Is it even possible to change the virtual disk image size online?

Yes, this is what qcow2_co_truncate() does (can be invoked, amongst
others, with the QMP command 'block_resize').

> Found a problem with my previous patch: the property was not actually set as a
> proper boolean option. Also, fixing the error output in iotest 103 (thanks
> Kevin for the catch!). V5 is on the way.

Maybe give the alternate thing a try with v5, as everyone seems to agree
that it's a nicer interface if we can make it work.

Also, a meta-comment: Leonid, would you mind sending plain text emails
instead of HTML-only?

Kevin

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

[Leonid Bloch]

On 07/25/2018 04:32 PM, Kevin Wolf wrote:
> Am 25.07.2018 um 14:32 hat Leonid Bloch geschrieben:
>> On 07/25/2018 03:22 PM, Eric Blake wrote:
>>
>>      On 07/25/2018 03:26 AM, Kevin Wolf wrote:
>>
>>          Only looking at the external interface for now, I wonder whether it
>>          would be nicer not to have two mutually exclusive options, but to make
>>          l2-cache-size an alternate that can take either an int like before
>>          (meaning the number of bytes) or a string/enum (with the only accepted
>>          value "full" for now).
>>
>>      That does sound interesting.
>>
>> This does, but currently QEMU supports QEMU_OPT_STRING, QEMU_OPT_BOOL,
>> QEMU_OPT_NUMBER, and QEMU_OPT_SIZE. Looks like it will require a more
>> fundamental change to accept an option that can be either a string or a size.
> 
> Hm, yes, good point. We wouldn't be able to parse the options purely
> with QemuOpts any more. So we would have to manually check for 'full' in
> the QDict before calling qemu_opts_absorb_qdict(). If it's there, we
> would have to process it and then delete it from the QDict before we
> feed the QDict to qemu_opts_absorb_qdict(), which would only accept a
> number there. A bit ugly, but should be workable.
> 
> Maybe this is really the time that we should convert qcow2 to use the
> QAPI types anyway, like some of the protocol drivers do internally now.
> Obviously, this is out of scope for this series, but it gives a
> perspective for how to get rid of the ugliness again.

I need to look into that. Thanks for the idea. But indeed looks like out 
of scope for this series.

> 
>>          Another interesting question is whether 'full' shouldn't keep meaning
>>          full throughout the lifetime of the BlockDriverState, i.e. should it
>>          keep adapting to the new size when the image size changes?
>>
>>
>>      Do we even resize the cache now for image size changes? If we use an enum,
>>      we could have two different values depending on whether the chosen cache
>>      size remains fixed or also tries to resize when the image grows.
> 
> We don't because we only support absolute cache sizes today. 'full'
> would be the first one that is relative to the image size.
> 
>> Is it even possible to change the virtual disk image size online?
> 
> Yes, this is what qcow2_co_truncate() does (can be invoked, amongst
> others, with the QMP command 'block_resize').

Cool! This does look like a good idea to resize the L2 cache 
accordingly, but maybe this is out of scope for now as well? The purpose 
of the current series is just to provide an option to automatically 
calculate the needed L2 cache size for covering the entire image, 
instead of using an external script to do that and feed the output to 
l2-cache-size.

> 
>> Found a problem with my previous patch: the property was not actually set as a
>> proper boolean option. Also, fixing the error output in iotest 103 (thanks
>> Kevin for the catch!). V5 is on the way.
> 
> Maybe give the alternate thing a try with v5, as everyone seems to agree
> that it's a nicer interface if we can make it work.

You mean with QDict? I'll look into that now. But already sent v5 before 
reading this email.

> 
> Also, a meta-comment: Leonid, would you mind sending plain text emails
> instead of HTML-only?

Sure, Kevin! Sorry, I thought Thunderbird as configured here is sending 
plain text. I was wrong. Now it should be fine. Thanks for the remark.

Leonid.

> 
> Kevin
> 

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

[Kevin Wolf]

Am 25.07.2018 um 17:23 hat Leonid Bloch geschrieben:
> On 07/25/2018 04:32 PM, Kevin Wolf wrote:
> > Am 25.07.2018 um 14:32 hat Leonid Bloch geschrieben:
> > > On 07/25/2018 03:22 PM, Eric Blake wrote:
> > > 
> > >      On 07/25/2018 03:26 AM, Kevin Wolf wrote:
> > > 
> > >          Only looking at the external interface for now, I wonder whether it
> > >          would be nicer not to have two mutually exclusive options, but to make
> > >          l2-cache-size an alternate that can take either an int like before
> > >          (meaning the number of bytes) or a string/enum (with the only accepted
> > >          value "full" for now).
> > > 
> > >      That does sound interesting.
> > > 
> > > This does, but currently QEMU supports QEMU_OPT_STRING, QEMU_OPT_BOOL,
> > > QEMU_OPT_NUMBER, and QEMU_OPT_SIZE. Looks like it will require a more
> > > fundamental change to accept an option that can be either a string or a size.
> > 
> > Hm, yes, good point. We wouldn't be able to parse the options purely
> > with QemuOpts any more. So we would have to manually check for 'full' in
> > the QDict before calling qemu_opts_absorb_qdict(). If it's there, we
> > would have to process it and then delete it from the QDict before we
> > feed the QDict to qemu_opts_absorb_qdict(), which would only accept a
> > number there. A bit ugly, but should be workable.
> > 
> > Maybe this is really the time that we should convert qcow2 to use the
> > QAPI types anyway, like some of the protocol drivers do internally now.
> > Obviously, this is out of scope for this series, but it gives a
> > perspective for how to get rid of the ugliness again.
> 
> I need to look into that. Thanks for the idea. But indeed looks like
> out of scope for this series.

QAPIfication of .bdrv_open() is a long-term goal for all block drivers
anyway, so if you're really interested in this, any work towards it is
welcome. But it definitely won't be needed to get the cache size thing
in. I only mentioned it to show that we wouldn't be stuck with the
direct QDict access forever.

> > >          Another interesting question is whether 'full' shouldn't keep meaning
> > >          full throughout the lifetime of the BlockDriverState, i.e. should it
> > >          keep adapting to the new size when the image size changes?
> > > 
> > > 
> > >      Do we even resize the cache now for image size changes? If we use an enum,
> > >      we could have two different values depending on whether the chosen cache
> > >      size remains fixed or also tries to resize when the image grows.
> > 
> > We don't because we only support absolute cache sizes today. 'full'
> > would be the first one that is relative to the image size.
> > 
> > > Is it even possible to change the virtual disk image size online?
> > 
> > Yes, this is what qcow2_co_truncate() does (can be invoked, amongst
> > others, with the QMP command 'block_resize').
> 
> Cool! This does look like a good idea to resize the L2 cache accordingly,
> but maybe this is out of scope for now as well? The purpose of the current
> series is just to provide an option to automatically calculate the needed L2
> cache size for covering the entire image, instead of using an external
> script to do that and feed the output to l2-cache-size.

I'm not sure. Probably both ways can be defended, but if I as a user
said l2-cache-size=full, and then resized my 10 GB image to 20 GB, I
think I would expect that the cache still covers the whole image and not
only half of it. I never said I wanted enough cache for 10 GB, but I
said that I wanted enough cache to cover the whole image.

The thing is that once we decide not to resize the cache yet (and
document the option accordingly), it becomes ABI and we can't change
that later any more. We would have to introduce another option that
enables adaption to changed image sizes. As I don't see a reason why you
could want l2-cache-size=full, but no adaption, I'd prefer to implement
it right away.

And I think it might be as simple as adding something like this at the
end of qcow2_co_truncate():

    /* Update cache size for l2-cache-size=full */
    qcow2_update_options(bs, bs->options, s->flags, NULL);

I'm ignoring errors here because I think the resize was still successful
if we couldn't update the cache size, but that could be discussed.

> > > Found a problem with my previous patch: the property was not actually set as a
> > > proper boolean option. Also, fixing the error output in iotest 103 (thanks
> > > Kevin for the catch!). V5 is on the way.
> > 
> > Maybe give the alternate thing a try with v5, as everyone seems to agree
> > that it's a nicer interface if we can make it work.
> 
> You mean with QDict? I'll look into that now. But already sent v5 before
> reading this email.

Yes, with reading it from the QDict. (Or whatever the simplest way is
that results in the right external interface, but I suppose this is the
one.)

> > Also, a meta-comment: Leonid, would you mind sending plain text emails
> > instead of HTML-only?
> 
> Sure, Kevin! Sorry, I thought Thunderbird as configured here is sending
> plain text. I was wrong. Now it should be fine. Thanks for the remark.

Thanks, that looks better!

Kevin

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

[Daniel P. Berrangé]

On Wed, Jul 25, 2018 at 06:23:45PM +0300, Leonid Bloch wrote:
> On 07/25/2018 04:32 PM, Kevin Wolf wrote:
> > >          Another interesting question is whether 'full' shouldn't keep meaning
> > >          full throughout the lifetime of the BlockDriverState, i.e. should it
> > >          keep adapting to the new size when the image size changes?
> > > 
> > > 
> > >      Do we even resize the cache now for image size changes? If we use an enum,
> > >      we could have two different values depending on whether the chosen cache
> > >      size remains fixed or also tries to resize when the image grows.
> > 
> > We don't because we only support absolute cache sizes today. 'full'
> > would be the first one that is relative to the image size.
> > 
> > > Is it even possible to change the virtual disk image size online?
> > 
> > Yes, this is what qcow2_co_truncate() does (can be invoked, amongst
> > others, with the QMP command 'block_resize').
> 
> Cool! This does look like a good idea to resize the L2 cache accordingly,
> but maybe this is out of scope for now as well? The purpose of the current
> series is just to provide an option to automatically calculate the needed L2
> cache size for covering the entire image, instead of using an external
> script to do that and feed the output to l2-cache-size.

Personally if I saw the description of the option and then found it didn't
"do the right thing" when resizing I'd consider the option broken. So I
think we should make it deal with resizes right away rather than implementing
a known bug.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Leonid Bloch]

>> You mean with QDict? I'll look into that now. But already sent v5 before
>> reading this email.
> 
> Yes, with reading it from the QDict. (Or whatever the simplest way is
> that results in the right external interface, but I suppose this is the
> one.)

Well, there is a problem with that: I can easily isolate
l2-cache-size from QDict, check if it is "full", and if it is - do 
whatever is needed, and delete this option before parsing. But what if 
it is "foo"? It will not get deleted, and the regular QEMU_OPT_SIZE 
parsing error will appear, stating that l2-cache-size "expects a 
non-negative number..." - no word about that it can expect "full" as 
well. Now, one can try to modify local_err->msg for this particular 
option, but this will require substantial additional logic. I think 
considering this, it would be easier to stick with a dedicated option, 
l2-cache-full.

Do you think there is a smarter way to parse the l2-cache-size option, 
so it would accept both size and "full", while handling errors 
correctly? It seems more elegant to have a single option, but the 
internal handling will be more elegant and simpler with two mutually 
exclusive options.

By the way, the L2 cache resizes now on image resize. Will send the 
changes in v6. Thanks for the suggestion!

Leonid.

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

[Kevin Wolf]

Am 26.07.2018 um 14:24 hat Leonid Bloch geschrieben:
> > > You mean with QDict? I'll look into that now. But already sent v5 before
> > > reading this email.
> > 
> > Yes, with reading it from the QDict. (Or whatever the simplest way is
> > that results in the right external interface, but I suppose this is the
> > one.)
> 
> Well, there is a problem with that: I can easily isolate
> l2-cache-size from QDict, check if it is "full", and if it is - do whatever
> is needed, and delete this option before parsing. But what if it is "foo"?
> It will not get deleted, and the regular QEMU_OPT_SIZE parsing error will
> appear, stating that l2-cache-size "expects a non-negative number..." - no
> word about that it can expect "full" as well. Now, one can try to modify
> local_err->msg for this particular option, but this will require substantial
> additional logic. I think considering this, it would be easier to stick with
> a dedicated option, l2-cache-full.
> 
> Do you think there is a smarter way to parse the l2-cache-size option, so it
> would accept both size and "full", while handling errors correctly? It seems
> more elegant to have a single option, but the internal handling will be more
> elegant and simpler with two mutually exclusive options.

I think we can live with the suboptimal error message for a while. Once
qcow2 is QAPIfied, it should become easy to improve it. Let's not choose
a worse design (that stays forever) for a temporarily better error
message.

> By the way, the L2 cache resizes now on image resize. Will send the changes
> in v6. Thanks for the suggestion!

Sounds good!

Kevin

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

[Leonid Bloch]

On 07/26/2018 05:42 PM, Kevin Wolf wrote:
> Am 26.07.2018 um 14:24 hat Leonid Bloch geschrieben:
>>>> You mean with QDict? I'll look into that now. But already sent v5 before
>>>> reading this email.
>>>
>>> Yes, with reading it from the QDict. (Or whatever the simplest way is
>>> that results in the right external interface, but I suppose this is the
>>> one.)
>>
>> Well, there is a problem with that: I can easily isolate
>> l2-cache-size from QDict, check if it is "full", and if it is - do whatever
>> is needed, and delete this option before parsing. But what if it is "foo"?
>> It will not get deleted, and the regular QEMU_OPT_SIZE parsing error will
>> appear, stating that l2-cache-size "expects a non-negative number..." - no
>> word about that it can expect "full" as well. Now, one can try to modify
>> local_err->msg for this particular option, but this will require substantial
>> additional logic. I think considering this, it would be easier to stick with
>> a dedicated option, l2-cache-full.
>>
>> Do you think there is a smarter way to parse the l2-cache-size option, so it
>> would accept both size and "full", while handling errors correctly? It seems
>> more elegant to have a single option, but the internal handling will be more
>> elegant and simpler with two mutually exclusive options.
> 
> I think we can live with the suboptimal error message for a while. Once
> qcow2 is QAPIfied, it should become easy to improve it. Let's not choose
> a worse design (that stays forever) for a temporarily better error
> message.

OK. I'll add a TODO then.

> 
>> By the way, the L2 cache resizes now on image resize. Will send the changes
>> in v6. Thanks for the suggestion!
> 
> Sounds good!
> 
> Kevin
> 

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

[Leonid Bloch]

On 07/26/2018 05:50 PM, Leonid Bloch wrote:
> On 07/26/2018 05:42 PM, Kevin Wolf wrote:
>> Am 26.07.2018 um 14:24 hat Leonid Bloch geschrieben:
>>>>> You mean with QDict? I'll look into that now. But already sent v5 
>>>>> before
>>>>> reading this email.
>>>>
>>>> Yes, with reading it from the QDict. (Or whatever the simplest way is
>>>> that results in the right external interface, but I suppose this is the
>>>> one.)
>>>
>>> Well, there is a problem with that: I can easily isolate
>>> l2-cache-size from QDict, check if it is "full", and if it is - do 
>>> whatever
>>> is needed, and delete this option before parsing. But what if it is 
>>> "foo"?
>>> It will not get deleted, and the regular QEMU_OPT_SIZE parsing error 
>>> will
>>> appear, stating that l2-cache-size "expects a non-negative number..." 
>>> - no
>>> word about that it can expect "full" as well. Now, one can try to modify
>>> local_err->msg for this particular option, but this will require 
>>> substantial
>>> additional logic. I think considering this, it would be easier to 
>>> stick with
>>> a dedicated option, l2-cache-full.
>>>
>>> Do you think there is a smarter way to parse the l2-cache-size 
>>> option, so it
>>> would accept both size and "full", while handling errors correctly? 
>>> It seems
>>> more elegant to have a single option, but the internal handling will 
>>> be more
>>> elegant and simpler with two mutually exclusive options.
>>
>> I think we can live with the suboptimal error message for a while. Once
>> qcow2 is QAPIfied, it should become easy to improve it. Let's not choose
>> a worse design (that stays forever) for a temporarily better error
>> message.
> 
> OK. I'll add a TODO then.

Another problem without a dedicated option, is that if l2-cache-size is 
processed and deleted, it can not be read again when needed for 
resizing. Without some *extremely* dirty tricks, that is.

Can QAPI be the solution? I've seen some examples, and it looks like the 
qcow2 driver already uses QAPI to some extent - I mean all the qdict 
stuff is from QAPI, no? Can you please point me to an example of how 
QAPI can solve the issue of an option that can accept both a size and a 
string?

Thanks,
Leonid.

>>
>>> By the way, the L2 cache resizes now on image resize. Will send the 
>>> changes
>>> in v6. Thanks for the suggestion!
>>
>> Sounds good!
>>
>> Kevin
>>

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

[Kevin Wolf]

Am 26.07.2018 um 21:43 hat Leonid Bloch geschrieben:
> On 07/26/2018 05:50 PM, Leonid Bloch wrote:
> > On 07/26/2018 05:42 PM, Kevin Wolf wrote:
> > > Am 26.07.2018 um 14:24 hat Leonid Bloch geschrieben:
> > > > > > You mean with QDict? I'll look into that now. But
> > > > > > already sent v5 before
> > > > > > reading this email.
> > > > > 
> > > > > Yes, with reading it from the QDict. (Or whatever the simplest way is
> > > > > that results in the right external interface, but I suppose this is the
> > > > > one.)
> > > > 
> > > > Well, there is a problem with that: I can easily isolate
> > > > l2-cache-size from QDict, check if it is "full", and if it is -
> > > > do whatever
> > > > is needed, and delete this option before parsing. But what if it
> > > > is "foo"?
> > > > It will not get deleted, and the regular QEMU_OPT_SIZE parsing
> > > > error will
> > > > appear, stating that l2-cache-size "expects a non-negative
> > > > number..." - no
> > > > word about that it can expect "full" as well. Now, one can try to modify
> > > > local_err->msg for this particular option, but this will require
> > > > substantial
> > > > additional logic. I think considering this, it would be easier
> > > > to stick with
> > > > a dedicated option, l2-cache-full.
> > > > 
> > > > Do you think there is a smarter way to parse the l2-cache-size
> > > > option, so it
> > > > would accept both size and "full", while handling errors
> > > > correctly? It seems
> > > > more elegant to have a single option, but the internal handling
> > > > will be more
> > > > elegant and simpler with two mutually exclusive options.
> > > 
> > > I think we can live with the suboptimal error message for a while. Once
> > > qcow2 is QAPIfied, it should become easy to improve it. Let's not choose
> > > a worse design (that stays forever) for a temporarily better error
> > > message.
> > 
> > OK. I'll add a TODO then.
> 
> Another problem without a dedicated option, is that if l2-cache-size is
> processed and deleted, it can not be read again when needed for resizing.
> Without some *extremely* dirty tricks, that is.

Are you sure? The way that the .bdrv_open() callbacks work, _all_
options that are processed are removed from the QDict. The difference is
just whether qemu_opts_absorb_qdict() removes them or you do that
manually. In the end, if options are left in the QDict, the block layer
returns an error because that means that an unknown option was
specified.

I suppose you use bs->options while resizing. This is a copy of the
QDict which is not affected by the removal of processed options.

> Can QAPI be the solution? I've seen some examples, and it looks like the
> qcow2 driver already uses QAPI to some extent - I mean all the qdict stuff
> is from QAPI, no? Can you please point me to an example of how QAPI can
> solve the issue of an option that can accept both a size and a string?

By using QAPI I mean using types like BlockdevOptionsQcow2. These types
are directly generated from the JSON schema, where it's possible to
specify an alternate of a string and an integer.

One example for an alternate in the JSON schema is BlockdevRef, where
you can give a string (to reference a node-name) or an inline
declaration of a block device. In JSON, it looks like this:

    { 'alternate': 'BlockdevRef',
      'data': { 'definition': 'BlockdevOptions',
                'reference': 'str' } }

And the generated C type for it is:

    struct BlockdevRef {
        QType type;
        union { /* union tag is @type */
            BlockdevOptions definition;
            char *reference;
        } u;
    };

In our case, we would get a union of int64_t and char* instead, and
still the QType type as a discriminator.

Kevin

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

[Leonid Bloch]

On July 26, 2018 11:19:03 PM EEST, Kevin Wolf <kwolf@redhat.com> wrote:
>Am 26.07.2018 um 21:43 hat Leonid Bloch geschrieben:
>> On 07/26/2018 05:50 PM, Leonid Bloch wrote:
>> > On 07/26/2018 05:42 PM, Kevin Wolf wrote:
>> > > Am 26.07.2018 um 14:24 hat Leonid Bloch geschrieben:
>> > > > > > You mean with QDict? I'll look into that now. But
>> > > > > > already sent v5 before
>> > > > > > reading this email.
>> > > > > 
>> > > > > Yes, with reading it from the QDict. (Or whatever the
>simplest way is
>> > > > > that results in the right external interface, but I suppose
>this is the
>> > > > > one.)
>> > > > 
>> > > > Well, there is a problem with that: I can easily isolate
>> > > > l2-cache-size from QDict, check if it is "full", and if it is -
>> > > > do whatever
>> > > > is needed, and delete this option before parsing. But what if
>it
>> > > > is "foo"?
>> > > > It will not get deleted, and the regular QEMU_OPT_SIZE parsing
>> > > > error will
>> > > > appear, stating that l2-cache-size "expects a non-negative
>> > > > number..." - no
>> > > > word about that it can expect "full" as well. Now, one can try
>to modify
>> > > > local_err->msg for this particular option, but this will
>require
>> > > > substantial
>> > > > additional logic. I think considering this, it would be easier
>> > > > to stick with
>> > > > a dedicated option, l2-cache-full.
>> > > > 
>> > > > Do you think there is a smarter way to parse the l2-cache-size
>> > > > option, so it
>> > > > would accept both size and "full", while handling errors
>> > > > correctly? It seems
>> > > > more elegant to have a single option, but the internal handling
>> > > > will be more
>> > > > elegant and simpler with two mutually exclusive options.
>> > > 
>> > > I think we can live with the suboptimal error message for a
>while. Once
>> > > qcow2 is QAPIfied, it should become easy to improve it. Let's not
>choose
>> > > a worse design (that stays forever) for a temporarily better
>error
>> > > message.
>> > 
>> > OK. I'll add a TODO then.
>> 
>> Another problem without a dedicated option, is that if l2-cache-size
>is
>> processed and deleted, it can not be read again when needed for
>resizing.
>> Without some *extremely* dirty tricks, that is.
>
>Are you sure? The way that the .bdrv_open() callbacks work, _all_
>options that are processed are removed from the QDict. The difference
>is
>just whether qemu_opts_absorb_qdict() removes them or you do that
>manually. In the end, if options are left in the QDict, the block layer
>returns an error because that means that an unknown option was
>specified.
>
>I suppose you use bs->options while resizing. This is a copy of the
>QDict which is not affected by the removal of processed options.

This probably explains why it works on the first resize, but fails on the second onward.

>
>> Can QAPI be the solution? I've seen some examples, and it looks like
>the
>> qcow2 driver already uses QAPI to some extent - I mean all the qdict
>stuff
>> is from QAPI, no? Can you please point me to an example of how QAPI
>can
>> solve the issue of an option that can accept both a size and a
>string?
>
>By using QAPI I mean using types like BlockdevOptionsQcow2. These types
>are directly generated from the JSON schema, where it's possible to
>specify an alternate of a string and an integer.
>
>One example for an alternate in the JSON schema is BlockdevRef, where
>you can give a string (to reference a node-name) or an inline
>declaration of a block device. In JSON, it looks like this:
>
>    { 'alternate': 'BlockdevRef',
>      'data': { 'definition': 'BlockdevOptions',
>                'reference': 'str' } }
>
>And the generated C type for it is:
>
>    struct BlockdevRef {
>        QType type;
>        union { /* union tag is @type */
>            BlockdevOptions definition;
>            char *reference;
>        } u;
>    };
>
>In our case, we would get a union of int64_t and char* instead, and
>still the QType type as a discriminator.

Thanks for the explanation!
But qcow2 already has the options in the JSON file, they're just not used for some reason?

Leonid.

>
>Kevin

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

[Kevin Wolf]

Am 26.07.2018 um 23:08 hat Leonid Bloch geschrieben:
> On July 26, 2018 11:19:03 PM EEST, Kevin Wolf <kwolf@redhat.com> wrote:
> >Am 26.07.2018 um 21:43 hat Leonid Bloch geschrieben:
> >> On 07/26/2018 05:50 PM, Leonid Bloch wrote:
> >> > On 07/26/2018 05:42 PM, Kevin Wolf wrote:
> >> > > Am 26.07.2018 um 14:24 hat Leonid Bloch geschrieben:
> >> > > > > > You mean with QDict? I'll look into that now. But
> >> > > > > > already sent v5 before
> >> > > > > > reading this email.
> >> > > > > 
> >> > > > > Yes, with reading it from the QDict. (Or whatever the
> >simplest way is
> >> > > > > that results in the right external interface, but I suppose
> >this is the
> >> > > > > one.)
> >> > > > 
> >> > > > Well, there is a problem with that: I can easily isolate
> >> > > > l2-cache-size from QDict, check if it is "full", and if it is -
> >> > > > do whatever
> >> > > > is needed, and delete this option before parsing. But what if
> >it
> >> > > > is "foo"?
> >> > > > It will not get deleted, and the regular QEMU_OPT_SIZE parsing
> >> > > > error will
> >> > > > appear, stating that l2-cache-size "expects a non-negative
> >> > > > number..." - no
> >> > > > word about that it can expect "full" as well. Now, one can try
> >to modify
> >> > > > local_err->msg for this particular option, but this will
> >require
> >> > > > substantial
> >> > > > additional logic. I think considering this, it would be easier
> >> > > > to stick with
> >> > > > a dedicated option, l2-cache-full.
> >> > > > 
> >> > > > Do you think there is a smarter way to parse the l2-cache-size
> >> > > > option, so it
> >> > > > would accept both size and "full", while handling errors
> >> > > > correctly? It seems
> >> > > > more elegant to have a single option, but the internal handling
> >> > > > will be more
> >> > > > elegant and simpler with two mutually exclusive options.
> >> > > 
> >> > > I think we can live with the suboptimal error message for a
> >while. Once
> >> > > qcow2 is QAPIfied, it should become easy to improve it. Let's not
> >choose
> >> > > a worse design (that stays forever) for a temporarily better
> >error
> >> > > message.
> >> > 
> >> > OK. I'll add a TODO then.
> >> 
> >> Another problem without a dedicated option, is that if l2-cache-size
> >is
> >> processed and deleted, it can not be read again when needed for
> >resizing.
> >> Without some *extremely* dirty tricks, that is.
> >
> >Are you sure? The way that the .bdrv_open() callbacks work, _all_
> >options that are processed are removed from the QDict. The difference
> >is
> >just whether qemu_opts_absorb_qdict() removes them or you do that
> >manually. In the end, if options are left in the QDict, the block layer
> >returns an error because that means that an unknown option was
> >specified.
> >
> >I suppose you use bs->options while resizing. This is a copy of the
> >QDict which is not affected by the removal of processed options.
> 
> This probably explains why it works on the first resize, but fails on
> the second onward.

That makes sense. You probably need to clone the QDict before passing
it to qcow2_update_options(), with qdict_clone_shallow().

> >> Can QAPI be the solution? I've seen some examples, and it looks like
> >the
> >> qcow2 driver already uses QAPI to some extent - I mean all the qdict
> >stuff
> >> is from QAPI, no? Can you please point me to an example of how QAPI
> >can
> >> solve the issue of an option that can accept both a size and a
> >string?
> >
> >By using QAPI I mean using types like BlockdevOptionsQcow2. These types
> >are directly generated from the JSON schema, where it's possible to
> >specify an alternate of a string and an integer.
> >
> >One example for an alternate in the JSON schema is BlockdevRef, where
> >you can give a string (to reference a node-name) or an inline
> >declaration of a block device. In JSON, it looks like this:
> >
> >    { 'alternate': 'BlockdevRef',
> >      'data': { 'definition': 'BlockdevOptions',
> >                'reference': 'str' } }
> >
> >And the generated C type for it is:
> >
> >    struct BlockdevRef {
> >        QType type;
> >        union { /* union tag is @type */
> >            BlockdevOptions definition;
> >            char *reference;
> >        } u;
> >    };
> >
> >In our case, we would get a union of int64_t and char* instead, and
> >still the QType type as a discriminator.
> 
> Thanks for the explanation!
> But qcow2 already has the options in the JSON file, they're just not
> used for some reason?

The block layer configuration is a bit of a mess because of backwards
compatibility. The JSON schema is in fact used for -blockdev and for the
blockdev-add QMP command, but not for the old option -drive and
drive_add.

In the case of -blockdev/blockdev-add, after validating the input
against the schema, the QAPI objects are converted to QDicts so they can
use the same code path as the old code. Eventually, we want to use QAPI
objects all the way, but that requires either expressing all -drive
options in terms of QAPI object or getting rid of -driver altogether.
This is still going to take some time.

Kevin

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

[Leonid Bloch]

On 07/27/2018 12:28 AM, Kevin Wolf wrote>
> That makes sense. You probably need to clone the QDict before passing
> it to qcow2_update_options(), with qdict_clone_shallow().

Sounds like a solution! Great, thanks!

>>>> Can QAPI be the solution? I've seen some examples, and it looks like
>>> the
>>>> qcow2 driver already uses QAPI to some extent - I mean all the qdict
>>> stuff
>>>> is from QAPI, no? Can you please point me to an example of how QAPI
>>> can
>>>> solve the issue of an option that can accept both a size and a
>>> string?
>>>
>>> By using QAPI I mean using types like BlockdevOptionsQcow2. These types
>>> are directly generated from the JSON schema, where it's possible to
>>> specify an alternate of a string and an integer.
>>>
>>> One example for an alternate in the JSON schema is BlockdevRef, where
>>> you can give a string (to reference a node-name) or an inline
>>> declaration of a block device. In JSON, it looks like this:
>>>
>>>     { 'alternate': 'BlockdevRef',
>>>       'data': { 'definition': 'BlockdevOptions',
>>>                 'reference': 'str' } }
>>>
>>> And the generated C type for it is:
>>>
>>>     struct BlockdevRef {
>>>         QType type;
>>>         union { /* union tag is @type */
>>>             BlockdevOptions definition;
>>>             char *reference;
>>>         } u;
>>>     };
>>>
>>> In our case, we would get a union of int64_t and char* instead, and
>>> still the QType type as a discriminator.
>>
>> Thanks for the explanation!
>> But qcow2 already has the options in the JSON file, they're just not
>> used for some reason?
> 
> The block layer configuration is a bit of a mess because of backwards
> compatibility. The JSON schema is in fact used for -blockdev and for the
> blockdev-add QMP command, but not for the old option -drive and
> drive_add.
> 
> In the case of -blockdev/blockdev-add, after validating the input
> against the schema, the QAPI objects are converted to QDicts so they can
> use the same code path as the old code. Eventually, we want to use QAPI
> objects all the way, but that requires either expressing all -drive
> options in terms of QAPI object or getting rid of -driver altogether.
> This is still going to take some time.

Thanks for the details! It's clear now!

Leonid.

> 
> Kevin
>