Patchwork [RFC,V1,12/14] qcow2: Add qcow2_dedup_update_metrics to compute dedup RAM usage.

login
register
mail settings
Submitter Benoît Canet
Date Jan. 16, 2013, 4:25 p.m.
Message ID <1358353508-5369-13-git-send-email-benoit@irqsave.net>
Download mbox | patch
Permalink /patch/213006/
State New
Headers show

Comments

Benoît Canet - Jan. 16, 2013, 4:25 p.m.
---
 block/qcow2-dedup.c |   13 +++++++++++++
 block/qcow2.h       |    1 +
 2 files changed, 14 insertions(+)
Eric Blake - Jan. 16, 2013, 8:10 p.m.
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.
Benoît Canet - Jan. 17, 2013, 11:11 a.m.
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.
Eric Blake - Jan. 17, 2013, 3:40 p.m.
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.

Patch

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