| Message ID | 1442589793-7105-15-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/18/2015 09:22 AM, Max Reitz wrote: > BlockAcctStats contains statistics about the data transferred from and > to the device; wr_highest_sector 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_sector 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_sector in > the BDS. > > Finally, wr_highest_sector is renamed to wr_highest_offset and given the > appropriate meaning. Externally, it is represented as an offset so there > is no point in doing something different internally. Its definition is > changed to match that in qapi/block-core.json which is "the offset after > the greatest byte written to". Doing so should not cause any harm since > if external programs tried to calculate the volume usage by > (wr_highest_offset + 512) / volume_size, after this patch they will just > assume the volume to be full slightly earlier than before. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Alberto Garcia <berto@igalia.com> Still holds, but... > +++ b/qmp-commands.hx > @@ -2481,8 +2481,8 @@ Each json-object contain the following: > - "wr_total_time_ns": total time spend on writes in nano-seconds (json-int) > - "rd_total_time_ns": total time spend on reads in nano-seconds (json-int) > - "flush_total_time_ns": total time spend on cache flushes in nano-seconds (json-int) > - - "wr_highest_offset": Highest offset of a sector written since the > - BlockDriverState has been opened (json-int) > + - "wr_highest_offset": The offset after the greatest byte written to the > + BlockDriverState since it has been opened (json-int) ...someday, I'd really like to have this stat show as non-zero even when first opening the device (before writing to it). Right now, you have no clue how full a backing device is prior to starting a block-commit; you have to start writing to it to get a feel for its current usage.
Am 18.09.2015 um 18:59 hat Eric Blake geschrieben: > On 09/18/2015 09:22 AM, Max Reitz wrote: > > BlockAcctStats contains statistics about the data transferred from and > > to the device; wr_highest_sector 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_sector 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_sector in > > the BDS. > > > > Finally, wr_highest_sector is renamed to wr_highest_offset and given the > > appropriate meaning. Externally, it is represented as an offset so there > > is no point in doing something different internally. Its definition is > > changed to match that in qapi/block-core.json which is "the offset after > > the greatest byte written to". Doing so should not cause any harm since > > if external programs tried to calculate the volume usage by > > (wr_highest_offset + 512) / volume_size, after this patch they will just > > assume the volume to be full slightly earlier than before. > > > > Signed-off-by: Max Reitz <mreitz@redhat.com> > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Reviewed-by: Alberto Garcia <berto@igalia.com> > > Still holds, but... > > > +++ b/qmp-commands.hx > > @@ -2481,8 +2481,8 @@ Each json-object contain the following: > > - "wr_total_time_ns": total time spend on writes in nano-seconds (json-int) > > - "rd_total_time_ns": total time spend on reads in nano-seconds (json-int) > > - "flush_total_time_ns": total time spend on cache flushes in nano-seconds (json-int) > > - - "wr_highest_offset": Highest offset of a sector written since the > > - BlockDriverState has been opened (json-int) > > + - "wr_highest_offset": The offset after the greatest byte written to the > > + BlockDriverState since it has been opened (json-int) > > ...someday, I'd really like to have this stat show as non-zero even when > first opening the device (before writing to it). Right now, you have no > clue how full a backing device is prior to starting a block-commit; you > have to start writing to it to get a feel for its current usage. With which value would it start? You don't want the file/device size because for block devices that's more than is actually used yet. What you really want to know is a number for the parent node, which isn't readily available for qcow2 (you would have to scan the refcounts in the last refcount block on startup) and doesn't really exist for most other formats (very few of them can be used on block devices, most rely on the file size). Kevin
On 09/21/2015 01:57 AM, Kevin Wolf wrote: >>> + - "wr_highest_offset": The offset after the greatest byte written to the >>> + BlockDriverState since it has been opened (json-int) >> >> ...someday, I'd really like to have this stat show as non-zero even when >> first opening the device (before writing to it). Right now, you have no >> clue how full a backing device is prior to starting a block-commit; you >> have to start writing to it to get a feel for its current usage. > > With which value would it start? You don't want the file/device size > because for block devices that's more than is actually used yet. > > What you really want to know is a number for the parent node, which > isn't readily available for qcow2 (you would have to scan the refcounts > in the last refcount block on startup) and doesn't really exist for most > other formats (very few of them can be used on block devices, most rely > on the file size). Libvirt only uses the stat on qcow2 format atop block devices, and uses it precisely because that is the one place where relying on device size is wrong. And yes, it really WOULD be nice for the value to read the last refcount block on startup, as that IS the correct value.
diff --git a/block/accounting.c b/block/accounting.c index 01d594f..a423560 100644 --- a/block/accounting.c +++ b/block/accounting.c @@ -47,14 +47,6 @@ void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie) } -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; - } -} - void block_acct_merge_done(BlockAcctStats *stats, enum BlockAcctType type, int num_requests) { diff --git a/block/io.c b/block/io.c index d4bc83b..21cc82a 100644 --- a/block/io.c +++ b/block/io.c @@ -1141,7 +1141,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_offset < offset + bytes) { + bs->wr_highest_offset = offset + bytes; + } if (ret >= 0) { bs->total_sectors = MAX(bs->total_sectors, sector_num + nb_sectors); diff --git a/block/qapi.c b/block/qapi.c index 2ce5097..d3cbc80 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -350,13 +350,13 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs, 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->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_offset; + 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 4c406cf..66637cd 100644 --- a/include/block/accounting.h +++ b/include/block/accounting.h @@ -40,7 +40,6 @@ typedef struct BlockAcctStats { uint64_t nr_ops[BLOCK_MAX_IOTYPE]; uint64_t total_time_ns[BLOCK_MAX_IOTYPE]; uint64_t merged[BLOCK_MAX_IOTYPE]; - uint64_t wr_highest_sector; } BlockAcctStats; typedef struct BlockAcctCookie { @@ -52,8 +51,6 @@ 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); void block_acct_merge_done(BlockAcctStats *stats, enum BlockAcctType type, int num_requests); diff --git a/include/block/block_int.h b/include/block/block_int.h index b7e1e16..67e05ac 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -403,6 +403,9 @@ struct BlockDriverState { /* I/O stats (display with "info blockstats"). */ BlockAcctStats stats; + /* Offset after the highest byte written to */ + uint64_t wr_highest_offset; + /* I/O Limits */ BlockLimits bl; diff --git a/qmp-commands.hx b/qmp-commands.hx index 9848fd8..8343289 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -2481,8 +2481,8 @@ Each json-object contain the following: - "wr_total_time_ns": total time spend on writes in nano-seconds (json-int) - "rd_total_time_ns": total time spend on reads in nano-seconds (json-int) - "flush_total_time_ns": total time spend on cache flushes in nano-seconds (json-int) - - "wr_highest_offset": Highest offset of a sector written since the - BlockDriverState has been opened (json-int) + - "wr_highest_offset": The offset after the greatest byte written to the + BlockDriverState since it has been opened (json-int) - "rd_merged": number of read requests that have been merged into another request (json-int) - "wr_merged": number of write requests that have been merged into