diff mbox

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

Message ID 1456518142-9849-4-git-send-email-kwolf@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kevin Wolf Feb. 26, 2016, 8:22 p.m. UTC
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(-)

Comments

Max Reitz March 2, 2016, 5:10 p.m. UTC | #1
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>
Kevin Wolf March 2, 2016, 5:27 p.m. UTC | #2
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>
>
diff mbox

Patch

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;