On Wed, Oct 12, 2011 at 8:39 PM, Adam Litke <agl@us.ibm.com> wrote:
On Wed, Oct 12, 2011 at 03:02:12PM +0800, Zhi Yong Wu wrote:
On Tue, Oct 11, 2011 at 11:19 PM, Adam Litke <agl@us.ibm.com> wrote:
On Mon, Oct 10, 2011 at 09:45:11PM +0800, Lei HH Li wrote:
Summary here.
Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 35 ++++++++++++++ src/qemu/qemu_driver.c | 54 +++++++++++++++++++++ src/qemu/qemu_migration.c | 16 +++--- src/qemu/qemu_monitor.c | 19 +++++++ src/qemu/qemu_monitor.h | 6 ++ src/qemu/qemu_monitor_json.c | 107 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++ src/qemu/qemu_monitor_text.c | 61 ++++++++++++++++++++++++ src/qemu/qemu_monitor_text.h | 6 ++ 9 files changed, 302 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cf99f89..c4d2938 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1728,6 +1728,41 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, } }
+ /*block I/O throttling*/ + if (disk->blkiothrottle.bps || disk->blkiothrottle.bps_rd + || disk->blkiothrottle.bps_wr || disk->blkiothrottle.iops + || disk->blkiothrottle.iops_rd || disk->blkiothrottle.iops_wr) {
The above suggests that you should dynamically allocate the blkiothrottle struct. Then you could reduce this check to: If the structure is dynamically allocated, it will be easy to leak memory although the checking is reduced.
Not using dynamic allocation because it is harder to do correctly is probably not the best reasoning. There is a virDomainDiskDefFree() function to help free dynamic memory in the disk definition. Anyway, there are also other ways to clean this up. For example, you could add another field to disk->blkiothrottle (.enabled?) to indicate whether throttling is active. Then you only have one Goot idea. variable to check. For the record, I still prefer using a pointer to blkiothrottle for this. I only express my opinion.:) I prefer using the structure with .enabled, not pointer. But the final implemetation depends on you and Li Lei.
if (disk->blkiothrottle) {
+ if (disk->blkiothrottle.bps) { + virBufferAsprintf(&opt, ",bps=%llu", + disk->blkiothrottle.bps); + } + + if (disk->blkiothrottle.bps_rd) { + virBufferAsprintf(&opt, ",bps_wr=%llu", + disk->blkiothrottle.bps_rd); + } + + if (disk->blkiothrottle.bps_wr) { + virBufferAsprintf(&opt, ",bps_wr=%llu", + disk->blkiothrottle.bps_wr); + } + + if (disk->blkiothrottle.iops) { + virBufferAsprintf(&opt, ",iops=%llu", + disk->blkiothrottle.iops); + } + + if (disk->blkiothrottle.iops_rd) { + virBufferAsprintf(&opt, ",iops_rd=%llu", + disk->blkiothrottle.iops_rd); + } + + if (disk->blkiothrottle.iops_wr) { + virBufferAsprintf(&opt, ",iops_wr=%llu", + disk->blkiothrottle.iops_wr); + } + } + if (virBufferError(&opt)) { virReportOOMError(); goto error; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5588d93..bbee9a3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10449,6 +10449,59 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, return ret; }
+static int +qemuDomainBlockIoThrottle(virDomainPtr dom, + const char *disk, + virDomainBlockIoThrottleInfoPtr info, + virDomainBlockIoThrottleInfoPtr reply, + unsigned int flags) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + qemuDomainObjPrivatePtr priv; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + const char *device = NULL; + int ret = -1; + + qemuDriverLock(driver); + virUUIDFormat(dom->uuid, uuidstr); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + } + + device = qemuDiskPathToAlias(vm, disk); + if (!device) { + goto cleanup; + } + + if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + qemuDomainObjEnterMonitorWithDriver(driver, vm); + priv = vm->privateData; + ret = qemuMonitorBlockIoThrottle(priv->mon, device, info, reply, flags); + qemuDomainObjExitMonitorWithDriver(driver, vm); + if (qemuDomainObjEndJob(driver, vm) == 0) { + vm = NULL; + goto cleanup; + } + +cleanup: + VIR_FREE(device); + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + static virDriver qemuDriver = { .no = VIR_DRV_QEMU, .name = "QEMU", @@ -10589,6 +10642,7 @@ static virDriver qemuDriver = { .domainGetBlockJobInfo = qemuDomainGetBlockJobInfo, /* 0.9.4 */ .domainBlockJobSetSpeed = qemuDomainBlockJobSetSpeed, /* 0.9.4 */ .domainBlockPull = qemuDomainBlockPull, /* 0.9.4 */ + .domainBlockIoThrottle = qemuDomainBlockIoThrottle, /* 0.9.4 */ };
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4516231..b932ef5 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1394,9 +1394,9 @@ struct _qemuMigrationSpec {
#define TUNNEL_SEND_BUF_SIZE 65536
-typedef struct _qemuMigrationIOThread qemuMigrationIOThread; -typedef qemuMigrationIOThread *qemuMigrationIOThreadPtr; -struct _qemuMigrationIOThread { +typedef struct _qemuMigrationIoThread qemuMigrationIoThread; +typedef qemuMigrationIoThread *qemuMigrationIoThreadPtr; +struct _qemuMigrationIoThread { virThread thread; virStreamPtr st; int sock;
Why the above name change? It seems a bit superfluous and it's causing a lot of unnecessary noise in this patch.
Yeah, it is unrelative.
@@ -1405,7 +1405,7 @@ struct _qemuMigrationIOThread {
static void qemuMigrationIOFunc(void *arg) { - qemuMigrationIOThreadPtr data = arg; + qemuMigrationIoThreadPtr data = arg; char *buffer; int nbytes = TUNNEL_SEND_BUF_SIZE;
@@ -1447,11 +1447,11 @@ error: }
-static qemuMigrationIOThreadPtr +static qemuMigrationIoThreadPtr qemuMigrationStartTunnel(virStreamPtr st, int sock) { - qemuMigrationIOThreadPtr io; + qemuMigrationIoThreadPtr io;
if (VIR_ALLOC(io) < 0) { virReportOOMError(); @@ -1474,7 +1474,7 @@ qemuMigrationStartTunnel(virStreamPtr st, }
static int -qemuMigrationStopTunnel(qemuMigrationIOThreadPtr io) +qemuMigrationStopTunnel(qemuMigrationIoThreadPtr io) { int rv = -1; virThreadJoin(&io->thread); @@ -1508,7 +1508,7 @@ qemuMigrationRun(struct qemud_driver *driver, unsigned int migrate_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; qemuDomainObjPrivatePtr priv = vm->privateData; qemuMigrationCookiePtr mig = NULL; - qemuMigrationIOThreadPtr iothread = NULL; + qemuMigrationIoThreadPtr iothread = NULL; int fd = -1; unsigned long migrate_speed = resource ? resource : priv->migMaxBandwidth;
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index c9dd69e..8995517 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2581,6 +2581,25 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon, return ret; }
+int qemuMonitorBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoThrottleInfoPtr info, + virDomainBlockIoThrottleInfoPtr reply, + unsigned int flags) +{ + int ret; + + VIR_DEBUG("mon=%p, device=%p, info=%p, reply=%p, flags=%x", + mon, device, info, reply, flags); + + if (mon->json) { + ret = qemuMonitorJSONBlockIoThrottle(mon, device, info, reply, flags); + } else { + ret = qemuMonitorTextBlockIoThrottle(mon, device, info, reply, flags); + } + return ret; +} + int qemuMonitorVMStatusToPausedReason(const char *status) { int st; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 3ec78ad..b897a66 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -516,6 +516,12 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon, virDomainBlockJobInfoPtr info, int mode);
+int qemuMonitorBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoThrottleInfoPtr info, + virDomainBlockIoThrottleInfoPtr reply, + 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 3d383c8..4c49baf 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3242,3 +3242,110 @@ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon, virJSONValueFree(reply); return ret; } + +static int +qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, + virDomainBlockIoThrottleInfoPtr reply) +{ + virJSONValuePtr io_throttle; + + io_throttle = virJSONValueObjectGet(result, "return"); + if (!io_throttle || io_throttle->type != VIR_JSON_TYPE_OBJECT) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block_set_io_throttle reply was missing device address")); + return -1; + } + + if (virJSONValueObjectGetNumberUlong(io_throttle, "bps", &reply->bps) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block_set_io_throttle reply was missing total throughput limit")); + return -1; + } + + if (virJSONValueObjectGetNumberUlong(io_throttle, "bps_rd", &reply->bps_rd) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block_set_io_throttle reply was missing read throughput limit")); + return -1; + } + + if (virJSONValueObjectGetNumberUlong(io_throttle, "bps_wr", &reply->bps_wr) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block_set_io_throttle reply was missing write throughput limit")); + return -1; + } + + if (virJSONValueObjectGetNumberUlong(io_throttle, "iops", &reply->iops) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block_set_io_throttle reply was missing total operations limit")); + return -1; + } + + if (virJSONValueObjectGetNumberUlong(io_throttle, "iops_rd", &reply->iops_rd) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block_set_io_throttle reply was missing read operations limit")); + return -1; + } + + if (virJSONValueObjectGetNumberUlong(io_throttle, "iops_wr", &reply->iops_wr) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block_set_io_throttle reply was missing write operations limit")); + return -1; + } + + return 0; +} + +int qemuMonitorJSONBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoThrottleInfoPtr info, + virDomainBlockIoThrottleInfoPtr reply, + unsigned int flags) +{ + int ret = -1; + virJSONValuePtr cmd = NULL; + virJSONValuePtr result = NULL; + + if (flags) { + cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle", + "s:device", device, + "U:bps", info->bps, + "U:bps_rd", info->bps_rd, + "U:bps_wr", info->bps_wr, + "U:iops", info->iops, + "U:iops_rd", info->iops_rd, + "U:iops_wr", info->iops_wr, + NULL);
What happens if the user did not specify values for all of the fields in info?
+ } else { + /* + cmd = qemuMonitorJSONMakeCommand("query_block", + "s:device", device, + NULL); + */
Hmm, what code do you actually want here? If this is a TODO for this RFC,
Yeah, it is.
please include a comment explaining this.
+ } + + if (!cmd) + return -1; + + ret = qemuMonitorJSONCommand(mon, cmd, &result); + + if (ret == 0 && virJSONValueObjectHasKey(result, "error")) { + if (qemuMonitorJSONHasError(result, "DeviceNotActive")) + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("No active operation on device: %s"), device); + else if (qemuMonitorJSONHasError(result, "NotSupported")) + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("Operation is not supported for device: %s"), device); + else + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected error")); + ret = -1; + } + + if (ret == 0 && !flags) + ret = qemuMonitorJSONBlockIoThrottleInfo(result, reply); + + virJSONValueFree(cmd); + virJSONValueFree(result); + return ret; +} + diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index a638b41..f146a49 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -250,4 +250,10 @@ int qemuMonitorJSONSetLink(qemuMonitorPtr mon, const char *name, enum virDomainNetInterfaceLinkState state);
+int qemuMonitorJSONBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoThrottleInfoPtr info, + virDomainBlockIoThrottleInfoPtr reply, + unsigned int flags); + #endif /* QEMU_MONITOR_JSON_H */ diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 51e8c5c..0d4632e 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -3396,3 +3396,64 @@ cleanup: VIR_FREE(reply); return ret; } + +static int qemuMonitorTextParseBlockIoThrottle(const char *text ATTRIBUTE_UNUSED, + const char *device ATTRIBUTE_UNUSED, + virDomainBlockIoThrottleInfoPtr reply ATTRIBUTE_UNUSED) +{ + int ret = 0; + + /* neet to further parse the result*/ + + if (ret < 0) + return -1; + + return ret; +} + +int qemuMonitorTextBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoThrottleInfoPtr info, + virDomainBlockIoThrottleInfoPtr reply, + unsigned int flags) +{ + char *cmd = NULL; + char *result = NULL; + int ret = 0; + + if (flags) { + ret = virAsprintf(&cmd, "block_set_io_throttle %s %llu %llu %llu %llu %llu %llu", + device, + info->bps, + info->bps_rd, + info->bps_wr, + info->iops, + info->iops_rd, + info->iops_wr); + } else { + /* + ret = virAsprintf(&cmd, "info block"); + */ + } + + if (ret < 0) { + virReportOOMError(); + return -1; + } + + if (qemuMonitorHMPCommand(mon, cmd, &result) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot run monitor command")); + ret = -1; + goto cleanup; + } + + if (0) { + ret = qemuMonitorTextParseBlockIoThrottle(result, device, reply); + } + +cleanup: + VIR_FREE(cmd); + VIR_FREE(result); + return ret; +} diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index cc2a252..66a23ac 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -243,4 +243,10 @@ int qemuMonitorTextSetLink(qemuMonitorPtr mon, const char *name, enum virDomainNetInterfaceLinkState state);
+int qemuMonitorTextBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoThrottleInfoPtr info, + virDomainBlockIoThrottleInfoPtr reply, + unsigned int flags); + #endif /* QEMU_MONITOR_TEXT_H */ -- 1.7.1
-- Adam Litke <agl@us.ibm.com> IBM Linux Technology Center
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Regards,
Zhi Yong Wu
-- Adam Litke <agl@us.ibm.com> IBM Linux Technology Center
-- Regards, Zhi Yong Wu