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

[Peter Krempa]

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 at huawei.com>
> >> Signed-off-by: Zhou Yimin <zhouyimin at huawei.com>
> >> ---
> >>  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.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150423/22b12c8e/attachment-0001.sig>