Re: [libvirt] [RFC PATCH 3/5] Implement virDomainBlockIoThrottle for the qemu driver

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&gt; 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&gt; > >> --- > >>  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&gt; > > 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&gt; IBM Linux Technology Center