On Wed, Oct 12, 2011 at 8:39 PM, Adam Litke <agl@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
>> > On Mon, Oct 10, 2011 at 09:45:11PM +0800, Lei HH Li wrote:
>> >
>> > Summary here.
>> >
>> >>
>> >> Signed-off-by: Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx>
>> >> ---
>> >> 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@xxxxxxxxxx>
>> > IBM Linux Technology Center
>> >
>> > --
>> > libvir-list mailing list
>> > libvir-list@xxxxxxxxxx
>> > https://www.redhat.com/mailman/listinfo/libvir-list
>> >
>>
>>
>>
>> --
>> Regards,
>>
>> Zhi Yong Wu
>>
>
> --
> Adam Litke <agl@xxxxxxxxxx>
> IBM Linux Technology Center
>
>
--
Regards,
Zhi Yong Wu
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list