Re: [Qemu-devel] [PATCH v6 9/9] qapi: query-blockstat: add driver specific file-posix stats

[Anton Nefedov]

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