[Qemu-devel] [RFC V1 00/14] QCOW2 deduplication metrics

[Qemu-devel] [RFC V1 00/14] QCOW2 deduplication metrics

[Benoît Canet]

This patchset implements QMP metrics for the qcow2 deduplication.

It applies on top of the "QCOW2 deduplication core functionality" patchset.

Benoît Canet (14):
  qcow2: Add deduplication metrics structures.
  qcow2: Initialize deduplication metrics.
  qcow2: Collect unaligned writes missing data reads metric.
  qcow2: Collect deduplicated cluster metric.
  qcow2: Collect undeduplicated cluster metric.
  qcow2: Count QCowHashNode creation metrics.
  qcow2: Count QCowHashNode removal from tree for metrics.
  qcow2: Count cluster deleted metric
  qcow2: Count deduplication refcount overflow metric.
  qapi: Add support for deduplication infos in qapi-schema.json.
  block: Add deduplication metrics to BlockDriverInfo.
  qcow2: Add qcow2_dedup_update_metrics to compute dedup RAM usage.
  qcow2: returns deduplication metrics and status via bdrv_get_info()
  qapi: Return virtual block device deduplication metrics in QMP

 block.c               |   36 ++++++++++++++++++++++++++++++++++++
 block/qcow2-dedup.c   |   36 ++++++++++++++++++++++++++++++++----
 block/qcow2.c         |    4 ++++
 block/qcow2.h         |    4 +++-
 include/block/block.h |   14 ++++++++++++++
 qapi-schema.json      |   40 +++++++++++++++++++++++++++++++++++++++-
 6 files changed, 128 insertions(+), 6 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [RFC V1 01/14] qcow2: Add deduplication metrics structures.

[Benoît Canet]

