| Message ID | 20190128151521.13875-1-antonkuchin@yandex-team.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qmp: Deprecate query-nodes option of query-blockstats | expand |
Am 28.01.2019 um 16:15 hat Anton Kuchin geschrieben: > This option is broken since a6baa60807 in v2.9 and returns mostly > zeroes instead of real stats because actual querring of BlockStats > that resides in blk is missing. > > And it makes no sense because with this option BlockDriverState-s > are iterated but BlockAcctStats belong to BlockBackend and not BDS > since 7f0e9da6f13 in v2.5 > > Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru> Isn't query-nodes the only way to get wr_highest_offset for the protocol layer? oVirt depends on this, as far as I know. Kevin
On 2019-01-28 16:15, Anton Kuchin wrote: > This option is broken since a6baa60807 in v2.9 and returns mostly > zeroes instead of real stats because actual querring of BlockStats s/querring/querying/ > that resides in blk is missing. > > And it makes no sense because with this option BlockDriverState-s > are iterated but BlockAcctStats belong to BlockBackend and not BDS > since 7f0e9da6f13 in v2.5 > > Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru> > --- [...] > @@ -577,21 +576,10 @@ BlockStatsList *qmp_query_blockstats(bool has_query_nodes, > { > BlockStatsList *head = NULL, **p_next = &head; > BlockBackend *blk; > - BlockDriverState *bs; > > /* Just to be safe if query_nodes is not always initialized */ > if (has_query_nodes && query_nodes) { > - for (bs = bdrv_next_node(NULL); bs; bs = bdrv_next_node(bs)) { > - BlockStatsList *info = g_malloc0(sizeof(*info)); > - AioContext *ctx = bdrv_get_aio_context(bs); > - > - aio_context_acquire(ctx); > - info->value = bdrv_query_bds_stats(bs, false); > - aio_context_release(ctx); > - > - *p_next = info; > - p_next = &info->next; > - } > + error_setg(errp, "Option query_nodes is deprecated"); You don't only deprecate the option here, you completely disable it ... that does not make too much sense IMHO. If it is really broken since multiple releases already and nobody complained or tried to fix it so far, I think it could be simply removed immediately. Otherwise, if it is only partly broken, please leave it in the current state (if it is not fixable), and only mark it as deprecated in the qemu-deprecated.texi and block-core.json documentation. > } else { > for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) { > BlockStatsList *info; > @@ -604,7 +592,7 @@ BlockStatsList *qmp_query_blockstats(bool has_query_nodes, > } > > aio_context_acquire(ctx); > - s = bdrv_query_bds_stats(blk_bs(blk), true); > + s = bdrv_query_bds_stats(blk_bs(blk)); > s->has_device = true; > s->device = g_strdup(blk_name(blk)); > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 91685be6c2..2dd5f6032c 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -892,13 +892,7 @@ > # > # Query the @BlockStats for all virtual block devices. > # > -# @query-nodes: If true, the command will query all the block nodes > -# that have a node name, in a list which will include "parent" > -# information, but not "backing". > -# If false or omitted, the behavior is as before - query all the > -# device backends, recursively including their "parent" and > -# "backing". Filter nodes that were created implicitly are > -# skipped over in this mode. (Since 2.3) > +# @query-nodes: deprecated since 3.2 > # > # Returns: A list of @BlockStats for each virtual block devices. > # > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi > index 219206a836..e1e04ced7d 100644 > --- a/qemu-deprecated.texi > +++ b/qemu-deprecated.texi > @@ -112,6 +112,11 @@ Use ``device_add'' for hotplugging vCPUs instead of ``cpu-add''. See > documentation of ``query-hotpluggable-cpus'' for additional > details. > > +@subsection query-blockstats (since 3.2) > + > +"query-nodes" parameter is not supported anymore because blockstats > +are not a prorerty of node. s/prorerty/property/ > @section Human Monitor Protocol (HMP) commands > > @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 3.1) > Thomas
On Mon, Jan 28, 2019 at 04:37:50PM +0100, Kevin Wolf wrote: > Am 28.01.2019 um 16:15 hat Anton Kuchin geschrieben: > > This option is broken since a6baa60807 in v2.9 and returns mostly > > zeroes instead of real stats because actual querring of BlockStats > > that resides in blk is missing. > > > > And it makes no sense because with this option BlockDriverState-s > > are iterated but BlockAcctStats belong to BlockBackend and not BDS > > since 7f0e9da6f13 in v2.5 > > > > Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru> > > Isn't query-nodes the only way to get wr_highest_offset for the protocol > layer? oVirt depends on this, as far as I know. Libvirt just invokes 'query-blockstats' with no arguments, so is not relying on 'query-nodes' working. Given that libvirt doesn't use it, it doesn't seem like this is relevant for oVirt unless they were using QMP passthrough from libvirt. That this has been broken since v2.9 though rather suggests oVirt doesn't use it. Regards, Daniel
On Mon, Jan 28, 2019 at 05:04:18PM +0100, Thomas Huth wrote: > On 2019-01-28 16:15, Anton Kuchin wrote: > > This option is broken since a6baa60807 in v2.9 and returns mostly > > zeroes instead of real stats because actual querring of BlockStats > > s/querring/querying/ > > > that resides in blk is missing. > > > > And it makes no sense because with this option BlockDriverState-s > > are iterated but BlockAcctStats belong to BlockBackend and not BDS > > since 7f0e9da6f13 in v2.5 > > > > Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru> > > --- > [...] > > @@ -577,21 +576,10 @@ BlockStatsList *qmp_query_blockstats(bool has_query_nodes, > > { > > BlockStatsList *head = NULL, **p_next = &head; > > BlockBackend *blk; > > - BlockDriverState *bs; > > > > /* Just to be safe if query_nodes is not always initialized */ > > if (has_query_nodes && query_nodes) { > > - for (bs = bdrv_next_node(NULL); bs; bs = bdrv_next_node(bs)) { > > - BlockStatsList *info = g_malloc0(sizeof(*info)); > > - AioContext *ctx = bdrv_get_aio_context(bs); > > - > > - aio_context_acquire(ctx); > > - info->value = bdrv_query_bds_stats(bs, false); > > - aio_context_release(ctx); > > - > > - *p_next = info; > > - p_next = &info->next; > > - } > > + error_setg(errp, "Option query_nodes is deprecated"); > > You don't only deprecate the option here, you completely disable it ... > that does not make too much sense IMHO. If it is really broken since > multiple releases already and nobody complained or tried to fix it so > far, I think it could be simply removed immediately. Otherwise, if it is > only partly broken, please leave it in the current state (if it is not > fixable), and only mark it as deprecated in the qemu-deprecated.texi and > block-core.json documentation. IIUC fro the commit message it is broken in the sense that it returns all zeros, instead of real values. If some app is using it then they have probably just been running with those all-zero values. This change turns it into a fatal error in QMP, which is in turn going to propagate back to apps which could turn the silent usage of bad data into a hard error. I'm torn about whether this must go through deprecation or not. Technically this should go through the deprecation process, but I don't much like the idea of continuing to feed back garbage to apps for another few releases. Apps are effectively broken already even if they don't realize it. Reporting a clear error will at least highlight this pre-existing brokeness in such apps. If we are going to raise an error though, IMHO this is not the right way to go about it. We should delete 'query-nodes' from the schema entirely, at which point the QMP parser will raise an error automatically upfront. This also allows apps to introspect to see if the parameter exists or not. Regards, Daniel
Am 28.01.2019 um 17:12 hat Daniel P. Berrangé geschrieben: > On Mon, Jan 28, 2019 at 04:37:50PM +0100, Kevin Wolf wrote: > > Am 28.01.2019 um 16:15 hat Anton Kuchin geschrieben: > > > This option is broken since a6baa60807 in v2.9 and returns mostly > > > zeroes instead of real stats because actual querring of BlockStats > > > that resides in blk is missing. > > > > > > And it makes no sense because with this option BlockDriverState-s > > > are iterated but BlockAcctStats belong to BlockBackend and not BDS > > > since 7f0e9da6f13 in v2.5 > > > > > > Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru> > > > > Isn't query-nodes the only way to get wr_highest_offset for the protocol > > layer? oVirt depends on this, as far as I know. > > Libvirt just invokes 'query-blockstats' with no arguments, so is not > relying on 'query-nodes' working. Given that libvirt doesn't use it, > it doesn't seem like this is relevant for oVirt unless they were > using QMP passthrough from libvirt. That this has been broken > since v2.9 though rather suggests oVirt doesn't use it. It's not broken at all. The type of the return value is just a bit messy because we mix statistics from the device with statistics from the nodes. query-nodes=true can't return device statistics, but the respective fields aren't optional in the schema either, so it just puts 0 there. This is ugly, but if you know which information you can use, it works fine, so cleaning it up was never considered to have a good justification for breaking compatibility. In any case, wr_highest_offset is part of the node statistics and it contains the correct value for each node. Kevin
On 28/01/2019 18:37, Kevin Wolf wrote: > Am 28.01.2019 um 16:15 hat Anton Kuchin geschrieben: >> This option is broken since a6baa60807 in v2.9 and returns mostly >> zeroes instead of real stats because actual querring of BlockStats >> that resides in blk is missing. >> >> And it makes no sense because with this option BlockDriverState-s >> are iterated but BlockAcctStats belong to BlockBackend and not BDS >> since 7f0e9da6f13 in v2.5 >> >> Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru> > Isn't query-nodes the only way to get wr_highest_offset for the protocol > layer? oVirt depends on this, as far as I know. Isn't it reported without query-nodes parameter as "wr_highest_offset" of "parent" stats entry? What we get with query-nodes is essentially the same output, but without correct data from backend and with one more copy of almost empty data from protocol layer. I'll try to find usages of this option in oVirt code. > > Kevin >
On Mon, Jan 28, 2019 at 6:13 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > On Mon, Jan 28, 2019 at 04:37:50PM +0100, Kevin Wolf wrote: > > Am 28.01.2019 um 16:15 hat Anton Kuchin geschrieben: > > > This option is broken since a6baa60807 in v2.9 and returns mostly > > > zeroes instead of real stats because actual querring of BlockStats > > > that resides in blk is missing. > > > > > > And it makes no sense because with this option BlockDriverState-s > > > are iterated but BlockAcctStats belong to BlockBackend and not BDS > > > since 7f0e9da6f13 in v2.5 > > > > > > Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru> > > > > Isn't query-nodes the only way to get wr_highest_offset for the protocol > > layer? oVirt depends on this, as far as I know. > We do, thanks for caring! Libvirt just invokes 'query-blockstats' with no arguments, so is not > relying on 'query-nodes' working. Given that libvirt doesn't use it, > it doesn't seem like this is relevant for oVirt unless they were > using QMP passthrough from libvirt. We don't use QMP passthrough. We use only libvirt public APIs. You can see what we use here: https://github.com/oVirt/vdsm/blob/master/lib/vdsm/virt/drivemonitor.py Nir > That this has been broken > since v2.9 though rather suggests oVirt doesn't use it. > > Regards, > Daniel > -- > |: https://berrange.com -o- > https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- > https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- > https://www.instagram.com/dberrange :| >
diff --git a/block/qapi.c b/block/qapi.c index c66f949db8..19d4b4ee42 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -502,8 +502,7 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk) &ds->x_flush_latency_histogram); } -static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs, - bool blk_level) +static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs) { BlockStats *s = NULL; @@ -517,7 +516,7 @@ static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs, /* Skip automatically inserted nodes that the user isn't aware of in * a BlockBackend-level command. Stay at the exact node for a node-level * command. */ - while (blk_level && bs->drv && bs->implicit) { + while (bs->drv && bs->implicit) { bs = backing_bs(bs); assert(bs); } @@ -531,12 +530,12 @@ static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs, if (bs->file) { s->has_parent = true; - s->parent = bdrv_query_bds_stats(bs->file->bs, blk_level); + s->parent = bdrv_query_bds_stats(bs->file->bs); } - if (blk_level && bs->backing) { + if (bs->backing) { s->has_backing = true; - s->backing = bdrv_query_bds_stats(bs->backing->bs, blk_level); + s->backing = bdrv_query_bds_stats(bs->backing->bs); } return s; @@ -577,21 +576,10 @@ BlockStatsList *qmp_query_blockstats(bool has_query_nodes, { BlockStatsList *head = NULL, **p_next = &head; BlockBackend *blk; - BlockDriverState *bs; /* Just to be safe if query_nodes is not always initialized */ if (has_query_nodes && query_nodes) { - for (bs = bdrv_next_node(NULL); bs; bs = bdrv_next_node(bs)) { - BlockStatsList *info = g_malloc0(sizeof(*info)); - AioContext *ctx = bdrv_get_aio_context(bs); - - aio_context_acquire(ctx); - info->value = bdrv_query_bds_stats(bs, false); - aio_context_release(ctx); - - *p_next = info; - p_next = &info->next; - } + error_setg(errp, "Option query_nodes is deprecated"); } else { for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) { BlockStatsList *info; @@ -604,7 +592,7 @@ BlockStatsList *qmp_query_blockstats(bool has_query_nodes, } aio_context_acquire(ctx); - s = bdrv_query_bds_stats(blk_bs(blk), true); + s = bdrv_query_bds_stats(blk_bs(blk)); s->has_device = true; s->device = g_strdup(blk_name(blk)); diff --git a/qapi/block-core.json b/qapi/block-core.json index 91685be6c2..2dd5f6032c 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -892,13 +892,7 @@ # # Query the @BlockStats for all virtual block devices. # -# @query-nodes: If true, the command will query all the block nodes -# that have a node name, in a list which will include "parent" -# information, but not "backing". -# If false or omitted, the behavior is as before - query all the -# device backends, recursively including their "parent" and -# "backing". Filter nodes that were created implicitly are -# skipped over in this mode. (Since 2.3) +# @query-nodes: deprecated since 3.2 # # Returns: A list of @BlockStats for each virtual block devices. # diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi index 219206a836..e1e04ced7d 100644 --- a/qemu-deprecated.texi +++ b/qemu-deprecated.texi @@ -112,6 +112,11 @@ Use ``device_add'' for hotplugging vCPUs instead of ``cpu-add''. See documentation of ``query-hotpluggable-cpus'' for additional details. +@subsection query-blockstats (since 3.2) + +"query-nodes" parameter is not supported anymore because blockstats +are not a prorerty of node. + @section Human Monitor Protocol (HMP) commands @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 3.1)
This option is broken since a6baa60807 in v2.9 and returns mostly zeroes instead of real stats because actual querring of BlockStats that resides in blk is missing. And it makes no sense because with this option BlockDriverState-s are iterated but BlockAcctStats belong to BlockBackend and not BDS since 7f0e9da6f13 in v2.5 Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru> --- block/qapi.c | 26 +++++++------------------- qapi/block-core.json | 8 +------- qemu-deprecated.texi | 5 +++++ 3 files changed, 13 insertions(+), 26 deletions(-)