[Qemu-devel] [PATCH 0/3] block/qapi: Include empty drives in query-blockstats

[Qemu-devel] [PATCH 0/3] block/qapi: Include empty drives in query-blockstats

[Kevin Wolf]

This series fixes a regression introduced by recent blockdev work. As a nice
side effect, it gets rid of at least one user of bs->blk.

Kevin Wolf (3):
  block/qapi: Factor out bdrv_query_blk_stats()
  block/qapi: Factor out bdrv_query_bds_stats()
  block/qapi: Include empty drives in query-blockstats

 block/qapi.c | 178 +++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 105 insertions(+), 73 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] block/qapi: Factor out bdrv_query_blk_stats()

[Kevin Wolf]

The new functions handles the data that is taken from the BlockBackend.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qapi.c | 131 +++++++++++++++++++++++++++++++----------------------------
 1 file changed, 68 insertions(+), 63 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index db2d3fb..c04f1d8 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -355,6 +355,73 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
     qapi_free_BlockInfo(info);
 }
 
+static void bdrv_query_blk_stats(BlockStats *s, BlockBackend *blk)
+{
+    BlockAcctStats *stats = blk_get_stats(blk);
+    BlockAcctTimedStats *ts = NULL;
+
+    s->stats->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ];
+    s->stats->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE];
+    s->stats->rd_operations = stats->nr_ops[BLOCK_ACCT_READ];
+    s->stats->wr_operations = stats->nr_ops[BLOCK_ACCT_WRITE];
+
+    s->stats->failed_rd_operations = stats->failed_ops[BLOCK_ACCT_READ];
+    s->stats->failed_wr_operations = stats->failed_ops[BLOCK_ACCT_WRITE];
+    s->stats->failed_flush_operations = stats->failed_ops[BLOCK_ACCT_FLUSH];
+
+    s->stats->invalid_rd_operations = stats->invalid_ops[BLOCK_ACCT_READ];
+    s->stats->invalid_wr_operations = stats->invalid_ops[BLOCK_ACCT_WRITE];
+    s->stats->invalid_flush_operations =
+        stats->invalid_ops[BLOCK_ACCT_FLUSH];
+
+    s->stats->rd_merged = stats->merged[BLOCK_ACCT_READ];
+    s->stats->wr_merged = stats->merged[BLOCK_ACCT_WRITE];
+    s->stats->flush_operations = stats->nr_ops[BLOCK_ACCT_FLUSH];
+    s->stats->wr_total_time_ns = stats->total_time_ns[BLOCK_ACCT_WRITE];
+    s->stats->rd_total_time_ns = stats->total_time_ns[BLOCK_ACCT_READ];
+    s->stats->flush_total_time_ns = stats->total_time_ns[BLOCK_ACCT_FLUSH];
+
+    s->stats->has_idle_time_ns = stats->last_access_time_ns > 0;
+    if (s->stats->has_idle_time_ns) {
+        s->stats->idle_time_ns = block_acct_idle_time_ns(stats);
+    }
+
+    s->stats->account_invalid = stats->account_invalid;
+    s->stats->account_failed = stats->account_failed;
+
+    while ((ts = block_acct_interval_next(stats, ts))) {
+        BlockDeviceTimedStatsList *timed_stats =
+            g_malloc0(sizeof(*timed_stats));
+        BlockDeviceTimedStats *dev_stats = g_malloc0(sizeof(*dev_stats));
+        timed_stats->next = s->stats->timed_stats;
+        timed_stats->value = dev_stats;
+        s->stats->timed_stats = timed_stats;
+
+        TimedAverage *rd = &ts->latency[BLOCK_ACCT_READ];
+        TimedAverage *wr = &ts->latency[BLOCK_ACCT_WRITE];
+        TimedAverage *fl = &ts->latency[BLOCK_ACCT_FLUSH];
+
+        dev_stats->interval_length = ts->interval_length;
+
+        dev_stats->min_rd_latency_ns = timed_average_min(rd);
+        dev_stats->max_rd_latency_ns = timed_average_max(rd);
+        dev_stats->avg_rd_latency_ns = timed_average_avg(rd);
+
+        dev_stats->min_wr_latency_ns = timed_average_min(wr);
+        dev_stats->max_wr_latency_ns = timed_average_max(wr);
+        dev_stats->avg_wr_latency_ns = timed_average_avg(wr);
+
+        dev_stats->min_flush_latency_ns = timed_average_min(fl);
+        dev_stats->max_flush_latency_ns = timed_average_max(fl);
+        dev_stats->avg_flush_latency_ns = timed_average_avg(fl);
+
+        dev_stats->avg_rd_queue_depth =
+            block_acct_queue_depth(ts, BLOCK_ACCT_READ);
+        dev_stats->avg_wr_queue_depth =
+            block_acct_queue_depth(ts, BLOCK_ACCT_WRITE);
+    }
+}
+
 static BlockStats *bdrv_query_stats(const BlockDriverState *bs,
                                     bool query_backing)
 {
@@ -374,69 +441,7 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs,
 
     s->stats = g_malloc0(sizeof(*s->stats));
     if (bs->blk) {
-        BlockAcctStats *stats = blk_get_stats(bs->blk);
-        BlockAcctTimedStats *ts = NULL;
-
-        s->stats->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ];
-        s->stats->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE];
-        s->stats->rd_operations = stats->nr_ops[BLOCK_ACCT_READ];
-        s->stats->wr_operations = stats->nr_ops[BLOCK_ACCT_WRITE];
-
-        s->stats->failed_rd_operations = stats->failed_ops[BLOCK_ACCT_READ];
-        s->stats->failed_wr_operations = stats->failed_ops[BLOCK_ACCT_WRITE];
-        s->stats->failed_flush_operations = stats->failed_ops[BLOCK_ACCT_FLUSH];
-
-        s->stats->invalid_rd_operations = stats->invalid_ops[BLOCK_ACCT_READ];
-        s->stats->invalid_wr_operations = stats->invalid_ops[BLOCK_ACCT_WRITE];
-        s->stats->invalid_flush_operations =
-            stats->invalid_ops[BLOCK_ACCT_FLUSH];
-
-        s->stats->rd_merged = stats->merged[BLOCK_ACCT_READ];
-        s->stats->wr_merged = stats->merged[BLOCK_ACCT_WRITE];
-        s->stats->flush_operations = stats->nr_ops[BLOCK_ACCT_FLUSH];
-        s->stats->wr_total_time_ns = stats->total_time_ns[BLOCK_ACCT_WRITE];
-        s->stats->rd_total_time_ns = stats->total_time_ns[BLOCK_ACCT_READ];
-        s->stats->flush_total_time_ns = stats->total_time_ns[BLOCK_ACCT_FLUSH];
-
-        s->stats->has_idle_time_ns = stats->last_access_time_ns > 0;
-        if (s->stats->has_idle_time_ns) {
-            s->stats->idle_time_ns = block_acct_idle_time_ns(stats);
-        }
-
-        s->stats->account_invalid = stats->account_invalid;
-        s->stats->account_failed = stats->account_failed;
-
-        while ((ts = block_acct_interval_next(stats, ts))) {
-            BlockDeviceTimedStatsList *timed_stats =
-                g_malloc0(sizeof(*timed_stats));
-            BlockDeviceTimedStats *dev_stats = g_malloc0(sizeof(*dev_stats));
-            timed_stats->next = s->stats->timed_stats;
-            timed_stats->value = dev_stats;
-            s->stats->timed_stats = timed_stats;
-
-            TimedAverage *rd = &ts->latency[BLOCK_ACCT_READ];
-            TimedAverage *wr = &ts->latency[BLOCK_ACCT_WRITE];
-            TimedAverage *fl = &ts->latency[BLOCK_ACCT_FLUSH];
-
-            dev_stats->interval_length = ts->interval_length;
-
-            dev_stats->min_rd_latency_ns = timed_average_min(rd);
-            dev_stats->max_rd_latency_ns = timed_average_max(rd);
-            dev_stats->avg_rd_latency_ns = timed_average_avg(rd);
-
-            dev_stats->min_wr_latency_ns = timed_average_min(wr);
-            dev_stats->max_wr_latency_ns = timed_average_max(wr);
-            dev_stats->avg_wr_latency_ns = timed_average_avg(wr);
-
-            dev_stats->min_flush_latency_ns = timed_average_min(fl);
-            dev_stats->max_flush_latency_ns = timed_average_max(fl);
-            dev_stats->avg_flush_latency_ns = timed_average_avg(fl);
-
-            dev_stats->avg_rd_queue_depth =
-                block_acct_queue_depth(ts, BLOCK_ACCT_READ);
-            dev_stats->avg_wr_queue_depth =
-                block_acct_queue_depth(ts, BLOCK_ACCT_WRITE);
-        }
+        bdrv_query_blk_stats(s, bs->blk);
     }
 
     s->stats->wr_highest_offset = bs->wr_highest_offset;
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/3] block/qapi: Factor out bdrv_query_bds_stats()

