Re: [Qemu-block] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats

[Eric Blake]

On 01/23/2018 05:28 AM, Anton Nefedov wrote:

>>> +
>>> +##
>>> +# @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.

> 
> Right, forgot about those unions.. Will fix.
> 
> (I guess I will need an extra enum, like BlockdevDriverWithStats with a
> single 'file' member, otherwise it seems to require to define data for
> each BlockdevDriver type)

Kevin also recently requested an easier way to make a flat union that
uses only a subset of a larger enum.  Maybe it's worth investing some
time in a QAPI generator patch to make this part easier (I have a
potential patch floating on one of my older branches that would allow
'branch':{} rather than having to declare a dummy type, but that's
slightly different than not even supplying the branches that you aren't
implementing).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats

[Anton Nefedov]

On 22/1/2018 11:55 PM, Eric Blake wrote:

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 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---



+++ 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.


+
+##
  # @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',


...[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.



Right, forgot about those unions.. Will fix.

(I guess I will need an extra enum, like BlockdevDriverWithStats with a
single 'file' member, otherwise it seems to require to define data for
each BlockdevDriver type)

/Anton

Re: [Qemu-block] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats

[Eric Blake]

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 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> ---

> +++ 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.

> +
> +##
>  # @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',

...[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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats

[Anton Nefedov]

A block driver can provide a callback to report driver-specific
statistics.

file-posix driver now reports discard statistics

Signed-off-by: Anton Nefedov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 qapi/block-core.json  | 37 +
 include/block/block.h |  1 +
 include/block/block_int.h |  1 +
 block.c   |  9 +
 block/file-posix.c| 21 +
 block/qapi.c  |  5 +
 6 files changed, 74 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3fa2d3a..0d9dc01 100644
--- a/qapi/block-core.json
+++ 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'
+  } }
+
+##
+# @BlockDriverStats:
+#
+# Statistics of a block driver (driver-specific)
+#
+# Since: 2.12
+##
+{ 'union': 'BlockDriverStats',
+  'data': {
+  'file': 'BlockDriverStatsFile'
+  } }
+
+##
 # @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',
'*parent': 'BlockStats',
'*backing': 'BlockStats'} }
 
diff --git a/include/block/block.h b/include/block/block.h
index 9b12774..2f20697 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -473,6 +473,7 @@ const char *bdrv_get_device_or_node_name(const 
BlockDriverState *bs);
 int bdrv_get_flags(BlockDriverState *bs);
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
 ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
+BlockDriverStats *bdrv_get_driver_stats(BlockDriverState *bs);
 void bdrv_round_to_clusters(BlockDriverState *bs,
 int64_t offset, int64_t bytes,
 int64_t *cluster_offset,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 29cafa4..6f56aeb 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -269,6 +269,7 @@ struct BlockDriver {
   Error **errp);
 int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
 ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
+BlockDriverStats *(*bdrv_get_stats)(BlockDriverState *bs);
 
 int coroutine_fn (*bdrv_save_vmstate)(BlockDriverState *bs,
   QEMUIOVector *qiov,
diff --git a/block.c b/block.c
index a8da4f2..8c03587 100644
--- a/block.c
+++ b/block.c
@@ -4062,6 +4062,15 @@ ImageInfoSpecific 
*bdrv_get_specific_info(BlockDriverState *bs)
 return NULL;
 }
 
+BlockDriverStats *bdrv_get_driver_stats(BlockDriverState *bs)
+{
+BlockDriver *drv = bs->drv;
+if (!drv || !drv->bdrv_get_stats) {
+return NULL;
+}
+return drv->bdrv_get_stats(bs);
+}
+
 void bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent event)
 {
 if (!bs || !bs->drv || !bs->drv->bdrv_debug_event) {
diff --git a/block/file-posix.c b/block/file-posix.c
index 544ae58..3ab92e6 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2240,6 +2240,25 @@ static int raw_get_info(BlockDriverState *bs, 
BlockDriverInfo *bdi)
 return 0;
 }
 
+static BlockDriverStats *raw_get_stats(BlockDriverState *bs)
+{
+BDRVRawState *s = bs->opaque;
+BlockDriverStats *stats = g_new(BlockDriverStats, 1);
+
+*stats = (BlockDriverStats){
+.type  = BLOCK_DRIVER_STATS_KIND_FILE,
+.u.file.data = g_new(BlockDriverStatsFile, 1),
+};
+
+*stats->u.file.data = (BlockDriverStatsFile){
+.discard_nb_ok = s->stats.discard_nb_ok,
+.discard_nb_failed = s->stats.discard_nb_failed,
+.discard_bytes_ok = s->stats.discard_bytes_ok,
+};
+
+return stats;
+}
+
 static QemuOptsList raw_create_opts = {
 .name = "raw-create-opts",
 .head = QTAILQ_HEAD_INITIALIZER(raw_create_opts.head),
@@ -2312,6 +2331,7 @@ BlockDriver bdrv_file = {
 .bdrv_get_info =