7:39 a.m.
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(a)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(a)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
variable to check. For the record, I still prefer using a pointer to
blkiothrottle for this.
>
> 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(a)us.ibm.com>
> IBM Linux Technology Center
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
Regards,
Zhi Yong Wu
--
Adam Litke <agl(a)us.ibm.com>
IBM Linux Technology Center