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

[Eric Blake]

On 02/06/2013 05:32 AM, Benoît Canet wrote:
> ---
>  block/qcow2-dedup.c |   11 +++++++++++
>  block/qcow2.h       |   18 ++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
> index 308b4f6..72ce1d7 100644
> --- a/block/qcow2-dedup.c
> +++ b/block/qcow2-dedup.c
> @@ -1311,3 +1311,14 @@ void qcow2_dedup_close(BlockDriverState *bs)
>  {
>      qcow2_dedup_free(bs);
>  }
> +
> +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 * sizeof(GTreeNode_Copy) * 2;
> +    s->dedup_metrics.ram_usage += nb_hashs * sizeof(QCowHashNode);
> +}
> diff --git a/block/qcow2.h b/block/qcow2.h
> index fc393d5..191b272 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -76,6 +76,23 @@
>  #define QCOW_STRATEGY_DISK    (1 << 1)
>  #define QCOW_STRATEGY_RUNNING (1 << 2)
>  
> +/* This is an internal structure of glib gtree.c
> + * copy it here so we can compute it's size
> + * from glib/gtree.c in glib sources
> + */
> +typedef struct _GTreeNode_Copy  GTreeNode_Copy;
> +
> +struct _GTreeNode_Copy
> +{
> +  gpointer   key;         /* key for this node */
> +  gpointer   value;       /* value stored at this node */
> +  GTreeNode_Copy *left;  /* left subtree */
> +  GTreeNode_Copy *right; /* right subtree */
> +  gint8      balance;     /* height (right) - height (left) */
> +  guint8     left_child;
> +  guint8     right_child;
> +};

This doesn't belong in the .h file; stick it in the .c since that is the
only place where you use it.  It still feels kind of risky to be copying
this opaque struct - what if a newer version of glib changes the struct
size?  Do you have any way to ensure that your copy will remain in sync
with the actual struct used by the version of glib that you are linked with?

Would it be better to propose that upstream glib add a new function for
getting memory usage of an opaque hash object, and that qemu merely omit
the memory usage statistic when linked with an older version of glib
that does not provide the new accessor?

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

signature.asc
Description: OpenPGP digital signature