---
 block/qcow2.h         |    3 ++-
 include/block/block.h |   11 +++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 29267a9..0729ff2 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -237,9 +237,10 @@ typedef struct BDRVQcowState {
     int32_t dedup_table_size;
     GTree *dedup_tree_by_hash;
     GTree *dedup_tree_by_sect;
+    CoMutex dedup_lock;
+    BlockDeduplicationMetrics dedup_metrics;
 
     CoMutex lock;
-    CoMutex dedup_lock;
 
     uint32_t crypt_method; /* current crypt method, 0 if no key yet */
     uint32_t crypt_method_header;
diff --git a/include/block/block.h b/include/block/block.h
index b81d200..16e1cf1 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -12,6 +12,17 @@
 typedef struct BlockDriver BlockDriver;
 typedef struct BlockJob BlockJob;
 
+typedef struct {
+    uint64_t deduplicated_clusters;
+    uint64_t non_deduplicated_clusters;
+    uint64_t missing_data_reads; /* reads used to complete partials clusters */
+    uint64_t ram_hash_creations;     /* RAM based lookup */
+    uint64_t ram_hash_deletions;     /* RAM based lookup */
+    uint64_t ram_usage;              /* RAM usage in bytes */
+    uint64_t deleted_clusters;       /* number of deleted clusters */
+    uint64_t refcount_overflows;     /* number of refcount overflows */
+} BlockDeduplicationMetrics;
+
 typedef struct BlockDriverInfo {
     /* in bytes, 0 if irrelevant */
     int cluster_size;
-- 
1.7.10.4

[Qemu-devel] [RFC V1 02/14] qcow2: Initialize deduplication metrics.

[Benoît Canet]

---
 block/qcow2-dedup.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
index 6cd1af4..997714b 100644
--- a/block/qcow2-dedup.c
+++ b/block/qcow2-dedup.c
@@ -1272,6 +1272,8 @@ int qcow2_dedup_init(BlockDriverState *bs)
 
     s->has_dedup = true;
 
+    memset(&s->dedup_metrics, 0, sizeof(s->dedup_metrics));
+
     ret = qcow2_dedup_alloc(bs);
 
     if (ret < 0) {
-- 
1.7.10.4

[Qemu-devel] [RFC V1 03/14] qcow2: Collect unaligned writes missing data reads metric.

[Benoît Canet]

---
 block/qcow2-dedup.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
index 997714b..e4920d4 100644
--- a/block/qcow2-dedup.c
+++ b/block/qcow2-dedup.c
@@ -130,6 +130,7 @@ int qcow2_dedup_read_missing_and_concatenate(BlockDriverState *bs,
 
     /* read beginning */
     if (cluster_beginning_nr) {
+        s->dedup_metrics.missing_data_reads++;
         ret = qcow2_read_cluster_data(bs,
                                       *data,
                                       cluster_beginning_sector,
@@ -153,6 +154,7 @@ int qcow2_dedup_read_missing_and_concatenate(BlockDriverState *bs,
 
     /* read and add ending */
     if (cluster_ending_nr) {
+        s->dedup_metrics.missing_data_reads++;
         ret = qcow2_read_cluster_data(bs,
                                       *data +
                                       (cluster_beginning_nr +
-- 
1.7.10.4

[Qemu-devel] [RFC V1 04/14] qcow2: Collect deduplicated cluster metric.

[Benoît Canet]

---
 block/qcow2-dedup.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
index e4920d4..716371c 100644
--- a/block/qcow2-dedup.c
+++ b/block/qcow2-dedup.c
@@ -400,10 +400,14 @@ static int qcow2_deduplicate_cluster(BlockDriverState *bs,
     }
 
     /* Increment the refcount of the cluster */
-    return update_refcount(bs,
-                           (hash_node->physical_sect /
-                            s->cluster_sectors) << s->cluster_bits,
-                            1, 1, true);
+    ret = update_refcount(bs,
+                          (hash_node->physical_sect /
+                          s->cluster_sectors) << s->cluster_bits,
+                          1, 1, true);
+
+    s->dedup_metrics.deduplicated_clusters++;
+
+    return ret;
 }
 
 /* This function tries to deduplicate a given cluster.
-- 
1.7.10.4

[Qemu-devel] [RFC V1 05/14] qcow2: Collect undeduplicated cluster metric.

[Benoît Canet]

---
 block/qcow2-dedup.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
index 716371c..0f095a9 100644
--- a/block/qcow2-dedup.c
+++ b/block/qcow2-dedup.c
@@ -524,6 +524,7 @@ static int qcow2_count_next_non_dedupable_clusters(BlockDriverState *bs,
                                                    uint8_t *data,
                                                    int left_to_process)
 {
+    BDRVQcowState *s = bs->opaque;
     int i;
     int ret = 0;
     QCowHashNode *hash_node;
@@ -546,6 +547,7 @@ static int qcow2_count_next_non_dedupable_clusters(BlockDriverState *bs,
 
         qcow2_build_and_insert_hash_node(bs, &ds->phash.hash);
         add_hash_to_undedupable_list(bs, ds);
+        s->dedup_metrics.non_deduplicated_clusters++;
     }
 
     return i;
-- 
1.7.10.4

[Qemu-devel] [RFC V1 06/14] qcow2: Count QCowHashNode creation metrics.

[Benoît Canet]

---
 block/qcow2-dedup.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
index 0f095a9..d22e2a4 100644
--- a/block/qcow2-dedup.c
+++ b/block/qcow2-dedup.c
@@ -289,6 +289,7 @@ static void qcow2_build_and_insert_hash_node(BlockDriverState *bs,
                                                  QCOW_FLAG_EMPTY,
                                                  QCOW_FLAG_EMPTY);
     g_tree_insert(s->dedup_tree_by_hash, &hash_node->hash, hash_node);
+    s->dedup_metrics.ram_hash_creations++;
 }
 
 /*
@@ -1043,6 +1044,7 @@ static void qcow2_dedup_insert_hash_node(BlockDriverState *bs,
 
     g_tree_insert(s->dedup_tree_by_hash, &hash_node->hash, hash_node);
     g_tree_insert(s->dedup_tree_by_sect, &hash_node->physical_sect, hash_node);
+    s->dedup_metrics.ram_hash_creations++;
 }
 
 /* This load the QCowHashNode corresponding to a given cluster index into ram
-- 
1.7.10.4

[Qemu-devel] [RFC V1 07/14] qcow2: Count QCowHashNode removal from tree for metrics.

[Benoît Canet]

---
 block/qcow2-dedup.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
index d22e2a4..64e5a13 100644
--- a/block/qcow2-dedup.c
+++ b/block/qcow2-dedup.c
@@ -841,6 +841,7 @@ static void qcow2_remove_hash_node(BlockDriverState *bs,
     BDRVQcowState *s = bs->opaque;
     g_tree_remove(s->dedup_tree_by_sect, &hash_node->physical_sect);
     g_tree_remove(s->dedup_tree_by_hash, &hash_node->hash);
+    s->dedup_metrics.ram_hash_deletions++;
 }
 
 /* This function removes a hash_node from the trees given a physical sector
-- 
1.7.10.4

[Qemu-devel] [RFC V1 08/14] qcow2: Count cluster deleted metric

[Benoît Canet]

---
 block/qcow2-dedup.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
index 64e5a13..4a1b184 100644
--- a/block/qcow2-dedup.c
+++ b/block/qcow2-dedup.c
@@ -991,6 +991,7 @@ void qcow2_dedup_refcount_zero_reached(BlockDriverState *bs,
 
     /* remove from ram if present so we won't dedup with it anymore */
     qcow2_remove_hash_node_by_sector(bs, physical_sect);
+    s->dedup_metrics.deleted_clusters++;
 }
 
 /* Force to use a new physical cluster and QCowHashNode when the refcount pass
-- 
1.7.10.4

[Qemu-devel] [RFC V1 09/14] qcow2: Count deduplication refcount overflow metric.

[Benoît Canet]

---
 block/qcow2-dedup.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
index 4a1b184..db23b71 100644
--- a/block/qcow2-dedup.c
+++ b/block/qcow2-dedup.c
@@ -1024,6 +1024,7 @@ void qcow2_dedup_refcount_half_max_reached(BlockDriverState *bs,
 
     /* remove the QCowHashNode from ram so we won't use it anymore for dedup */
     qcow2_remove_hash_node(bs, hash_node);
+    s->dedup_metrics.refcount_overflows++;
 }
 
 bool qcow2_dedup_is_running(BlockDriverState *bs)
-- 
1.7.10.4

[Qemu-devel] [RFC V1 10/14] qapi: Add support for deduplication infos in qapi-schema.json.

[Benoît Canet]

---
 qapi-schema.json |   40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 5dfa052..1a5014c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -720,6 +720,40 @@
 { 'command': 'query-block', 'returns': ['BlockInfo'] }
 
 ##
+# @BlockDeviceDedupInfo
+#
+# Statistics of the deduplication on a virtual block device implementing it
+# since QEMU startup.
+#
+# @deduplicated-clusters:     Number of clusters which where deduplicated.
+#
+# @non-deduplicated-clusters: Number of clusters which where not deduplicated.
+#
+# @missing-data-reads:        Number of reads which where done to complete
+#                             unaligned or sub cluster sized writes.
+#
+# @ram-hash-creations:        Number of cluster hash created in RAM.
+#
+# @ram-hash-deletions:        Number of cluster hash deleted in RAM.
+#
+# @ram-usage:                 Number of bytes of RAM used.
+#
+# @deleted-clusters:          Number of deleted cluster when refcount < 0
+#
+# @refcount-overflows:        Number of refcount overflows
+#
+# @running:                   True if deduplication is running
+#
+# Since: 1.5.0
+##
+{ 'type': 'BlockDeviceDedupInfo',
+  'data': {'deduplicated-clusters': 'int', 'non-deduplicated-clusters': 'int',
+           'missing-data-reads': 'int', 'ram-hash-creations': 'int',
+           'ram-hash-deletions': 'int', 'ram-usage': 'int',
+           'deleted-clusters': 'int', 'refcount-overflows': 'int',
+           'running': 'bool' } }
+
+##
 # @BlockDeviceStats:
 #
 # Statistics of a virtual block device or a block backing device.
@@ -747,13 +781,17 @@
 #                     growable sparse files (like qcow2) that are used on top
 #                     of a physical device.
 #
+# @deduplication: #optional @BlockDeviceDedupInfo describing deduplication
+#                           metrics (since 1.5)
+#
 # Since: 0.14.0
 ##
 { 'type': 'BlockDeviceStats',
   'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'rd_operations': 'int',
            'wr_operations': 'int', 'flush_operations': 'int',
            'flush_total_time_ns': 'int', 'wr_total_time_ns': 'int',
-           'rd_total_time_ns': 'int', 'wr_highest_offset': 'int' } }
+           'rd_total_time_ns': 'int', 'wr_highest_offset': 'int',
+           '*deduplication': 'BlockDeviceDedupInfo' } }
 
 ##
 # @BlockStats:
-- 
1.7.10.4

[Qemu-devel] [RFC V1 11/14] block: Add deduplication metrics to BlockDriverInfo.

[Benoît Canet]

---
 include/block/block.h |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index 16e1cf1..2043560 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -29,6 +29,9 @@ typedef struct BlockDriverInfo {
     /* offset at which the VM state can be saved (0 if not possible) */
     int64_t vm_state_offset;
     bool is_dirty;
+    bool has_dedup;
+    bool dedup_running;
+    BlockDeduplicationMetrics dedup_metrics;
 } BlockDriverInfo;
 
 typedef struct BlockFragInfo {
-- 
1.7.10.4

[Qemu-devel] [RFC V1 12/14] qcow2: Add qcow2_dedup_update_metrics to compute dedup RAM usage.

[Benoît Canet]

---
 block/qcow2-dedup.c |   13 +++++++++++++
 block/qcow2.h       |    1 +
 2 files changed, 14 insertions(+)

diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
index db23b71..4305746 100644
--- a/block/qcow2-dedup.c
+++ b/block/qcow2-dedup.c
@@ -1311,3 +1311,16 @@ void qcow2_dedup_close(BlockDriverState *bs)
 {
     qcow2_dedup_free(bs);
 }
+
+#define GTREE_NODE_SIZE sizeof(int) * 5
+
+void qcow2_dedup_update_metrics(BlockDriverState *bs)
+{
+    BDRVQcowState *s = bs->opaque;
+
+    uint64_t nb_hashs = s->dedup_metrics.ram_hash_creations -
+                        s->dedup_metrics.ram_hash_deletions;
+
+    s->dedup_metrics.ram_usage = nb_hashs * GTREE_NODE_SIZE * 2;
+    s->dedup_metrics.ram_usage += nb_hashs * sizeof(QCowHashNode);
+}
diff --git a/block/qcow2.h b/block/qcow2.h
index 0729ff2..d8e8539 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -510,5 +510,6 @@ void qcow2_dedup_refcount_half_max_reached(BlockDriverState *bs,
 bool qcow2_dedup_is_running(BlockDriverState *bs);
 int qcow2_dedup_init(BlockDriverState *bs);
 void qcow2_dedup_close(BlockDriverState *bs);
+void qcow2_dedup_update_metrics(BlockDriverState *bs);
 
 #endif
-- 
1.7.10.4

[Qemu-devel] [RFC V1 13/14] qcow2: returns deduplication metrics and status via bdrv_get_info()

[Benoît Canet]

---
 block/qcow2.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 753fce0..e442268 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1868,6 +1868,10 @@ static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     BDRVQcowState *s = bs->opaque;
     bdi->cluster_size = s->cluster_size;
     bdi->vm_state_offset = qcow2_vm_state_offset(s);
+    bdi->has_dedup = s->has_dedup;
+    bdi->dedup_running = s->dedup_status == QCOW_DEDUP_STARTED;
+    qcow2_dedup_update_metrics(bs);
+    memcpy(&bdi->dedup_metrics, &s->dedup_metrics, sizeof(bdi->dedup_metrics));
     return 0;
 }
 
-- 
1.7.10.4

[Qemu-devel] [RFC V1 14/14] qapi: Return virtual block device deduplication metrics in QMP

[Benoît Canet]

---
 block.c |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/block.c b/block.c
index 4e28c55..a245653 100644
--- a/block.c
+++ b/block.c
@@ -2921,6 +2921,40 @@ BlockInfoList *qmp_query_block(Error **errp)
     return head;
 }
 
+static void bdrv_get_dedup_metrics(const BlockDriverState *bs,
+                                   BlockDeviceStats *stats)
+{
+    BlockDriverInfo bdi;
+
+    if (bdrv_get_info((BlockDriverState *) bs, &bdi) < 0) {
+        return;
+    }
+
+    if (!bdi.has_dedup) {
+        return;
+    }
+
+    stats->has_deduplication = true;
+    stats->deduplication = g_malloc0(sizeof(*stats->deduplication));
+    stats->deduplication->deduplicated_clusters =
+        bdi.dedup_metrics.deduplicated_clusters;
+    stats->deduplication->non_deduplicated_clusters =
+        bdi.dedup_metrics.non_deduplicated_clusters;
+    stats->deduplication->missing_data_reads =
+        bdi.dedup_metrics.missing_data_reads;
+    stats->deduplication->ram_hash_creations =
+        bdi.dedup_metrics.ram_hash_creations;
+    stats->deduplication->ram_hash_deletions =
+        bdi.dedup_metrics.ram_hash_deletions;
+    stats->deduplication->ram_usage =
+        bdi.dedup_metrics.ram_usage;
+    stats->deduplication->deleted_clusters =
+        bdi.dedup_metrics.deleted_clusters;
+    stats->deduplication->refcount_overflows =
+        bdi.dedup_metrics.refcount_overflows;
+    stats->deduplication->running = bdi.dedup_running;
+}
+
 BlockStats *bdrv_query_stats(const BlockDriverState *bs)
 {
     BlockStats *s;
@@ -2943,6 +2977,8 @@ BlockStats *bdrv_query_stats(const BlockDriverState *bs)
     s->stats->rd_total_time_ns = bs->total_time_ns[BDRV_ACCT_READ];
     s->stats->flush_total_time_ns = bs->total_time_ns[BDRV_ACCT_FLUSH];
 
+    bdrv_get_dedup_metrics(bs, s->stats);
+
     if (bs->file) {
         s->has_parent = true;
         s->parent = bdrv_query_stats(bs->file);
-- 
1.7.10.4

Re: [Qemu-devel] [RFC V1 10/14] qapi: Add support for deduplication infos in qapi-schema.json.

[Eric Blake]

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

On 01/16/2013 09:25 AM, Benoît Canet wrote:
> ---
>  qapi-schema.json |   40 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 

> +#
> +# @deleted-clusters:          Number of deleted cluster when refcount < 0

Is it really when refcount goes negative, or when it reaches zero?
Overall it seems nice.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [RFC V1 12/14] qcow2: Add qcow2_dedup_update_metrics to compute dedup RAM usage.

[Eric Blake]

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

On 01/16/2013 09:25 AM, Benoît Canet wrote:
> ---
>  block/qcow2-dedup.c |   13 +++++++++++++
>  block/qcow2.h       |    1 +
>  2 files changed, 14 insertions(+)
> 
> diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
> index db23b71..4305746 100644
> --- a/block/qcow2-dedup.c
> +++ b/block/qcow2-dedup.c
> @@ -1311,3 +1311,16 @@ void qcow2_dedup_close(BlockDriverState *bs)
>  {
>      qcow2_dedup_free(bs);
>  }
> +
> +#define GTREE_NODE_SIZE sizeof(int) * 5

Improperly parenthesized.  Also, this feels like a magic number, is
there an actual sizeof(struct) you could use instead of hand-computing
how much is used per node?

> +
> +void qcow2_dedup_update_metrics(BlockDriverState *bs)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +
> +    uint64_t nb_hashs = s->dedup_metrics.ram_hash_creations -
> +                        s->dedup_metrics.ram_hash_deletions;
> +
> +    s->dedup_metrics.ram_usage = nb_hashs * GTREE_NODE_SIZE * 2;

But you got lucky that order of operations didn't care about the missing
() here.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [RFC V1 12/14] qcow2: Add qcow2_dedup_update_metrics to compute dedup RAM usage.

[Benoît Canet]

Le Wednesday 16 Jan 2013 à 13:10:12 (-0700), Eric Blake a écrit :
> On 01/16/2013 09:25 AM, Benoît Canet wrote:
> > ---
> >  block/qcow2-dedup.c |   13 +++++++++++++
> >  block/qcow2.h       |    1 +
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
> > index db23b71..4305746 100644
> > --- a/block/qcow2-dedup.c
> > +++ b/block/qcow2-dedup.c
> > @@ -1311,3 +1311,16 @@ void qcow2_dedup_close(BlockDriverState *bs)
> >  {
> >      qcow2_dedup_free(bs);
> >  }
> > +
> > +#define GTREE_NODE_SIZE sizeof(int) * 5
> 
> Improperly parenthesized.  Also, this feels like a magic number, is
> there an actual sizeof(struct) you could use instead of hand-computing
> how much is used per node?

No the glib implementation totally hide it's structures.

Re: [Qemu-devel] [RFC V1 10/14] qapi: Add support for deduplication infos in qapi-schema.json.

[Benoît Canet]

> +# @running:                   True if deduplication is running

Internally QCOW2 deduplication state switch between STOPPED, STARTING, RUNNING
and STOPPING.

Should the running field be a status field reflecting all theses states in a
string for example ?
Or is the boolean ok ?

Regards

Benoît

Re: [Qemu-devel] [RFC V1 10/14] qapi: Add support for deduplication infos in qapi-schema.json.

[Eric Blake]

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

On 01/17/2013 05:15 AM, Benoît Canet wrote:
>> +# @running:                   True if deduplication is running
> 
> Internally QCOW2 deduplication state switch between STOPPED, STARTING, RUNNING
> and STOPPING.
> 
> Should the running field be a status field reflecting all theses states in a
> string for example ?
> Or is the boolean ok ?

Outputting an enum of all four states may be the best for debugging
purposes.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [RFC V1 12/14] qcow2: Add qcow2_dedup_update_metrics to compute dedup RAM usage.

[Eric Blake]

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

On 01/17/2013 04:11 AM, Benoît Canet wrote:
> Le Wednesday 16 Jan 2013 à 13:10:12 (-0700), Eric Blake a écrit :
>> On 01/16/2013 09:25 AM, Benoît Canet wrote:
>>> ---
>>>  block/qcow2-dedup.c |   13 +++++++++++++
>>>  block/qcow2.h       |    1 +
>>>  2 files changed, 14 insertions(+)
>>>
>>> diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
>>> index db23b71..4305746 100644
>>> --- a/block/qcow2-dedup.c
>>> +++ b/block/qcow2-dedup.c
>>> @@ -1311,3 +1311,16 @@ void qcow2_dedup_close(BlockDriverState *bs)
>>>  {
>>>      qcow2_dedup_free(bs);
>>>  }
>>> +
>>> +#define GTREE_NODE_SIZE sizeof(int) * 5
>>
>> Improperly parenthesized.  Also, this feels like a magic number, is
>> there an actual sizeof(struct) you could use instead of hand-computing
>> how much is used per node?
> 
> No the glib implementation totally hide it's structures.

Also, are you sure that this value is correct on both 32-bit and 64-bit
machines, or does a gtree node include a void* which changes how much
memory it uses based on platform?  Even adding a comment pointing to the
internal glib implementation that you were copying from would be
helpful, so that it doesn't feel quite so magic.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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