Eric Blake <ebl...@redhat.com> writes:
> On 01/19/2018 06:50 AM, 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.nefe...@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
>> ---
>
>> +++ b/qapi/block-core.json
>> @@ -774,6 +774,40 @@
>> 'timed_stats': ['BlockDeviceTimedStats'] } }
>>
>> ##
>> +# @BlockDriverStatsFile:
>> +#
>> +# 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 2.12
>> +##
>> +{ 'struct': 'BlockDriverStatsFile',
>> + 'data': {
>> + 'discard_nb_ok': 'int',
>> + 'discard_nb_failed': 'int',
>> + 'discard_bytes_ok': 'int'
>> + } }
>
> New interfaces should prefer '-' over '_', where possible (a reason for
> using '_' is if the fields are alongside pre-existing fields that
> already used '_'). Let's see how this gets used...[1]
>
>> +
>> +##
>> +# @BlockDriverStats:
>> +#
>> +# Statistics of a block driver (driver-specific)
>> +#
>> +# Since: 2.12
>> +##
>> +{ 'union': 'BlockDriverStats',
>> + 'data': {
>> + 'file': 'BlockDriverStatsFile'
>> + } }
>
> Markus has been adamant that we add no new "simple unions" (unions with
> a 'discriminator' field) - because they are anything but simple in the
> long run.
Indeed. You could make this a flat union, similar to BlockdevOptions:
{ 'union': 'BlockDriverStats':
'base': { 'driver': 'BlockdevDriver' },
'discriminator': 'driver',
'data': {
'file': 'BlockDriverStatsFile',
... } }
However:
>> +
>> +##
>> # @BlockStats:
>> #
>> # Statistics of a virtual block device or a block backing device.
>> @@ -785,6 +819,8 @@
>> #
>> # @stats: A @BlockDeviceStats for the device.
>> #
>> +# @driver-stats: Optional driver-specific statistics. (Since 2.12)
>> +#
>> # @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
>> @@ -798,6 +834,7 @@
>> { 'struct': 'BlockStats',
>> 'data': {'*device': 'str', '*node-name': 'str',
>> 'stats': 'BlockDeviceStats',
>> + '*driver-stats': 'BlockDriverStats',
You're adding a union of driver-specific stats to a struct of generic
stats. That's unnecessarily complicated. Instead, turn the struct of
generic stats into a flat union, like this:
{ 'union': 'BlockStats',
'base': { ... the generic stats, i.e. the members of BlockStats
before this patch ...
'driver': 'BlockdevDriver' }
'discriminator': 'driver',
'data': {
'file': 'BlockDriverStatsFile',
... } }
> ...[1] You are using it alongside a struct that already uses '-'
> (node-name), so you should use dashes.
>
> So, the difference between your proposal (a simple union) and using a
> "flat union", on the wire, is yours:
>
> "return": { ..., "driver-stats": { "type": "file", "data": {
> "discard_nb_ok: ... } } }
>
> vs. a flat union:
>
> "return": { ..., "driver-stats": { "driver": "file", "discard-nb-ok":
> ... } }
>
> where you can benefit from less nesting and a saner discriminator name.
My proposal peels off yet another level of nesting.
