[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]