[libvirt] [PATCH 0/2] blkiotune tweaks
As mentioned here: https://www.redhat.com/archives/libvir-list/2011-November/msg01720.html Eric Blake (2): conf: reject duplicate paths in device weights qemu: amend existing table of device weights src/conf/domain_conf.c | 10 ++++++ src/qemu/qemu_driver.c | 80 +++++++++++++++++++++++++++++++++++------------ tools/virsh.pod | 4 ++- 3 files changed, 72 insertions(+), 22 deletions(-) -- 1.7.7.3
The next patch will make it possible to have virDomainSetBlkioParameters leave device weights unchanged if they are not mentioned in the incoming string, but this only works if the list of block weights does not allow duplicate paths. Technically, a user can still confuse libvirt by passing alternate spellings that resolve to the same device, but it is not worth worrying about working around that kind of abuse. * src/conf/domain_conf.c (virDomainDefParseXML): Require unique paths. --- src/conf/domain_conf.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d50a5c7..8221c28 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6908,10 +6908,20 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, goto no_memory; for (i = 0; i < n; i++) { + int j; if (virDomainBlkioDeviceWeightParseXML(nodes[i], &def->blkio.devices[i]) < 0) goto error; def->blkio.ndevices++; + for (j = 0; j < i; j++) { + if (STREQ(def->blkio.devices[j].path, + def->blkio.devices[i].path)) { + virDomainReportError(VIR_ERR_XML_ERROR, + _("duplicate device weight path '%s'"), + def->blkio.devices[i].path); + goto error; + } + } } VIR_FREE(nodes); -- 1.7.7.3
On Tue, Nov 29, 2011 at 02:11:43PM -0700, Eric Blake wrote:
The next patch will make it possible to have virDomainSetBlkioParameters leave device weights unchanged if they are not mentioned in the incoming string, but this only works if the list of block weights does not allow duplicate paths. Technically, a user can still confuse libvirt by passing alternate spellings that resolve to the same device, but it is not worth worrying about working around that kind of abuse.
* src/conf/domain_conf.c (virDomainDefParseXML): Require unique paths. --- src/conf/domain_conf.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d50a5c7..8221c28 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6908,10 +6908,20 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, goto no_memory;
for (i = 0; i < n; i++) { + int j; if (virDomainBlkioDeviceWeightParseXML(nodes[i], &def->blkio.devices[i]) < 0) goto error; def->blkio.ndevices++; + for (j = 0; j < i; j++) { + if (STREQ(def->blkio.devices[j].path, + def->blkio.devices[i].path)) { + virDomainReportError(VIR_ERR_XML_ERROR, + _("duplicate device weight path '%s'"), + def->blkio.devices[i].path); + goto error; + } + } } VIR_FREE(nodes);
-- 1.7.7.3
ACK. -- Thanks, Hu Tao
Prior to this patch, for a running dom, the commands: $ virsh blkiotune dom --device-weights /dev/sda,502,/dev/sdb,498 $ virsh blkiotune dom --device-weights /dev/sda,503 $ virsh blkiotune dom weight : 500 device_weight : /dev/sda,503 claim that /dev/sdb no longer has a non-default weight, but directly querying cgroups says otherwise: $ cat /cgroup/blkio/libvirt/qemu/dom/blkio.weight_device 8:0 503 8:16 498 After this patch, an explicit 0 is required to remove a device path from the XML, and omitting a device path that was previously specified leaves that device path untouched in the XML, to match cgroups behavior. * src/qemu/qemu_driver.c (parseBlkioWeightDeviceStr): Rename... (qemuDomainParseDeviceWeightStr): ...and use correct type. (qemuDomainSetBlkioParameters): After parsing string, modify rather than replacing existing table. * tools/virsh.pod (blkiotune): Tweak wording. --- src/qemu/qemu_driver.c | 80 +++++++++++++++++++++++++++++++++++------------ tools/virsh.pod | 4 ++- 2 files changed, 62 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 105bdde..81d11c2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5887,8 +5887,8 @@ cleanup: * for example, /dev/disk/by-path/pci-0000:00:1f.2-scsi-0:0:0:0,800 */ static int -parseBlkioWeightDeviceStr(char *deviceWeightStr, - virBlkioDeviceWeightPtr *dw, int *size) +qemuDomainParseDeviceWeightStr(char *deviceWeightStr, + virBlkioDeviceWeightPtr *dw, size_t *size) { char *temp; int ndevices = 0; @@ -5965,6 +5965,41 @@ cleanup: return -1; } +/* Modify def to reflect all device weight changes described in tmp. */ +static int +qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *def, size_t *def_size, + virBlkioDeviceWeightPtr tmp, size_t tmp_size) +{ + int i, j; + virBlkioDeviceWeightPtr dw; + + for (i = 0; i < tmp_size; i++) { + bool found = false; + + dw = &tmp[i]; + for (j = 0; j < *def_size; j++) { + if (STREQ(dw->path, (*def)[j].path)) { + found = true; + (*def)[j].weight = dw->weight; + break; + } + } + if (!found) { + if (!dw->weight) + continue; + if (VIR_EXPAND_N(*def, *def_size, 1) < 0) { + virReportOOMError(); + return -1; + } + (*def)[*def_size - 1].path = dw->path; + (*def)[*def_size - 1].weight = dw->weight; + dw->path = NULL; + } + } + + return 0; +} + static int qemuDomainSetBlkioParameters(virDomainPtr dom, virTypedParameterPtr params, int nparams, @@ -6056,7 +6091,7 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, ret = -1; } } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT)) { - int ndevices; + size_t ndevices; virBlkioDeviceWeightPtr devices = NULL; if (param->type != VIR_TYPED_PARAM_STRING) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", @@ -6066,9 +6101,9 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, continue; } - if (parseBlkioWeightDeviceStr(params[i].value.s, - &devices, - &ndevices) < 0) { + if (qemuDomainParseDeviceWeightStr(params[i].value.s, + &devices, + &ndevices) < 0) { ret = -1; continue; } @@ -6088,14 +6123,16 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, ret = -1; continue; } - virBlkioDeviceWeightArrayClear(vm->def->blkio.devices, - vm->def->blkio.ndevices); - VIR_FREE(vm->def->blkio.devices); - vm->def->blkio.devices = devices; - vm->def->blkio.ndevices = ndevices; + if (qemuDomainMergeDeviceWeights(&vm->def->blkio.devices, + &vm->def->blkio.ndevices, + devices, ndevices) < 0) + ret = -1; + virBlkioDeviceWeightArrayClear(devices, ndevices); + VIR_FREE(devices); } else { qemuReportError(VIR_ERR_INVALID_ARG, - _("Parameter `%s' not supported"), param->field); + _("Parameter `%s' not supported"), + param->field); ret = -1; } } @@ -6127,7 +6164,7 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, persistentDef->blkio.weight = params[i].value.ui; } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT)) { virBlkioDeviceWeightPtr devices = NULL; - int ndevices; + size_t ndevices; if (param->type != VIR_TYPED_PARAM_STRING) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("invalid type for device_weight tunable, " @@ -6135,17 +6172,18 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, ret = -1; continue; } - if (parseBlkioWeightDeviceStr(params[i].value.s, - &devices, - &ndevices) < 0) { + if (qemuDomainParseDeviceWeightStr(params[i].value.s, + &devices, + &ndevices) < 0) { ret = -1; continue; } - virBlkioDeviceWeightArrayClear(persistentDef->blkio.devices, - persistentDef->blkio.ndevices); - VIR_FREE(persistentDef->blkio.devices); - persistentDef->blkio.devices = devices; - persistentDef->blkio.ndevices = ndevices; + if (qemuDomainMergeDeviceWeights(&vm->def->blkio.devices, + &vm->def->blkio.ndevices, + devices, ndevices) < 0) + ret = -1; + virBlkioDeviceWeightArrayClear(devices, ndevices); + VIR_FREE(devices); } else { qemuReportError(VIR_ERR_INVALID_ARG, _("Parameter `%s' not supported"), diff --git a/tools/virsh.pod b/tools/virsh.pod index d606f99..01b8538 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1055,7 +1055,9 @@ I<--weight> is in range [100, 1000]. B<device-weights> is a single string listing one or more device/weight pairs, in the format of /path/to/device,weight,/path/to/device,weight. Each weight is in the range [100, 1000], or the value 0 to remove that -device from per-device listings. +device from per-device listings. Only the devices listed in the string +are modified; any existing per-device weights for other devices remain +unchanged. If I<--live> is specified, affect a running guest. If I<--config> is specified, affect the next boot of a persistent guest. -- 1.7.7.3
On Tue, Nov 29, 2011 at 02:11:44PM -0700, Eric Blake wrote:
Prior to this patch, for a running dom, the commands:
$ virsh blkiotune dom --device-weights /dev/sda,502,/dev/sdb,498 $ virsh blkiotune dom --device-weights /dev/sda,503 $ virsh blkiotune dom weight : 500 device_weight : /dev/sda,503
claim that /dev/sdb no longer has a non-default weight, but directly querying cgroups says otherwise:
$ cat /cgroup/blkio/libvirt/qemu/dom/blkio.weight_device 8:0 503 8:16 498
After this patch, an explicit 0 is required to remove a device path from the XML, and omitting a device path that was previously specified leaves that device path untouched in the XML, to match cgroups behavior.
* src/qemu/qemu_driver.c (parseBlkioWeightDeviceStr): Rename... (qemuDomainParseDeviceWeightStr): ...and use correct type. (qemuDomainSetBlkioParameters): After parsing string, modify rather than replacing existing table. * tools/virsh.pod (blkiotune): Tweak wording. --- src/qemu/qemu_driver.c | 80 +++++++++++++++++++++++++++++++++++------------ tools/virsh.pod | 4 ++- 2 files changed, 62 insertions(+), 22 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 105bdde..81d11c2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5887,8 +5887,8 @@ cleanup: * for example, /dev/disk/by-path/pci-0000:00:1f.2-scsi-0:0:0:0,800 */ static int -parseBlkioWeightDeviceStr(char *deviceWeightStr, - virBlkioDeviceWeightPtr *dw, int *size) +qemuDomainParseDeviceWeightStr(char *deviceWeightStr, + virBlkioDeviceWeightPtr *dw, size_t *size) { char *temp; int ndevices = 0; @@ -5965,6 +5965,41 @@ cleanup: return -1; }
+/* Modify def to reflect all device weight changes described in tmp. */ +static int +qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *def, size_t *def_size, + virBlkioDeviceWeightPtr tmp, size_t tmp_size) +{ + int i, j; + virBlkioDeviceWeightPtr dw; + + for (i = 0; i < tmp_size; i++) { + bool found = false; + + dw = &tmp[i]; + for (j = 0; j < *def_size; j++) { + if (STREQ(dw->path, (*def)[j].path)) { + found = true; + (*def)[j].weight = dw->weight; + break; + } + } + if (!found) { + if (!dw->weight) + continue; + if (VIR_EXPAND_N(*def, *def_size, 1) < 0) { + virReportOOMError(); + return -1; + } + (*def)[*def_size - 1].path = dw->path; + (*def)[*def_size - 1].weight = dw->weight; + dw->path = NULL; + } + } + + return 0; +} + static int qemuDomainSetBlkioParameters(virDomainPtr dom, virTypedParameterPtr params, int nparams, @@ -6056,7 +6091,7 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, ret = -1; } } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT)) { - int ndevices; + size_t ndevices; virBlkioDeviceWeightPtr devices = NULL; if (param->type != VIR_TYPED_PARAM_STRING) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", @@ -6066,9 +6101,9 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, continue; }
- if (parseBlkioWeightDeviceStr(params[i].value.s, - &devices, - &ndevices) < 0) { + if (qemuDomainParseDeviceWeightStr(params[i].value.s, + &devices, + &ndevices) < 0) { ret = -1; continue; } @@ -6088,14 +6123,16 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, ret = -1; continue; } - virBlkioDeviceWeightArrayClear(vm->def->blkio.devices, - vm->def->blkio.ndevices); - VIR_FREE(vm->def->blkio.devices); - vm->def->blkio.devices = devices; - vm->def->blkio.ndevices = ndevices; + if (qemuDomainMergeDeviceWeights(&vm->def->blkio.devices, + &vm->def->blkio.ndevices, + devices, ndevices) < 0) + ret = -1; + virBlkioDeviceWeightArrayClear(devices, ndevices); + VIR_FREE(devices); } else { qemuReportError(VIR_ERR_INVALID_ARG, - _("Parameter `%s' not supported"), param->field); + _("Parameter `%s' not supported"), + param->field); ret = -1; } } @@ -6127,7 +6164,7 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, persistentDef->blkio.weight = params[i].value.ui; } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT)) { virBlkioDeviceWeightPtr devices = NULL; - int ndevices; + size_t ndevices; if (param->type != VIR_TYPED_PARAM_STRING) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("invalid type for device_weight tunable, " @@ -6135,17 +6172,18 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, ret = -1; continue; } - if (parseBlkioWeightDeviceStr(params[i].value.s, - &devices, - &ndevices) < 0) { + if (qemuDomainParseDeviceWeightStr(params[i].value.s, + &devices, + &ndevices) < 0) { ret = -1; continue; } - virBlkioDeviceWeightArrayClear(persistentDef->blkio.devices, - persistentDef->blkio.ndevices); - VIR_FREE(persistentDef->blkio.devices); - persistentDef->blkio.devices = devices; - persistentDef->blkio.ndevices = ndevices; + if (qemuDomainMergeDeviceWeights(&vm->def->blkio.devices, + &vm->def->blkio.ndevices, + devices, ndevices) < 0) + ret = -1; + virBlkioDeviceWeightArrayClear(devices, ndevices); + VIR_FREE(devices); } else { qemuReportError(VIR_ERR_INVALID_ARG, _("Parameter `%s' not supported"), diff --git a/tools/virsh.pod b/tools/virsh.pod index d606f99..01b8538 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1055,7 +1055,9 @@ I<--weight> is in range [100, 1000]. B<device-weights> is a single string listing one or more device/weight pairs, in the format of /path/to/device,weight,/path/to/device,weight. Each weight is in the range [100, 1000], or the value 0 to remove that -device from per-device listings. +device from per-device listings. Only the devices listed in the string +are modified; any existing per-device weights for other devices remain +unchanged.
If I<--live> is specified, affect a running guest. If I<--config> is specified, affect the next boot of a persistent guest. -- 1.7.7.3
ACK. -- Thanks, Hu Tao
On 11/29/2011 07:37 PM, Hu Tao wrote:
On Tue, Nov 29, 2011 at 02:11:44PM -0700, Eric Blake wrote:
Prior to this patch, for a running dom, the commands:
$ virsh blkiotune dom --device-weights /dev/sda,502,/dev/sdb,498 $ virsh blkiotune dom --device-weights /dev/sda,503 $ virsh blkiotune dom weight : 500 device_weight : /dev/sda,503
claim that /dev/sdb no longer has a non-default weight, but directly querying cgroups says otherwise:
$ cat /cgroup/blkio/libvirt/qemu/dom/blkio.weight_device 8:0 503 8:16 498
After this patch, an explicit 0 is required to remove a device path from the XML, and omitting a device path that was previously specified leaves that device path untouched in the XML, to match cgroups behavior.
ACK.
Thanks; series pushed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Hu Tao