| Message ID | 1511196664-85304-2-git-send-email-anton.nefedov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon 20 Nov 2017 05:50:58 PM CET, Anton Nefedov wrote: > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > qapi/block-core.json | 20 ++++++++++++++++++++ > include/block/accounting.h | 1 + > block/qapi.c | 6 ++++++ > 3 files changed, 27 insertions(+) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 76bf50f..ba2705d 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -730,6 +730,23 @@ > # @timed_stats: Statistics specific to the set of previously defined > # intervals of time (Since 2.5) > # > +# @unmap_bytes: The number of bytes unmapped by the device (Since 2.12) > +# > +# @unmap_operations: The number of unmap operations performed by the device > +# (Since 2.12) > +# > +# @unmap_total_time_ns: Total time spent on unmap operations in nano-seconds > +# (Since 2.12) > +# > +# @unmap_merged: Number of unmap requests that have been merged into another > +# request (Since 2.12) > +# > +# @failed_unmap_operations: The number of failed unmap operations performed > +# by the device (Since 2.12) > +# > +# @invalid_unmap_operations: The number of invalid unmap operations performed > +# by the device (Since 2.12) > +# > # Since: 0.14.0 Admittedtly this is just a style issue and I don't know what others think, but wouldn't it be nicer if the documentation of new fields is located together with the existing ones? E.g @rd_bytes @wr_bytes +@unmap_bytes @rd_operations @wr_operations @flush_operations +@unmap_operations [...] @failed_rd_operations @failed_wr_operations @failed_flush_operations +@failed_unmap_operations etc. > ds->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ]; > ds->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE]; > + ds->unmap_bytes = stats->nr_bytes[BLOCK_ACCT_UNMAP]; > ds->rd_operations = stats->nr_ops[BLOCK_ACCT_READ]; > ds->wr_operations = stats->nr_ops[BLOCK_ACCT_WRITE]; > + ds->unmap_operations = stats->nr_ops[BLOCK_ACCT_UNMAP]; Essentially like this :) The patch is fine with me either way. Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
On 12/05/2017 09:09 AM, Alberto Garcia wrote: > On Mon 20 Nov 2017 05:50:58 PM CET, Anton Nefedov wrote: >> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> qapi/block-core.json | 20 ++++++++++++++++++++ >> include/block/accounting.h | 1 + >> block/qapi.c | 6 ++++++ >> 3 files changed, 27 insertions(+) >> > > Admittedtly this is just a style issue and I don't know what others > think, but wouldn't it be nicer if the documentation of new fields is > located together with the existing ones? > > E.g > > @rd_bytes > @wr_bytes > +@unmap_bytes > @rd_operations > @wr_operations > @flush_operations > +@unmap_operations > Grouping is fine, and your suggestion makes sense to me. One of the nice things of QMP is that since it is name/value based, you don't have to worry about inserting a name in the middle if that makes more sense logically (it doesn't break backwards compatibility with old QMP clients, the way that inserting something into the middle of a C struct breaks all callers that were using the old struct layout).
On 5/12/2017 6:19 PM, Eric Blake wrote: > On 12/05/2017 09:09 AM, Alberto Garcia wrote: >> On Mon 20 Nov 2017 05:50:58 PM CET, Anton Nefedov wrote: >>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> >>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> qapi/block-core.json | 20 ++++++++++++++++++++ >>> include/block/accounting.h | 1 + >>> block/qapi.c | 6 ++++++ >>> 3 files changed, 27 insertions(+) >>> > >> >> Admittedtly this is just a style issue and I don't know what others >> think, but wouldn't it be nicer if the documentation of new fields is >> located together with the existing ones? >> >> E.g >> >> @rd_bytes >> @wr_bytes >> +@unmap_bytes >> @rd_operations >> @wr_operations >> @flush_operations >> +@unmap_operations >> > > Grouping is fine, and your suggestion makes sense to me. One of the > nice things of QMP is that since it is name/value based, you don't have > to worry about inserting a name in the middle if that makes more sense > logically (it doesn't break backwards compatibility with old QMP > clients, the way that inserting something into the middle of a C struct > breaks all callers that were using the old struct layout). > Thank you, agree, will fix /Anton
diff --git a/qapi/block-core.json b/qapi/block-core.json index 76bf50f..ba2705d 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -730,6 +730,23 @@ # @timed_stats: Statistics specific to the set of previously defined # intervals of time (Since 2.5) # +# @unmap_bytes: The number of bytes unmapped by the device (Since 2.12) +# +# @unmap_operations: The number of unmap operations performed by the device +# (Since 2.12) +# +# @unmap_total_time_ns: Total time spent on unmap operations in nano-seconds +# (Since 2.12) +# +# @unmap_merged: Number of unmap requests that have been merged into another +# request (Since 2.12) +# +# @failed_unmap_operations: The number of failed unmap operations performed +# by the device (Since 2.12) +# +# @invalid_unmap_operations: The number of invalid unmap operations performed +# by the device (Since 2.12) +# # Since: 0.14.0 ## { 'struct': 'BlockDeviceStats', @@ -741,6 +758,9 @@ 'failed_rd_operations': 'int', 'failed_wr_operations': 'int', 'failed_flush_operations': 'int', 'invalid_rd_operations': 'int', 'invalid_wr_operations': 'int', 'invalid_flush_operations': 'int', + 'unmap_bytes' : 'int', 'unmap_operations': 'int', + 'unmap_total_time_ns': 'int', 'unmap_merged': 'int', + 'failed_unmap_operations': 'int', 'invalid_unmap_operations': 'int', 'account_invalid': 'bool', 'account_failed': 'bool', 'timed_stats': ['BlockDeviceTimedStats'] } } diff --git a/include/block/accounting.h b/include/block/accounting.h index b833d26..4e53c4a 100644 --- a/include/block/accounting.h +++ b/include/block/accounting.h @@ -35,6 +35,7 @@ enum BlockAcctType { BLOCK_ACCT_READ, BLOCK_ACCT_WRITE, BLOCK_ACCT_FLUSH, + BLOCK_ACCT_UNMAP, BLOCK_MAX_IOTYPE, }; diff --git a/block/qapi.c b/block/qapi.c index fc10f0a..6e110f2 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -396,24 +396,30 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk) ds->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ]; ds->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE]; + ds->unmap_bytes = stats->nr_bytes[BLOCK_ACCT_UNMAP]; ds->rd_operations = stats->nr_ops[BLOCK_ACCT_READ]; ds->wr_operations = stats->nr_ops[BLOCK_ACCT_WRITE]; + ds->unmap_operations = stats->nr_ops[BLOCK_ACCT_UNMAP]; ds->failed_rd_operations = stats->failed_ops[BLOCK_ACCT_READ]; ds->failed_wr_operations = stats->failed_ops[BLOCK_ACCT_WRITE]; ds->failed_flush_operations = stats->failed_ops[BLOCK_ACCT_FLUSH]; + ds->failed_unmap_operations = stats->failed_ops[BLOCK_ACCT_UNMAP]; ds->invalid_rd_operations = stats->invalid_ops[BLOCK_ACCT_READ]; ds->invalid_wr_operations = stats->invalid_ops[BLOCK_ACCT_WRITE]; ds->invalid_flush_operations = stats->invalid_ops[BLOCK_ACCT_FLUSH]; + ds->invalid_unmap_operations = stats->invalid_ops[BLOCK_ACCT_UNMAP]; ds->rd_merged = stats->merged[BLOCK_ACCT_READ]; ds->wr_merged = stats->merged[BLOCK_ACCT_WRITE]; + ds->unmap_merged = stats->merged[BLOCK_ACCT_UNMAP]; ds->flush_operations = stats->nr_ops[BLOCK_ACCT_FLUSH]; ds->wr_total_time_ns = stats->total_time_ns[BLOCK_ACCT_WRITE]; ds->rd_total_time_ns = stats->total_time_ns[BLOCK_ACCT_READ]; ds->flush_total_time_ns = stats->total_time_ns[BLOCK_ACCT_FLUSH]; + ds->unmap_total_time_ns = stats->total_time_ns[BLOCK_ACCT_UNMAP]; ds->has_idle_time_ns = stats->last_access_time_ns > 0; if (ds->has_idle_time_ns) {