[libvirt] [RFC PATCH 3/5] Implement virDomainBlockIoThrottle for the qemu driver
Adam Litke
agl at us.ibm.com
Tue Oct 11 15:19:53 UTC 2011
On Mon, Oct 10, 2011 at 09:45:11PM +0800, Lei HH Li wrote:
Summary here.
>
> Signed-off-by: Zhi Yong Wu <wuzhy at 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 (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.
> @@ -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,
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 at us.ibm.com>
IBM Linux Technology Center
More information about the libvir-list
mailing list