[Kevin Wolf]

The new functions handles the data that is taken from the
BlockDriverState.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qapi.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index c04f1d8..31ae879 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -355,6 +355,9 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
     qapi_free_BlockInfo(info);
 }
 
+static BlockStats *bdrv_query_stats(const BlockDriverState *bs,
+                                    bool query_backing);
+
 static void bdrv_query_blk_stats(BlockStats *s, BlockBackend *blk)
 {
     BlockAcctStats *stats = blk_get_stats(blk);
@@ -422,13 +425,9 @@ static void bdrv_query_blk_stats(BlockStats *s, BlockBackend *blk)
     }
 }
 
-static BlockStats *bdrv_query_stats(const BlockDriverState *bs,
-                                    bool query_backing)
+static void bdrv_query_bds_stats(BlockStats *s, const BlockDriverState *bs,
+                                 bool query_backing)
 {
-    BlockStats *s;
-
-    s = g_malloc0(sizeof(*s));
-
     if (bdrv_get_device_name(bs)[0]) {
         s->has_device = true;
         s->device = g_strdup(bdrv_get_device_name(bs));
@@ -439,11 +438,6 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs,
         s->node_name = g_strdup(bdrv_get_node_name(bs));
     }
 
-    s->stats = g_malloc0(sizeof(*s->stats));
-    if (bs->blk) {
-        bdrv_query_blk_stats(s, bs->blk);
-    }
-
     s->stats->wr_highest_offset = bs->wr_highest_offset;
 
     if (bs->file) {
@@ -456,6 +450,21 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs,
         s->backing = bdrv_query_stats(bs->backing->bs, query_backing);
     }
 
+}
+
+static BlockStats *bdrv_query_stats(const BlockDriverState *bs,
+                                    bool query_backing)
+{
+    BlockStats *s;
+
+    s = g_malloc0(sizeof(*s));
+    s->stats = g_malloc0(sizeof(*s->stats));
+
+    if (bs->blk) {
+        bdrv_query_blk_stats(s, bs->blk);
+    }
+    bdrv_query_bds_stats(s, bs, query_backing);
+
     return s;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/3] block/qapi: Include empty drives in query-blockstats

