| Message ID | 1433360659-1915-15-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/03/2015 01:43 PM, Max Reitz wrote: > As the comment above bdrv_get_stats() says, BlockAcctStats is something > which belongs to the device instead of each BlockDriverState. This patch > therefore moves it into the BlockBackend. Again, Berto may want to eventually report stats for both BDS and BB, but that can come later. For now, this is reasonable refactoring. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 11 ----------- > block/block-backend.c | 5 ++++- > block/io.c | 6 +++++- > block/qapi.c | 24 ++++++++++++++---------- > include/block/block.h | 2 -- > include/block/block_int.h | 3 --- > 6 files changed, 23 insertions(+), 28 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com>
On Wed 03 Jun 2015 10:52:34 PM CEST, Eric Blake wrote: >> As the comment above bdrv_get_stats() says, BlockAcctStats is >> something which belongs to the device instead of each >> BlockDriverState. This patch therefore moves it into the >> BlockBackend. > > Again, Berto may want to eventually report stats for both BDS and BB, > but that can come later. For now, this is reasonable refactoring. Yeah, this change makes sense and I was actually planning to do something similar in my patch series. The only problem that I see with this is that the data is stored in the right place but the API is (still) wrong. query-blockstats queries BDSs, but the information we'll get in return it's either the stats from the BlockBackend (for root nodes) or all zeroes (for the rest), not the stats from the BDSs themselves. Ideally we would need one way to query information from the BlockBackend (that we already have) and another way to query information from the BDS (that we don't have yet). But I guess we have to look for a way to do this without breaking the API compatibility. And for the record, my priorities at the moment are the stats from the BlockBackend, not the BDS. Berto
On Wed 03 Jun 2015 09:43:55 PM CEST, Max Reitz wrote: > As the comment above bdrv_get_stats() says, BlockAcctStats is something > which belongs to the device instead of each BlockDriverState. This patch > therefore moves it into the BlockBackend. > > Signed-off-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
On 06/05/2015 04:47 AM, Alberto Garcia wrote: > On Wed 03 Jun 2015 10:52:34 PM CEST, Eric Blake wrote: > >>> As the comment above bdrv_get_stats() says, BlockAcctStats is >>> something which belongs to the device instead of each >>> BlockDriverState. This patch therefore moves it into the >>> BlockBackend. >> >> Again, Berto may want to eventually report stats for both BDS and BB, >> but that can come later. For now, this is reasonable refactoring. > > Yeah, this change makes sense and I was actually planning to do > something similar in my patch series. > > The only problem that I see with this is that the data is stored in the > right place but the API is (still) wrong. query-blockstats queries BDSs, > but the information we'll get in return it's either the stats from the > BlockBackend (for root nodes) or all zeroes (for the rest), not the > stats from the BDSs themselves. > > Ideally we would need one way to query information from the BlockBackend > (that we already have) and another way to query information from the BDS > (that we don't have yet). But I guess we have to look for a way to do > this without breaking the API compatibility. > > And for the record, my priorities at the moment are the stats from the > BlockBackend, not the BDS. We already have: query-block - reports stats on BB query-blockstats - reports stats on BDS I don't know if it makes more sense to have a single shared struct that both calls can report, for the things that make sense in both places, but I also know that in my current efforts to add write-threshold support to libvirt, there were several stats that I wish were available from the opposite command of where it is currently found. Back-compat says it is okay to duplicate output in multiple locations, but not to completely move it from one to the other, although the effort of duplication may be a maintenance nightmare.
diff --git a/block.c b/block.c index dbfd4c7..c06eb38 100644 --- a/block.c +++ b/block.c @@ -4009,14 +4009,3 @@ void bdrv_refresh_filename(BlockDriverState *bs) QDECREF(json); } } - -/* This accessor function purpose is to allow the device models to access the - * BlockAcctStats structure embedded inside a BlockDriverState without being - * aware of the BlockDriverState structure layout. - * It will go away when the BlockAcctStats structure will be moved inside - * the device models. - */ -BlockAcctStats *bdrv_get_stats(BlockDriverState *bs) -{ - return &bs->stats; -} diff --git a/block/block-backend.c b/block/block-backend.c index 163d329..d036070 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -34,6 +34,9 @@ struct BlockBackend { /* the block size for which the guest device expects atomicity */ int guest_block_size; + + /* I/O stats (display with "info blockstats"). */ + BlockAcctStats stats; }; typedef struct BlockBackendAIOCB { @@ -870,7 +873,7 @@ void blk_io_unplug(BlockBackend *blk) BlockAcctStats *blk_get_stats(BlockBackend *blk) { - return bdrv_get_stats(blk->bs); + return &blk->stats; } void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk, diff --git a/block/io.c b/block/io.c index 110c5d1..3461ed0 100644 --- a/block/io.c +++ b/block/io.c @@ -24,6 +24,7 @@ #include "trace.h" #include "sysemu/qtest.h" +#include "sysemu/block-backend.h" #include "block/blockjob.h" #include "block/block_int.h" @@ -1890,7 +1891,10 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs, } } - block_acct_merge_done(&bs->stats, BLOCK_ACCT_WRITE, num_reqs - outidx - 1); + if (bs->blk) { + block_acct_merge_done(blk_get_stats(bs->blk), BLOCK_ACCT_WRITE, + num_reqs - outidx - 1); + } return outidx + 1; } diff --git a/block/qapi.c b/block/qapi.c index aed5ab3..2850f82 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -338,16 +338,20 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs, } s->stats = g_malloc0(sizeof(*s->stats)); - s->stats->rd_bytes = bs->stats.nr_bytes[BLOCK_ACCT_READ]; - s->stats->wr_bytes = bs->stats.nr_bytes[BLOCK_ACCT_WRITE]; - s->stats->rd_operations = bs->stats.nr_ops[BLOCK_ACCT_READ]; - s->stats->wr_operations = bs->stats.nr_ops[BLOCK_ACCT_WRITE]; - s->stats->rd_merged = bs->stats.merged[BLOCK_ACCT_READ]; - s->stats->wr_merged = bs->stats.merged[BLOCK_ACCT_WRITE]; - s->stats->flush_operations = bs->stats.nr_ops[BLOCK_ACCT_FLUSH]; - s->stats->wr_total_time_ns = bs->stats.total_time_ns[BLOCK_ACCT_WRITE]; - s->stats->rd_total_time_ns = bs->stats.total_time_ns[BLOCK_ACCT_READ]; - s->stats->flush_total_time_ns = bs->stats.total_time_ns[BLOCK_ACCT_FLUSH]; + if (bs->blk) { + BlockAcctStats *stats = blk_get_stats(bs->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]; + s->stats->wr_operations = stats->nr_ops[BLOCK_ACCT_WRITE]; + 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->wr_highest_offset = bs->wr_highest_offset; diff --git a/include/block/block.h b/include/block/block.h index e6a8610..248e00c 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -591,6 +591,4 @@ void bdrv_io_plug(BlockDriverState *bs); void bdrv_io_unplug(BlockDriverState *bs); void bdrv_flush_io_queue(BlockDriverState *bs); -BlockAcctStats *bdrv_get_stats(BlockDriverState *bs); - #endif diff --git a/include/block/block_int.h b/include/block/block_int.h index ca3dad6..2820d04 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -383,9 +383,6 @@ struct BlockDriverState { CoQueue throttled_reqs[2]; bool io_limits_enabled; - /* I/O stats (display with "info blockstats"). */ - BlockAcctStats stats; - /* Offset after the highest byte written to */ uint64_t wr_highest_offset;
As the comment above bdrv_get_stats() says, BlockAcctStats is something which belongs to the device instead of each BlockDriverState. This patch therefore moves it into the BlockBackend. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 11 ----------- block/block-backend.c | 5 ++++- block/io.c | 6 +++++- block/qapi.c | 24 ++++++++++++++---------- include/block/block.h | 2 -- include/block/block_int.h | 3 --- 6 files changed, 23 insertions(+), 28 deletions(-)