Anton Nefedov <anton.nefe...@virtuozzo.com> writes: > 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
With the comment tweak: Acked-by: Markus Armbruster <arm...@redhat.com>