* [Qemu-devel] [PATCH v2 1/8] qapi: group BlockDeviceStats fields
2018-01-19 12:49 [Qemu-devel] [PATCH v2 0/8] discard blockstats Anton Nefedov
@ 2018-01-19 12:50 ` Anton Nefedov
2018-02-06 13:12 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 2/8] qapi: add unmap to BlockDeviceStats Anton Nefedov
` (6 subsequent siblings)
7 siblings, 1 reply; 35+ messages in thread
From: Anton Nefedov @ 2018-01-19 12:50 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, mreitz, armbru, jsnow, pbonzini, eblake, den,
Anton Nefedov
Make the stat fields definition slightly more readable.
Cosmetic change only.
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
qapi/block-core.json | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e94a688..2e0665e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -733,14 +733,26 @@
# Since: 0.14.0
##
{ 'struct': 'BlockDeviceStats',
- 'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'rd_operations': 'int',
- 'wr_operations': 'int', 'flush_operations': 'int',
+ 'data': {'rd_bytes': 'int', 'wr_bytes': 'int',
+
+ 'rd_operations': 'int', 'wr_operations': 'int',
+ 'flush_operations': 'int',
+
'flush_total_time_ns': 'int', 'wr_total_time_ns': 'int',
- 'rd_total_time_ns': 'int', 'wr_highest_offset': 'int',
- 'rd_merged': 'int', 'wr_merged': 'int', '*idle_time_ns': 'int',
+ 'rd_total_time_ns': 'int',
+
+ 'wr_highest_offset': 'int',
+
+ 'rd_merged': 'int', 'wr_merged': 'int',
+
+ '*idle_time_ns': 'int',
+
'failed_rd_operations': 'int', 'failed_wr_operations': 'int',
- 'failed_flush_operations': 'int', 'invalid_rd_operations': 'int',
- 'invalid_wr_operations': 'int', 'invalid_flush_operations': 'int',
+ 'failed_flush_operations': 'int',
+
+ 'invalid_rd_operations': 'int', 'invalid_wr_operations': 'int',
+ 'invalid_flush_operations': 'int',
+
'account_invalid': 'bool', 'account_failed': 'bool',
'timed_stats': ['BlockDeviceTimedStats'] } }
--
2.7.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/8] qapi: group BlockDeviceStats fields
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 1/8] qapi: group BlockDeviceStats fields Anton Nefedov
@ 2018-02-06 13:12 ` Alberto Garcia
2018-02-12 16:18 ` Anton Nefedov
0 siblings, 1 reply; 35+ messages in thread
From: Alberto Garcia @ 2018-02-06 13:12 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel
Cc: kwolf, den, qemu-block, armbru, mreitz, pbonzini
On Fri 19 Jan 2018 01:50:00 PM CET, Anton Nefedov wrote:
> Make the stat fields definition slightly more readable.
> Cosmetic change only.
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
> qapi/block-core.json | 24 ++++++++++++++++++------
> 1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e94a688..2e0665e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -733,14 +733,26 @@
> # Since: 0.14.0
> ##
> { 'struct': 'BlockDeviceStats',
> - 'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'rd_operations': 'int',
> - 'wr_operations': 'int', 'flush_operations': 'int',
> + 'data': {'rd_bytes': 'int', 'wr_bytes': 'int',
> +
> + 'rd_operations': 'int', 'wr_operations': 'int',
> + 'flush_operations': 'int',
> +
> 'flush_total_time_ns': 'int', 'wr_total_time_ns': 'int',
> - 'rd_total_time_ns': 'int', 'wr_highest_offset': 'int',
> - 'rd_merged': 'int', 'wr_merged': 'int', '*idle_time_ns': 'int',
> + 'rd_total_time_ns': 'int',
It would be nice to reorder these following the read-write-flush order.
This way these would be rd_total_time_ns, wr_total_time_ns and
flush_total_time_ns.
The order of the fields in the documentation would also need to be
changed.
Berto
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/8] qapi: group BlockDeviceStats fields
2018-02-06 13:12 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
@ 2018-02-12 16:18 ` Anton Nefedov
0 siblings, 0 replies; 35+ messages in thread
From: Anton Nefedov @ 2018-02-12 16:18 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel
Cc: kwolf, den, qemu-block, armbru, mreitz, pbonzini
On 6/2/2018 4:12 PM, Alberto Garcia wrote:
> On Fri 19 Jan 2018 01:50:00 PM CET, Anton Nefedov wrote:
>> Make the stat fields definition slightly more readable.
>> Cosmetic change only.
>>
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> ---
>> qapi/block-core.json | 24 ++++++++++++++++++------
>> 1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index e94a688..2e0665e 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -733,14 +733,26 @@
>> # Since: 0.14.0
>> ##
>> { 'struct': 'BlockDeviceStats',
>> - 'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'rd_operations': 'int',
>> - 'wr_operations': 'int', 'flush_operations': 'int',
>> + 'data': {'rd_bytes': 'int', 'wr_bytes': 'int',
>> +
>> + 'rd_operations': 'int', 'wr_operations': 'int',
>> + 'flush_operations': 'int',
>> +
>> 'flush_total_time_ns': 'int', 'wr_total_time_ns': 'int',
>> - 'rd_total_time_ns': 'int', 'wr_highest_offset': 'int',
>> - 'rd_merged': 'int', 'wr_merged': 'int', '*idle_time_ns': 'int',
>> + 'rd_total_time_ns': 'int',
>
> It would be nice to reorder these following the read-write-flush order.
> This way these would be rd_total_time_ns, wr_total_time_ns and
> flush_total_time_ns.
>
> The order of the fields in the documentation would also need to be
> changed.
>
> Berto
>
Agree, done.
I'm also moving here the spelling fixes proposed by Erik.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v2 2/8] qapi: add unmap to BlockDeviceStats
2018-01-19 12:49 [Qemu-devel] [PATCH v2 0/8] discard blockstats Anton Nefedov
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 1/8] qapi: group BlockDeviceStats fields Anton Nefedov
@ 2018-01-19 12:50 ` Anton Nefedov
2018-01-22 20:47 ` Eric Blake
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 3/8] ide: account UNMAP (TRIM) operations Anton Nefedov
` (5 subsequent siblings)
7 siblings, 1 reply; 35+ messages in thread
From: Anton Nefedov @ 2018-01-19 12:50 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, mreitz, armbru, jsnow, pbonzini, eblake, den,
Anton Nefedov
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
qapi/block-core.json | 29 +++++++++++++++++++++++------
include/block/accounting.h | 1 +
block/qapi.c | 6 ++++++
3 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 2e0665e..3fa2d3a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -674,6 +674,8 @@
#
# @wr_bytes: The number of bytes written by the device.
#
+# @unmap_bytes: The number of bytes unmapped by the device (Since 2.12)
+#
# @rd_operations: The number of read operations performed by the device.
#
# @wr_operations: The number of write operations performed by the device.
@@ -681,6 +683,9 @@
# @flush_operations: The number of cache flush operations performed by the
# device (since 0.15.0)
#
+# @unmap_operations: The number of unmap operations performed by the device
+# (Since 2.12)
+#
# @flush_total_time_ns: Total time spend on cache flushes in nano-seconds
# (since 0.15.0).
#
@@ -688,6 +693,9 @@
#
# @rd_total_time_ns: Total_time_spend on reads in nano-seconds (since 0.15.0).
#
+# @unmap_total_time_ns: Total time spent on unmap operations in nano-seconds
+# (Since 2.12)
+#
# @wr_highest_offset: The offset after the greatest byte written to the
# device. The intended use of this information is for
# growable sparse files (like qcow2) that are used on top
@@ -699,6 +707,9 @@
# @wr_merged: Number of write requests that have been merged into another
# request (Since 2.3).
#
+# @unmap_merged: Number of unmap requests that have been merged into another
+# request (Since 2.12)
+#
# @idle_time_ns: Time since the last I/O operation, in
# nanoseconds. If the field is absent it means that
# there haven't been any operations yet (Since 2.5).
@@ -712,6 +723,9 @@
# @failed_flush_operations: The number of failed flush operations
# performed by the device (Since 2.5)
#
+# @failed_unmap_operations: The number of failed unmap operations performed
+# by the device (Since 2.12)
+#
# @invalid_rd_operations: The number of invalid read operations
# performed by the device (Since 2.5)
#
@@ -721,6 +735,9 @@
# @invalid_flush_operations: The number of invalid flush operations
# performed by the device (Since 2.5)
#
+# @invalid_unmap_operations: The number of invalid unmap operations performed
+# by the device (Since 2.12)
+#
# @account_invalid: Whether invalid operations are included in the
# last access statistics (Since 2.5)
#
@@ -733,25 +750,25 @@
# Since: 0.14.0
##
{ 'struct': 'BlockDeviceStats',
- 'data': {'rd_bytes': 'int', 'wr_bytes': 'int',
+ 'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'unmap_bytes' : 'int',
'rd_operations': 'int', 'wr_operations': 'int',
- 'flush_operations': 'int',
+ 'flush_operations': 'int', 'unmap_operations': 'int',
'flush_total_time_ns': 'int', 'wr_total_time_ns': 'int',
- 'rd_total_time_ns': 'int',
+ 'rd_total_time_ns': 'int', 'unmap_total_time_ns': 'int',
'wr_highest_offset': 'int',
- 'rd_merged': 'int', 'wr_merged': 'int',
+ 'rd_merged': 'int', 'wr_merged': 'int', 'unmap_merged': 'int',
'*idle_time_ns': 'int',
'failed_rd_operations': 'int', 'failed_wr_operations': 'int',
- 'failed_flush_operations': 'int',
+ 'failed_flush_operations': 'int', 'failed_unmap_operations': 'int',
'invalid_rd_operations': 'int', 'invalid_wr_operations': 'int',
- 'invalid_flush_operations': 'int',
+ 'invalid_flush_operations': 'int', 'invalid_unmap_operations': 'int',
'account_invalid': 'bool', 'account_failed': 'bool',
'timed_stats': ['BlockDeviceTimedStats'] } }
diff --git a/include/block/accounting.h b/include/block/accounting.h
index b833d26..4e53c4a 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -35,6 +35,7 @@ enum BlockAcctType {
BLOCK_ACCT_READ,
BLOCK_ACCT_WRITE,
BLOCK_ACCT_FLUSH,
+ BLOCK_ACCT_UNMAP,
BLOCK_MAX_IOTYPE,
};
diff --git a/block/qapi.c b/block/qapi.c
index fc10f0a..6e110f2 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -396,24 +396,30 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
ds->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ];
ds->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE];
+ ds->unmap_bytes = stats->nr_bytes[BLOCK_ACCT_UNMAP];
ds->rd_operations = stats->nr_ops[BLOCK_ACCT_READ];
ds->wr_operations = stats->nr_ops[BLOCK_ACCT_WRITE];
+ ds->unmap_operations = stats->nr_ops[BLOCK_ACCT_UNMAP];
ds->failed_rd_operations = stats->failed_ops[BLOCK_ACCT_READ];
ds->failed_wr_operations = stats->failed_ops[BLOCK_ACCT_WRITE];
ds->failed_flush_operations = stats->failed_ops[BLOCK_ACCT_FLUSH];
+ ds->failed_unmap_operations = stats->failed_ops[BLOCK_ACCT_UNMAP];
ds->invalid_rd_operations = stats->invalid_ops[BLOCK_ACCT_READ];
ds->invalid_wr_operations = stats->invalid_ops[BLOCK_ACCT_WRITE];
ds->invalid_flush_operations =
stats->invalid_ops[BLOCK_ACCT_FLUSH];
+ ds->invalid_unmap_operations = stats->invalid_ops[BLOCK_ACCT_UNMAP];
ds->rd_merged = stats->merged[BLOCK_ACCT_READ];
ds->wr_merged = stats->merged[BLOCK_ACCT_WRITE];
+ ds->unmap_merged = stats->merged[BLOCK_ACCT_UNMAP];
ds->flush_operations = stats->nr_ops[BLOCK_ACCT_FLUSH];
ds->wr_total_time_ns = stats->total_time_ns[BLOCK_ACCT_WRITE];
ds->rd_total_time_ns = stats->total_time_ns[BLOCK_ACCT_READ];
ds->flush_total_time_ns = stats->total_time_ns[BLOCK_ACCT_FLUSH];
+ ds->unmap_total_time_ns = stats->total_time_ns[BLOCK_ACCT_UNMAP];
ds->has_idle_time_ns = stats->last_access_time_ns > 0;
if (ds->has_idle_time_ns) {
--
2.7.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/8] qapi: add unmap to BlockDeviceStats
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 2/8] qapi: add unmap to BlockDeviceStats Anton Nefedov
@ 2018-01-22 20:47 ` Eric Blake
2018-01-23 10:35 ` Anton Nefedov
0 siblings, 1 reply; 35+ messages in thread
From: Eric Blake @ 2018-01-22 20:47 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel
Cc: qemu-block, kwolf, mreitz, armbru, jsnow, pbonzini, den
[-- Attachment #1: Type: text/plain, Size: 1191 bytes --]
On 01/19/2018 06:50 AM, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
> qapi/block-core.json | 29 +++++++++++++++++++++++------
> include/block/accounting.h | 1 +
> block/qapi.c | 6 ++++++
> 3 files changed, 30 insertions(+), 6 deletions(-)
>
> @@ -688,6 +693,9 @@
> #
> # @rd_total_time_ns: Total_time_spend on reads in nano-seconds (since 0.15.0).
While we are here, we could change s/Total_time_spend/Total time spent/
> #
> +# @unmap_total_time_ns: Total time spent on unmap operations in nano-seconds
> +# (Since 2.12)
Also, s/nano-seconds/nanoseconds/ (for both lines, if we are touching both).
The QAPI maintainer can touch that up (that may be me, depending on
Markus' schedule in the next few weeks); but I'm not seeing any UI
problems with the addition, so
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/8] qapi: add unmap to BlockDeviceStats
2018-01-22 20:47 ` Eric Blake
@ 2018-01-23 10:35 ` Anton Nefedov
0 siblings, 0 replies; 35+ messages in thread
From: Anton Nefedov @ 2018-01-23 10:35 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: qemu-block, kwolf, mreitz, armbru, jsnow, pbonzini, den
On 22/1/2018 11:47 PM, Eric Blake wrote:
> On 01/19/2018 06:50 AM, Anton Nefedov wrote:
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>> qapi/block-core.json | 29 +++++++++++++++++++++++------
>> include/block/accounting.h | 1 +
>> block/qapi.c | 6 ++++++
>> 3 files changed, 30 insertions(+), 6 deletions(-)
>>
>
>> @@ -688,6 +693,9 @@
>> #
>> # @rd_total_time_ns: Total_time_spend on reads in nano-seconds (since 0.15.0).
>
> While we are here, we could change s/Total_time_spend/Total time spent/
>
>> #
>> +# @unmap_total_time_ns: Total time spent on unmap operations in nano-seconds
>> +# (Since 2.12)
>
> Also, s/nano-seconds/nanoseconds/ (for both lines, if we are touching both).
>
> The QAPI maintainer can touch that up (that may be me, depending on
> Markus' schedule in the next few weeks); but I'm not seeing any UI
> problems with the addition, so
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
Thanks, fixed
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v2 3/8] ide: account UNMAP (TRIM) operations
2018-01-19 12:49 [Qemu-devel] [PATCH v2 0/8] discard blockstats Anton Nefedov
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 1/8] qapi: group BlockDeviceStats fields Anton Nefedov
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 2/8] qapi: add unmap to BlockDeviceStats Anton Nefedov
@ 2018-01-19 12:50 ` Anton Nefedov
2018-01-22 20:48 ` Eric Blake
2018-02-07 14:39 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 4/8] scsi: store unmap offset and nb_sectors in request struct Anton Nefedov
` (4 subsequent siblings)
7 siblings, 2 replies; 35+ messages in thread
From: Anton Nefedov @ 2018-01-19 12:50 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, mreitz, armbru, jsnow, pbonzini, eblake, den,
Anton Nefedov
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
hw/ide/core.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 5be72d4..6fdc936 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -443,6 +443,14 @@ static void ide_issue_trim_cb(void *opaque, int ret)
TrimAIOCB *iocb = opaque;
IDEState *s = iocb->s;
+ if (iocb->i >= 0) {
+ if (ret >= 0) {
+ block_acct_done(blk_get_stats(s->blk), &s->acct);
+ } else {
+ block_acct_failed(blk_get_stats(s->blk), &s->acct);
+ }
+ }
+
if (ret >= 0) {
while (iocb->j < iocb->qiov->niov) {
int j = iocb->j;
@@ -460,10 +468,15 @@ static void ide_issue_trim_cb(void *opaque, int ret)
}
if (!ide_sect_range_ok(s, sector, count)) {
+ block_acct_invalid(blk_get_stats(s->blk),
+ BLOCK_ACCT_UNMAP);
iocb->is_invalid = true;
goto done;
}
+ block_acct_start(blk_get_stats(s->blk), &s->acct,
+ count << BDRV_SECTOR_BITS, BLOCK_ACCT_UNMAP);
+
/* Got an entry! Submit and exit. */
iocb->aiocb = blk_aio_pdiscard(s->blk,
sector << BDRV_SECTOR_BITS,
--
2.7.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v2 3/8] ide: account UNMAP (TRIM) operations
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 3/8] ide: account UNMAP (TRIM) operations Anton Nefedov
@ 2018-01-22 20:48 ` Eric Blake
2018-01-23 10:39 ` Anton Nefedov
2018-02-07 14:39 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
1 sibling, 1 reply; 35+ messages in thread
From: Eric Blake @ 2018-01-22 20:48 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel
Cc: qemu-block, kwolf, mreitz, armbru, jsnow, pbonzini, den
[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]
On 01/19/2018 06:50 AM, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
> hw/ide/core.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> @@ -460,10 +468,15 @@ static void ide_issue_trim_cb(void *opaque, int ret)
> }
>
> if (!ide_sect_range_ok(s, sector, count)) {
> + block_acct_invalid(blk_get_stats(s->blk),
> + BLOCK_ACCT_UNMAP);
> iocb->is_invalid = true;
> goto done;
> }
>
> + block_acct_start(blk_get_stats(s->blk), &s->acct,
> + count << BDRV_SECTOR_BITS, BLOCK_ACCT_UNMAP);
We're still mixing bytes- and block-based reporting; how easy or hard
would it be to flip block_acct_start() and friends to be byte-based?
But not the subject of this series, per se.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v2 3/8] ide: account UNMAP (TRIM) operations
2018-01-22 20:48 ` Eric Blake
@ 2018-01-23 10:39 ` Anton Nefedov
0 siblings, 0 replies; 35+ messages in thread
From: Anton Nefedov @ 2018-01-23 10:39 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: qemu-block, kwolf, mreitz, armbru, jsnow, pbonzini, den
On 22/1/2018 11:48 PM, Eric Blake wrote:
> On 01/19/2018 06:50 AM, Anton Nefedov wrote:
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> ---
>> hw/ide/core.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>
>> @@ -460,10 +468,15 @@ static void ide_issue_trim_cb(void *opaque, int ret)
>> }
>>
>> if (!ide_sect_range_ok(s, sector, count)) {
>> + block_acct_invalid(blk_get_stats(s->blk),
>> + BLOCK_ACCT_UNMAP);
>> iocb->is_invalid = true;
>> goto done;
>> }
>>
>> + block_acct_start(blk_get_stats(s->blk), &s->acct,
>> + count << BDRV_SECTOR_BITS, BLOCK_ACCT_UNMAP);
>
> We're still mixing bytes- and block-based reporting; how easy or hard
> would it be to flip block_acct_start() and friends to be byte-based?
Quite easy, they already are :)
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/8] ide: account UNMAP (TRIM) operations
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 3/8] ide: account UNMAP (TRIM) operations Anton Nefedov
2018-01-22 20:48 ` Eric Blake
@ 2018-02-07 14:39 ` Alberto Garcia
1 sibling, 0 replies; 35+ messages in thread
From: Alberto Garcia @ 2018-02-07 14:39 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel
Cc: kwolf, den, qemu-block, armbru, mreitz, pbonzini
On Fri 19 Jan 2018 01:50:02 PM CET, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v2 4/8] scsi: store unmap offset and nb_sectors in request struct
2018-01-19 12:49 [Qemu-devel] [PATCH v2 0/8] discard blockstats Anton Nefedov
` (2 preceding siblings ...)
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 3/8] ide: account UNMAP (TRIM) operations Anton Nefedov
@ 2018-01-19 12:50 ` Anton Nefedov
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 5/8] scsi: move unmap error checking to the complete callback Anton Nefedov
` (3 subsequent siblings)
7 siblings, 0 replies; 35+ messages in thread
From: Anton Nefedov @ 2018-01-19 12:50 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, mreitz, armbru, jsnow, pbonzini, eblake, den,
Anton Nefedov
it allows to report it in the error handler
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
hw/scsi/scsi-disk.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 49d2559..7b8e0ed 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1625,8 +1625,6 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
{
SCSIDiskReq *r = data->r;
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
- uint64_t sector_num;
- uint32_t nb_sectors;
assert(r->req.aiocb == NULL);
if (scsi_disk_req_check_error(r, ret, false)) {
@@ -1634,16 +1632,16 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
}
if (data->count > 0) {
- sector_num = ldq_be_p(&data->inbuf[0]);
- nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
- if (!check_lba_range(s, sector_num, nb_sectors)) {
+ r->sector = ldq_be_p(&data->inbuf[0]);
+ r->sector_count = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
+ if (!check_lba_range(s, r->sector, r->sector_count)) {
scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
goto done;
}
r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
- sector_num * s->qdev.blocksize,
- nb_sectors * s->qdev.blocksize,
+ r->sector * s->qdev.blocksize,
+ r->sector_count * s->qdev.blocksize,
scsi_unmap_complete, data);
data->count--;
data->inbuf += 16;
--
2.7.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [PATCH v2 5/8] scsi: move unmap error checking to the complete callback
2018-01-19 12:49 [Qemu-devel] [PATCH v2 0/8] discard blockstats Anton Nefedov
` (3 preceding siblings ...)
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 4/8] scsi: store unmap offset and nb_sectors in request struct Anton Nefedov
@ 2018-01-19 12:50 ` Anton Nefedov
2018-02-07 15:00 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 6/8] scsi: account unmap operations Anton Nefedov
` (2 subsequent siblings)
7 siblings, 1 reply; 35+ messages in thread
From: Anton Nefedov @ 2018-01-19 12:50 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, mreitz, armbru, jsnow, pbonzini, eblake, den,
Anton Nefedov
This will help to account the operation in the following commit.
The difference is that we don't call scsi_disk_req_check_error() before
the 1st discard iteration anymore. That function also checks if
the request is cancelled, however it shouldn't get canceled until it
yields in blk_aio() functions anyway.
Same approach is already used for emulate_write_same.
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
hw/scsi/scsi-disk.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 7b8e0ed..693a754 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1627,9 +1627,6 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
assert(r->req.aiocb == NULL);
- if (scsi_disk_req_check_error(r, ret, false)) {
- goto done;
- }
if (data->count > 0) {
r->sector = ldq_be_p(&data->inbuf[0]);
@@ -1665,7 +1662,12 @@ static void scsi_unmap_complete(void *opaque, int ret)
r->req.aiocb = NULL;
aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
- scsi_unmap_complete_noio(data, ret);
+ if (scsi_disk_req_check_error(r, ret, false)) {
+ scsi_req_unref(&r->req);
+ g_free(data);
+ } else {
+ scsi_unmap_complete_noio(data, ret);
+ }
aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
}
--
2.7.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH v2 5/8] scsi: move unmap error checking to the complete callback
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 5/8] scsi: move unmap error checking to the complete callback Anton Nefedov
@ 2018-02-07 15:00 ` Alberto Garcia
0 siblings, 0 replies; 35+ messages in thread
From: Alberto Garcia @ 2018-02-07 15:00 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel
Cc: kwolf, den, qemu-block, armbru, mreitz, pbonzini
On Fri 19 Jan 2018 01:50:04 PM CET, Anton Nefedov wrote:
> This will help to account the operation in the following commit.
>
> The difference is that we don't call scsi_disk_req_check_error() before
> the 1st discard iteration anymore. That function also checks if
> the request is cancelled, however it shouldn't get canceled until it
> yields in blk_aio() functions anyway.
> Same approach is already used for emulate_write_same.
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
> @@ -1665,7 +1662,12 @@ static void scsi_unmap_complete(void *opaque, int ret)
> r->req.aiocb = NULL;
>
> aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
> - scsi_unmap_complete_noio(data, ret);
> + if (scsi_disk_req_check_error(r, ret, false)) {
> + scsi_req_unref(&r->req);
> + g_free(data);
It would be nice not to have the cleanup code duplicated here, but I
don't see any obvious alternative.
Berto
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v2 6/8] scsi: account unmap operations
2018-01-19 12:49 [Qemu-devel] [PATCH v2 0/8] discard blockstats Anton Nefedov
` (4 preceding siblings ...)
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 5/8] scsi: move unmap error checking to the complete callback Anton Nefedov
@ 2018-01-19 12:50 ` Anton Nefedov
2018-02-07 15:03 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 7/8] file-posix: account discard operations Anton Nefedov
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats Anton Nefedov
7 siblings, 1 reply; 35+ messages in thread
From: Anton Nefedov @ 2018-01-19 12:50 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, mreitz, armbru, jsnow, pbonzini, eblake, den,
Anton Nefedov
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
hw/scsi/scsi-disk.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 693a754..6881664 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1636,6 +1636,10 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
goto done;
}
+ block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
+ r->sector_count * s->qdev.blocksize,
+ BLOCK_ACCT_UNMAP);
+
r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
r->sector * s->qdev.blocksize,
r->sector_count * s->qdev.blocksize,
@@ -1662,10 +1666,11 @@ static void scsi_unmap_complete(void *opaque, int ret)
r->req.aiocb = NULL;
aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
- if (scsi_disk_req_check_error(r, ret, false)) {
+ if (scsi_disk_req_check_error(r, ret, true)) {
scsi_req_unref(&r->req);
g_free(data);
} else {
+ block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
scsi_unmap_complete_noio(data, ret);
}
aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
@@ -1712,10 +1717,12 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
return;
invalid_param_len:
+ block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
scsi_check_condition(r, SENSE_CODE(INVALID_PARAM_LEN));
return;
invalid_field:
+ block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
}
--
2.7.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [PATCH v2 7/8] file-posix: account discard operations
2018-01-19 12:49 [Qemu-devel] [PATCH v2 0/8] discard blockstats Anton Nefedov
` (5 preceding siblings ...)
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 6/8] scsi: account unmap operations Anton Nefedov
@ 2018-01-19 12:50 ` Anton Nefedov
2018-02-07 15:10 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats Anton Nefedov
7 siblings, 1 reply; 35+ messages in thread
From: Anton Nefedov @ 2018-01-19 12:50 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, mreitz, armbru, jsnow, pbonzini, eblake, den,
Anton Nefedov
This will help to identify how many of the user-issued discard operations
(accounted on a device level) have actually suceeded down on the host file
(even though the numbers will not be exactly the same if non-raw format
driver is used (e.g. qcow2 sending metadata discards)).
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/file-posix.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 36ee89e..544ae58 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -158,6 +158,11 @@ typedef struct BDRVRawState {
bool page_cache_inconsistent:1;
bool has_fallocate;
bool needs_alignment;
+ struct {
+ int64_t discard_nb_ok;
+ int64_t discard_nb_failed;
+ int64_t discard_bytes_ok;
+ } stats;
PRManager *pr_mgr;
} BDRVRawState;
@@ -1458,6 +1463,16 @@ static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
return ret;
}
+static void raw_account_discard(BDRVRawState *s, uint64_t nbytes, int ret)
+{
+ if (ret) {
+ s->stats.discard_nb_failed++;
+ } else {
+ s->stats.discard_nb_ok++;
+ s->stats.discard_bytes_ok += nbytes;
+ }
+}
+
static int aio_worker(void *arg)
{
RawPosixAIOData *aiocb = arg;
@@ -1494,6 +1509,7 @@ static int aio_worker(void *arg)
break;
case QEMU_AIO_DISCARD:
ret = handle_aiocb_discard(aiocb);
+ raw_account_discard(aiocb->bs->opaque, aiocb->aio_nbytes, ret);
break;
case QEMU_AIO_WRITE_ZEROES:
ret = handle_aiocb_write_zeroes(aiocb);
@@ -2654,8 +2670,9 @@ static coroutine_fn BlockAIOCB *hdev_aio_pdiscard(BlockDriverState *bs,
BlockCompletionFunc *cb, void *opaque)
{
BDRVRawState *s = bs->opaque;
-
- if (fd_open(bs) < 0) {
+ int ret = fd_open(bs);
+ if (ret < 0) {
+ raw_account_discard(s, bytes, ret);
return NULL;
}
return paio_submit(bs, s->fd, offset, NULL, bytes,
--
2.7.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH v2 7/8] file-posix: account discard operations
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 7/8] file-posix: account discard operations Anton Nefedov
@ 2018-02-07 15:10 ` Alberto Garcia
2018-02-12 16:19 ` Anton Nefedov
0 siblings, 1 reply; 35+ messages in thread
From: Alberto Garcia @ 2018-02-07 15:10 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel
Cc: kwolf, den, qemu-block, armbru, mreitz, pbonzini
On Fri 19 Jan 2018 01:50:06 PM CET, Anton Nefedov wrote:
> This will help to identify how many of the user-issued discard operations
> (accounted on a device level) have actually suceeded down on the host file
> (even though the numbers will not be exactly the same if non-raw format
> driver is used (e.g. qcow2 sending metadata discards)).
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/file-posix.c | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 36ee89e..544ae58 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -158,6 +158,11 @@ typedef struct BDRVRawState {
> bool page_cache_inconsistent:1;
> bool has_fallocate;
> bool needs_alignment;
> + struct {
> + int64_t discard_nb_ok;
> + int64_t discard_nb_failed;
> + int64_t discard_bytes_ok;
> + } stats;
Shouldn't this new structure be defined in a header file so other
drivers can use it? Or did you define it here because you don't see that
happening soon?
The rest of the patch looks good.
Berto
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH v2 7/8] file-posix: account discard operations
2018-02-07 15:10 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
@ 2018-02-12 16:19 ` Anton Nefedov
2018-02-16 11:36 ` Alberto Garcia
0 siblings, 1 reply; 35+ messages in thread
From: Anton Nefedov @ 2018-02-12 16:19 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel
Cc: kwolf, den, qemu-block, armbru, mreitz, pbonzini
On 7/2/2018 6:10 PM, Alberto Garcia wrote:
> On Fri 19 Jan 2018 01:50:06 PM CET, Anton Nefedov wrote:
>> This will help to identify how many of the user-issued discard operations
>> (accounted on a device level) have actually suceeded down on the host file
>> (even though the numbers will not be exactly the same if non-raw format
>> driver is used (e.g. qcow2 sending metadata discards)).
>>
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> block/file-posix.c | 21 +++++++++++++++++++--
>> 1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 36ee89e..544ae58 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -158,6 +158,11 @@ typedef struct BDRVRawState {
>> bool page_cache_inconsistent:1;
>> bool has_fallocate;
>> bool needs_alignment;
>> + struct {
>> + int64_t discard_nb_ok;
>> + int64_t discard_nb_failed;
>> + int64_t discard_bytes_ok;
>> + } stats;
>
> Shouldn't this new structure be defined in a header file so other
> drivers can use it? Or did you define it here because you don't see that
> happening soon?
>
I guess there's no reason to burden the common header files as long as
it's not really used anywhere else.
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH v2 7/8] file-posix: account discard operations
2018-02-12 16:19 ` Anton Nefedov
@ 2018-02-16 11:36 ` Alberto Garcia
0 siblings, 0 replies; 35+ messages in thread
From: Alberto Garcia @ 2018-02-16 11:36 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel
Cc: kwolf, den, qemu-block, armbru, mreitz, pbonzini
On Mon 12 Feb 2018 05:19:57 PM CET, Anton Nefedov wrote:
>>> @@ -158,6 +158,11 @@ typedef struct BDRVRawState {
>>> bool page_cache_inconsistent:1;
>>> bool has_fallocate;
>>> bool needs_alignment;
>>> + struct {
>>> + int64_t discard_nb_ok;
>>> + int64_t discard_nb_failed;
>>> + int64_t discard_bytes_ok;
>>> + } stats;
>>
>> Shouldn't this new structure be defined in a header file so other
>> drivers can use it? Or did you define it here because you don't see that
>> happening soon?
>>
>
> I guess there's no reason to burden the common header files as long as
> it's not really used anywhere else.
Fair enough,
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
2018-01-19 12:49 [Qemu-devel] [PATCH v2 0/8] discard blockstats Anton Nefedov
` (6 preceding siblings ...)
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 7/8] file-posix: account discard operations Anton Nefedov
@ 2018-01-19 12:50 ` Anton Nefedov
2018-01-22 20:55 ` Eric Blake
7 siblings, 1 reply; 35+ messages in thread
From: Anton Nefedov @ 2018-01-19 12:50 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, kwolf, mreitz, armbru, jsnow, pbonzini, eblake, den,
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 <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
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 = raw_get_info,
.bdrv_get_allocated_file_size
= raw_get_allocated_file_size,
+ .bdrv_get_stats = raw_get_stats,
.bdrv_check_perm = raw_check_perm,
.bdrv_set_perm = raw_set_perm,
.bdrv_abort_perm_update = raw_abort_perm_update,
@@ -2790,6 +2810,7 @@ static BlockDriver bdrv_host_device = {
.bdrv_get_info = raw_get_info,
.bdrv_get_allocated_file_size
= raw_get_allocated_file_size,
+ .bdrv_get_stats = raw_get_stats,
.bdrv_check_perm = raw_check_perm,
.bdrv_set_perm = raw_set_perm,
.bdrv_abort_perm_update = raw_abort_perm_update,
diff --git a/block/qapi.c b/block/qapi.c
index 6e110f2..4191e6c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -489,6 +489,11 @@ static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs,
s->stats->wr_highest_offset = stat64_get(&bs->wr_highest_offset);
+ s->driver_stats = bdrv_get_driver_stats(bs);
+ if (s->driver_stats) {
+ s->has_driver_stats = true;
+ }
+
if (bs->file) {
s->has_parent = true;
s->parent = bdrv_query_bds_stats(bs->file->bs, blk_level);
--
2.7.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
2018-01-19 12:50 ` [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats Anton Nefedov
@ 2018-01-22 20:55 ` Eric Blake
2018-01-23 11:28 ` Anton Nefedov
2018-02-03 15:59 ` Markus Armbruster
0 siblings, 2 replies; 35+ messages in thread
From: Eric Blake @ 2018-01-22 20:55 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel
Cc: qemu-block, kwolf, mreitz, armbru, jsnow, pbonzini, den
[-- Attachment #1: Type: text/plain, Size: 2936 bytes --]
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.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@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.
> +
> +##
> # @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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
2018-01-22 20:55 ` Eric Blake
@ 2018-01-23 11:28 ` Anton Nefedov
2018-01-23 14:53 ` Eric Blake
2018-02-03 15:59 ` Markus Armbruster
1 sibling, 1 reply; 35+ messages in thread
From: Anton Nefedov @ 2018-01-23 11:28 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: qemu-block, kwolf, mreitz, armbru, jsnow, pbonzini, den
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 <anton.nefedov@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@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.
>
>> +
>> +##
>> # @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
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
2018-01-23 11:28 ` Anton Nefedov
@ 2018-01-23 14:53 ` Eric Blake
0 siblings, 0 replies; 35+ messages in thread
From: Eric Blake @ 2018-01-23 14:53 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel
Cc: qemu-block, kwolf, mreitz, armbru, jsnow, pbonzini, den
[-- Attachment #1: Type: text/plain, Size: 1284 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
2018-01-22 20:55 ` Eric Blake
2018-01-23 11:28 ` Anton Nefedov
@ 2018-02-03 15:59 ` Markus Armbruster
2018-02-12 16:38 ` Anton Nefedov
2018-06-07 14:32 ` Anton Nefedov
1 sibling, 2 replies; 35+ messages in thread
From: Markus Armbruster @ 2018-02-03 15:59 UTC (permalink / raw)
To: Eric Blake
Cc: Anton Nefedov, qemu-devel, kwolf, den, qemu-block, mreitz,
pbonzini, jsnow
Eric Blake <eblake@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.nefedov@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@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.
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
2018-02-03 15:59 ` Markus Armbruster
@ 2018-02-12 16:38 ` Anton Nefedov
2018-02-15 10:23 ` Anton Nefedov
2018-06-07 14:32 ` Anton Nefedov
1 sibling, 1 reply; 35+ messages in thread
From: Anton Nefedov @ 2018-02-12 16:38 UTC (permalink / raw)
To: Markus Armbruster, Eric Blake
Cc: qemu-devel, kwolf, den, qemu-block, mreitz, pbonzini, jsnow
On 3/2/2018 6:59 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 01/19/2018 06:50 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.
>
> 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.
>
The output is better indeed, thanks; a little drawback is now we need to
pass the whole BlockStats to the driver so it fills its stats.
e.g. the interface:
void (*bdrv_get_stats)(BlockDriverState *bs, BlockStats *stats);
And that BlockdevDriver subset type (or a generator patch) still seems
to be needed
{ 'enum' : 'BlockdevDriverWithStats',
'data' : [ 'file' ] }
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
2018-02-12 16:38 ` Anton Nefedov
@ 2018-02-15 10:23 ` Anton Nefedov
2018-02-15 14:32 ` Eric Blake
0 siblings, 1 reply; 35+ messages in thread
From: Anton Nefedov @ 2018-02-15 10:23 UTC (permalink / raw)
To: Markus Armbruster, Eric Blake
Cc: qemu-devel, kwolf, den, qemu-block, mreitz, pbonzini, jsnow
On 12/2/2018 7:38 PM, Anton Nefedov wrote:
>
>
> On 3/2/2018 6:59 PM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 01/19/2018 06:50 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.
>>
>> 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.
>>
>
> The output is better indeed, thanks; a little drawback is now we need to
> pass the whole BlockStats to the driver so it fills its stats.
>
> e.g. the interface:
>
> void (*bdrv_get_stats)(BlockDriverState *bs, BlockStats *stats);
>
> And that BlockdevDriver subset type (or a generator patch) still seems
> to be needed
>
> { 'enum' : 'BlockdevDriverWithStats',
> 'data' : [ 'file' ] }
Hmm, actually it seems the driver-specific-stats should either be
optional, or we need to handle the rest of the drivers in the generated
code.
(i.e. with the dummy enum as above, what should the mandatory 'driver'
field be when there's e.g. nbd driver? It will default to 0, and that is
BLOCKDEV_DRIVER_WITH_STATS_FILE, so it will be interpreted as 'file' in
qmp output)
If we patch the generator, I guess we could add smth like a new tag to
the union that has no data for some discriminators?
e.g.
{ 'union': 'BlockStats',
'base': {'*device': 'str', '*node-name': 'str',
'stats': 'BlockDeviceStats',
'*parent': 'BlockStats',
'*backing': 'BlockStats',
'driver': 'BlockdevDriver'},
'discriminator': 'driver',
'data-optional': { <---- instead of 'data'
'file': 'BlockDriverStatsFile',
} }
Then the generator would need to:
- pick either 'data-optional' or 'data' members
- skip 'data missing branch' check for such unions
- do not abort() when visiting the union and discriminator pointing
to no data type
I can try and implement this if there's no other suggestion?
/Anton
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
2018-02-15 10:23 ` Anton Nefedov
@ 2018-02-15 14:32 ` Eric Blake
0 siblings, 0 replies; 35+ messages in thread
From: Eric Blake @ 2018-02-15 14:32 UTC (permalink / raw)
To: Anton Nefedov, Markus Armbruster
Cc: qemu-devel, kwolf, den, qemu-block, mreitz, pbonzini, jsnow
On 02/15/2018 04:23 AM, Anton Nefedov wrote:
>> The output is better indeed, thanks; a little drawback is now we need to
>> pass the whole BlockStats to the driver so it fills its stats.
>>
>> e.g. the interface:
>>
>> void (*bdrv_get_stats)(BlockDriverState *bs, BlockStats *stats);
>>
>> And that BlockdevDriver subset type (or a generator patch) still seems
>> to be needed
>>
>> { 'enum' : 'BlockdevDriverWithStats',
>> 'data' : [ 'file' ] }
>
> Hmm, actually it seems the driver-specific-stats should either be
> optional, or we need to handle the rest of the drivers in the generated
> code.
No, we don't want a new enum. Rather, use the existing enum, and use an
empty type for all the branches of the union that have no additional
stats to add.
>
> (i.e. with the dummy enum as above, what should the mandatory 'driver'
> field be when there's e.g. nbd driver? It will default to 0, and that is
> BLOCKDEV_DRIVER_WITH_STATS_FILE, so it will be interpreted as 'file' in
> qmp output)
>
>
> If we patch the generator, I guess we could add smth like a new tag to
> the union that has no data for some discriminators?
Right now, the way to write a union where only some branches add fields is:
{ 'struct': 'SomeEmptyType', 'data': {} }
{ 'union': 'MyUnion', 'base':'Base', 'discriminator':'foo',
'data': { 'file': 'ExtraFields',
'nbd': 'SomeEmptyType',
... } }
I also have a patch that I can revive (posted months ago) that would allow:
'data': { 'file': 'ExtraFields',
'nbd': {},
... } }
so that we don't have to explicitly create a pointless empty type for
the branches that don't care.
Meanwhile, here's an example post that uses the explicit empty type idiom:
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg03991.html
>
> e.g.
>
> { 'union': 'BlockStats',
> 'base': {'*device': 'str', '*node-name': 'str',
> 'stats': 'BlockDeviceStats',
> '*parent': 'BlockStats',
> '*backing': 'BlockStats',
> 'driver': 'BlockdevDriver'},
> 'discriminator': 'driver',
> 'data-optional': { <---- instead of 'data'
> 'file': 'BlockDriverStatsFile',
> } }
>
> Then the generator would need to:
> - pick either 'data-optional' or 'data' members
I'd rather keep it 'data': {} for the list of branches that have
additional members, and add a new boolean, maybe 'partial-data':true, as
the witness of whether the generator requires coverage for all enum
values, or will implicitly allow the uncovered enum values as though
they had an empty type.
> - skip 'data missing branch' check for such unions
> - do not abort() when visiting the union and discriminator pointing
> to no data type
>
> I can try and implement this if there's no other suggestion?
Or I could fold that into my patch from months ago. Let's see what I
can come up with this week.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
2018-02-03 15:59 ` Markus Armbruster
2018-02-12 16:38 ` Anton Nefedov
@ 2018-06-07 14:32 ` Anton Nefedov
2018-06-07 15:09 ` Markus Armbruster
1 sibling, 1 reply; 35+ messages in thread
From: Anton Nefedov @ 2018-06-07 14:32 UTC (permalink / raw)
To: Markus Armbruster, Eric Blake
Cc: qemu-devel, kwolf, den, qemu-block, mreitz, pbonzini, jsnow
On 3/2/2018 6:59 PM, Markus Armbruster wrote:
> Eric Blake <eblake@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.nefedov@virtuozzo.com>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
[..]
>>> +##
>>> +# @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?
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
2018-06-07 14:32 ` Anton Nefedov
@ 2018-06-07 15:09 ` Markus Armbruster
2018-06-07 15:23 ` Anton Nefedov
0 siblings, 1 reply; 35+ messages in thread
From: Markus Armbruster @ 2018-06-07 15:09 UTC (permalink / raw)
To: Anton Nefedov
Cc: Eric Blake, kwolf, den, qemu-block, qemu-devel, mreitz, pbonzini, jsnow
Anton Nefedov <anton.nefedov@virtuozzo.com> writes:
> On 3/2/2018 6:59 PM, Markus Armbruster wrote:
>> Eric Blake <eblake@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.nefedov@virtuozzo.com>
>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
> [..]
>>>> +##
>>>> +# @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?
> 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?
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
2018-06-07 15:09 ` Markus Armbruster
@ 2018-06-07 15:23 ` Anton Nefedov
2018-06-07 18:45 ` Eric Blake
0 siblings, 1 reply; 35+ messages in thread
From: Anton Nefedov @ 2018-06-07 15:23 UTC (permalink / raw)
To: Markus Armbruster
Cc: Eric Blake, kwolf, den, qemu-block, qemu-devel, mreitz, pbonzini, jsnow
On 7/6/2018 6:09 PM, Markus Armbruster wrote:
> Anton Nefedov <anton.nefedov@virtuozzo.com> writes:
>
>> On 3/2/2018 6:59 PM, Markus Armbruster wrote:
>>> Eric Blake <eblake@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.nefedov@virtuozzo.com>
>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>> [..]
>>>>> +##
>>>>> +# @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.
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
2018-06-07 15:23 ` Anton Nefedov
@ 2018-06-07 18:45 ` Eric Blake
2018-06-08 5:29 ` Markus Armbruster
0 siblings, 1 reply; 35+ messages in thread
From: Eric Blake @ 2018-06-07 18:45 UTC (permalink / raw)
To: Anton Nefedov, Markus Armbruster
Cc: kwolf, den, qemu-block, qemu-devel, mreitz, pbonzini, jsnow
On 06/07/2018 10:23 AM, Anton Nefedov wrote:
>>> 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
Okay, that sounds like the driver stats have to be optional, present
only when blk->bs is non-NULL.
>
>>> 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',
> >>> ... } }
That would require 'driver' to always resolve to something, even when
there is no driver (unless we create a superset enum that adds 'none'
beyond what 'BlockdevDriver' supports).
>
> 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.
This makes the most sense to me - we're stuck with a layer of nesting,
but that's because driver-stats truly is optional (we don't always have
access to a driver).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
2018-06-07 18:45 ` Eric Blake
@ 2018-06-08 5:29 ` Markus Armbruster
2018-06-13 10:36 ` Anton Nefedov
0 siblings, 1 reply; 35+ messages in thread
From: Markus Armbruster @ 2018-06-08 5:29 UTC (permalink / raw)
To: Eric Blake
Cc: Anton Nefedov, kwolf, den, qemu-block, qemu-devel, mreitz,
pbonzini, jsnow
Eric Blake <eblake@redhat.com> writes:
> On 06/07/2018 10:23 AM, Anton Nefedov wrote:
>>>> 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
>
> Okay, that sounds like the driver stats have to be optional, present
> only when blk->bs is non-NULL.
>
>>
>>>> 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',
>> >>> ... } }
>
> That would require 'driver' to always resolve to something, even when
> there is no driver (unless we create a superset enum that adds 'none'
> beyond what 'BlockdevDriver' supports).
>
>>
>> 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.
>
> This makes the most sense to me - we're stuck with a layer of nesting,
> but that's because driver-stats truly is optional (we don't always
> have access to a driver).
Agree.
Now we have to name the thing. You propose @driver-stats. Servicable,
but let's review how existing driver-specific things are named.
BlockdevOptions and BlockdevCreateOptions have them inline, thus no
name.
ImageInfo has '*format-specific': 'ImageInfoSpecific'.
If we follow ImageInfo precedence, we get '*driver-specific':
'BlockStatsSpecific'. driver-specific I like well enough.
BlockStatsSpecific less so. BlockStatsDriverSpecific feels better, but
perhaps a bit long.
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
2018-06-08 5:29 ` Markus Armbruster
@ 2018-06-13 10:36 ` Anton Nefedov
2018-06-13 11:40 ` Markus Armbruster
0 siblings, 1 reply; 35+ messages in thread
From: Anton Nefedov @ 2018-06-13 10:36 UTC (permalink / raw)
To: Markus Armbruster, Eric Blake
Cc: kwolf, den, qemu-block, qemu-devel, mreitz, pbonzini, jsnow
On 8/6/2018 8:29 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 06/07/2018 10:23 AM, Anton Nefedov wrote:
>>>>> 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
>>
>> Okay, that sounds like the driver stats have to be optional, present
>> only when blk->bs is non-NULL.
>>
>>>
>>>>> 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',
>>> >>> ... } }
>>
>> That would require 'driver' to always resolve to something, even when
>> there is no driver (unless we create a superset enum that adds 'none'
>> beyond what 'BlockdevDriver' supports).
>>
>>>
>>> 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.
>>
>> This makes the most sense to me - we're stuck with a layer of nesting,
>> but that's because driver-stats truly is optional (we don't always
>> have access to a driver).
>
> Agree.
>
> Now we have to name the thing. You propose @driver-stats. Servicable,
> but let's review how existing driver-specific things are named.
>
> BlockdevOptions and BlockdevCreateOptions have them inline, thus no
> name.
>
> ImageInfo has '*format-specific': 'ImageInfoSpecific'.
>
> If we follow ImageInfo precedence, we get '*driver-specific':
> 'BlockStatsSpecific'. driver-specific I like well enough.
> BlockStatsSpecific less so. BlockStatsDriverSpecific feels better, but
> perhaps a bit long.
>
following it further we will have BlockStatsDriverSpecificFile which is
even longer.
Personally, both
'*driver-specific': 'BlockDriverStats'; 'BlockDriverStatsFile'
'*driver-specific': 'BlockStatsSpecific'; 'BlockStatsSpecificFile'
look right to me.
Function signatures look ok too except that BlockDriverStats is easy to
confuse with BlockDriverState
BlockDriverStats *bdrv_get_stats(BlockDriverState *bs);
BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs);
so maybe BlockStatsSpecific indeed?
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
2018-06-13 10:36 ` Anton Nefedov
@ 2018-06-13 11:40 ` Markus Armbruster
0 siblings, 0 replies; 35+ messages in thread
From: Markus Armbruster @ 2018-06-13 11:40 UTC (permalink / raw)
To: Anton Nefedov
Cc: Markus Armbruster, Eric Blake, kwolf, den, qemu-block,
qemu-devel, mreitz, pbonzini, jsnow
Anton Nefedov <anton.nefedov@virtuozzo.com> writes:
> On 8/6/2018 8:29 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 06/07/2018 10:23 AM, Anton Nefedov wrote:
>>>>>> 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
>>>
>>> Okay, that sounds like the driver stats have to be optional, present
>>> only when blk->bs is non-NULL.
>>>
>>>>
>>>>>> 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',
>>>> >>> ... } }
>>>
>>> That would require 'driver' to always resolve to something, even when
>>> there is no driver (unless we create a superset enum that adds 'none'
>>> beyond what 'BlockdevDriver' supports).
>>>
>>>>
>>>> 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.
>>>
>>> This makes the most sense to me - we're stuck with a layer of nesting,
>>> but that's because driver-stats truly is optional (we don't always
>>> have access to a driver).
>>
>> Agree.
>>
>> Now we have to name the thing. You propose @driver-stats. Servicable,
>> but let's review how existing driver-specific things are named.
>>
>> BlockdevOptions and BlockdevCreateOptions have them inline, thus no
>> name.
>>
>> ImageInfo has '*format-specific': 'ImageInfoSpecific'.
>>
>> If we follow ImageInfo precedence, we get '*driver-specific':
>> 'BlockStatsSpecific'. driver-specific I like well enough.
>> BlockStatsSpecific less so. BlockStatsDriverSpecific feels better, but
>> perhaps a bit long.
>>
>
> following it further we will have BlockStatsDriverSpecificFile which is
> even longer.
>
> Personally, both
> '*driver-specific': 'BlockDriverStats'; 'BlockDriverStatsFile'
> '*driver-specific': 'BlockStatsSpecific'; 'BlockStatsSpecificFile'
> look right to me.
>
> Function signatures look ok too except that BlockDriverStats is easy to
> confuse with BlockDriverState
>
> BlockDriverStats *bdrv_get_stats(BlockDriverState *bs);
> BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs);
>
> so maybe BlockStatsSpecific indeed?
Works for me, but block maintainers have veto powers :)
^ permalink raw reply [flat|nested] 35+ messages in thread