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

[Qemu-devel] [PATCH v4 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 v2:

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

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 |  4 +++-
 tests/qemu-iotests/137     |  2 ++
 tests/qemu-iotests/137.out |  4 +++-
 9 files changed, 75 insertions(+), 19 deletions(-)

-- 
2.14.1

[Qemu-devel] [PATCH v4 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 | 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 v4 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 v4 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..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/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 v4 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..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

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

[Leonid Bloch]

   On 07/25/2018 01:17 AM, Leonid Bloch wrote:

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

   * 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

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 |  4 +++-
 tests/qemu-iotests/137     |  2 ++
 tests/qemu-iotests/137.out |  4 +++-
 9 files changed, 75 insertions(+), 19 deletions(-)

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

[Eric Blake]

On 07/24/2018 05:20 PM, Leonid Bloch wrote:

meta-comment: a hint for more effective emails below

>> -------------------------
>> 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 v2:
> * from v3
>> 1) Grammar change commit message fix

Visually, it's hard to pick out an inline reply prefixed with "*" amid a 
bunch of lines prefixed with ">", with no other hinting. (Actually, I'm 
finding it easier to read your email in my reply window, where 
thunderbird chose to use ">>" for double-quoted lines vs. "> *" for the 
single quoted line, which has a distinct whitespace change in column 2 
that the original email window did not.  But then again, I've been 
bitten by Thunderbird displaying quoting differently to me than it 
renders to the end recipient, so I don't know if "> *" will still have a 
space by the time you see this reply of mine).

I find that it is much more legible to always include a blank line 
around both ends of any text I type, as the eye is much quicker at 
picking out the absence of any character in column 1 than it is on 
deciphering which marks in column 1 serve as the indicator of 
transitions between quoted vs. new content in the thread.

>> 2) Rewording the documentation more concisely
>> 3) Squashing the l2-cache-full docs commit to the one that introduces this
>>     feature

Also, when replying to an archived list, it's okay to trim quoted text 
down to just the context relevant to the reply, to let the reader 
quickly reach the new content, rather than preserving the entire 
original email and forcing the reader to scroll through a wall of text 
just to locate the added thoughts. (Yes, some mail clients do a better 
job of coloring quoted text differently, and/or collapsing quoted 
material, so that not every reader has to scroll, but not everyone 
agrees on the ideal mail client). This email wasn't too bad, but you'll 
find instances of me making these same metacomments on other emails over 
the years if you search the archives :)

Finally, thanks for contributing, and for your rapid turnaround 
incorporating suggestions from my earlier reviews!  I know that 
sometimes when I make observations about making the review process more 
efficient for everyone involved, it makes me come across as a 
curmudgeonly old miser. In my efforts to be terse, I often forget to 
also be human and compliment contributors for making an effort in the 
first place, regardless of whether future efforts can be made even more 
efficient.

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

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

[Leonid Bloch]

   Thanks for the review and for the comments, Eric!
   One quick remark: I do usually leave blank lines around inline replies,
   but this time Thunderbird made it look as if there are blank lines when
   I was writing, when apparently there were not. :]
   Leonid.

   On 07/25/2018 01:44 AM, Eric Blake wrote:

     On 07/24/2018 05:20 PM, Leonid Bloch wrote:
     meta-comment: a hint for more effective emails below

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

     * from v3

     1) Grammar change commit message fix

     Visually, it's hard to pick out an inline reply prefixed with "*"
     amid a bunch of lines prefixed with ">", with no other hinting.
     (Actually, I'm finding it easier to read your email in my reply
     window, where thunderbird chose to use ">>" for double-quoted lines
     vs. "> *" for the single quoted line, which has a distinct
     whitespace change in column 2 that the original email window did
     not.  But then again, I've been bitten by Thunderbird displaying
     quoting differently to me than it renders to the end recipient, so I
     don't know if "> *" will still have a space by the time you see this
     reply of mine).
     I find that it is much more legible to always include a blank line
     around both ends of any text I type, as the eye is much quicker at
     picking out the absence of any character in column 1 than it is on
     deciphering which marks in column 1 serve as the indicator of
     transitions between quoted vs. new content in the thread.

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

     Also, when replying to an archived list, it's okay to trim quoted
     text down to just the context relevant to the reply, to let the
     reader quickly reach the new content, rather than preserving the
     entire original email and forcing the reader to scroll through a
     wall of text just to locate the added thoughts. (Yes, some mail
     clients do a better job of coloring quoted text differently, and/or
     collapsing quoted material, so that not every reader has to scroll,
     but not everyone agrees on the ideal mail client). This email wasn't
     too bad, but you'll find instances of me making these same
     metacomments on other emails over the years if you search the
     archives :)
     Finally, thanks for contributing, and for your rapid turnaround
     incorporating suggestions from my earlier reviews!  I know that
     sometimes when I make observations about making the review process
     more efficient for everyone involved, it makes me come across as a
     curmudgeonly old miser. In my efforts to be terse, I often forget to
     also be human and compliment contributors for making an effort in
     the first place, regardless of whether future efforts can be made
     even more efficient.