[Kevin Wolf]

Since commit 5ec18f8c, query-blockstats didn't return the statistics of
drives without media any more because such drives have only a BB now,
but not a BDS any more.

This patch fixes the regression so that query-blockstats iterates over
BBs by default and empty drives are displayed again.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qapi.c | 48 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 31ae879..6a4869a 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -355,7 +355,8 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
     qapi_free_BlockInfo(info);
 }
 
-static BlockStats *bdrv_query_stats(const BlockDriverState *bs,
+static BlockStats *bdrv_query_stats(BlockBackend *blk,
+                                    const BlockDriverState *bs,
                                     bool query_backing);
 
 static void bdrv_query_blk_stats(BlockStats *s, BlockBackend *blk)
@@ -363,6 +364,9 @@ static void bdrv_query_blk_stats(BlockStats *s, BlockBackend *blk)
     BlockAcctStats *stats = blk_get_stats(blk);
     BlockAcctTimedStats *ts = NULL;
 
+    s->has_device = true;
+    s->device = g_strdup(blk_name(blk));
+
     s->stats->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ];
     s->stats->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE];
     s->stats->rd_operations = stats->nr_ops[BLOCK_ACCT_READ];
@@ -428,11 +432,6 @@ static void bdrv_query_blk_stats(BlockStats *s, BlockBackend *blk)
 static void bdrv_query_bds_stats(BlockStats *s, const BlockDriverState *bs,
                                  bool query_backing)
 {
-    if (bdrv_get_device_name(bs)[0]) {
-        s->has_device = true;
-        s->device = g_strdup(bdrv_get_device_name(bs));
-    }
-
     if (bdrv_get_node_name(bs)[0]) {
         s->has_node_name = true;
         s->node_name = g_strdup(bdrv_get_node_name(bs));
@@ -442,17 +441,18 @@ static void bdrv_query_bds_stats(BlockStats *s, const BlockDriverState *bs,
 
     if (bs->file) {
         s->has_parent = true;
-        s->parent = bdrv_query_stats(bs->file->bs, query_backing);
+        s->parent = bdrv_query_stats(NULL, bs->file->bs, query_backing);
     }
 
     if (query_backing && bs->backing) {
         s->has_backing = true;
-        s->backing = bdrv_query_stats(bs->backing->bs, query_backing);
+        s->backing = bdrv_query_stats(NULL, bs->backing->bs, query_backing);
     }
 
 }
 
-static BlockStats *bdrv_query_stats(const BlockDriverState *bs,
+static BlockStats *bdrv_query_stats(BlockBackend *blk,
+                                    const BlockDriverState *bs,
                                     bool query_backing)
 {
     BlockStats *s;
@@ -460,10 +460,12 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs,
     s = g_malloc0(sizeof(*s));
     s->stats = g_malloc0(sizeof(*s->stats));
 
-    if (bs->blk) {
-        bdrv_query_blk_stats(s, bs->blk);
+    if (blk) {
+        bdrv_query_blk_stats(s, blk);
+    }
+    if (bs) {
+        bdrv_query_bds_stats(s, bs, query_backing);
     }
-    bdrv_query_bds_stats(s, bs, query_backing);
 
     return s;
 }
@@ -491,22 +493,38 @@ BlockInfoList *qmp_query_block(Error **errp)
     return head;
 }
 
+static bool next_query_bds(BlockBackend **blk, BlockDriverState **bs,
+                           bool query_nodes)
+{
+    if (query_nodes) {
+        *bs = bdrv_next_node(*bs);
+        return !!*bs;
+    }
+
+    *blk = blk_next(*blk);
+    *bs = *blk ? blk_bs(*blk) : NULL;
+
+    return !!*blk;
+}
+
 BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
                                      bool query_nodes,
                                      Error **errp)
 {
     BlockStatsList *head = NULL, **p_next = &head;
+    BlockBackend *blk = NULL;
     BlockDriverState *bs = NULL;
 
     /* Just to be safe if query_nodes is not always initialized */
     query_nodes = has_query_nodes && query_nodes;
 
-    while ((bs = query_nodes ? bdrv_next_node(bs) : bdrv_next(bs))) {
+    while (next_query_bds(&blk, &bs, query_nodes)) {
         BlockStatsList *info = g_malloc0(sizeof(*info));
-        AioContext *ctx = bdrv_get_aio_context(bs);
+        AioContext *ctx = blk ? blk_get_aio_context(blk)
+                              : bdrv_get_aio_context(bs);
 
         aio_context_acquire(ctx);
-        info->value = bdrv_query_stats(bs, !query_nodes);
+        info->value = bdrv_query_stats(blk, bs, !query_nodes);
         aio_context_release(ctx);
 
         *p_next = info;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/3] block/qapi: Factor out bdrv_query_blk_stats()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 359 bytes --]

On 26.02.2016 21:22, Kevin Wolf wrote:
> The new functions handles the data that is taken from the BlockBackend.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qapi.c | 131 +++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 68 insertions(+), 63 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH 2/3] block/qapi: Factor out bdrv_query_bds_stats()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 567 bytes --]

On 26.02.2016 21:22, Kevin Wolf wrote:
> The new functions handles the data that is taken from the
> BlockDriverState.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qapi.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Now that I see s->stats->wr_highest_offset in this function, thus
piquing my interest in s->stats... How about another patch which makes
bdrv_query_blk_stats() take the BlockDeviceStats pointer instead of
BlockStats?

Max


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

Re: [Qemu-devel] [PATCH 2/3] block/qapi: Factor out bdrv_query_bds_stats()

[Kevin Wolf]

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

Am 02.03.2016 um 17:47 hat Max Reitz geschrieben:
> On 26.02.2016 21:22, Kevin Wolf wrote:
> > The new functions handles the data that is taken from the
> > BlockDriverState.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qapi.c | 31 ++++++++++++++++++++-----------
> >  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> Now that I see s->stats->wr_highest_offset in this function, thus
> piquing my interest in s->stats... How about another patch which makes
> bdrv_query_blk_stats() take the BlockDeviceStats pointer instead of
> BlockStats?

Hm, doesn't work any more after patch 3. Which doesn't necessarily mean
that it's a bad thought, maybe s->device can be set in the caller
instead.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 2/3] block/qapi: Factor out bdrv_query_bds_stats()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 951 bytes --]

