On 13/12/2018 3:20 PM, Markus Armbruster wrote:
> I'm reviewing just the QAPI schema today.
>
> Anton Nefedov <anton.nefe...@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.nefe...@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
