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

[Eric Blake]

On 05/20/2011 03:11 PM, Paolo Bonzini wrote:
> On 05/16/2011 07:30 PM, Eric Blake wrote:
>> +        while (priv->jobSignals&  QEMU_JOB_SIGNAL_BLKSTAT)
>> +            ignore_value(virCondWait(&priv->signalCond,&vm->lock));
>> +
>> +        priv->jobSignalsData.statDevName = disk->info.alias;
>> +        priv->jobSignalsData.blockStat = stats;
>> +        priv->jobSignalsData.statRetCode = -1;
>> +        priv->jobSignals |= QEMU_JOB_SIGNAL_BLKSTAT;
>> +
>> +        while (priv->jobSignals&  QEMU_JOB_SIGNAL_BLKSTAT)
>> +            ignore_value(virCondWait(&priv->signalCond,&vm->lock));
> 
> I'm not sure that the original problem with jobSignals is fixed (the 
> one that you solved by splitting the retcode field).  A similar race 
> can happen with two threads, both requesting blkstat.
> 
> 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 1 processes thread 2's query, and clears priv->jobSignals
> Thread 3 sees priv->jobSignals == 0 and queries BlockStats.
> Thread 1 processes thread 3's query, and clears priv->jobSignals
> Thread 2 gets condition lock, but sees the answer intended for
> thread 3.

I think we're safe.  It shouldn't matter if thread 2 reads thread 3's
answer (because they are both read-only queries, and should be getting
the same answer; or even if the answers differ, reading the newer answer
is not horrible because it is still an accurate answer).  The harder
problem is if thread 3 starts overwriting the result area to NULL before
setting the jobSignals request, and before thread 2 can read the result
area; but we aren't doing a memset (that is, the query code is only ever
reading the result data, and it is only ever being modified while
jobSignals is set).

However,

> 
> I think the code should use two condition variables.  signalCond is 
> broadcast when you _can start_ a job, resultCond is broadcast when a 
> job has finished.

you probably have a point here.  Using two variables will make it much
easier to prove correctness, especially if we add other interfaces.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110520/f60261dd/attachment-0001.sig>