qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v6 9/9] qapi: query-blockstat: add


From: Markus Armbruster
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v6 9/9] qapi: query-blockstat: add driver specific file-posix stats
Date: Thu, 21 Feb 2019 13:36:56 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)

Anton Nefedov <address@hidden> writes:

> 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

With the comment tweak:
Acked-by: Markus Armbruster <address@hidden>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]