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

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