[PATCH for 7.10] qemu: monitor: Fix usage of 'query-blockstats'

Peter Krempa posted 1 patch 2 years, 4 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/0f4b6516c9d7821fb1b755de41c0bfc99d9b0b05.1637842731.git.pkrempa@redhat.com
src/qemu/qemu_monitor_json.c | 52 +++++++++++++++++++++++++++++++++---
tests/qemumonitorjsontest.c  |  2 ++
2 files changed, 50 insertions(+), 4 deletions(-)
[PATCH for 7.10] qemu: monitor: Fix usage of 'query-blockstats'
Posted by Peter Krempa 2 years, 4 months ago
Commit bc24810c2cab modified code querying blockstats to use the
'query-nodes' parameter so that we can fetch stats also for images which
are not attached to a frontend such as block copy and backup scratch
images.

Unfortunately that broke the old blockstats because if 'query-nodes' is
enabled qemu doesn't output the 'qdev' parameter which our code used for
matching to the disk and also qemu neglects to populate the frontend
stats at all so we can't even switch to using nodenema for matching.

To fix this we need to do two calls, one with 'query-nodes' disabled
using the old logic to populate everything and then an aditional one
which populates all the remaining images.

Closes: https://gitlab.com/libvirt/libvirt/-/issues/246
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_monitor_json.c | 52 +++++++++++++++++++++++++++++++++---
 tests/qemumonitorjsontest.c  |  2 ++
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 3030cff664..56f0b22b2a 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2446,6 +2446,30 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValue *dev,
 }


+static int
+qemuMonitorJSONGetOneBlockStatsNodeInfo(virJSONValue *dev,
+                                        GHashTable *hash)
+{
+    qemuBlockStats *bstats = NULL;
+    int nstats = 0;
+    const char *nodename = NULL;
+
+    if (!(nodename = virJSONValueObjectGetString(dev, "node-name")))
+        return 0;
+
+    /* we already have the stats */
+    if (g_hash_table_contains(hash, nodename))
+        return 0;
+
+    if (!(bstats = qemuMonitorJSONBlockStatsCollectData(dev, &nstats)))
+        return -1;
+
+    g_hash_table_insert(hash, g_strdup(nodename), bstats);
+
+    return nstats;
+}
+
+
 virJSONValue *
 qemuMonitorJSONQueryBlockstats(qemuMonitor *mon,
                                bool queryNodes)
@@ -2475,13 +2499,14 @@ qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitor *mon,
     int nstats = 0;
     int rc;
     size_t i;
-    g_autoptr(virJSONValue) devices = NULL;
+    g_autoptr(virJSONValue) blockstatsDevices = NULL;
+    g_autoptr(virJSONValue) blockstatsNodes = NULL;

-    if (!(devices = qemuMonitorJSONQueryBlockstats(mon, true)))
+    if (!(blockstatsDevices = qemuMonitorJSONQueryBlockstats(mon, false)))
         return -1;

-    for (i = 0; i < virJSONValueArraySize(devices); i++) {
-        virJSONValue *dev = virJSONValueArrayGet(devices, i);
+    for (i = 0; i < virJSONValueArraySize(blockstatsDevices); i++) {
+        virJSONValue *dev = virJSONValueArrayGet(blockstatsDevices, i);
         const char *dev_name;

         if (!dev || virJSONValueGetType(dev) != VIR_JSON_TYPE_OBJECT) {
@@ -2505,6 +2530,25 @@ qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitor *mon,
             nstats = rc;
     }

+    if (!(blockstatsNodes = qemuMonitorJSONQueryBlockstats(mon, true)))
+        return -1;
+
+    for (i = 0; i < virJSONValueArraySize(blockstatsNodes); i++) {
+        virJSONValue *dev = virJSONValueArrayGet(blockstatsNodes, i);
+
+        if (!dev || virJSONValueGetType(dev) != VIR_JSON_TYPE_OBJECT) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("blockstats device entry was not in expected format"));
+            return -1;
+        }
+
+        if ((rc = qemuMonitorJSONGetOneBlockStatsNodeInfo(dev, hash)) < 0)
+            return -1;
+
+        if (rc > nstats)
+            nstats = rc;
+    }
+
     return nstats;
 }

diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 0de523007b..bcf5caa9a4 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1615,6 +1615,8 @@ testQemuMonitorJSONqemuMonitorJSONGetAllBlockStatsInfo(const void *opaque)

     if (qemuMonitorTestAddItem(test, "query-blockstats", reply) < 0)
         return -1;
+    if (qemuMonitorTestAddItem(test, "query-blockstats", reply) < 0)
+        return -1;

 #define CHECK0FULL(var, value, varformat, valformat) \
     if (stats->var != value) { \
-- 
2.31.1

Re: [PATCH for 7.10] qemu: monitor: Fix usage of 'query-blockstats'
Posted by Ján Tomko 2 years, 4 months ago
On a Thursday in 2021, Peter Krempa wrote:
>Commit bc24810c2cab modified code querying blockstats to use the
>'query-nodes' parameter so that we can fetch stats also for images which
>are not attached to a frontend such as block copy and backup scratch
>images.
>
>Unfortunately that broke the old blockstats because if 'query-nodes' is
>enabled qemu doesn't output the 'qdev' parameter which our code used for
>matching to the disk and also qemu neglects to populate the frontend
>stats at all so we can't even switch to using nodenema for matching.

s/nodenema/nodename/
or did you mean nodenemark?

>
>To fix this we need to do two calls, one with 'query-nodes' disabled
>using the old logic to populate everything and then an aditional one

*additional

>which populates all the remaining images.
>
>Closes: https://gitlab.com/libvirt/libvirt/-/issues/246
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/qemu/qemu_monitor_json.c | 52 +++++++++++++++++++++++++++++++++---
> tests/qemumonitorjsontest.c  |  2 ++
> 2 files changed, 50 insertions(+), 4 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
Re: [PATCH for 7.10] qemu: monitor: Fix usage of 'query-blockstats'
Posted by Erik Skultety 2 years, 4 months ago
On Thu, Nov 25, 2021 at 01:19:57PM +0100, Peter Krempa wrote:
> Commit bc24810c2cab modified code querying blockstats to use the
> 'query-nodes' parameter so that we can fetch stats also for images which
> are not attached to a frontend such as block copy and backup scratch
> images.
> 
> Unfortunately that broke the old blockstats because if 'query-nodes' is
> enabled qemu doesn't output the 'qdev' parameter which our code used for
> matching to the disk and also qemu neglects to populate the frontend
> stats at all so we can't even switch to using nodenema for matching.
> 
> To fix this we need to do two calls, one with 'query-nodes' disabled
> using the old logic to populate everything and then an aditional one
> which populates all the remaining images.
> 
> Closes: https://gitlab.com/libvirt/libvirt/-/issues/246
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---

Tested-by: Erik Skultety <eskultet@redhat.com>