[Qemu-devel] [PATCH 0/3] qcow2: Fix MIN_L2_CACHE_SIZE

[Qemu-devel] [PATCH 0/3] qcow2: Fix MIN_L2_CACHE_SIZE

[Max Reitz]

This series fixes MIN_L2_CACHE_SIZE (which should not be 1, but 2
(clusters)), and introduces a new constant, DEFAULT_L2_CACHE_CLUSTERS,
so the default cache size is no longer always a fixed size in bytes but
is also guaranteed to be able to hold a sane amount of L2 tables (which
was determined to be 8 using the "proposal without protest" method).

So, in effect, after patch 3 of this series, the L2 cache (unless
overridden by the user, of course) will either be 1 MB in size or be
able to hold eight L2 tables, whichever is more.


Max Reitz (3):
  qcow2: Set MIN_L2_CACHE_SIZE to 2
  iotests: qcow2 COW with minimal L2 cache size
  qcow2: Add DEFAULT_L2_CACHE_CLUSTERS

 block/qcow2.c              | 11 ++++++++---
 block/qcow2.h              |  5 ++++-
 tests/qemu-iotests/103     | 10 ++++++++++
 tests/qemu-iotests/103.out |  5 +++++
 4 files changed, 27 insertions(+), 4 deletions(-)

-- 
2.4.1

[Qemu-devel] [PATCH 1/3] qcow2: Set MIN_L2_CACHE_SIZE to 2

[Max Reitz]

The L2 cache must cover at least two L2 tables, because during COW two
L2 tables are accessed simultaneously.

Reported-by: Alexander Graf <agraf@suse.de>
Cc: qemu-stable <qemu-stable@nongnu.org>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 0076512..aa20022 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -62,7 +62,8 @@
 #define MIN_CLUSTER_BITS 9
 #define MAX_CLUSTER_BITS 21
 
-#define MIN_L2_CACHE_SIZE 1 /* cluster */
+/* Must be at least 2 to cover COW */
+#define MIN_L2_CACHE_SIZE 2 /* clusters */
 
 /* Must be at least 4 to cover all cases of refcount table growth */
 #define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */
-- 
2.4.1

Re: [Qemu-devel] [PATCH 1/3] qcow2: Set MIN_L2_CACHE_SIZE to 2

[Max Reitz]

On 01.06.2015 18:09, Max Reitz wrote:
> The L2 cache must cover at least two L2 tables, because during COW two
> L2 tables are accessed simultaneously.
>
> Reported-by: Alexander Graf <agraf@suse.de>
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/qcow2.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 0076512..aa20022 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -62,7 +62,8 @@
>   #define MIN_CLUSTER_BITS 9
>   #define MAX_CLUSTER_BITS 21
>   
> -#define MIN_L2_CACHE_SIZE 1 /* cluster */
> +/* Must be at least 2 to cover COW */
> +#define MIN_L2_CACHE_SIZE 2 /* clusters */
>   
>   /* Must be at least 4 to cover all cases of refcount table growth */
>   #define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */

Of course git send-email ignored the qemu-stable line... CC-ing manually.

Max

Re: [Qemu-devel] [PATCH 1/3] qcow2: Set MIN_L2_CACHE_SIZE to 2

[Alexander Graf]

On 01.06.15 18:09, Max Reitz wrote:
> The L2 cache must cover at least two L2 tables, because during COW two
> L2 tables are accessed simultaneously.
> 
> Reported-by: Alexander Graf <agraf@suse.de>
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Tested-by: Alexander Graf <agraf@suse.de>


Alex

> ---
>  block/qcow2.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 0076512..aa20022 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -62,7 +62,8 @@
>  #define MIN_CLUSTER_BITS 9
>  #define MAX_CLUSTER_BITS 21
>  
> -#define MIN_L2_CACHE_SIZE 1 /* cluster */
> +/* Must be at least 2 to cover COW */
> +#define MIN_L2_CACHE_SIZE 2 /* clusters */
>  
>  /* Must be at least 4 to cover all cases of refcount table growth */
>  #define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */
> 

Re: [Qemu-devel] [PATCH 1/3] qcow2: Set MIN_L2_CACHE_SIZE to 2

[Alberto Garcia]

On Mon 01 Jun 2015 06:09:17 PM CEST, Max Reitz <mreitz@redhat.com> wrote:

> The L2 cache must cover at least two L2 tables, because during COW two
> L2 tables are accessed simultaneously.
>
> Reported-by: Alexander Graf <agraf@suse.de>
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

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

Berto

[Qemu-devel] [PATCH 2/3] iotests: qcow2 COW with minimal L2 cache size

[Max Reitz]

This adds a test case to test 103 for performing a COW operation in a
qcow2 image using an L2 cache with minimal size (which should be at
least two clusters so the COW can access both source and destination
simultaneously).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/103     | 10 ++++++++++
 tests/qemu-iotests/103.out |  5 +++++
 2 files changed, 15 insertions(+)

diff --git a/tests/qemu-iotests/103 b/tests/qemu-iotests/103
index ccab551..fa9a3c1 100755
--- a/tests/qemu-iotests/103
+++ b/tests/qemu-iotests/103
@@ -93,6 +93,16 @@ $QEMU_IO -c "open -o l2-cache-size=1M,refcount-cache-size=0.25M $TEST_IMG" \
          -c 'read -P 42 0 64k' \
     | _filter_qemu_io
 
