A block driver can provide a callback to report driver-specific
statistics.
file-posix driver now reports discard statistics
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
qapi/block-core.json | 38 ++++++++++++++++++++++++++++++++++++++
include/block/block.h | 1 +
include/block/block_int.h | 1 +
block.c | 9 +++++++++
block/file-posix.c | 32 ++++++++++++++++++++++++++++++++
block/qapi.c | 5 +++++
6 files changed, 86 insertions(+)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 959358ccc4..b100e852c7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -877,6 +877,41 @@
'*x_wr_latency_histogram': 'BlockLatencyHistogramInfo',
'*x_flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
+##
+# @BlockStatsSpecificFile:
+#
+# File driver statistics
+#
+# @discard-nb-ok: The number of succeeded discard operations performed by
+# the driver.
+#
+# @discard-nb-failed: The number of failed discard operations performed by
+# the driver.
+#
+# @discard-bytes-ok: The number of bytes discarded by the driver.
+#
+# Since: 4.0
+##
+{ 'struct': 'BlockStatsSpecificFile',
+ 'data': {
+ 'discard-nb-ok': 'int',
+ 'discard-nb-failed': 'int',
+ 'discard-bytes-ok': 'int' } }
+
+##
+# @BlockStatsSpecific:
+#
+# Block driver specific statistics
+#
+# Since: 4.0
+##
+{ 'union': 'BlockStatsSpecific',
+ 'base': { 'driver': 'BlockdevDriver' },
+ 'discriminator': 'driver',
+ 'data': {
+ 'file': 'BlockStatsSpecificFile',
+ 'host_device': 'BlockStatsSpecificFile' } }
+
##
# @BlockStats:
#
@@ -892,6 +927,8 @@
#
# @stats: A @BlockDeviceStats for the device.
#
+# @driver-specific: Optional driver-specific stats. (Since 4.0)
+#
# @parent: This describes the file block device if it has one.
# Contains recursively the statistics of the underlying
# protocol (e.g. the host file for a qcow2 image). If there is
@@ -905,6 +942,7 @@
{ 'struct': 'BlockStats',
'data': {'*device': 'str', '*qdev': 'str', '*node-name': 'str',
'stats': 'BlockDeviceStats',
+ '*driver-specific': 'BlockStatsSpecific',
'*parent': 'BlockStats',
'*backing': 'BlockStats'} }
diff --git a/include/block/block.h b/include/block/block.h
index 7f5453b45b..d6ee3e99c9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -478,6 +478,7 @@ const char *bdrv_get_device_or_node_name(const BlockDriverState *bs);
int bdrv_get_flags(BlockDriverState *bs);
int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
+BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs);
void bdrv_round_to_clusters(BlockDriverState *bs,
int64_t offset, int64_t bytes,
int64_t *cluster_offset,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f605622216..236d4aceef 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -320,6 +320,7 @@ struct BlockDriver {
Error **errp);
int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
+ BlockStatsSpecific *(*bdrv_get_specific_stats)(BlockDriverState *bs);
int coroutine_fn (*bdrv_save_vmstate)(BlockDriverState *bs,
QEMUIOVector *qiov,
diff --git a/block.c b/block.c
index 811239ca23..5540f4f187 100644
--- a/block.c
+++ b/block.c
@@ -4327,6 +4327,15 @@ ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs)
return NULL;
}
+BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs)
+{
+ BlockDriver *drv = bs->drv;
+ if (!drv || !drv->bdrv_get_specific_stats) {
+ return NULL;
+ }
+ return drv->bdrv_get_specific_stats(bs);
+}
+
void bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent event)
{
if (!bs || !bs->drv || !bs->drv->bdrv_debug_event) {
diff --git a/block/file-posix.c b/block/file-posix.c
index 1589dbf3ab..9c686961bc 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2660,6 +2660,36 @@ static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
return 0;
}
+static BlockStatsSpecificFile get_blockstats_specific_file(BlockDriverState *bs)
+{
+ BDRVRawState *s = bs->opaque;
+ return (BlockStatsSpecificFile) {
+ .discard_nb_ok = s->stats.discard_nb_ok,
+ .discard_nb_failed = s->stats.discard_nb_failed,
+ .discard_bytes_ok = s->stats.discard_bytes_ok,
+ };
+}
+
+static BlockStatsSpecific *raw_get_specific_stats(BlockDriverState *bs)
+{
+ BlockStatsSpecific *stats = g_new(BlockStatsSpecific, 1);
+
+ stats->driver = BLOCKDEV_DRIVER_FILE;
+ stats->u.file = get_blockstats_specific_file(bs);
+
+ return stats;
+}
+
+static BlockStatsSpecific *hdev_get_specific_stats(BlockDriverState *bs)
+{
+ BlockStatsSpecific *stats = g_new(BlockStatsSpecific, 1);
+
+ stats->driver = BLOCKDEV_DRIVER_HOST_DEVICE;
+ stats->u.host_device = get_blockstats_specific_file(bs);
+
+ return stats;
+}
+
static QemuOptsList raw_create_opts = {
.name = "raw-create-opts",
.head = QTAILQ_HEAD_INITIALIZER(raw_create_opts.head),
@@ -2771,6 +2801,7 @@ BlockDriver bdrv_file = {
.bdrv_get_info = raw_get_info,
.bdrv_get_allocated_file_size
= raw_get_allocated_file_size,
+ .bdrv_get_specific_stats = raw_get_specific_stats,
.bdrv_check_perm = raw_check_perm,
.bdrv_set_perm = raw_set_perm,
.bdrv_abort_perm_update = raw_abort_perm_update,
@@ -3255,6 +3286,7 @@ static BlockDriver bdrv_host_device = {
.bdrv_get_info = raw_get_info,
.bdrv_get_allocated_file_size
= raw_get_allocated_file_size,
+ .bdrv_get_specific_stats = hdev_get_specific_stats,
.bdrv_check_perm = raw_check_perm,
.bdrv_set_perm = raw_set_perm,
.bdrv_abort_perm_update = raw_abort_perm_update,
diff --git a/block/qapi.c b/block/qapi.c
index df31f351d2..74f762ea6c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -535,6 +535,11 @@ static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs,
s->stats->wr_highest_offset = stat64_get(&bs->wr_highest_offset);
+ s->driver_specific = bdrv_get_specific_stats(bs);
+ if (s->driver_specific) {
+ s->has_driver_specific = true;
+ }
+
if (bs->file) {
s->has_parent = true;
s->parent = bdrv_query_bds_stats(bs->file->bs, blk_level);
--
2.17.1
30.11.2018 17:47, Anton Nefedov wrote: > A block driver can provide a callback to report driver-specific > statistics. > > file-posix driver now reports discard statistics > > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir
I'm reviewing just the QAPI schema today.
Anton Nefedov <anton.nefedov@virtuozzo.com> writes:
> A block driver can provide a callback to report driver-specific
> statistics.
>
> file-posix driver now reports discard statistics
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
> qapi/block-core.json | 38 ++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 1 +
> include/block/block_int.h | 1 +
> block.c | 9 +++++++++
> block/file-posix.c | 32 ++++++++++++++++++++++++++++++++
> block/qapi.c | 5 +++++
> 6 files changed, 86 insertions(+)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 959358ccc4..b100e852c7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -877,6 +877,41 @@
> '*x_wr_latency_histogram': 'BlockLatencyHistogramInfo',
> '*x_flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
>
> +##
> +# @BlockStatsSpecificFile:
> +#
> +# File driver statistics
> +#
> +# @discard-nb-ok: The number of succeeded discard operations performed by
successful discard operations
> +# the driver.
> +#
> +# @discard-nb-failed: The number of failed discard operations performed by
> +# the driver.
> +#
> +# @discard-bytes-ok: The number of bytes discarded by the driver.
> +#
> +# Since: 4.0
> +##
> +{ 'struct': 'BlockStatsSpecificFile',
> + 'data': {
> + 'discard-nb-ok': 'int',
> + 'discard-nb-failed': 'int',
> + 'discard-bytes-ok': 'int' } }
Should these be unsigned?
For what it's worth, similar counters nearby are also 'int'.
> +
> +##
> +# @BlockStatsSpecific:
> +#
> +# Block driver specific statistics
> +#
> +# Since: 4.0
> +##
> +{ 'union': 'BlockStatsSpecific',
> + 'base': { 'driver': 'BlockdevDriver' },
> + 'discriminator': 'driver',
> + 'data': {
> + 'file': 'BlockStatsSpecificFile',
> + 'host_device': 'BlockStatsSpecificFile' } }
> +
> ##
> # @BlockStats:
> #
> @@ -892,6 +927,8 @@
> #
> # @stats: A @BlockDeviceStats for the device.
> #
> +# @driver-specific: Optional driver-specific stats. (Since 4.0)
> +#
> # @parent: This describes the file block device if it has one.
> # Contains recursively the statistics of the underlying
> # protocol (e.g. the host file for a qcow2 image). If there is
> @@ -905,6 +942,7 @@
> { 'struct': 'BlockStats',
> 'data': {'*device': 'str', '*qdev': 'str', '*node-name': 'str',
> 'stats': 'BlockDeviceStats',
> + '*driver-specific': 'BlockStatsSpecific',
> '*parent': 'BlockStats',
> '*backing': 'BlockStats'} }
>
Feels awkward.
When is @driver-specific present? Exactly when the driver is 'file' or
'host_device'? If that's correct, then turning BlockStats into a union
would be clearer and reduce parenthesises on the wire:
{ 'union': 'BlockStats',
'base': {
'driver': 'BlockdevDriver',
... all the other existing members of BlockStats ... }
'discriminator': 'driver',
'data': {
'file': 'BlockStatsSpecificFile',
'host_device': 'BlockStatsSpecificFile' } }
[...]
On 13/12/2018 3:20 PM, Markus Armbruster wrote:
> I'm reviewing just the QAPI schema today.
>
> Anton Nefedov <anton.nefedov@virtuozzo.com> writes:
>
>> A block driver can provide a callback to report driver-specific
>> statistics.
>>
>> file-posix driver now reports discard statistics
>>
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> ---
>> qapi/block-core.json | 38 ++++++++++++++++++++++++++++++++++++++
>> include/block/block.h | 1 +
>> include/block/block_int.h | 1 +
>> block.c | 9 +++++++++
>> block/file-posix.c | 32 ++++++++++++++++++++++++++++++++
>> block/qapi.c | 5 +++++
>> 6 files changed, 86 insertions(+)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 959358ccc4..b100e852c7 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -877,6 +877,41 @@
>> '*x_wr_latency_histogram': 'BlockLatencyHistogramInfo',
>> '*x_flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
>>
>> +##
>> +# @BlockStatsSpecificFile:
>> +#
>> +# File driver statistics
>> +#
>> +# @discard-nb-ok: The number of succeeded discard operations performed by
>
> successful discard operations
>
Fixed.
>> +# the driver.
>> +#
>> +# @discard-nb-failed: The number of failed discard operations performed by
>> +# the driver.
>> +#
>> +# @discard-bytes-ok: The number of bytes discarded by the driver.
>> +#
>> +# Since: 4.0
>> +##
>> +{ 'struct': 'BlockStatsSpecificFile',
>> + 'data': {
>> + 'discard-nb-ok': 'int',
>> + 'discard-nb-failed': 'int',
>> + 'discard-bytes-ok': 'int' } }
>
> Should these be unsigned?
>
> For what it's worth, similar counters nearby are also 'int'.
>
And I just added these symmetrically.
Probably shouldn't have - let these be uint64.
>> +
>> +##
>> +# @BlockStatsSpecific:
>> +#
>> +# Block driver specific statistics
>> +#
>> +# Since: 4.0
>> +##
>> +{ 'union': 'BlockStatsSpecific',
>> + 'base': { 'driver': 'BlockdevDriver' },
>> + 'discriminator': 'driver',
>> + 'data': {
>> + 'file': 'BlockStatsSpecificFile',
>> + 'host_device': 'BlockStatsSpecificFile' } }
>> +
>> ##
>> # @BlockStats:
>> #
>> @@ -892,6 +927,8 @@
>> #
>> # @stats: A @BlockDeviceStats for the device.
>> #
>> +# @driver-specific: Optional driver-specific stats. (Since 4.0)
>> +#
>> # @parent: This describes the file block device if it has one.
>> # Contains recursively the statistics of the underlying
>> # protocol (e.g. the host file for a qcow2 image). If there is
>> @@ -905,6 +942,7 @@
>> { 'struct': 'BlockStats',
>> 'data': {'*device': 'str', '*qdev': 'str', '*node-name': 'str',
>> 'stats': 'BlockDeviceStats',
>> + '*driver-specific': 'BlockStatsSpecific',
>> '*parent': 'BlockStats',
>> '*backing': 'BlockStats'} }
>>
>
> Feels awkward.
>
> When is @driver-specific present? Exactly when the driver is 'file' or
> 'host_device'? If that's correct, then turning BlockStats into a union
> would be clearer and reduce parenthesises on the wire:
>
> { 'union': 'BlockStats',
> 'base': {
> 'driver': 'BlockdevDriver',
> ... all the other existing members of BlockStats ... }
> 'discriminator': 'driver',
> 'data': {
> 'file': 'BlockStatsSpecificFile',
> 'host_device': 'BlockStatsSpecificFile' } }
>
> [...]
>
this series drags for quite a while - we already discussed this :)
In short: Blockdev does not always have driver, so it's either this
or adding weird BlockdevDriver values like "none".
http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg01845.html
Anton Nefedov <anton.nefedov@virtuozzo.com> writes:
> On 13/12/2018 3:20 PM, Markus Armbruster wrote:
>> I'm reviewing just the QAPI schema today.
>>
>> Anton Nefedov <anton.nefedov@virtuozzo.com> writes:
>>
>>> A block driver can provide a callback to report driver-specific
>>> statistics.
>>>
>>> file-posix driver now reports discard statistics
>>>
>>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
[...]
>>> @@ -892,6 +927,8 @@
>>> #
>>> # @stats: A @BlockDeviceStats for the device.
>>> #
>>> +# @driver-specific: Optional driver-specific stats. (Since 4.0)
>>> +#
>>> # @parent: This describes the file block device if it has one.
>>> # Contains recursively the statistics of the underlying
>>> # protocol (e.g. the host file for a qcow2 image). If there is
>>> @@ -905,6 +942,7 @@
>>> { 'struct': 'BlockStats',
>>> 'data': {'*device': 'str', '*qdev': 'str', '*node-name': 'str',
>>> 'stats': 'BlockDeviceStats',
>>> + '*driver-specific': 'BlockStatsSpecific',
>>> '*parent': 'BlockStats',
>>> '*backing': 'BlockStats'} }
>>>
>>
>> Feels awkward.
>>
>> When is @driver-specific present? Exactly when the driver is 'file' or
>> 'host_device'? If that's correct, then turning BlockStats into a union
>> would be clearer and reduce parenthesises on the wire:
>>
>> { 'union': 'BlockStats',
>> 'base': {
>> 'driver': 'BlockdevDriver',
>> ... all the other existing members of BlockStats ... }
>> 'discriminator': 'driver',
>> 'data': {
>> 'file': 'BlockStatsSpecificFile',
>> 'host_device': 'BlockStatsSpecificFile' } }
>>
>> [...]
>>
>
> this series drags for quite a while - we already discussed this :)
> In short: Blockdev does not always have driver, so it's either this
> or adding weird BlockdevDriver values like "none".
>
> http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg01845.html
You're right.
Anton Nefedov <anton.nefedov@virtuozzo.com> writes:
> On 13/12/2018 3:20 PM, Markus Armbruster wrote:
>> I'm reviewing just the QAPI schema today.
>>
>> Anton Nefedov <anton.nefedov@virtuozzo.com> writes:
>>
>>> A block driver can provide a callback to report driver-specific
>>> statistics.
>>>
>>> file-posix driver now reports discard statistics
>>>
>>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>>> ---
>>> qapi/block-core.json | 38 ++++++++++++++++++++++++++++++++++++++
>>> include/block/block.h | 1 +
>>> include/block/block_int.h | 1 +
>>> block.c | 9 +++++++++
>>> block/file-posix.c | 32 ++++++++++++++++++++++++++++++++
>>> block/qapi.c | 5 +++++
>>> 6 files changed, 86 insertions(+)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 959358ccc4..b100e852c7 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -877,6 +877,41 @@
>>> '*x_wr_latency_histogram': 'BlockLatencyHistogramInfo',
>>> '*x_flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
>>>
>>> +##
>>> +# @BlockStatsSpecificFile:
>>> +#
>>> +# File driver statistics
>>> +#
>>> +# @discard-nb-ok: The number of succeeded discard operations performed by
>>
>> successful discard operations
>>
>
> Fixed.
>
>>> +# the driver.
>>> +#
>>> +# @discard-nb-failed: The number of failed discard operations performed by
>>> +# the driver.
>>> +#
>>> +# @discard-bytes-ok: The number of bytes discarded by the driver.
>>> +#
>>> +# Since: 4.0
>>> +##
>>> +{ 'struct': 'BlockStatsSpecificFile',
>>> + 'data': {
>>> + 'discard-nb-ok': 'int',
>>> + 'discard-nb-failed': 'int',
>>> + 'discard-bytes-ok': 'int' } }
>>
>> Should these be unsigned?
>>
>> For what it's worth, similar counters nearby are also 'int'.
>>
>
> And I just added these symmetrically.
> Probably shouldn't have - let these be uint64.
>
>>> +
>>> +##
>>> +# @BlockStatsSpecific:
>>> +#
>>> +# Block driver specific statistics
>>> +#
>>> +# Since: 4.0
>>> +##
>>> +{ 'union': 'BlockStatsSpecific',
>>> + 'base': { 'driver': 'BlockdevDriver' },
>>> + 'discriminator': 'driver',
>>> + 'data': {
>>> + 'file': 'BlockStatsSpecificFile',
>>> + 'host_device': 'BlockStatsSpecificFile' } }
>>> +
>>> ##
>>> # @BlockStats:
>>> #
>>> @@ -892,6 +927,8 @@
>>> #
>>> # @stats: A @BlockDeviceStats for the device.
>>> #
>>> +# @driver-specific: Optional driver-specific stats. (Since 4.0)
>>> +#
>>> # @parent: This describes the file block device if it has one.
>>> # Contains recursively the statistics of the underlying
>>> # protocol (e.g. the host file for a qcow2 image). If there is
>>> @@ -905,6 +942,7 @@
>>> { 'struct': 'BlockStats',
>>> 'data': {'*device': 'str', '*qdev': 'str', '*node-name': 'str',
>>> 'stats': 'BlockDeviceStats',
>>> + '*driver-specific': 'BlockStatsSpecific',
>>> '*parent': 'BlockStats',
>>> '*backing': 'BlockStats'} }
>>>
>>
>> Feels awkward.
>>
>> When is @driver-specific present? Exactly when the driver is 'file' or
>> 'host_device'? If that's correct, then turning BlockStats into a union
>> would be clearer and reduce parenthesises on the wire:
>>
>> { 'union': 'BlockStats',
>> 'base': {
>> 'driver': 'BlockdevDriver',
>> ... all the other existing members of BlockStats ... }
>> 'discriminator': 'driver',
>> 'data': {
>> 'file': 'BlockStatsSpecificFile',
>> 'host_device': 'BlockStatsSpecificFile' } }
>>
>> [...]
>>
>
> this series drags for quite a while - we already discussed this :)
> In short: Blockdev does not always have driver, so it's either this
> or adding weird BlockdevDriver values like "none".
>
> http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg01845.html
With the comment tweak:
Acked-by: Markus Armbruster <armbru@redhat.com>
© 2016 - 2026 Red Hat, Inc.