| Message ID | 1456518142-9849-4-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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 --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;
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(-)