Re: [libvirt] [PATCH] qemu: allow blkstat/blkinfo calls during migration

On 05/10/2011 09:57 AM, Federico Simoncelli wrote: Can you also provide a summary of these messages, as well as a summary of your code changes, in your commit message, so that future 'git log' readers won't have to fire up a browser? Something like: Add some additional job signal flags for doing blkstat/blkinfo during a migration. Add a condition variable that can be used to efficiently wait for the migration code to clear the signal flag. You've previously contributed under a gmail account; any preference which of the two accounts should be listed in AUTHORS? This attempts to initialize the condition variable on every call to qemuDomainObjBeginJob, which is undefined behavior after the first initialization. Also, I don't see any counterpart virCondDestroy in your patch, which is a resource leak. I think you meant to add the init in qemuDomainObjPrivateAlloc, and a counterpart destroy in qemuDomainObjPrivateFree. EEK! When did we ever initialize priv->jobCond? I'm impressed that the qemu driver even works in the first place, since it is calling virCondWaitUntil on an uninitialized condition! [That is, we're implicitly relying on PTHREAD_COND_INITIALIZER just _happening_ to be an all-zeros initialization in Linux, where qemuDomainObjPrivateAlloc zero-initialized the condition variable, but relying on this implicit behavior violates POSIX] What happens if both APIs are called in the window before either can be acted on? Shouldn't you have two returnCode fields, one for each QEMU_JOB_SIGNAL_BLK* bit, so they aren't stomping on each other? Comment is a bit misleading; it is used to coordinate between migration and safe query APIs. Odd indentation. Per src/qemu/THREADS.txt, we need to increase the reference count prior to virCondWait, and decrement it after we read jobSignalsData.returnCode. Otherwise, because the condition value is dropping the vm lock, it is possible that the vm could go away in the meantime, and without holding an extra reference to the vm, we risk trying to regain vm->lock after it has been garbage collected by another thread. The qemuDomainObjBeginJob for the non-migration case already took care of that for us. Missing a check that the vm is still active (it could have died in the time it took to get the job condition). Spurious whitespace change. Inconsistent indentation. Same problem with needing to temporarily increase vm's reference count. Yep, I've definitely convinced myself that you need two returnCode slots. Thread 1 starts a migration. Thread 2 queries BlockStats, waits for the condition variable and priv->jobSignals to be 0. Thread 2 sets priv->jobSignals to non-zero, then waits for condition variable and priv->jobSignals to clear BLKSTATS. Thread 3 queries BlockInfo. Thread 1 processes the BlockStats query, and clears priv->jobSignals Thread 3 then gets condition lock instead of thread 2, and requests blkinfo Thread 1 then gets condition lock instead of thread 2, and nukes the returnCode Thread 2 finally gets condition lock, but sees the answer intended for thread 3. Missing a check if vm is still active. Pre-existing, but I think we should process all signals on a single pass, rather than just the first signal we encounter (which then delays the remaining signals for the next sleep and time around the loop). Overall, I like the idea, but we need a v2. Do you need me to help? -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org