+echo
+echo '=== Testing minimal L2 cache and COW ==='
+echo
+
+$QEMU_IMG snapshot -c foo "$TEST_IMG"
+# This requires a COW operation, which accesses two L2 tables simultaneously
+# (COW source and destination), so there must be enough space in the cache to
+# place both tables there (and qemu should not crash)
+$QEMU_IO -c "open -o cache-size=0 $TEST_IMG" -c 'write 0 64k' | _filter_qemu_io
+
 # success, all done
 echo '*** done'
 rm -f $seq.full
diff --git a/tests/qemu-iotests/103.out b/tests/qemu-iotests/103.out
index ee705b0..d05f49f 100644
--- a/tests/qemu-iotests/103.out
+++ b/tests/qemu-iotests/103.out
@@ -26,4 +26,9 @@ read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing minimal L2 cache and COW ===
+
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
2.4.1

Re: [Qemu-devel] [PATCH 2/3] iotests: qcow2 COW with minimal L2 cache size

[Alberto Garcia]

On Mon 01 Jun 2015 06:09:18 PM CEST, Max Reitz wrote:
> This adds a test case to test 103 for performing a COW operation in a
> qcow2 image using an L2 cache with minimal size (which should be at
> least two clusters so the COW can access both source and destination
> simultaneously).
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

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

Berto

[Qemu-devel] [PATCH 3/3] qcow2: Add DEFAULT_L2_CACHE_CLUSTERS

[Max Reitz]

If a relatively large cluster size is chosen, the default of 1 MB L2
cache is not really appropriate. In this case, unless overridden by the
user, the default cache size should not be determined by its size in
bytes but by the number of L2 tables (clusters) it is supposed to
contain.

Note that without this patch, MIN_L2_CACHE_SIZE will effectively take
over the same role. However, providing space for just two L2 tables is
not enough to be the default.

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

diff --git a/block/qcow2.c b/block/qcow2.c
index f7b4cc6..c4f6938 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -483,9 +483,11 @@ static const char *overlap_bool_option_names[QCOW2_OL_MAX_BITNR] = {
     [QCOW2_OL_INACTIVE_L2_BITNR]    = QCOW2_OPT_OVERLAP_INACTIVE_L2,
 };
 
-static void read_cache_sizes(QemuOpts *opts, uint64_t *l2_cache_size,
+static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
+                             uint64_t *l2_cache_size,
                              uint64_t *refcount_cache_size, Error **errp)
 {
+    BDRVQcowState *s = bs->opaque;
     uint64_t combined_cache_size;
     bool l2_cache_size_set, refcount_cache_size_set, combined_cache_size_set;
 
@@ -525,7 +527,9 @@ static void read_cache_sizes(QemuOpts *opts, uint64_t *l2_cache_size,
         }
     } else {
         if (!l2_cache_size_set && !refcount_cache_size_set) {
-            *l2_cache_size = DEFAULT_L2_CACHE_BYTE_SIZE;
+            *l2_cache_size = MAX(DEFAULT_L2_CACHE_BYTE_SIZE,
+                                 (uint64_t)DEFAULT_L2_CACHE_CLUSTERS
+                                 * s->cluster_size);
             *refcount_cache_size = *l2_cache_size
                                  / DEFAULT_L2_REFCOUNT_SIZE_RATIO;
         } else if (!l2_cache_size_set) {
@@ -803,7 +807,8 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    read_cache_sizes(opts, &l2_cache_size, &refcount_cache_size, &local_err);
+    read_cache_sizes(bs, opts, &l2_cache_size, &refcount_cache_size,
+                     &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
diff --git a/block/qcow2.h b/block/qcow2.h
index aa20022..5936d29 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -68,6 +68,8 @@
 /* Must be at least 4 to cover all cases of refcount table growth */
 #define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */
 
+/* Whichever is more */
+#define DEFAULT_L2_CACHE_CLUSTERS 8 /* clusters */
 #define DEFAULT_L2_CACHE_BYTE_SIZE 1048576 /* bytes */
 
 /* The refblock cache needs only a fourth of the L2 cache size to cover as many
-- 
2.4.1

Re: [Qemu-devel] [PATCH 3/3] qcow2: Add DEFAULT_L2_CACHE_CLUSTERS

[Alberto Garcia]

On Mon 01 Jun 2015 06:09:19 PM CEST, Max Reitz wrote:
> If a relatively large cluster size is chosen, the default of 1 MB L2
> cache is not really appropriate. In this case, unless overridden by the
> user, the default cache size should not be determined by its size in
> bytes but by the number of L2 tables (clusters) it is supposed to
> contain.
>
> Note that without this patch, MIN_L2_CACHE_SIZE will effectively take
> over the same role. However, providing space for just two L2 tables is
> not enough to be the default.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

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

Berto

Re: [Qemu-devel] [PATCH 0/3] qcow2: Fix MIN_L2_CACHE_SIZE

[Kevin Wolf]

Am 01.06.2015 um 18:09 hat Max Reitz geschrieben:
> This series fixes MIN_L2_CACHE_SIZE (which should not be 1, but 2
> (clusters)), and introduces a new constant, DEFAULT_L2_CACHE_CLUSTERS,
> so the default cache size is no longer always a fixed size in bytes but
> is also guaranteed to be able to hold a sane amount of L2 tables (which
> was determined to be 8 using the "proposal without protest" method).
> 
> So, in effect, after patch 3 of this series, the L2 cache (unless
> overridden by the user, of course) will either be 1 MB in size or be
> able to hold eight L2 tables, whichever is more.

Thanks, applied to the block branch.

Kevin