On Mon, Oct 10, 2011 at 09:45:10PM +0800, Lei HH Li wrote: Patch summary please.

Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> --- daemon/remote.c | 51 ++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 53 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 24 ++++++++++++++++++- src/remote_protocol-structs | 21 +++++++++++++++- 4 files changed, 147 insertions(+), 2 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c index 245d41c..2b277b3 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1796,6 +1796,57 @@ cleanup: return rv; }

+static int +remoteDispatchDomainBlockIoThrottle(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessageHeaderPtr hdr ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_block_io_throttle_args *args, + remote_domain_block_io_throttle_ret *ret) +{ + virDomainPtr dom = NULL; + virDomainBlockIoThrottleInfo tmp; + virDomainBlockIoThrottleInfo reply; + int rv = -1; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(dom = get_nonnull_domain(priv->conn, args->dom))) + goto cleanup; + + if (args) { + tmp.bps = args->bps; + tmp.bps_rd = args->bps_rd; + tmp.bps_wr = args->bps_wr; + tmp.iops = args->iops; + tmp.iops_rd = args->iops_rd; + tmp.iops_wr = args->iops_wr; + } + + rv = virDomainBlockIoThrottle(dom, args->disk, &tmp, &reply, args->flags); + if (rv <= 0) + goto cleanup; + + ret->bps = reply.bps; + ret->bps_rd = reply.bps_rd; + ret->bps_wr = reply.bps_wr; + ret->iops = reply.iops; + ret->iops_rd = reply.iops_rd; + ret->iops_wr = reply.iops_wr; + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (dom) + virDomainFree(dom); + return rv; +}

/*-------------------------------------------------------------*/

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 2b2f41e..5dbc793 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2136,6 +2136,58 @@ done: return rv; }

+static int remoteDomainBlockIoThrottle(virDomainPtr domain, + const char *disk, + virDomainBlockIoThrottleInfoPtr info, + virDomainBlockIoThrottleInfoPtr reply, + unsigned int flags) +{ + int rv = -1; + remote_domain_block_io_throttle_args args; + remote_domain_block_io_throttle_ret ret; + struct private_data *priv = domain->conn->privateData; + + remoteDriverLock(priv); + + memset(&args, 0, sizeof(args)); + memset(&ret, 0, sizeof(ret)); + + make_nonnull_domain(&args.dom, domain); + args.disk = (char *)disk; + args.bps = info->bps; + args.bps_rd = info->bps_rd; + args.bps_wr = info->bps_wr; + args.iops = info->iops; + args.iops_rd = info->iops_rd; + args.iops_wr = info->iops_wr; + args.flags = flags; + + if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_BLOCK_IO_THROTTLE, + (xdrproc_t) xdr_remote_domain_block_io_throttle_args, + (char *) &args, + (xdrproc_t) xdr_remote_domain_block_io_throttle_ret, + (char *) &ret) == -1) { + goto done; + } + + if (ret.bps || ret.bps_rd || ret.bps_wr + || ret.iops || ret.iops_rd || ret.iops_wr) {

This is ugly. You are detecting whether the device is throttled by scanning all attributes for a non-zero value. Why not change the return value of the API itself to give you this information. virDomainBlockIoThrottle() returns < 0 on error, 0 on success (with no throttling active), and 1 on success (with throttling information to report).

+ reply->bps = ret.bps; + reply->bps_rd = ret.bps_rd; + reply->bps_wr = ret.bps_wr; + reply->iops = ret.iops; + reply->iops_rd = ret.iops_rd; + reply->iops_wr = ret.iops_wr; + rv = 1; + } else { + rv = 0; + } + +done: + remoteDriverUnlock(priv); + return rv; +} + /*----------------------------------------------------------------------*/

static virDrvOpenStatus ATTRIBUTE_NONNULL (1) @@ -4431,6 +4483,7 @@ static virDriver remote_driver = { .domainGetBlockJobInfo = remoteDomainGetBlockJobInfo, /* 0.9.4 */ .domainBlockJobSetSpeed = remoteDomainBlockJobSetSpeed, /* 0.9.4 */ .domainBlockPull = remoteDomainBlockPull, /* 0.9.4 */ + .domainBlockIoThrottle = remoteDomainBlockIoThrottle, /* 0.9.4 */

Use the next planned version number (as of today: 0.9.8).

};

static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index c8a92fd..ab11a70 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1073,6 +1073,27 @@ struct remote_domain_block_pull_args { unsigned int flags; };

+struct remote_domain_block_io_throttle_args { + remote_nonnull_domain dom; + remote_nonnull_string disk; + unsigned hyper bps; + unsigned hyper bps_rd; + unsigned hyper bps_wr; + unsigned hyper iops; + unsigned hyper iops_rd; + unsigned hyper iops_wr; + unsigned int flags; +}; + +struct remote_domain_block_io_throttle_ret { + unsigned hyper bps; + unsigned hyper bps_rd; + unsigned hyper bps_wr; + unsigned hyper iops; + unsigned hyper iops_rd; + unsigned hyper iops_wr; +}; + /* Network calls: */

