Re: [Qemu-devel] [PATCH v5 01/11] qcow2: Calculate refcount block entry count

[Kevin Wolf <kwolf@redhat.com>]

Am 21.10.2014 um 18:26 hat Max Reitz geschrieben:
> On 2014-10-20 at 16:48, Kevin Wolf wrote:
> >Am 20.10.2014 um 16:39 hat Max Reitz geschrieben:
> >>On 20.10.2014 at 16:25, Kevin Wolf wrote:
> >>>Am 29.08.2014 um 23:40 hat Max Reitz geschrieben:
> >>>>The size of a refblock entry is (in theory) variable; calculate
> >>>>therefore the number of entries per refblock and the according bit shift
> >>>>(1 << x == entry count) when opening an image.
> >>>>
> >>>>Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>>>---
> >>>>  block/qcow2.c | 2 ++
> >>>>  block/qcow2.h | 2 ++
> >>>>  2 files changed, 4 insertions(+)
> >>>>
> >>>>diff --git a/block/qcow2.c b/block/qcow2.c
> >>>>index f9e045f..172ad00 100644
> >>>>--- a/block/qcow2.c
> >>>>+++ b/block/qcow2.c
> >>>>@@ -689,6 +689,8 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
> >>>>      s->l2_bits = s->cluster_bits - 3; /* L2 is always one cluster */
> >>>>      s->l2_size = 1 << s->l2_bits;
> >>>>+    s->refcount_block_bits = s->cluster_bits - (s->refcount_order - 3);
> >>>>+    s->refcount_block_size = 1 << s->refcount_block_bits;
> >>>>      bs->total_sectors = header.size / 512;
> >>>>      s->csize_shift = (62 - (s->cluster_bits - 8));
> >>>>      s->csize_mask = (1 << (s->cluster_bits - 8)) - 1;
> >>>>diff --git a/block/qcow2.h b/block/qcow2.h
> >>>>index 6aeb7ea..7c01fb7 100644
> >>>>--- a/block/qcow2.h
> >>>>+++ b/block/qcow2.h
> >>>>@@ -222,6 +222,8 @@ typedef struct BDRVQcowState {
> >>>>      int l2_size;
> >>>>      int l1_size;
> >>>>      int l1_vm_state_index;
> >>>>+    int refcount_block_bits;
> >>>>+    int refcount_block_size;
> >>>Might just be me, but size sounds to me as if the unit were bytes. Would
> >>>you mind renaming this as refcount_block_entries or refblock_entries?
> >>If I'm doing a v7, no. Otherwise, well, see l1_size and l2_size. ;-)
> >Good point. This has confused people more than once, so it's probably
> >not just me.
> 
> Okay, now that I've done it and was about to send the series and
> just wanted to convert a local variable named refcount_table_size to
> refcount_table_entries, I decided not do do this. I'll call it
> refcount_block_size in v7 as well.
> 
> The reason for this is that I started looking for "_size" in
> block/qcow2-refcount.c. "_entries" is never used, the number of
> entries per L1, L2 or refcount table is always foo_size. In
> BDRVQcowState, there's not only l1_size and l2_size, but
> refcount_table_size as well. Calling it refcount_block_entries
> without renaming those would be extremely weird, and renaming those
> does not seem like a viable option to me. Furthermore, I'd find it
> extremely confusing to have "_entries" in some places and "_size" in
> others when there's no difference between the two. Currently, people
> ask "Why is this foo_size an entry count? ... Well, okay, that seems
> to be just the way it is." With foo_entries, it'll be "Why is this
> foo_size an entry count when bar_entries exists, so shouldn't it be
> foo_entries if they want it to be an entry count?"
> 
> I'll keep it as refcount_block_size, although I'm afraid reverting
> all these changes will be as hard as having made them in the first
> place... Oh, well, here goes.

Fair enough. Perhaps I should prepare a series renaming some things in
qcow2 once your current series are in (most importantly BDRVQcowState,
which also exists in qcow1 with the same name and makes debugging with
gdb harder than necessary...)

Kevin