qemu-block
[Top][All Lists]
Advanced

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

Re: [Qemu-block] [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add


From: Anton Nefedov
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
Date: Thu, 7 Jun 2018 17:32:04 +0300
User-agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0



On 3/2/2018 6:59 PM, Markus Armbruster wrote:
Eric Blake <address@hidden> 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 <address@hidden>
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
[..]
+##
+# @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',
       ... } }


hi,

(resurrecting this series now that there is no-more-dummy-enums patchset
https://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg06836.html )

If we introduce BlockdevDriver as a discriminator as Markus suggests
above, we need some way to define its value.
I guess one would be to check blk->bs->drv->format_name but it won't
always work; often it's even blk->bs == NULL.

I guess we could add a default ('unspecified'?) to BlockdevDriver enum?
But I'd rather leave an optional BlockDriverStats union (but make it
flat). Only the drivers that provide these stats will need to set
BlockdevDriver field. What do you think?



reply via email to

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