Re: [Qemu-devel] [PATCH 18/41] block-migration: add lock

[Paolo Bonzini]

Il 22/02/2013 11:59, Juan Quintela ha scritto:
> Paolo Bonzini <address@hidden> wrote:
>> Some state is shared between the block migration code and its AIO
>> callbacks.  Once block migration will run outside the iothread,
>> the block migration code and the AIO callbacks will be able to
>> run concurrently.  Protect the critical sections with a separate
>> lock.  Do the same for completed_sectors, which can be used from
>> the monitor.
>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
> 
> Reviewed-by: Juan Quintela <address@hidden>
> 
> 
> For this idiom:
> 
>>      for (sector = bmds->cur_dirty; sector < bmds->total_sectors;) {
>> +        blk_mig_lock();
>>          if (bmds_aio_inflight(bmds, sector)) {
>> +            blk_mig_unlock();
>>              bdrv_drain_all();
>> +        } else {
>> +            blk_mig_unlock();
>>          }
>>          if (bdrv_get_dirty(bmds->bs, sector)) {
>>  
> 
> I find easier to understand to do:
> 
>      for (sector = bmds->cur_dirty; sector < bmds->total_sectors;) {
>         bool drain;
>         blk_mig_lock();
>         drain = bmds_aio_inflight(bmds, sector)) {
>         blk_mig_unlock();
>         if (drain)
>             bdrv_drain_all();
>         }
>         if (bdrv_get_dirty(bmds->bs, sector)) {
> 
> 
> But it is up-to-you if you have to resend the series.

Sure, I can change that.  I don't like it either way, in truth. :)

Paolo

>> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
>> index 96a194b..10becb6 100644
>> --- a/include/qemu/atomic.h
>> +++ b/include/qemu/atomic.h
>> @@ -16,6 +16,7 @@
>>   */
>>  #define smp_wmb()   barrier()
>>  #define smp_rmb()   barrier()
>> +
>>  /*
>>   * We use GCC builtin if it's available, as that can use
>>   * mfence on 32 bit as well, e.g. if built with -march=pentium-m.
> 
> This hunk obviously don't belong to this commit O:-)
> 
>