[libvirt] [PATCH 0/2] bulk api: implement block group
This patchset implement the block group for bulk stats, currently only support JSON monitor. Li Wei (2): qemu: implement block group for bulk stats. virsh: add block group for bulk stats. include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 13 ++++ src/qemu/qemu_driver.c | 31 ++++++++ src/qemu/qemu_monitor.c | 23 ++++++ src/qemu/qemu_monitor.h | 5 ++ src/qemu/qemu_monitor_json.c | 170 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++ tools/virsh-domain-monitor.c | 7 ++ tools/virsh.pod | 4 +- 9 files changed, 257 insertions(+), 2 deletions(-) -- 1.9.3
This patch add the block group for bulk stats. The following typed parameter used for each block stats: block.count - number of block devices in this domain block.0.name - name of the block device block.0.rd_bytes - number of read bytes block.0.rd_operations - number of read requests block.0.rd_total_time - total time spend on cache reads in nano-seconds block.0.wr_bytes - number of write bytes block.0.wr_operations - number of write requests block.0.wr_total_time - total time spend on cache write in nano-seconds block.0.flush_operations - total flush requests block.0.flush_total_time - total time spend on cache flushing in nano-seconds Signed-off-by: Li Wei <lw@cn.fujitsu.com> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 13 ++++ src/qemu/qemu_driver.c | 31 ++++++++ src/qemu/qemu_monitor.c | 23 ++++++ src/qemu/qemu_monitor.h | 5 ++ src/qemu/qemu_monitor_json.c | 170 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++ 7 files changed, 248 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 9358314..36c4fec 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2511,6 +2511,7 @@ struct _virDomainStatsRecord { typedef enum { VIR_DOMAIN_STATS_STATE = (1 << 0), /* return domain state */ + VIR_DOMAIN_STATS_BLOCK = (1 << 1), /* return block stats */ } virDomainStatsTypes; typedef enum { diff --git a/src/libvirt.c b/src/libvirt.c index 5d8f01c..ca0d071 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21632,6 +21632,19 @@ virConnectGetAllDomainStats(virConnectPtr conn, * "state.reason" - reason for entering given state, returned as int from * virDomain*Reason enum corresponding to given state. * + * VIR_DOMAIN_STATS_BLOCK: Return block device stats. The typed parameter keys + * are in this format: + * "block.count" - number of block devices in this domain + * "block.0.name" - name of the block device + * "block.0.rd_bytes" - number of read bytes + * "block.0.rd_operations" - number of read requests + * "block.0.rd_total_time" - total time spend on cache reads in nano-seconds + * "block.0.wr_bytes" - number of write bytes + * "block.0.wr_operations" - number of write requests + * "block.0.wr_total_time" - total time spend on cache write in nano-seconds + * "block.0.flush_operations" - total flush requests + * "block.0.flush_total_time" - total time spend on cache flushing in nano-seconds + * * Using 0 for @stats returns all stats groups supported by the given * hypervisor. * diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 239a300..ef4d3be 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17221,6 +17221,36 @@ qemuDomainGetStatsState(virDomainObjPtr dom, } +static int +qemuDomainGetStatsBlock(virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int flags) +{ + int ret; + qemuDomainObjPrivatePtr priv = dom->privateData; + + /* only valid for active domain, ignore inactive ones silently */ + if (!virDomainObjIsActive(dom)) + return 0; + + if (qemuDomainObjBeginJob(qemu_driver, dom, QEMU_JOB_QUERY) < 0) + return -1; + + qemuDomainObjEnterMonitor(qemu_driver, dom); + ret = qemuMonitorDomainGetStatsBlock(priv->mon, + record, + maxparams, + flags); + qemuDomainObjExitMonitor(qemu_driver, dom); + + if (qemuDomainObjEndJob(qemu_driver, dom) < 0) + return -1; + + return ret; +} + + typedef int (*qemuDomainGetStatsFunc)(virDomainObjPtr dom, virDomainStatsRecordPtr record, @@ -17234,6 +17264,7 @@ struct qemuDomainGetStatsWorker { static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE}, + { qemuDomainGetStatsBlock, VIR_DOMAIN_STATS_BLOCK}, { NULL, 0 } }; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5b2952a..83d1dc3 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4071,3 +4071,26 @@ qemuMonitorRTCResetReinjection(qemuMonitorPtr mon) return qemuMonitorJSONRTCResetReinjection(mon); } + +int +qemuMonitorDomainGetStatsBlock(qemuMonitorPtr mon, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int flags) +{ + VIR_DEBUG("mon=%p", mon); + + if (!mon) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + } + + if (!mon->json) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("JSON monitor is required")); + return -1; + } + + return qemuMonitorJSONDomainGetStatsBlock(mon, record, maxparams, flags); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 4fd6f01..6f77ecc 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -792,6 +792,11 @@ int qemuMonitorGetGuestCPU(qemuMonitorPtr mon, int qemuMonitorRTCResetReinjection(qemuMonitorPtr mon); +int +qemuMonitorDomainGetStatsBlock(qemuMonitorPtr mon, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int flags); /** * When running two dd process and using <> redirection, we need a * shell that will not truncate files. These two strings serve that diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 62e7d5d..282635a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5872,3 +5872,173 @@ qemuMonitorJSONRTCResetReinjection(qemuMonitorPtr mon) virJSONValueFree(reply); return ret; } + +int +qemuMonitorJSONDomainGetStatsBlock(qemuMonitorPtr mon, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED) +{ + int ret, i, nblocks = 0; + virJSONValuePtr cmd, devices; + virJSONValuePtr reply = NULL; + char field[VIR_TYPED_PARAM_FIELD_LENGTH]; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-blockstats", NULL))) + return -1; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + if (ret < 0) + goto cleanup; + + ret = -1; + + devices = virJSONValueObjectGet(reply, "return"); + if (!devices || devices->type != VIR_JSON_TYPE_ARRAY) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("blockstats reply was missing device list")); + goto cleanup; + } + + if (virTypedParamsAddInt(&record->params, + &record->nparams, + maxparams, + "block.count", + virJSONValueArraySize(devices)) < 0) + goto cleanup; + + for (i = 0; i < virJSONValueArraySize(devices); i++) { + virJSONValuePtr dev = virJSONValueArrayGet(devices, i); + virJSONValuePtr stats; + long long int llvalue; + const char *block_name; + + if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("blockstats device entry was not in expected format")); + goto cleanup; + } + + if ((block_name = virJSONValueObjectGetString(dev, "device")) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("blockstats device entry was not in expected format")); + goto cleanup; + } + + if ((stats = virJSONValueObjectGet(dev, "stats")) == NULL || + stats->type != VIR_JSON_TYPE_OBJECT) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("blockstats device entry was not in expected format")); + goto cleanup; + } + + snprintf(field, VIR_TYPED_PARAM_FIELD_LENGTH, "block.%d.name", nblocks); + if (virTypedParamsAddString(&record->params, + &record->nparams, + maxparams, + field, + block_name) < 0) + goto cleanup; + + if (virJSONValueObjectGetNumberLong(stats, "rd_bytes", &llvalue) < 0) + goto cleanup; + + snprintf(field, VIR_TYPED_PARAM_FIELD_LENGTH, "block.%d.rd_bytes", nblocks); + if (virTypedParamsAddLLong(&record->params, + &record->nparams, + maxparams, + field, + llvalue) < 0) + goto cleanup; + + if (virJSONValueObjectGetNumberLong(stats, "rd_operations", &llvalue) < 0) + goto cleanup; + + snprintf(field, VIR_TYPED_PARAM_FIELD_LENGTH, "block.%d.rd_operations", nblocks); + if (virTypedParamsAddLLong(&record->params, + &record->nparams, + maxparams, + field, + llvalue) < 0) + goto cleanup; + + if (virJSONValueObjectGetNumberLong(stats, "rd_total_time_ns", &llvalue) < 0) + goto cleanup; + + snprintf(field, VIR_TYPED_PARAM_FIELD_LENGTH, "block.%d.rd_total_time", nblocks); + if (virTypedParamsAddLLong(&record->params, + &record->nparams, + maxparams, + field, + llvalue) < 0) + goto cleanup; + + if (virJSONValueObjectGetNumberLong(stats, "wr_bytes", &llvalue) < 0) + goto cleanup; + + snprintf(field, VIR_TYPED_PARAM_FIELD_LENGTH, "block.%d.wr_bytes", nblocks); + if (virTypedParamsAddLLong(&record->params, + &record->nparams, + maxparams, + field, + llvalue) < 0) + goto cleanup; + + if (virJSONValueObjectGetNumberLong(stats, "wr_operations", &llvalue) < 0) + goto cleanup; + + snprintf(field, VIR_TYPED_PARAM_FIELD_LENGTH, "block.%d.wr_operations", nblocks); + if (virTypedParamsAddLLong(&record->params, + &record->nparams, + maxparams, + field, + llvalue) < 0) + goto cleanup; + + if (virJSONValueObjectGetNumberLong(stats, "wr_total_time_ns", &llvalue) < 0) + goto cleanup; + + snprintf(field, VIR_TYPED_PARAM_FIELD_LENGTH, "block.%d.wr_total_time", nblocks); + if (virTypedParamsAddLLong(&record->params, + &record->nparams, + maxparams, + field, + llvalue) < 0) + goto cleanup; + + if (virJSONValueObjectGetNumberLong(stats, "flush_operations", &llvalue) < 0) + goto cleanup; + + snprintf(field, VIR_TYPED_PARAM_FIELD_LENGTH, "block.%d.flush_operations", nblocks); + if (virTypedParamsAddLLong(&record->params, + &record->nparams, + maxparams, + field, + llvalue) < 0) + goto cleanup; + + if (virJSONValueObjectGetNumberLong(stats, "flush_total_time_ns", &llvalue) < 0) + goto cleanup; + + snprintf(field, VIR_TYPED_PARAM_FIELD_LENGTH, "block.%d.flush_total_time", nblocks); + if (virTypedParamsAddLLong(&record->params, + &record->nparams, + maxparams, + field, + llvalue) < 0) + goto cleanup; + + nblocks++; + } + + ret = 0; + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + + return ret; +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index d8c9308..a763c82 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -439,4 +439,9 @@ int qemuMonitorJSONGetGuestCPU(qemuMonitorPtr mon, virCPUDataPtr *data); int qemuMonitorJSONRTCResetReinjection(qemuMonitorPtr mon); + +int qemuMonitorJSONDomainGetStatsBlock(qemuMonitorPtr mon, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int flags); #endif /* QEMU_MONITOR_JSON_H */ -- 1.9.3
On 08/29/2014 01:56 AM, Li Wei wrote:
This patch add the block group for bulk stats. The following typed parameter used for each block stats: block.count - number of block devices in this domain block.0.name - name of the block device block.0.rd_bytes - number of read bytes block.0.rd_operations - number of read requests block.0.rd_total_time - total time spend on cache reads in nano-seconds block.0.wr_bytes - number of write bytes block.0.wr_operations - number of write requests block.0.wr_total_time - total time spend on cache write in nano-seconds block.0.flush_operations - total flush requests block.0.flush_total_time - total time spend on cache flushing in nano-seconds
Signed-off-by: Li Wei <lw@cn.fujitsu.com> ---
+++ b/src/libvirt.c @@ -21632,6 +21632,19 @@ virConnectGetAllDomainStats(virConnectPtr conn, * "state.reason" - reason for entering given state, returned as int from * virDomain*Reason enum corresponding to given state. * + * VIR_DOMAIN_STATS_BLOCK: Return block device stats. The typed parameter keys + * are in this format: + * "block.count" - number of block devices in this domain + * "block.0.name" - name of the block device + * "block.0.rd_bytes" - number of read bytes + * "block.0.rd_operations" - number of read requests + * "block.0.rd_total_time" - total time spend on cache reads in nano-seconds + * "block.0.wr_bytes" - number of write bytes + * "block.0.wr_operations" - number of write requests + * "block.0.wr_total_time" - total time spend on cache write in nano-seconds + * "block.0.flush_operations" - total flush requests + * "block.0.flush_total_time" - total time spend on cache flushing in nano-seconds
s/nano-seconds/nanoseconds/ Missing expected units (such as unsigned long long) Looks like Francesco has taken this patch and modified it into a newer version.
+static int +qemuDomainGetStatsBlock(virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int flags) +{ + int ret; + qemuDomainObjPrivatePtr priv = dom->privateData; + + /* only valid for active domain, ignore inactive ones silently */ + if (!virDomainObjIsActive(dom)) + return 0; + + if (qemuDomainObjBeginJob(qemu_driver, dom, QEMU_JOB_QUERY) < 0) + return -1; + + qemuDomainObjEnterMonitor(qemu_driver, dom);
Data race. You cannot safely call qemuDomainObjEnterMonitor unless you have checked that the domain is active _after_ qemuDomainObjBeginJob. Checking for an active domain prior to beginning the job doesn't hurt, but then you have to repeat the check, so it is easier to just swap the two instead of checking twice. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/03/2014 07:19 AM, Eric Blake wrote:
On 08/29/2014 01:56 AM, Li Wei wrote:
This patch add the block group for bulk stats. The following typed parameter used for each block stats: block.count - number of block devices in this domain block.0.name - name of the block device block.0.rd_bytes - number of read bytes block.0.rd_operations - number of read requests block.0.rd_total_time - total time spend on cache reads in nano-seconds block.0.wr_bytes - number of write bytes block.0.wr_operations - number of write requests block.0.wr_total_time - total time spend on cache write in nano-seconds block.0.flush_operations - total flush requests block.0.flush_total_time - total time spend on cache flushing in nano-seconds
Signed-off-by: Li Wei <lw@cn.fujitsu.com> ---
+++ b/src/libvirt.c @@ -21632,6 +21632,19 @@ virConnectGetAllDomainStats(virConnectPtr conn, * "state.reason" - reason for entering given state, returned as int from * virDomain*Reason enum corresponding to given state. * + * VIR_DOMAIN_STATS_BLOCK: Return block device stats. The typed parameter keys + * are in this format: + * "block.count" - number of block devices in this domain + * "block.0.name" - name of the block device + * "block.0.rd_bytes" - number of read bytes + * "block.0.rd_operations" - number of read requests + * "block.0.rd_total_time" - total time spend on cache reads in nano-seconds + * "block.0.wr_bytes" - number of write bytes + * "block.0.wr_operations" - number of write requests + * "block.0.wr_total_time" - total time spend on cache write in nano-seconds + * "block.0.flush_operations" - total flush requests + * "block.0.flush_total_time" - total time spend on cache flushing in nano-seconds
s/nano-seconds/nanoseconds/ Missing expected units (such as unsigned long long)
Looks like Francesco has taken this patch and modified it into a newer version.
Thanks for telling me this, I'll discard this patch and focus on reviewing Francesco's one. Thanks.
+static int +qemuDomainGetStatsBlock(virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int flags) +{ + int ret; + qemuDomainObjPrivatePtr priv = dom->privateData; + + /* only valid for active domain, ignore inactive ones silently */ + if (!virDomainObjIsActive(dom)) + return 0; + + if (qemuDomainObjBeginJob(qemu_driver, dom, QEMU_JOB_QUERY) < 0) + return -1; + + qemuDomainObjEnterMonitor(qemu_driver, dom);
Data race. You cannot safely call qemuDomainObjEnterMonitor unless you have checked that the domain is active _after_ qemuDomainObjBeginJob. Checking for an active domain prior to beginning the job doesn't hurt, but then you have to repeat the check, so it is easier to just swap the two instead of checking twice.
On 09/03/14 01:19, Eric Blake wrote:
On 08/29/2014 01:56 AM, Li Wei wrote:
This patch add the block group for bulk stats. The following typed parameter used for each block stats: block.count - number of block devices in this domain block.0.name - name of the block device block.0.rd_bytes - number of read bytes block.0.rd_operations - number of read requests block.0.rd_total_time - total time spend on cache reads in nano-seconds block.0.wr_bytes - number of write bytes block.0.wr_operations - number of write requests block.0.wr_total_time - total time spend on cache write in nano-seconds block.0.flush_operations - total flush requests block.0.flush_total_time - total time spend on cache flushing in nano-seconds
Signed-off-by: Li Wei <lw@cn.fujitsu.com> ---
+++ b/src/libvirt.c @@ -21632,6 +21632,19 @@ virConnectGetAllDomainStats(virConnectPtr conn, * "state.reason" - reason for entering given state, returned as int from * virDomain*Reason enum corresponding to given state. * + * VIR_DOMAIN_STATS_BLOCK: Return block device stats. The typed parameter keys + * are in this format: + * "block.count" - number of block devices in this domain + * "block.0.name" - name of the block device + * "block.0.rd_bytes" - number of read bytes + * "block.0.rd_operations" - number of read requests + * "block.0.rd_total_time" - total time spend on cache reads in nano-seconds + * "block.0.wr_bytes" - number of write bytes + * "block.0.wr_operations" - number of write requests + * "block.0.wr_total_time" - total time spend on cache write in nano-seconds + * "block.0.flush_operations" - total flush requests + * "block.0.flush_total_time" - total time spend on cache flushing in nano-seconds
s/nano-seconds/nanoseconds/ Missing expected units (such as unsigned long long)
Looks like Francesco has taken this patch and modified it into a newer version.
+static int +qemuDomainGetStatsBlock(virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int flags) +{ + int ret; + qemuDomainObjPrivatePtr priv = dom->privateData; + + /* only valid for active domain, ignore inactive ones silently */ + if (!virDomainObjIsActive(dom)) + return 0; + + if (qemuDomainObjBeginJob(qemu_driver, dom, QEMU_JOB_QUERY) < 0) + return -1; + + qemuDomainObjEnterMonitor(qemu_driver, dom);
Data race. You cannot safely call qemuDomainObjEnterMonitor unless you have checked that the domain is active _after_ qemuDomainObjBeginJob. Checking for an active domain prior to beginning the job doesn't hurt, but then you have to repeat the check, so it is easier to just swap the two instead of checking twice.
Also we should have a look at Francesco's series who implements the block stats too. Peter
Add "--block" option to "domstats" command for querying block stats. Signed-off-by: Li Wei <lw@cn.fujitsu.com> --- tools/virsh-domain-monitor.c | 7 +++++++ tools/virsh.pod | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 055d8d2..67efd61 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1972,6 +1972,10 @@ static const vshCmdOptDef opts_domstats[] = { .type = VSH_OT_BOOL, .help = N_("report domain state"), }, + {.name = "block", + .type = VSH_OT_BOOL, + .help = N_("report block device stats"), + }, {.name = "list-active", .type = VSH_OT_BOOL, .help = N_("list only active domains"), @@ -2063,6 +2067,9 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "state")) stats |= VIR_DOMAIN_STATS_STATE; + if (vshCommandOptBool(cmd, "block")) + stats |= VIR_DOMAIN_STATS_BLOCK; + if (vshCommandOptBool(cmd, "list-active")) flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE; diff --git a/tools/virsh.pod b/tools/virsh.pod index ea9267e..7d57f6b 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -813,7 +813,7 @@ that require a block device name (such as I<domblkinfo> or I<snapshot-create> for disk snapshots) will accept either target or unique source names printed by this command. -=item B<domstats> [I<--raw>] [I<--enforce>] [I<--state>] +=item B<domstats> [I<--raw>] [I<--enforce>] [I<--state>] [I<--block>] [[I<--list-active>] [I<--list-inactive>] [I<--list-persistent>] [I<--list-transient>] [I<--list-running>] [I<--list-paused>] [I<--list-shutoff>] [I<--list-other>]] | [I<domain> ...] @@ -831,7 +831,7 @@ behavior use the I<--raw> flag. The individual statistics groups are selectable via specific flags. By default all supported statistics groups are returned. Supported -statistics groups flags are: I<--state>. +statistics groups flags are: I<--state>, I<--block>. Selecting a specific statistics groups doesn't guarantee that the daemon supports the selected group of stats. Flag I<--enforce> -- 1.9.3
On 08/29/2014 01:56 AM, Li Wei wrote:
Add "--block" option to "domstats" command for querying block stats.
Signed-off-by: Li Wei <lw@cn.fujitsu.com> --- tools/virsh-domain-monitor.c | 7 +++++++ tools/virsh.pod | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-)
ACK; but depends on the API change going in first. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Li Wei -
Peter Krempa