On 11/12/2014 07:18 AM, Ján Tomko wrote:

On 11/12/2014 02:04 PM, Matthias Gatto wrote:

...

Fix for this: http://www.redhat.com/archives/libvir-list/2014-November/msg00324.html

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a5ed8ed..f674ba9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3762,7 +3762,7 @@ qemuBuildDriveStr(virConnectPtr conn, disk->blkdeviotune.write_iops_sec_max); }

- if (disk->blkdeviotune.write_iops_sec_max) { + if (disk->blkdeviotune.size_iops_sec) { virBufferAsprintf(&opt, ",iops_size=%llu", disk->blkdeviotune.size_iops_sec); }

It would be nice to have this tested in qemuxml2argvtest.

You mean something like: diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index fe58a24..c52b647 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1265,6 +1265,9 @@ mymain(void) DO_TEST("numad-static-memory-auto-vcpu", NONE); DO_TEST("blkdeviotune", QEMU_CAPS_NAME, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_IOTUNE); + DO_TEST("blkdeviotune-max", QEMU_CAPS_NAME, QEMU_CAPS_DEVICE, + QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_IOTUNE, + QEMU_CAPS_DRIVE_IOTUNE_MAX); DO_TEST("multifunction-pci-device", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, Plus of course the obligatory .xml and .args files... Note that at most only 9 parameters can be given because there is XML config failure if any of the "total_" values are provided with any of their corresponding "read_"/"write_" values. My test file has one disk with: <iotune> <total_bytes_sec>5000</total_bytes_sec> <total_iops_sec>6000</total_iops_sec> <total_bytes_sec_max>10000</total_bytes_sec_max> <total_iops_sec_max>6000</total_iops_sec_max> </iotune> and the other with <iotune> <read_bytes_sec>5000</read_bytes_sec> <write_bytes_sec>5000</write_bytes_sec> <read_iops_sec>3500</read_iops_sec> <write_iops_sec>3500</write_iops_sec> <read_bytes_sec_max>5000</read_bytes_sec_max> <write_bytes_sec_max>5000</write_bytes_sec_max> <read_iops_sec_max>3500</read_iops_sec_max> <write_iops_sec_max>3500</write_iops_sec_max> <size_iops_sec_max>2000</size_iops_sec_max> </iotune> where the .args file has: ... -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-ide0-0-0,cache=off,\ bps=5000,iops=6000,bps_max=10000,iops_max=6000 -device \ ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -drive file=/dev/HostVG/QEMUGuest2,if=none,id=drive-ide0-0-1,cache=off,\ bps_rd=5000,bps_wr=5000,iops_rd=3500,iops_wr=3500,bps_rd_max=5000,\ bps_wr_max=5000,iops_rd_max=3500,iops_wr_max=3500 \ ... Testing without the QEMU_CAPS_DRIVE_IOTUNE_MAX results in: 333) QEMU XML-2-ARGV blkdeviotune-max ... libvirt: QEMU Driver error : unsupported configuration: there is some block I/O throttling paramater that are not supported with this QEMU binary(need QEMU 1.7 or superior) FAILED NOTE: Parameter is spelled wrong... and the message doesn't read well, something I can fix. John

Need to fix the commit message to something like "Resolve Coverity DEADCODE" (something I just realized for 1/2 which could read Resolve Coverity COPY_PASTE_ERROR" Something I can take care of when pushing. On 11/12/2014 08:04 AM, Matthias Gatto wrote:

reported here: http://www.redhat.com/archives/libvir-list/2014-November/msg00327.html

I could have just remove bool supportMaxOptions variable, but if I had do this, we could not check anymore if the nparams variable is superior to QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX.

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com> --- src/qemu/qemu_driver.c | 1 + 1 file changed, 1 insertion(+)

While the following does work...

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 56e8430..61c4af6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17024,6 +17024,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,

if (flags & VIR_DOMAIN_AFFECT_LIVE) { priv = vm->privateData; + supportMaxOptions = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX); qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply, supportMaxOptions); qemuDomainObjExitMonitor(driver, vm);

Would perhaps the following be "cleaner": diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 56e8430..5124e56 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17003,14 +17003,15 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, &persistentDef) < 0) goto endjob; + /* If the VM is running, we can check if the current VM can use + * optional parameters or not. We didn't made this check sooner + * because we need the VM data to do so. */ + priv = vm->privateData; + if (flags & VIR_DOMAIN_AFFECT_LIVE) + supportMaxOptions = virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_DRIVE_IOTUNE_MAX); + if ((*nparams) == 0) { - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - priv = vm->privateData; - /* If the VM is running, we can check if the current VM can use - * optional parameters or not. We didn't made this check sooner - * because we need the VM data to do so. */ - supportMaxOptions = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX); - } *nparams = supportMaxOptions ? QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX : QEMU_NB_BLOCK_IO_TUNE_PARAM; ret = 0; @@ -17023,7 +17024,6 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - priv = vm->privateData; qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply, supportMaxOptions); qemuDomainObjExitMonitor(driver, vm); The difference being moving the setting of 'priv' to be made regardless of flags and supportMaxOptions only once prior to using in either place. The second sentence of your comment is a bit confusing too - I assume the "VM data" you are referencing is the vm obtained much earlier. Or are you trying to indicate that perhaps one of the interceding calls is necessary? In any case, does the above look reasonable? Tks - John