struct remote_num_of_networks_ret { @@ -2525,7 +2546,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_MIGRATE_GET_MAX_SPEED = 242, /* autogen autogen */ REMOTE_PROC_DOMAIN_BLOCK_STATS_FLAGS = 243, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_SNAPSHOT_GET_PARENT = 244, /* autogen autogen */ - REMOTE_PROC_DOMAIN_RESET = 245 /* autogen autogen */ + REMOTE_PROC_DOMAIN_RESET = 245, /* autogen autogen */ + REMOTE_PROC_DOMAIN_BLOCK_IO_THROTTLE = 246 /* skipgen skipgen */

/* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 69175cc..8f0cfcd 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -757,6 +757,25 @@ struct remote_domain_block_pull_args { uint64_t bandwidth; u_int flags; }; +struct remote_domain_block_io_throttle_args { + remote_nonnull_domain dom; + remote_nonnull_string disk; + uint64_t bps; + uint64_t bps_rd; + uint64_t bps_wr; + uint64_t iops; + uint64_t iops_rd; + uint64_t iops_wr; + u_int flags; +}; +struct remote_domain_block_io_throttle_ret { + uint64_t bps; + uint64_t bps_rd; + uint64_t bps_wr; + uint64_t iops; + uint64_t iops_rd; + uint64_t iops_wr; +}; struct remote_num_of_networks_ret { int num; }; @@ -1970,5 +1989,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_MIGRATE_GET_MAX_SPEED = 242, REMOTE_PROC_DOMAIN_BLOCK_STATS_FLAGS = 243, REMOTE_PROC_DOMAIN_SNAPSHOT_GET_PARENT = 244, - REMOTE_PROC_DOMAIN_RESET = 245, + REMOTE_PROC_DOMAIN_BLOCK_IO_THROTTLE = 245,

Oops, you removed REMOTE_PROC_DOMAIN_RESET :). Just append your new rpc ID.

}; -- 1.7.1

-- Adam Litke <agl@us.ibm.com> IBM Linux Technology Center

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 (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@us.ibm.com> IBM Linux Technology Center

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 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@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

On Mon, Oct 10, 2011 at 09:45:09PM +0800, Lei HH Li wrote:

Hi Lei.  You are missing a patch summary at the top of this email.  In your summary you want to let reviewers know what the patch is doing.  For example, this patch defines the new virDomainBlockIoThrottle API and specifies the XML schema.  Also at the top of the patch you have an opportunity to explain why you made a particular design decision.  For example, you could explain why you chose I think so:). We should explain why we create one new libvirt commands, not extending blkiotune. BTW: Can we CCed these patches to those related developers to get

On Tue, Oct 11, 2011 at 10:59 PM, Adam Litke <agl@us.ibm.com> wrote: their comments? (e.g, Daniel, Gui JianFeng, etc)

to represent the throttling inside the <disk> tag rather than alongside the <blkiotune> settings.

...

Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> ---  include/libvirt/libvirt.h.in |   22 ++++++++++++  src/conf/domain_conf.c       |   77 ++++++++++++++++++++++++++++++++++++++++++  src/conf/domain_conf.h       |   11 ++++++  src/driver.h                 |   11 ++++++  src/libvirt.c                |   66 ++++++++++++++++++++++++++++++++++++  src/libvirt_public.syms      |    1 +  src/util/xml.h               |    3 ++  7 files changed, 191 insertions(+), 0 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 07617be..f7b892d 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1573,6 +1573,28 @@ int    virDomainBlockJobSetSpeed(virDomainPtr dom, const char *path,  int           virDomainBlockPull(virDomainPtr dom, const char *path,                                   unsigned long bandwidth, unsigned int flags);

+/* + * Block I/O throttling support + */ + +typedef unsigned long long virDomainBlockIoThrottleUnits; + +typedef struct _virDomainBlockIoThrottleInfo virDomainBlockIoThrottleInfo; +struct _virDomainBlockIoThrottleInfo { +    virDomainBlockIoThrottleUnits bps; +    virDomainBlockIoThrottleUnits bps_rd; +    virDomainBlockIoThrottleUnits bps_wr; +    virDomainBlockIoThrottleUnits iops; +    virDomainBlockIoThrottleUnits iops_rd; +    virDomainBlockIoThrottleUnits iops_wr; +}; +typedef virDomainBlockIoThrottleInfo *virDomainBlockIoThrottleInfoPtr;

I don't think it is necessary to use a typedef for the unsigned long long values in the virDomainBlockIoThrottleInfo structure.  Just use unsigned long long directly.

You might also want to consider using virTypedParameter's for this structure. It would allow us to add additional fields in the future.

...

+ +int    virDomainBlockIoThrottle(virDomainPtr dom,

The libvirt project style is to place the function return value on its own line:

int virDomainBlockIoThrottle(virDomainPtr dom, ...

...

+                                const char *disk, +                                virDomainBlockIoThrottleInfoPtr info, +                                virDomainBlockIoThrottleInfoPtr reply, +                                unsigned int flags);

 /*   * NUMA support diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 944cfa9..d0ba07e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2422,6 +2422,42 @@ virDomainDiskDefParseXML(virCapsPtr caps,                  iotag = virXMLPropString(cur, "io");                  ioeventfd = virXMLPropString(cur, "ioeventfd");                  event_idx = virXMLPropString(cur, "event_idx"); +            } else if (xmlStrEqual(cur->name, BAD_CAST "iothrottle")) { +                char *io_throttle = NULL; +                io_throttle = virXMLPropString(cur, "bps"); +                if (io_throttle) { +                    def->blkiothrottle.bps = strtoull(io_throttle, NULL, 10); +                    VIR_FREE(io_throttle); +                } + +                io_throttle = virXMLPropString(cur, "bps_rd"); +                if (io_throttle) { +                    def->blkiothrottle.bps_rd = strtoull(io_throttle, NULL, 10); +                    VIR_FREE(io_throttle); +                } + +                io_throttle = virXMLPropString(cur, "bps_wr"); +                if (io_throttle) { +                    def->blkiothrottle.bps_wr = strtoull(io_throttle, NULL, 10); +                    VIR_FREE(io_throttle); +                } + +                io_throttle = virXMLPropString(cur, "iops"); +                if (io_throttle) { +                    def->blkiothrottle.iops = strtoull(io_throttle, NULL, 10); +                    VIR_FREE(io_throttle); +                } + +                io_throttle = virXMLPropString(cur, "iops_rd"); +                if (io_throttle) { +                    def->blkiothrottle.iops_rd = strtoull(io_throttle, NULL, 10); +                    VIR_FREE(io_throttle); +                } + +                io_throttle = virXMLPropString(cur, "iops_wr"); +                if (io_throttle) { +                    def->blkiothrottle.iops_wr = strtoull(io_throttle, NULL, 10); +                }              } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {                  def->readonly = 1;              } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) { @@ -9249,6 +9285,47 @@ virDomainDiskDefFormat(virBufferPtr buf,      virBufferAsprintf(buf, "      <target dev='%s' bus='%s'/>\n",                        def->dst, bus);

+    /*disk I/O throttling*/ +    if (def->blkiothrottle.bps +        || def->blkiothrottle.bps_rd +        || def->blkiothrottle.bps_wr +        || def->blkiothrottle.iops +        || def->blkiothrottle.iops_rd +        || def->blkiothrottle.iops_wr) { +        virBufferAsprintf(buf, "      <iothrottle"); +        if (def->blkiothrottle.bps) { +            virBufferAsprintf(buf, " bps='%llu'", +                              def->blkiothrottle.bps); +        } + +        if (def->blkiothrottle.bps_rd) { +            virBufferAsprintf(buf, " bps_rd='%llu'", +                              def->blkiothrottle.bps_rd); +        } + +        if (def->blkiothrottle.bps_wr) { +            virBufferAsprintf(buf, " bps_wr='%llu'", +                              def->blkiothrottle.bps_wr); +        } + +        if (def->blkiothrottle.iops) { +            virBufferAsprintf(buf, " iops='%llu'", +                              def->blkiothrottle.iops); +        } + +        if (def->blkiothrottle.iops_rd) { +            virBufferAsprintf(buf, " iops_rd='%llu'", +                              def->blkiothrottle.iops_rd); +        } + +        if (def->blkiothrottle.iops_wr) { +            virBufferAsprintf(buf, " iops_wr='%llu'", +                              def->blkiothrottle.iops_wr); +        } + +        virBufferAsprintf(buf, "/>\n"); +    } +      if (def->bootIndex)          virBufferAsprintf(buf, "      <boot order='%d'/>\n", def->bootIndex);      if (def->readonly) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e07fd2f..b60a6de 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -283,6 +283,17 @@ struct _virDomainDiskDef {      virDomainDiskHostDefPtr hosts;      char *driverName;      char *driverType; + +    /*disk I/O throttling*/ +    struct { +        unsigned long long bps; +        unsigned long long bps_rd; +        unsigned long long bps_wr; +        unsigned long long iops; +        unsigned long long iops_rd; +        unsigned long long iops_wr; +    } blkiothrottle; +      char *serial;      int cachemode;      int error_policy;  /* enum virDomainDiskErrorPolicy */ diff --git a/src/driver.h b/src/driver.h index f85a1b1..741e196 100644 --- a/src/driver.h +++ b/src/driver.h @@ -725,6 +725,16 @@ typedef int      (*virDrvDomainBlockPull)(virDomainPtr dom, const char *path,                               unsigned long bandwidth, unsigned int flags);

+/* + * Block I/O throttling support + */ + +typedef int +    (*virDrvDomainBlockIoThrottle)(virDomainPtr dom, +                                   const char *disk, +                                   virDomainBlockIoThrottleInfoPtr info, +                                   virDomainBlockIoThrottleInfoPtr reply, +                                   unsigned int flags);

 /**   * _virDriver: @@ -881,6 +891,7 @@ struct _virDriver {      virDrvDomainGetBlockJobInfo domainGetBlockJobInfo;      virDrvDomainBlockJobSetSpeed domainBlockJobSetSpeed;      virDrvDomainBlockPull domainBlockPull; +    virDrvDomainBlockIoThrottle domainBlockIoThrottle;  };

 typedef int diff --git a/src/libvirt.c b/src/libvirt.c index 182e031..5f4514c 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -16706,3 +16706,69 @@ error:      virDispatchError(dom->conn);      return -1;  } + +/** + * virDomainBlockIoThrottle: + * @dom: pointer to domain object + * @disk: Fully-qualified disk name + * @info: specify block I/O limits in bytes + * @reply: store block I/O limits setting

This really needs to be split into two separate functions:

virDomainSetBlockIoThrottle - Set block I/O limits for a device  - Requires a connection in 'write' mode.  - Limits (info) structure passed as an input parameter virDomainGetBlockIoThrottle - Get the current block I/O limits for a device  - Works on a read-only connection.  - Current limits are written to the output parameter (info)

Consider using virTypedParameter to allow future extension:

int virDomainSetBlockIoThrottle(virDomainPtr dom,                            const char *disk,                            virTypedParameterPtr params,                            int *nparams, unsigned int flags);

int virDomainGetBlockIoThrottle(virDomainPtr dom,                            const char *disk,                            virTypedParameterPtr params,                            int *nparams, unsigned int flags);

You can use the API virDomainGetBlkioParameters as an example for all of this.

...

+ * @flags: indicate if it set or display block I/O limits info + * + * This function is mainly to enable Block I/O throttling function in libvirt. + * It is used to set or display block I/O throttling setting for specified domain. + * + * Returns 0 if the operation has started, -1 on failure. + */ +int virDomainBlockIoThrottle(virDomainPtr dom, +                             const char *disk, +                             virDomainBlockIoThrottleInfoPtr info, +                             virDomainBlockIoThrottleInfoPtr reply, +                             unsigned int flags) +{ +    virConnectPtr conn; + +    VIR_DOMAIN_DEBUG(dom, "disk=%p, info=%p, reply=%p,flags=%x", +                     disk, info, reply, flags); + +    virResetLastError(); + +    if (!VIR_IS_CONNECTED_DOMAIN (dom)) { +        virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); +        virDispatchError(NULL); +        return -1; +    } +    conn = dom->conn; + +    if (dom->conn->flags & VIR_CONNECT_RO) { +        virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); +        goto error; +    } + +    if (!disk) { +        virLibDomainError(VIR_ERR_INVALID_ARG, +                           _("disk name is NULL"));

We are using a newer error convention now.  This should be:           virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); Be sure to fix all of these in your patch.

...

+        goto error; +    } + +    if (!reply) { +        virLibDomainError(VIR_ERR_INVALID_ARG, +                           _("reply is NULL")); +        goto error; +    } + +    if (conn->driver->domainBlockIoThrottle) { +        int ret; +        ret = conn->driver->domainBlockIoThrottle(dom, disk, info, reply, flags); +        if (ret < 0) +            goto error; +        return ret; +    } + +    virLibDomainError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: +    virDispatchError(dom->conn); +    return -1; +} + diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index afea29b..0a79167 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -493,6 +493,7 @@ LIBVIRT_0.9.7 {      global:          virDomainReset;          virDomainSnapshotGetParent; +        virDomainBlockIoThrottle;  } LIBVIRT_0.9.5;

 # .... define new API here using predicted next version number .... diff --git a/src/util/xml.h b/src/util/xml.h index d30e066..14b35b2 100644 --- a/src/util/xml.h +++ b/src/util/xml.h @@ -50,6 +50,9 @@ xmlNodePtr          virXPathNode(const char *xpath,  int              virXPathNodeSet(const char *xpath,                                   xmlXPathContextPtr ctxt,                                   xmlNodePtr **list); +int     virXMLStringToULongLong (const char *stringval, +                                 unsigned long long *value); +  char *          virXMLPropString(xmlNodePtr node,                                   const char *name);

-- 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