[libvirt] [PATCHv6 4/7] Implement virDomain{Set, Get}BlockIoTune for the qemu driver

[Lei Li]

On 12/01/2011 02:15 AM, Eric Blake wrote:
> On 11/23/2011 02:44 PM, Eric Blake wrote:
>> From: Lei Li<lilei at linux.vnet.ibm.com>
>>
>> Implement the block I/O throttle setting and getting support to qemu
>> driver.
>>
>> Signed-off-by: Lei Li<lilei at linux.vnet.ibm.com>
>> Signed-off-by: Zhi Yong Wu<wuzhy at linux.vnet.ibm.com>
>> Signed-off-by: Eric Blake<eblake at redhat.com>
>> +qemuDomainSetBlockIoTune(virDomainPtr dom,
>> +                         const char *disk,
>> +                         virTypedParameterPtr params,
>> +                         int nparams,
>> +                         unsigned int flags)
>> +{
>> +
>> +    if (flags&  VIR_DOMAIN_AFFECT_CONFIG) {
>> +        if (!vm->persistent) {
>> +            qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> +                            _("cannot change persistent config of a transient domain"));
>> +            goto endjob;
>> +        }
>> +        if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm)))
>> +            goto endjob;
>> +    }
>> +
>> +
>> +    if (flags&  VIR_DOMAIN_AFFECT_CONFIG) {
>> +        sa_assert(persistentDef);
>> +        int idx = virDomainDiskIndexByName(vm->def, disk, true);
> Oops - this should be on persistentDef, not vm->def.
>
>> +        if (i<  0)
>> +            goto endjob;
>> +        persistentDef->disks[idx]->blkdeviotune = info;
> And this assignment should be delayed...
>
>> +    }
>> +
>> +    if (flags&  VIR_DOMAIN_AFFECT_LIVE) {
>> +        priv = vm->privateData;
>> +        qemuDomainObjEnterMonitorWithDriver(driver, vm);
>> +        ret = qemuMonitorSetBlockIoThrottle(priv->mon, device,&info);
>> +        qemuDomainObjExitMonitorWithDriver(driver, vm);
>> +    }
>> +
>> +    if (flags&  VIR_DOMAIN_AFFECT_CONFIG) {
> to here, after we know the live change (if any) took place.  Not to
> mention that we must not get here if the live change failed.  Here's
> what I'm squashing in:
>
> diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
> index 698a961..ce4cba1 100644
> --- i/src/qemu/qemu_driver.c
> +++ w/src/qemu/qemu_driver.c
> @@ -11080,6 +11080,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>       int ret = -1;
>       int i;
>       bool isActive;
> +    int idx = -1;
>
>       virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
>                     VIR_DOMAIN_AFFECT_CONFIG, -1);
> @@ -11126,6 +11127,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>           }
>           if (!(persistentDef =
> virDomainObjGetPersistentDef(driver->caps, vm)))
>               goto endjob;
> +        idx = virDomainDiskIndexByName(persistentDef, disk, true);
> +        if (i<  0)
> +            goto endjob;
>       }
>
>       for (i = 0; i<  nparams; i++) {
> @@ -11177,22 +11181,18 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>           goto endjob;
>       }
>
> -    if (flags&  VIR_DOMAIN_AFFECT_CONFIG) {
> -        sa_assert(persistentDef);
> -        int idx = virDomainDiskIndexByName(vm->def, disk, true);
> -        if (i<  0)
> -            goto endjob;
> -        persistentDef->disks[idx]->blkdeviotune = info;
> -    }
> -
>       if (flags&  VIR_DOMAIN_AFFECT_LIVE) {
>           priv = vm->privateData;
>           qemuDomainObjEnterMonitorWithDriver(driver, vm);
>           ret = qemuMonitorSetBlockIoThrottle(priv->mon, device,&info);
>           qemuDomainObjExitMonitorWithDriver(driver, vm);
>       }
> +    if (ret<  0)
> +        goto endjob;
Hi Eric,

Thanks again for your kind help! An error occurred when I test it.

virsh #blkdeviotune kvm vda --total_bytes_sec 80000 --config
error: Unable to change block I/O throttle
error: An error occurred, but the cause is unknown

I dig into the code, here is a logic error.
The initial value of ret = -1, if just set --config, it will
goto endjob directly without doing its really job here.

I guess the purpose you add these two lines is to check if live
option succeeded, how about just get rid of these two lines.
since the relevant check has already done in qemu monitor code.
If live option failed it will return ret = -1 with error report,
and if ret<  0, src/libvirt.c will goto error.

I submit a patch based on above to avoid this error.


>       if (flags&  VIR_DOMAIN_AFFECT_CONFIG) {
> +        sa_assert(persistentDef&&  idx>= 0);
> +        persistentDef->disks[idx]->blkdeviotune = info;
>           ret = virDomainSaveConfig(driver->configDir, persistentDef);
>           if (ret<  0) {
>               qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
>
>
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


-- 
Lei