|
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 18:23:55 +0300 |
User-agent: | Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 |
On 7/6/2018 6:09 PM, Markus Armbruster wrote:
Anton Nefedov <address@hidden> writes: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.There is no blk->bs, at least not if blk is a BlockBackend *. I figure the problem you're trying to describe is query-blockstats running into BlockBackends that aren't associated with a BlockDriverState (blk->root is null), and thus aren't associated with a BlockDriver. Correct?
Sorry, yes, exactly
I guess we could add a default ('unspecified'?) to BlockdevDriver enum?This part I understand, but...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?I'm not sure I got this part. Care to sketch the QAPI schema snippet?
You earlier proposed: >>> 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', >>> ... } } But I meant to leave it as: + { 'union': 'BlockDriverStats': + 'base': { 'driver': 'BlockdevDriver' }, + 'discriminator': 'driver', + 'data': { + 'file': 'BlockDriverStatsFile' } } { 'struct': 'BlockStats', 'data': {'*device': 'str', '*node-name': 'str', 'stats': 'BlockDeviceStats', + '*driver-stats': 'BlockDriverStats', '*parent': 'BlockStats', '*backing': 'BlockStats'} } so those block backends which do not provide driver stats do not need to set BlockdevDriver field.
[Prev in Thread] | Current Thread | [Next in Thread] |