This patch adds a parameter --weight-device to virsh command blkiotune for setting/getting blkio.weight_device. --- include/libvirt/libvirt.h.in | 9 ++ src/conf/domain_conf.c | 133 ++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 15 ++++ src/libvirt_private.syms | 1 + src/qemu/qemu_cgroup.c | 22 ++++++ src/qemu/qemu_driver.c | 171 +++++++++++++++++++++++++++++++++++++++++- src/util/cgroup.c | 33 ++++++++ src/util/cgroup.h | 3 + tools/virsh.c | 31 ++++++++ tools/virsh.pod | 5 +- 10 files changed, 417 insertions(+), 6 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index da2080e..5d5bc8d 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1042,6 +1042,15 @@ char * virDomainGetSchedulerType(virDomainPtr domain, #define VIR_DOMAIN_BLKIO_WEIGHT "weight" +/** + * VIR_DOMAIN_BLKIO_WEIGHT_DEVICE: + * + * Macro for the blkio tunable weight_device: it represents the + * per device weight. + */ + +#define VIR_DOMAIN_BLKIO_WEIGHT_DEVICE "weight_device" + /* Set Blkio tunables for the domain*/ int virDomainSetBlkioParameters(virDomainPtr domain, virTypedParameterPtr params, diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 44212cf..d972a1b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -520,6 +520,101 @@ VIR_ENUM_IMPL(virDomainNumatuneMemMode, VIR_DOMAIN_NUMATUNE_MEM_LAST, #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE +/** + * virBlkioWeightDeviceToStr: + * + * This function returns a string representing device weights that is + * suitable for writing to /cgroup/blkio/blkio.weight_device, given + * a list of weight devices. + */ +int virBlkioWeightDeviceToStr(virBlkioWeightDevicePtr weightdevices, + int ndevices, + char **result) +{ + int len = 0; + int ret = -1; + int i, j; + char **weight_devices; + char *str; + + if (VIR_ALLOC_N(weight_devices, ndevices) < 0) { + goto fail_nomem1; + } + for (i = 0; i < ndevices; i++) { + int tmp; + tmp = virAsprintf(&weight_devices[i], "%d:%d %d", + weightdevices[i].major, + weightdevices[i].minor, + weightdevices[i].weight); + if (tmp < 0) { + goto fail_nomem2; + } + len += tmp + 1; /* 1 for '\n' and the trailing '\0' */ + } + + if (VIR_ALLOC_N(str, len) < 0) { + goto fail_nomem2; + } + for (i = 0; i < ndevices; i++) { + strcat(str, weight_devices[i]); + strcat(str, "\n"); + } + str[len-1] = '\0'; + + *result = str; + + ret = 0; + +fail_nomem2: + for (j = 0; j < i; j++) + VIR_FREE(weight_devices[i]); + VIR_FREE(weight_devices); +fail_nomem1: + if (ret != 0) + virReportOOMError(); + return ret; +} + +/** + * virDomainBlkioWeightDeviceParseXML + * + * this function parses a XML node: + * + * <device> + * <major>major</major> + * <minor>minor</minor> + * <weight>weight</weight> + * </device> + * + * and fills a virBlkioWeightDevice struct. + */ +static int virDomainBlkioWeightDeviceParseXML(xmlNodePtr root, + virBlkioWeightDevicePtr dw) +{ + xmlNodePtr node; + + if (!dw) + return -1; + + node = root->children; + while (node) { + if (node->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(node->name, BAD_CAST "major")) { + dw->major = atoi((const char *)xmlNodeGetContent(node)); + } else if (xmlStrEqual(node->name, BAD_CAST "minor")) { + dw->minor = atoi((const char *)xmlNodeGetContent(node)); + } else if (xmlStrEqual(node->name, BAD_CAST "weight")) { + dw->weight = atoi((const char *)xmlNodeGetContent(node)); + } + } + node = node->next; + } + + return 0; +} + + + static void virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) { @@ -6070,6 +6165,20 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, &def->blkio.weight) < 0) def->blkio.weight = 0; + n = virXPathNodeSet("./blkiotune/device", ctxt, &nodes); + if (n > 0) { + if (VIR_ALLOC_N(def->blkio.weight_devices, n) < 0) { + virReportOOMError(); + goto error; + } + + for (i = 0; i < n; i++) { + virDomainBlkioWeightDeviceParseXML(nodes[i], &def->blkio.weight_devices[i]); + } + def->blkio.ndevices = n; + VIR_FREE(nodes); + } + /* Extract other memory tunables */ if (virXPathULong("string(./memtune/hard_limit)", ctxt, &def->mem.hard_limit) < 0) @@ -9948,10 +10057,28 @@ virDomainDefFormatInternal(virDomainDefPtr def, def->mem.cur_balloon); /* add blkiotune only if there are any */ - if (def->blkio.weight) { + if (def->blkio.weight || def->blkio.weight_devices) { virBufferAsprintf(&buf, " <blkiotune>\n"); - virBufferAsprintf(&buf, " <weight>%u</weight>\n", - def->blkio.weight); + + if (def->blkio.weight) + virBufferAsprintf(&buf, " <weight>%u</weight>\n", + def->blkio.weight); + + if (def->blkio.weight_devices) { + int i; + + for (i = 0; i < def->blkio.ndevices; i++) { + virBufferAsprintf(&buf, " <device>\n"); + virBufferAsprintf(&buf, " <major>%d</major>\n", + def->blkio.weight_devices[i].major); + virBufferAsprintf(&buf, " <minor>%d</minor>\n", + def->blkio.weight_devices[i].minor); + virBufferAsprintf(&buf, " <weight>%d</weight>\n", + def->blkio.weight_devices[i].weight); + virBufferAsprintf(&buf, " </device>\n"); + } + } + virBufferAsprintf(&buf, " </blkiotune>\n"); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8382d28..e142649 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1174,6 +1174,19 @@ struct _virDomainNumatuneDef { /* Future NUMA tuning related stuff should go here. */ }; +typedef struct _virBlkioWeightDevice virBlkioWeightDevice; +typedef virBlkioWeightDevice *virBlkioWeightDevicePtr; +struct _virBlkioWeightDevice { + int major; + int minor; + int weight; +}; + +int virBlkioWeightDeviceToStr(virBlkioWeightDevicePtr deviceWeights, + int ndevices, + char **result); + + /* * Guest VM main configuration * @@ -1191,6 +1204,8 @@ struct _virDomainDef { struct { unsigned int weight; + int ndevices; + virBlkioWeightDevicePtr weight_devices; } blkio; struct { diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2a453bc..6dd2593 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -226,6 +226,7 @@ virDomainAuditVcpu; # domain_conf.h +virBlkioWeightDeviceToStr; virDiskNameToBusDeviceIndex; virDiskNameToIndex; virDomainActualNetDefFree; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 2a10bd2..eb06871 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -312,6 +312,28 @@ int qemuSetupCgroup(struct qemud_driver *driver, } } + if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) { + for (i = 0; i < vm->def->blkio.ndevices; i++) { + char *tmp = NULL; + + virAsprintf(&tmp, "%d:%d %d", + vm->def->blkio.weight_devices[i].major, + vm->def->blkio.weight_devices[i].minor, + vm->def->blkio.weight_devices[i].weight); + if (tmp) { + rc = virCgroupSetBlkioWeightDevice(cgroup, tmp); + VIR_FREE(tmp); + tmp = NULL; + if (rc != 0) { + virReportSystemError(-rc, + _("Unable to set io device weight for domain %s"), + vm->def->name); + goto cleanup; + } + } + } + } + if (vm->def->mem.hard_limit != 0 || vm->def->mem.soft_limit != 0 || vm->def->mem.swap_hard_limit != 0) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f21122d..23d96e0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -44,6 +44,7 @@ #include <sys/ioctl.h> #include <sys/un.h> #include <byteswap.h> +#include <ctype.h> #include "qemu_driver.h" @@ -111,7 +112,7 @@ # define KVM_CAP_NR_VCPUS 9 /* returns max vcpus per vm */ #endif -#define QEMU_NB_BLKIO_PARAM 1 +#define QEMU_NB_BLKIO_PARAM 2 static void processWatchdogEvent(void *data, void *opaque); @@ -5601,6 +5602,86 @@ cleanup: return ret; } +/* weightDeviceStr in the form of major:minor,weight;nmajor:minor,weight + * for example, 8:0,300;8:16,800;8:32,1000 + */ +static int parseBlkioWeightDeviceStr(const char *weightDeviceStr, virBlkioWeightDevicePtr *dw, int *size) +{ + const char *temp; + int nDevice = 0; + int i; + virBlkioWeightDevicePtr result = NULL; + + if (!dw) + return -1; + if (*dw) + return -1; + + temp = weightDeviceStr; + while (temp) { + temp = strchr(temp, ';'); + if (temp) { + temp++; + if (*temp == '\0') + break; + nDevice++; + } + } + nDevice++; + + if (VIR_ALLOC_N(result, nDevice) < 0) { + virReportOOMError(); + return -1; + } + + i = 0; + temp = weightDeviceStr; + while (temp && i < nDevice) { + const char *p = temp; + while (isdigit(*p) && ++p); + if (!p || *p != ':') + goto fail; + + result[i].major = atoi(temp); + + temp = p + 1; + if (!temp) + goto fail; + + p = temp; + while (isdigit(*p) && ++p); + if (!p || *p != ',') + goto fail; + + result[i].minor = atoi(temp); + + temp = p + 1; + if (!temp) + goto fail; + + p = temp; + while (isdigit(*p) && ++p); + if (!p || (*p != ';' && *p != '\0')) + goto fail; + + result[i].weight = atoi(temp); + + i++; + if (*p == '\0') + break; + temp = p + 1; + } + + *dw = result; + *size = i; + + return 0; + +fail: + VIR_FREE(result); + return -1; +} + static int qemuDomainSetBlkioParameters(virDomainPtr dom, virTypedParameterPtr params, int nparams, @@ -5667,10 +5748,10 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, ret = 0; if (flags & VIR_DOMAIN_AFFECT_LIVE) { for (i = 0; i < nparams; i++) { + int rc; virTypedParameterPtr param = &params[i]; if (STREQ(param->field, VIR_DOMAIN_BLKIO_WEIGHT)) { - int rc; if (param->type != VIR_TYPED_PARAM_UINT) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("invalid type for blkio weight tunable, expected a 'unsigned int'")); @@ -5691,6 +5772,43 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, _("unable to set blkio weight tunable")); ret = -1; } + } else if(STREQ(param->field, VIR_DOMAIN_BLKIO_WEIGHT_DEVICE)) { + int ndevices; + virBlkioWeightDevicePtr tmp = NULL; + if (param->type != VIR_TYPED_PARAM_STRING) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid type for blkio weight_device tunable, expected a 'char *'")); + ret = -1; + continue; + } + + if (parseBlkioWeightDeviceStr(params[i].value.s, + &tmp, + &ndevices) < 0) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid format for blkio weight_device")); + ret = -1; + continue; + } + for (i = 0; i < ndevices; i++) { + char *weight_device = NULL; + + virAsprintf(&weight_device, "%d:%d %d", + tmp[i].major, + tmp[i].minor, + tmp[i].weight); + if (weight_device) { + rc = virCgroupSetBlkioWeightDevice(group, weight_device); + VIR_FREE(weight_device); + weight_device = NULL; + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to set blkio weight_device tunable")); + ret = -1; + break; + } + } + } } else { qemuReportError(VIR_ERR_INVALID_ARG, _("Parameter `%s' not supported"), param->field); @@ -5720,6 +5838,21 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, } persistentDef->blkio.weight = params[i].value.ui; + } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_WEIGHT_DEVICE)) { + virBlkioWeightDevicePtr tmp = NULL; + int ndevices; + if (parseBlkioWeightDeviceStr(params[i].value.s, + &tmp, + &ndevices) < 0) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("invalid device weight format: %s"), + params[i].value.s); + ret = -1; + continue; + } + VIR_FREE(persistentDef->blkio.weight_devices); + persistentDef->blkio.weight_devices = tmp; + persistentDef->blkio.ndevices = ndevices; } else { qemuReportError(VIR_ERR_INVALID_ARG, _("Parameter `%s' not supported"), param->field); @@ -5746,6 +5879,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, { struct qemud_driver *driver = dom->conn->privateData; int i; + char *weight_device; virCgroupPtr group = NULL; virDomainObjPtr vm = NULL; virDomainDefPtr persistentDef = NULL; @@ -5839,6 +5973,23 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, } param->value.ui = val; break; + case 1: /* blkio.weight_device */ + { + param->type = VIR_TYPED_PARAM_STRING; + rc = virCgroupGetBlkioWeightDevice(group, &weight_device); + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to get blkio weight_device")); + goto cleanup; + } + if (virStrcpyStatic(param->field, VIR_DOMAIN_BLKIO_WEIGHT_DEVICE) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Field blkio weight_device too long for destination")); + goto cleanup; + } + param->value.s = weight_device; + break; + } default: break; @@ -5862,6 +6013,22 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, param->value.ui = persistentDef->blkio.weight; break; + case 1: /* blkio.weight_device */ + { + if (virBlkioWeightDeviceToStr(persistentDef->blkio.weight_devices, + persistentDef->blkio.ndevices, + &param->value.s) < 0) + goto cleanup; + param->type = VIR_TYPED_PARAM_STRING; + if (virStrcpyStatic(param->field, VIR_DOMAIN_BLKIO_WEIGHT_DEVICE) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Field blkio weight_device too long for destination")); + goto cleanup; + } + + break; + } + default: break; /* should not hit here */ diff --git a/src/util/cgroup.c b/src/util/cgroup.c index f3ec80f..e29c964 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -977,6 +977,39 @@ int virCgroupGetBlkioWeight(virCgroupPtr group, unsigned int *weight) } /** + * virCgroupSetBlkioWeightDevice: + * + * @group: The cgroup to change io weight device for + * @weight_device: The Weight device for this cgroup + * + * Returns: 0 on success + */ +int virCgroupSetBlkioWeightDevice(virCgroupPtr group, + const char *weight_device) +{ + return virCgroupSetValueStr(group, + VIR_CGROUP_CONTROLLER_BLKIO, + "blkio.weight_device", + weight_device); +} + +/** + * virCgroupGetBlkioWeightDevice: + * + * @group: The cgroup to get weight_device for + * @weight_device: returned weight_device string + * + * Returns: 0 on success + */ +int virCgroupGetBlkioWeightDevice(virCgroupPtr group, + char **weight_device) +{ + return virCgroupGetValueStr(group, + VIR_CGROUP_CONTROLLER_BLKIO, + "blkio.weight_device", weight_device); +} + +/** * virCgroupSetMemory: * * @group: The cgroup to change memory for diff --git a/src/util/cgroup.h b/src/util/cgroup.h index d190bb3..87e196b 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -55,6 +55,9 @@ int virCgroupAddTask(virCgroupPtr group, pid_t pid); int virCgroupSetBlkioWeight(virCgroupPtr group, unsigned int weight); int virCgroupGetBlkioWeight(virCgroupPtr group, unsigned int *weight); +int virCgroupSetBlkioWeightDevice(virCgroupPtr group, const char *weight_device); +int virCgroupGetBlkioWeightDevice(virCgroupPtr group, char **weight_device); + int virCgroupSetMemory(virCgroupPtr group, unsigned long long kb); int virCgroupGetMemoryUsage(virCgroupPtr group, unsigned long *kb); diff --git a/tools/virsh.c b/tools/virsh.c index 15b9bdd..3ed8e70 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3983,6 +3983,8 @@ static const vshCmdOptDef opts_blkiotune[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"weight", VSH_OT_INT, VSH_OFLAG_NONE, N_("IO Weight in range [100, 1000]")}, + {"weight-device", VSH_OT_STRING, VSH_OFLAG_NONE, + N_("per device IO Weight, in the form of major:minor,weight")}, {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, @@ -3993,6 +3995,7 @@ static bool cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) { virDomainPtr dom; + const char *weight_device = NULL; int weight = 0; int nparams = 0; int rv = 0; @@ -4037,6 +4040,16 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) } } + rv = vshCommandOptString(cmd, "weight-device", &weight_device); + if (rv < 0) { + vshError(ctl, "%s", + _("Unable to parse string parameter")); + goto cleanup; + } + if (rv > 0) { + nparams++; + } + if (nparams == 0) { /* get the number of blkio parameters */ if (virDomainGetBlkioParameters(dom, NULL, &nparams, flags) != 0) { @@ -4084,6 +4097,10 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) vshPrint(ctl, "%-15s: %d\n", params[i].field, params[i].value.b); break; + case VIR_TYPED_PARAM_STRING: + vshPrint(ctl, "%-15s: %s\n", params[i].field, + params[i].value.s); + break; default: vshPrint(ctl, "unimplemented blkio parameter type\n"); } @@ -4104,6 +4121,20 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) sizeof(temp->field)); weight = 0; } + + if (weight_device) { + int len = strlen(weight_device) + 1; + temp->type = VIR_TYPED_PARAM_STRING; + temp->value.s = vshMalloc(ctl, len); + if (!temp->value.s) { + goto cleanup; + } + strncpy(temp->value.s, weight_device, strlen(weight_device)); + temp->value.s[len-1] = '\0'; + weight_device = NULL; + strncpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT_DEVICE, + sizeof(temp->field)); + } } if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) vshError(ctl, "%s", _("Unable to change blkio parameters")); diff --git a/tools/virsh.pod b/tools/virsh.pod index 81d7a1e..75bf24e 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -835,12 +835,15 @@ value are kilobytes (i.e. blocks of 1024 bytes). =back -=item B<blkiotune> I<domain-id> [I<--weight> B<weight>] [[I<--config>] +=item B<blkiotune> I<domain-id> [I<--weight> B<weight>] +[I<--weight-device> B<weight-device>] [[I<--config>] [I<--live>] | [I<--current>]] Display or set the blkio parameters. QEMU/KVM supports I<--weight>. I<--weight> is in range [100, 1000]. +B<weight-device> is in the format of major:minor,weight;major:minor,weight. + If I<--live> is specified, affect a running guest. If I<--config> is specified, affect the next boot of a persistent guest. If I<--current> is specified, affect the current guest state. -- 1.7.3.1

On 09/06/2011 04:44 AM, Daniel Veillard wrote: New enum value is probably okay, and new char * field to a union doesn't change the struct size, but I'm quite worried about the on-the-wire compatibility aspect of this change. If a new server sends the new enum value and a string, will the old server that does not know that enum value properly reject the rpc call, or will it cause a crash or other bad things to happen? Huh? remote_typed_param has already had an official release. The addition is to a union, but this really is a case of modifying an existing on-the-wire layout, so I hope that the fact that it was an enumerated union with a new enumeration value is what is making this safe. Also, before you push this, please install pdwtags (from the dwarves package if you use Fedora), and fix src/remote_protocol-structs so that 'make check' passes. Actually, please post that diff before pushing the patch - I want to verify that the size of remote_typed_param doesn't change; if it does change, then we have an incompatible change, and need to think of a different approach. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Tue, Sep 06, 2011 at 07:09:53AM -0600, Eric Blake wrote: If a new server sends a string parameter to an old client, you will get a fatal error on the client decoding the XDR data. This will cause libvirt to report an XDR decoding error. It (probably) isn't fatal, since we've read the entire packet off the wire the next RPC call should still work. It is still not too pleasant though since old virsh will not work with new libvirtd IIUC, so I don't think we can do this without getting a better compat story here which does not require changing existing apps like virsh. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|