[Qemu-devel] [RFC PATCH 0/2] s/size/entries/ when dealing with non-byte units

[Qemu-devel] [RFC PATCH 0/2] s/size/entries/ when dealing with non-byte units

[Eric Blake]

I mentioned this while reviewing Berto's series on L2 slice handling;
this is a first cut at patches that I think are worth doing throughout
the qcow2 code base if we like the idea.

Eric Blake (2):
  qcow2: Prefer 'entries' over 'size' for non-byte values in spec
  qcow2: Prefer 'entries' over 'size' during cache creation

 docs/interop/qcow2.txt |  4 ++--
 block/qcow2.h          |  4 ++--
 block/qcow2.c          | 21 +++++++++++----------
 3 files changed, 15 insertions(+), 14 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/2] qcow2: Prefer 'entries' over 'size' for non-byte values in spec

[Eric Blake]

We want to limit the use of the term 'size' for only values that
count by bytes.  Renaming fields in the spec does not invalidate
any existing implementation, but may make future implementations
easier to write.

A reasonable followup would be to rename internal qemu code that
operates on qcow2 images to also use the distinction between
size and entries in variable names.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/interop/qcow2.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index d7fdb1fee31..597d3f261d5 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -47,7 +47,7 @@ The first cluster of a qcow2 image contains the file header:
                     1 for AES encryption
                     2 for LUKS encryption

-         36 - 39:   l1_size
+         36 - 39:   l1_entries
                     Number of entries in the active L1 table

          40 - 47:   l1_table_offset
@@ -538,7 +538,7 @@ Structure of a bitmap directory entry:
                     (described below) for the bitmap starts. Must be aligned to
                     a cluster boundary.

-         8 - 11:    bitmap_table_size
+         8 - 11:    bitmap_table_entries
                     Number of entries in the bitmap table of the bitmap.

         12 - 15:    flags
-- 
2.14.3

[Qemu-devel] [PATCH 2/2] qcow2: Prefer 'entries' over 'size' during cache creation

[Eric Blake]

Using 'size' for anything other than bytes is difficult to
reason about; let's rename entries related to the number of
entries in a cache accordingly.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/qcow2.h |  4 ++--
 block/qcow2.c | 21 +++++++++++----------
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 883802241fb..0daf8e6d6f8 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -68,10 +68,10 @@
 #define MAX_CLUSTER_BITS 21

 /* Must be at least 2 to cover COW */
-#define MIN_L2_CACHE_SIZE 2 /* cache entries */
+#define MIN_L2_CACHE_ENTRIES 2

 /* Must be at least 4 to cover all cases of refcount table growth */
-#define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */
+#define MIN_REFCOUNT_CACHE_ENTRIES 4

 /* Whichever is more */
 #define DEFAULT_L2_CACHE_CLUSTERS 8 /* clusters */
diff --git a/block/qcow2.c b/block/qcow2.c
index 288b5299d80..f25c33df1d1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -843,6 +843,7 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
     const char *opt_overlap_check, *opt_overlap_check_template;
     int overlap_check_template = 0;
     uint64_t l2_cache_size, l2_cache_entry_size, refcount_cache_size;
+    uint64_t l2_cache_entries, refcount_cache_entries;
     int i;
     const char *encryptfmt;
     QDict *encryptopts = NULL;
@@ -869,21 +870,21 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
         goto fail;
     }

-    l2_cache_size /= l2_cache_entry_size;
-    if (l2_cache_size < MIN_L2_CACHE_SIZE) {
-        l2_cache_size = MIN_L2_CACHE_SIZE;
+    l2_cache_entries = l2_cache_size / l2_cache_entry_size;
+    if (l2_cache_entries < MIN_L2_CACHE_ENTRIES) {
+        l2_cache_entries = MIN_L2_CACHE_ENTRIES;
     }
-    if (l2_cache_size > INT_MAX) {
+    if (l2_cache_entries > INT_MAX) {
         error_setg(errp, "L2 cache size too big");
         ret = -EINVAL;
         goto fail;
     }

-    refcount_cache_size /= s->cluster_size;
-    if (refcount_cache_size < MIN_REFCOUNT_CACHE_SIZE) {
-        refcount_cache_size = MIN_REFCOUNT_CACHE_SIZE;
+    refcount_cache_entries = refcount_cache_size / s->cluster_size;
+    if (refcount_cache_entries < MIN_REFCOUNT_CACHE_ENTRIES) {
+        refcount_cache_entries = MIN_REFCOUNT_CACHE_ENTRIES;
     }
-    if (refcount_cache_size > INT_MAX) {
+    if (refcount_cache_entries > INT_MAX) {
         error_setg(errp, "Refcount cache size too big");
         ret = -EINVAL;
         goto fail;
@@ -908,9 +909,9 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
     }

     r->l2_slice_size = l2_cache_entry_size / sizeof(uint64_t);
-    r->l2_table_cache = qcow2_cache_create(bs, l2_cache_size,
+    r->l2_table_cache = qcow2_cache_create(bs, l2_cache_entries,
                                            l2_cache_entry_size);
-    r->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size,
+    r->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_entries,
                                                  s->cluster_size);
     if (r->l2_table_cache == NULL || r->refcount_block_cache == NULL) {
         error_setg(errp, "Could not allocate metadata caches");
-- 
2.14.3

Re: [Qemu-devel] [RFC PATCH 0/2] s/size/entries/ when dealing with non-byte units

[Max Reitz]

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

On 2018-02-14 00:33, Eric Blake wrote:
> I mentioned this while reviewing Berto's series on L2 slice handling;
> this is a first cut at patches that I think are worth doing throughout
> the qcow2 code base if we like the idea.

I like the idea. :-)

The patches look good to me.

Max

> Eric Blake (2):
>   qcow2: Prefer 'entries' over 'size' for non-byte values in spec
>   qcow2: Prefer 'entries' over 'size' during cache creation
> 
>  docs/interop/qcow2.txt |  4 ++--
>  block/qcow2.h          |  4 ++--
>  block/qcow2.c          | 21 +++++++++++----------
>  3 files changed, 15 insertions(+), 14 deletions(-)
> 



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

Re: [Qemu-devel] [RFC PATCH 0/2] s/size/entries/ when dealing with non-byte units

[Alberto Garcia]

On Wed 14 Feb 2018 12:33:22 AM CET, Eric Blake wrote:
> I mentioned this while reviewing Berto's series on L2 slice handling;
> this is a first cut at patches that I think are worth doing throughout
> the qcow2 code base if we like the idea.
>
> Eric Blake (2):
>   qcow2: Prefer 'entries' over 'size' for non-byte values in spec
>   qcow2: Prefer 'entries' over 'size' during cache creation

I also like the idea. We'd need to change a lot of variables all over
the place, but things will look much more readable.

Berto

Re: [Qemu-devel] [Qemu-block] [RFC PATCH 0/2] s/size/entries/ when dealing with non-byte units

[Kevin Wolf]

Am 14.02.2018 um 00:33 hat Eric Blake geschrieben:
> I mentioned this while reviewing Berto's series on L2 slice handling;
> this is a first cut at patches that I think are worth doing throughout
> the qcow2 code base if we like the idea.

I agree it's a good change.

While we're at it, something I noticed in your block status series:
If something points to bytes, it should be an 'offset', and if it
points to entries, it's an 'index'. You changed a few things to byte
granularity, but still call them 'index'. Maybe we should clean that up
as well.

Kevin