Re: [libvirt] [PATCH] qemu: lock: unlock vm during qemuBuildCommandLine

On Thu, Apr 23, 2015 at 11:19:34 +0200, Michal Privoznik wrote: > On 23.04.2015 10:30, Peter Krempa wrote: >> On Thu, Apr 23, 2015 at 11:44:49 +0800, zhang bo wrote: >>> The function qemuBuildCommandLine() may take a long time, for example >>> if we configure tens of vifs for the guest, each may cost hundrands of >>> milliseconds to create tap dev, senconds in total. Thus, unlock vm >>> before calling it. >>> >>> Signed-off-by: Zhang Bo <oscar.zhangbo(a)huawei.com&gt; >>> Signed-off-by: Zhou Yimin <zhouyimin(a)huawei.com&gt; >>> --- >>> src/qemu/qemu_process.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c >>> index 753afe8..d1aaaec 100644 >>> --- a/src/qemu/qemu_process.c >>> +++ b/src/qemu/qemu_process.c >>> @@ -4628,14 +4628,18 @@ int qemuProcessStart(virConnectPtr conn, >>> } >>> >>> VIR_DEBUG("Building emulator command line"); >>> + virObjectUnlock(vm); >>> if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, >>> priv->monJSON, priv->qemuCaps, >>> migrateFrom, stdin_fd, snapshot, vmop, >>> &buildCommandLineCallbacks, false, >>> qemuCheckFips(), >>> priv->autoNodeset, >>> - &nnicindexes, &nicindexes))) >>> + &nnicindexes, &nicindexes))) { >>> + virObjectLock(vm); >>> goto cleanup; >>> + } >>> + virObjectLock(vm); >> >> Why do you need to unlock the object? The VM is starting at this point >> so you won't be able to save any time since APIs will either be blocked >> by a job or by the fact that the VM was not started. > > Not true. We still allow QUERY jobs, or APIs that lock a domain object > but don't necessarily take a job. For instance, if in one thread I'm > doing virDomainGetOSType(), virDomainGetInfo(), ... (essentially 'virsh > dominfo') while the other just starts the domain. I don't see a reason > why virDomainGetOSType() should wait for the domain to be started up. > Domain state should have no affect on the guest OS type, should it? OK that is technically right. I wanted to point out that most of the stats are available only for online machines or it shouldn't be much of a problem to dealy the delivery. Your example certainly doesn't illustrate why the delay to format the command line should be a problem when using libvirt. Or are you polling virDomainGetOSType every millisecond? I am curious to see why the delay would be a problem.

> > On the other hand, I don't think we can just lock and unlock the domain > object as we please. qemuBuildCommandLine is a very complex function and > as such it calls many others. Some of them may rely on the fact, that > the object is locked by caller. Well, you definitely can't since almost everything in there is accessing vm->def. Locking semantics would be broken right in the line after @vm was unlocked by dereferencing it.