[libvirt] [PATCH] cgroup:fix bug to keep --device-weights value persistent
src/qemu/qemu_driver.c When run "virsh blkiotune dom --device-weights /dev/sda,400 --config" it couldn't be persistent after dom restart. The patch fix it. --- src/qemu/qemu_driver.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 51 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d66140b..1a53088 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5975,9 +5975,13 @@ qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *def, size_t *def_size, virReportOOMError(); return -1; } - (*def)[*def_size - 1].path = dw->path; + (*def)[*def_size - 1].path = strdup(dw->path); + if (!(*def)[*def_size - 1].path) { + virReportOOMError(); + return -1; + } + (*def)[*def_size - 1].weight = dw->weight; - dw->path = NULL; } } @@ -5985,6 +5989,46 @@ qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *def, size_t *def_size, } static int +qemuDomainiDefineDeviceWeights(virDomainDefPtr persistentDef, + virBlkioDeviceWeightPtr devices, size_t ndevices) +{ + int i; + virBlkioDeviceWeightPtr dw, result = NULL; + + if (!persistentDef->blkio.devices) { + if (VIR_ALLOC_N(result, ndevices) < 0) { + virReportOOMError(); + goto cleanup; + } + + for (i = 0; i < ndevices; i++) { + dw = &devices[i]; + result[i].path = strdup(dw->path); + if (!result[i].path) { + virReportOOMError(); + goto cleanup; + } + result[i].weight = dw->weight; + } + + persistentDef->blkio.devices = result; + } else { + if (qemuDomainMergeDeviceWeights(&persistentDef->blkio.devices, + &persistentDef->blkio.ndevices, + devices, ndevices) < 0) + return -1; + } + + persistentDef->blkio.ndevices = ndevices; + return 0; + +cleanup: + virBlkioDeviceWeightArrayClear(result, ndevices); + VIR_FREE(result); + return -1; +} + +static int qemuDomainSetBlkioParameters(virDomainPtr dom, virTypedParameterPtr params, int nparams, @@ -6116,6 +6160,11 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, ret = -1; continue; } + if (qemuDomainiDefineDeviceWeights(persistentDef, + devices, + ndevices) < 0) + ret = -1; + if (qemuDomainMergeDeviceWeights(&vm->def->blkio.devices, &vm->def->blkio.ndevices, devices, ndevices) < 0) -- 1.7.7.5
ping :) On 02/02/2012 07:57 PM, Guannan Ren wrote:
src/qemu/qemu_driver.c When run "virsh blkiotune dom --device-weights /dev/sda,400 --config" it couldn't be persistent after dom restart. The patch fix it.
--- src/qemu/qemu_driver.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 51 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d66140b..1a53088 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5975,9 +5975,13 @@ qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *def, size_t *def_size, virReportOOMError(); return -1; } - (*def)[*def_size - 1].path = dw->path; + (*def)[*def_size - 1].path = strdup(dw->path); + if (!(*def)[*def_size - 1].path) { + virReportOOMError(); + return -1; + } + (*def)[*def_size - 1].weight = dw->weight; - dw->path = NULL; } }
@@ -5985,6 +5989,46 @@ qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *def, size_t *def_size, }
static int +qemuDomainiDefineDeviceWeights(virDomainDefPtr persistentDef, + virBlkioDeviceWeightPtr devices, size_t ndevices) +{ + int i; + virBlkioDeviceWeightPtr dw, result = NULL; + + if (!persistentDef->blkio.devices) { + if (VIR_ALLOC_N(result, ndevices)< 0) { + virReportOOMError(); + goto cleanup; + } + + for (i = 0; i< ndevices; i++) { + dw =&devices[i]; + result[i].path = strdup(dw->path); + if (!result[i].path) { + virReportOOMError(); + goto cleanup; + } + result[i].weight = dw->weight; + } + + persistentDef->blkio.devices = result; + } else { + if (qemuDomainMergeDeviceWeights(&persistentDef->blkio.devices, +&persistentDef->blkio.ndevices, + devices, ndevices)< 0) + return -1; + } + + persistentDef->blkio.ndevices = ndevices; + return 0; + +cleanup: + virBlkioDeviceWeightArrayClear(result, ndevices); + VIR_FREE(result); + return -1; +} + +static int qemuDomainSetBlkioParameters(virDomainPtr dom, virTypedParameterPtr params, int nparams, @@ -6116,6 +6160,11 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, ret = -1; continue; } + if (qemuDomainiDefineDeviceWeights(persistentDef, + devices, + ndevices)< 0) + ret = -1; + if (qemuDomainMergeDeviceWeights(&vm->def->blkio.devices, &vm->def->blkio.ndevices, devices, ndevices)< 0)
On 02/02/2012 04:57 AM, Guannan Ren wrote:
src/qemu/qemu_driver.c When run "virsh blkiotune dom --device-weights /dev/sda,400 --config" it couldn't be persistent after dom restart. The patch fix it.
--- src/qemu/qemu_driver.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 51 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d66140b..1a53088 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5975,9 +5975,13 @@ qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *def, size_t *def_size, virReportOOMError(); return -1; } - (*def)[*def_size - 1].path = dw->path; + (*def)[*def_size - 1].path = strdup(dw->path);
This much indirection makes life harder. Can we declare an intermediate variable of the right type, and initialize it with &(*def)[*def_size - 1], then use mydef->path and such through the rest of the code? But that's an independent comment about the code. At any rate, the strdup here would make sense, if we were feeding the temporary array that supplied dw into two different *def pointers. But in reality, the caller is creating the temporary array twice, so this feels like it is just churn.
+ if (!(*def)[*def_size - 1].path) { + virReportOOMError(); + return -1; + } + (*def)[*def_size - 1].weight = dw->weight; - dw->path = NULL; } }
@@ -5985,6 +5989,46 @@ qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *def, size_t *def_size, }
static int +qemuDomainiDefineDeviceWeights(virDomainDefPtr persistentDef,
s/DomainiDefine/DomainDefine/ throughout the patch. Actually, this function looks like it is reinventing qemuDomainMergeDeviceWeights.
@@ -6116,6 +6160,11 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, ret = -1; continue; } + if (qemuDomainiDefineDeviceWeights(persistentDef, + devices, + ndevices) < 0) + ret = -1; +
I'm not sure we need this; instead,
if (qemuDomainMergeDeviceWeights(&vm->def->blkio.devices, &vm->def->blkio.ndevices,
Isn't the real bug that we are calling MergeDeviceWeights on vm->def instead of on persistentDef? Does this simpler patch do the trick? (I should probably split it into two pathes - the first hunk is cosmetic, the second fixes the bug). diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 3f940e8..06b30be 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -5975,35 +5975,40 @@ cleanup: return -1; } -/* Modify def to reflect all device weight changes described in tmp. */ +/* Modify dest_array to reflect all device weight changes described in + * src_array. */ static int -qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *def, size_t *def_size, - virBlkioDeviceWeightPtr tmp, size_t tmp_size) +qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *dest_array, + size_t *dest_size, + virBlkioDeviceWeightPtr src_array, + size_t src_size) { int i, j; - virBlkioDeviceWeightPtr dw; + virBlkioDeviceWeightPtr dest, src; - for (i = 0; i < tmp_size; i++) { + for (i = 0; i < src_size; i++) { bool found = false; - dw = &tmp[i]; - for (j = 0; j < *def_size; j++) { - if (STREQ(dw->path, (*def)[j].path)) { + src = &src_array[i]; + for (j = 0; j < *dest_size; j++) { + dest = &(*dest_array)[j]; + if (STREQ(src->path, dest->path)) { found = true; - (*def)[j].weight = dw->weight; + dest->weight = src->weight; break; } } if (!found) { - if (!dw->weight) + if (!src->weight) continue; - if (VIR_EXPAND_N(*def, *def_size, 1) < 0) { + if (VIR_EXPAND_N(*dest_array, *dest_size, 1) < 0) { virReportOOMError(); return -1; } - (*def)[*def_size - 1].path = dw->path; - (*def)[*def_size - 1].weight = dw->weight; - dw->path = NULL; + dest = &(*dest_array)[*dest_size - 1]; + dest->path = src->path; + dest->weight = src->weight; + src->path = NULL; } } @@ -6142,8 +6147,8 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, ret = -1; continue; } - if (qemuDomainMergeDeviceWeights(&vm->def->blkio.devices, - &vm->def->blkio.ndevices, + if (qemuDomainMergeDeviceWeights(&persistentDef->blkio.devices, + &persistentDef->blkio.ndevices, devices, ndevices) < 0) ret = -1; virBlkioDeviceWeightArrayClear(devices, ndevices); -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/08/2012 08:37 AM, Eric Blake wrote:
On 02/02/2012 04:57 AM, Guannan Ren wrote:
src/qemu/qemu_driver.c When run "virsh blkiotune dom --device-weights /dev/sda,400 --config" it couldn't be persistent after dom restart. The patch fix it.
--- src/qemu/qemu_driver.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 51 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d66140b..1a53088 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5975,9 +5975,13 @@ qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *def, size_t *def_size, virReportOOMError(); return -1; } - (*def)[*def_size - 1].path = dw->path; + (*def)[*def_size - 1].path = strdup(dw->path);
This much indirection makes life harder. Can we declare an intermediate variable of the right type, and initialize it with&(*def)[*def_size - 1], then use mydef->path and such through the rest of the code? But that's an independent comment about the code.
At any rate, the strdup here would make sense, if we were feeding the temporary array that supplied dw into two different *def pointers. But in reality, the caller is creating the temporary array twice, so this feels like it is just churn.
+ if (!(*def)[*def_size - 1].path) { + virReportOOMError(); + return -1; + } + (*def)[*def_size - 1].weight = dw->weight; - dw->path = NULL; } }
@@ -5985,6 +5989,46 @@ qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *def, size_t *def_size, }
static int +qemuDomainiDefineDeviceWeights(virDomainDefPtr persistentDef, s/DomainiDefine/DomainDefine/ throughout the patch.
Actually, this function looks like it is reinventing qemuDomainMergeDeviceWeights.
@@ -6116,6 +6160,11 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, ret = -1; continue; } + if (qemuDomainiDefineDeviceWeights(persistentDef, + devices, + ndevices)< 0) + ret = -1; + I'm not sure we need this; instead,
if (qemuDomainMergeDeviceWeights(&vm->def->blkio.devices, &vm->def->blkio.ndevices,
Isn't the real bug that we are calling MergeDeviceWeights on vm->def instead of on persistentDef? Does this simpler patch do the trick? (I should probably split it into two pathes - the first hunk is cosmetic, the second fixes the bug).
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 3f940e8..06b30be 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -5975,35 +5975,40 @@ cleanup: return -1; }
-/* Modify def to reflect all device weight changes described in tmp. */ +/* Modify dest_array to reflect all device weight changes described in + * src_array. */ static int -qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *def, size_t *def_size, - virBlkioDeviceWeightPtr tmp, size_t tmp_size) +qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *dest_array, + size_t *dest_size, + virBlkioDeviceWeightPtr src_array, + size_t src_size) { int i, j; - virBlkioDeviceWeightPtr dw; + virBlkioDeviceWeightPtr dest, src;
- for (i = 0; i< tmp_size; i++) { + for (i = 0; i< src_size; i++) { bool found = false;
- dw =&tmp[i]; - for (j = 0; j< *def_size; j++) { - if (STREQ(dw->path, (*def)[j].path)) { + src =&src_array[i]; + for (j = 0; j< *dest_size; j++) { + dest =&(*dest_array)[j]; + if (STREQ(src->path, dest->path)) { found = true; - (*def)[j].weight = dw->weight; + dest->weight = src->weight; break; } } if (!found) { - if (!dw->weight) + if (!src->weight) continue; - if (VIR_EXPAND_N(*def, *def_size, 1)< 0) { + if (VIR_EXPAND_N(*dest_array, *dest_size, 1)< 0) { virReportOOMError(); return -1; } - (*def)[*def_size - 1].path = dw->path; - (*def)[*def_size - 1].weight = dw->weight; - dw->path = NULL; + dest =&(*dest_array)[*dest_size - 1]; + dest->path = src->path; + dest->weight = src->weight; + src->path = NULL; } }
@@ -6142,8 +6147,8 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, ret = -1; continue; } - if (qemuDomainMergeDeviceWeights(&vm->def->blkio.devices, -&vm->def->blkio.ndevices, + if (qemuDomainMergeDeviceWeights(&persistentDef->blkio.devices, + &persistentDef->blkio.ndevices, devices, ndevices)< 0) ret = -1; virBlkioDeviceWeightArrayClear(devices, ndevices);
Thanks, it is using duplicated function to do the job. Your patch is perfect except the possible not correct indentation in last couple of lines.
On 02/08/2012 06:49 AM, Guannan Ren wrote:
Isn't the real bug that we are calling MergeDeviceWeights on vm->def instead of on persistentDef? Does this simpler patch do the trick? (I should probably split it into two pathes - the first hunk is cosmetic, the second fixes the bug).
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 3f940e8..06b30be 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -5975,35 +5975,40 @@ cleanup: return -1; }
-/* Modify def to reflect all device weight changes described in tmp. */ +/* Modify dest_array to reflect all device weight changes described in + * src_array. */ static int -qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *def, size_t *def_size, - virBlkioDeviceWeightPtr tmp, size_t tmp_size) +qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *dest_array, + size_t *dest_size, + virBlkioDeviceWeightPtr src_array, + size_t src_size) {
@@ -6142,8 +6147,8 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, ret = -1; continue; } - if (qemuDomainMergeDeviceWeights(&vm->def->blkio.devices, -&vm->def->blkio.ndevices, + if (qemuDomainMergeDeviceWeights(&persistentDef->blkio.devices, + &persistentDef->blkio.ndevices, devices, ndevices)< 0) ret = -1; virBlkioDeviceWeightArrayClear(devices, ndevices);
Thanks, it is using duplicated function to do the job. Your patch is perfect except the possible not correct indentation in last couple of lines.
Thanks; I've now pushed this as two patches. The bad indentation on the last hunk was an artifact of me pasting the patch into thunderbird, and not an actual flaw with what I pushed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Guannan Ren