On 02.03.2016 17:52, Kevin Wolf wrote:
> Am 02.03.2016 um 17:47 hat Max Reitz geschrieben:
>> On 26.02.2016 21:22, Kevin Wolf wrote:
>>> The new functions handles the data that is taken from the
>>> BlockDriverState.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  block/qapi.c | 31 ++++++++++++++++++++-----------
>>>  1 file changed, 20 insertions(+), 11 deletions(-)
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>> Now that I see s->stats->wr_highest_offset in this function, thus
>> piquing my interest in s->stats... How about another patch which makes
>> bdrv_query_blk_stats() take the BlockDeviceStats pointer instead of
>> BlockStats?
> 
> Hm, doesn't work any more after patch 3.

I just noticed. :-)

>                                          Which doesn't necessarily mean
> that it's a bad thought, maybe s->device can be set in the caller
> instead.

Seems reasonable to me.

Max


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

Re: [Qemu-devel] [PATCH 3/3] block/qapi: Include empty drives in query-blockstats

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2930 bytes --]

On 26.02.2016 21:22, Kevin Wolf wrote:
> Since commit 5ec18f8c, query-blockstats didn't return the statistics of
> drives without media any more because such drives have only a BB now,
> but not a BDS any more.
> 
> This patch fixes the regression so that query-blockstats iterates over
> BBs by default and empty drives are displayed again.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qapi.c | 48 +++++++++++++++++++++++++++++++++---------------
>  1 file changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index 31ae879..6a4869a 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -355,7 +355,8 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
>      qapi_free_BlockInfo(info);
>  }
>  
> -static BlockStats *bdrv_query_stats(const BlockDriverState *bs,
> +static BlockStats *bdrv_query_stats(BlockBackend *blk,
> +                                    const BlockDriverState *bs,
>                                      bool query_backing);
>  
>  static void bdrv_query_blk_stats(BlockStats *s, BlockBackend *blk)
> @@ -363,6 +364,9 @@ static void bdrv_query_blk_stats(BlockStats *s, BlockBackend *blk)
>      BlockAcctStats *stats = blk_get_stats(blk);
>      BlockAcctTimedStats *ts = NULL;
>  
> +    s->has_device = true;
> +    s->device = g_strdup(blk_name(blk));
> +
>      s->stats->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ];
>      s->stats->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE];
>      s->stats->rd_operations = stats->nr_ops[BLOCK_ACCT_READ];
> @@ -428,11 +432,6 @@ static void bdrv_query_blk_stats(BlockStats *s, BlockBackend *blk)
>  static void bdrv_query_bds_stats(BlockStats *s, const BlockDriverState *bs,
>                                   bool query_backing)
>  {
> -    if (bdrv_get_device_name(bs)[0]) {
> -        s->has_device = true;
> -        s->device = g_strdup(bdrv_get_device_name(bs));
> -    }
> -
>      if (bdrv_get_node_name(bs)[0]) {
>          s->has_node_name = true;
>          s->node_name = g_strdup(bdrv_get_node_name(bs));
> @@ -442,17 +441,18 @@ static void bdrv_query_bds_stats(BlockStats *s, const BlockDriverState *bs,
>  
>      if (bs->file) {
>          s->has_parent = true;
> -        s->parent = bdrv_query_stats(bs->file->bs, query_backing);
> +        s->parent = bdrv_query_stats(NULL, bs->file->bs, query_backing);
>      }
>  
>      if (query_backing && bs->backing) {
>          s->has_backing = true;
> -        s->backing = bdrv_query_stats(bs->backing->bs, query_backing);
> +        s->backing = bdrv_query_stats(NULL, bs->backing->bs, query_backing);

This breaks my very important use case of having BBs on backing BDSs!!!

Kidding aside: Maybe someone actually wants to do this? Should we feel
bad for breaking query-blockstats for those BBs?

If no:

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH 3/3] block/qapi: Include empty drives in query-blockstats

[Kevin Wolf]

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

Am 02.03.2016 um 18:10 hat Max Reitz geschrieben:
> On 26.02.2016 21:22, Kevin Wolf wrote:
> > Since commit 5ec18f8c, query-blockstats didn't return the statistics of
> > drives without media any more because such drives have only a BB now,
> > but not a BDS any more.
> > 
> > This patch fixes the regression so that query-blockstats iterates over
> > BBs by default and empty drives are displayed again.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qapi.c | 48 +++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 33 insertions(+), 15 deletions(-)
> > 
> > diff --git a/block/qapi.c b/block/qapi.c
> > index 31ae879..6a4869a 100644
> > --- a/block/qapi.c
> > +++ b/block/qapi.c
> > @@ -355,7 +355,8 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
> >      qapi_free_BlockInfo(info);
> >  }
> >  
> > -static BlockStats *bdrv_query_stats(const BlockDriverState *bs,
> > +static BlockStats *bdrv_query_stats(BlockBackend *blk,
> > +                                    const BlockDriverState *bs,
> >                                      bool query_backing);
> >  
> >  static void bdrv_query_blk_stats(BlockStats *s, BlockBackend *blk)
> > @@ -363,6 +364,9 @@ static void bdrv_query_blk_stats(BlockStats *s, BlockBackend *blk)
> >      BlockAcctStats *stats = blk_get_stats(blk);
> >      BlockAcctTimedStats *ts = NULL;
> >  
> > +    s->has_device = true;
> > +    s->device = g_strdup(blk_name(blk));
> > +
> >      s->stats->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ];
> >      s->stats->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE];
> >      s->stats->rd_operations = stats->nr_ops[BLOCK_ACCT_READ];
> > @@ -428,11 +432,6 @@ static void bdrv_query_blk_stats(BlockStats *s, BlockBackend *blk)
> >  static void bdrv_query_bds_stats(BlockStats *s, const BlockDriverState *bs,
> >                                   bool query_backing)
> >  {
> > -    if (bdrv_get_device_name(bs)[0]) {
> > -        s->has_device = true;
> > -        s->device = g_strdup(bdrv_get_device_name(bs));
> > -    }
> > -
> >      if (bdrv_get_node_name(bs)[0]) {
> >          s->has_node_name = true;
> >          s->node_name = g_strdup(bdrv_get_node_name(bs));
> > @@ -442,17 +441,18 @@ static void bdrv_query_bds_stats(BlockStats *s, const BlockDriverState *bs,
> >  
> >      if (bs->file) {
> >          s->has_parent = true;
> > -        s->parent = bdrv_query_stats(bs->file->bs, query_backing);
> > +        s->parent = bdrv_query_stats(NULL, bs->file->bs, query_backing);
> >      }
> >  
> >      if (query_backing && bs->backing) {
> >          s->has_backing = true;
> > -        s->backing = bdrv_query_stats(bs->backing->bs, query_backing);
> > +        s->backing = bdrv_query_stats(NULL, bs->backing->bs, query_backing);
> 
> This breaks my very important use case of having BBs on backing BDSs!!!
> 
> Kidding aside: Maybe someone actually wants to do this? Should we feel
> bad for breaking query-blockstats for those BBs?

This is only possible since commit d9b7b057 ('block: Allow references
for backing files'), which wasn't in 2.5 yet.

Apart from that, you still get the information because that second BB is
enumerated as well. It's just not duplicated in the return value of a
different BB any more. I think we can consider that a bug fix.

Kevin

> If no:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 




[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]