| Message ID | 1422288204-29271-11-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/26/2015 09:02 AM, Max Reitz wrote: > BlockAcctStats contains statistics about the data transferred from and > to the device; wr_highest_offset does not fit in with the rest. > > Furthermore, those statistics are supposed to be specific for a certain > device and not necessarily for a BDS (see the comment above > bdrv_get_stats()); on the other hand, wr_highest_offset may be a rather > important information to know for each BDS. When BlockAcctStats is > finally removed from the BDS, we will want to keep wr_highest_offset in > the BDS. Yes, I recently did work in libvirt to expose wr_highest_offset of backing images during block commit (qemu still isn't populating it on images opened only for read, but the point remains that it is a statistic tied to the BDS, not the BB). On the other hand, even the other statistics might make sense on both BDS and BB level (at the BB level, how many bytes has the guest read/written; at the BDS level, how many bytes were serviced by the active layer vs. delegated to a backing layer). I'm not sure if we are set up for that fine of a level of reporting yet, but we shouldn't make it hard to implement later. But for now, I agree with separating the definite BDS-only stat, leaving the rest of the struct usable for either BDS or BB. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 4 +++- > block/accounting.c | 9 --------- > block/qapi.c | 4 ++-- > include/block/accounting.h | 3 --- > include/block/block_int.h | 3 +++ > 5 files changed, 8 insertions(+), 15 deletions(-) > > diff --git a/block.c b/block.c > +++ b/include/block/block_int.h > @@ -366,6 +366,9 @@ struct BlockDriverState { > /* I/O stats (display with "info blockstats"). */ > BlockAcctStats stats; > > + /* Highest sector index written to */ > + uint64_t wr_highest_sector; Umm, now would be a great time to track this in bytes instead of sectors, if that is not too difficult to do.
On 2015-01-27 at 15:01, Eric Blake wrote: > On 01/26/2015 09:02 AM, Max Reitz wrote: >> BlockAcctStats contains statistics about the data transferred from and >> to the device; wr_highest_offset does not fit in with the rest. >> >> Furthermore, those statistics are supposed to be specific for a certain >> device and not necessarily for a BDS (see the comment above >> bdrv_get_stats()); on the other hand, wr_highest_offset may be a rather >> important information to know for each BDS. When BlockAcctStats is >> finally removed from the BDS, we will want to keep wr_highest_offset in >> the BDS. > Yes, I recently did work in libvirt to expose wr_highest_offset of > backing images during block commit (qemu still isn't populating it on > images opened only for read, but the point remains that it is a > statistic tied to the BDS, not the BB). > > On the other hand, even the other statistics might make sense on both > BDS and BB level (at the BB level, how many bytes has the guest > read/written; at the BDS level, how many bytes were serviced by the > active layer vs. delegated to a backing layer). > > I'm not sure if we are set up for that fine of a level of reporting yet, > but we shouldn't make it hard to implement later. But for now, I agree > with separating the definite BDS-only stat, leaving the rest of the > struct usable for either BDS or BB. > >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block.c | 4 +++- >> block/accounting.c | 9 --------- >> block/qapi.c | 4 ++-- >> include/block/accounting.h | 3 --- >> include/block/block_int.h | 3 +++ >> 5 files changed, 8 insertions(+), 15 deletions(-) >> >> diff --git a/block.c b/block.c >> +++ b/include/block/block_int.h >> @@ -366,6 +366,9 @@ struct BlockDriverState { >> /* I/O stats (display with "info blockstats"). */ >> BlockAcctStats stats; >> >> + /* Highest sector index written to */ >> + uint64_t wr_highest_sector; > Umm, now would be a great time to track this in bytes instead of > sectors, if that is not too difficult to do. Fine with me, will do. (on a different note, please refer to the "RESEND" thread or replace john@redhat.com by jsnow@redhat.com in the CC list (my bad)) Max
diff --git a/block.c b/block.c index eff92ca..5db71c6 100644 --- a/block.c +++ b/block.c @@ -3312,7 +3312,9 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs, bdrv_set_dirty(bs, sector_num, nb_sectors); - block_acct_highest_sector(&bs->stats, sector_num, nb_sectors); + if (bs->wr_highest_sector < sector_num + nb_sectors - 1) { + bs->wr_highest_sector = sector_num + nb_sectors - 1; + } if (ret >= 0) { bs->total_sectors = MAX(bs->total_sectors, sector_num + nb_sectors); diff --git a/block/accounting.c b/block/accounting.c index 18102f0..c77b6c2 100644 --- a/block/accounting.c +++ b/block/accounting.c @@ -45,12 +45,3 @@ void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie) stats->total_time_ns[cookie->type] += qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - cookie->start_time_ns; } - - -void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num, - unsigned int nb_sectors) -{ - if (stats->wr_highest_sector < sector_num + nb_sectors - 1) { - stats->wr_highest_sector = sector_num + nb_sectors - 1; - } -} diff --git a/block/qapi.c b/block/qapi.c index 8c3b9d9..4e97574 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -338,13 +338,13 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs, 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->wr_highest_offset = - bs->stats.wr_highest_sector * BDRV_SECTOR_SIZE; 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]; + s->stats->wr_highest_offset = bs->wr_highest_sector * BDRV_SECTOR_SIZE; + if (bs->file) { s->has_parent = true; s->parent = bdrv_query_stats(bs->file, query_backing); diff --git a/include/block/accounting.h b/include/block/accounting.h index 50b42b3..9089b67 100644 --- a/include/block/accounting.h +++ b/include/block/accounting.h @@ -39,7 +39,6 @@ typedef struct BlockAcctStats { uint64_t nr_bytes[BLOCK_MAX_IOTYPE]; uint64_t nr_ops[BLOCK_MAX_IOTYPE]; uint64_t total_time_ns[BLOCK_MAX_IOTYPE]; - uint64_t wr_highest_sector; } BlockAcctStats; typedef struct BlockAcctCookie { @@ -51,7 +50,5 @@ typedef struct BlockAcctCookie { void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie, int64_t bytes, enum BlockAcctType type); void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie); -void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num, - unsigned int nb_sectors); #endif diff --git a/include/block/block_int.h b/include/block/block_int.h index c6ab73a..e309d8a 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -366,6 +366,9 @@ struct BlockDriverState { /* I/O stats (display with "info blockstats"). */ BlockAcctStats stats; + /* Highest sector index written to */ + uint64_t wr_highest_sector; + /* I/O Limits */ BlockLimits bl;
BlockAcctStats contains statistics about the data transferred from and to the device; wr_highest_offset does not fit in with the rest. Furthermore, those statistics are supposed to be specific for a certain device and not necessarily for a BDS (see the comment above bdrv_get_stats()); on the other hand, wr_highest_offset may be a rather important information to know for each BDS. When BlockAcctStats is finally removed from the BDS, we will want to keep wr_highest_offset in the BDS. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 4 +++- block/accounting.c | 9 --------- block/qapi.c | 4 ++-- include/block/accounting.h | 3 --- include/block/block_int.h | 3 +++ 5 files changed, 8 insertions(+), 15 deletions(-)