Re: [Qemu-block] [PATCH 16/17] block: protect modification of dirty bitmaps with a mutex

[Paolo Bonzini]

On 05/05/2017 12:36, Stefan Hajnoczi wrote:
> On Thu, Apr 20, 2017 at 02:00:57PM +0200, Paolo Bonzini wrote:
>> @@ -410,6 +442,18 @@ int bdrv_get_dirty(BlockDriverState *bs, 
>> BdrvDirtyBitmap *bitmap,
>>      }
>>  }
>>  
>> +int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>> +                   int64_t sector)
> 
> Is it a good idea to offer an unlocked bdrv_get_dirty() API?  It
> encourages non-atomic access to the bitmap, e.g.
> 
>   if (bdrv_get_dirty()) {
>       ...do something outside the lock...
>       bdrv_reset_dirty_bitmap();
>   }
> 
> The unlocked API should be test-and-set/clear instead so that callers
> automatically avoid race conditions.

I'm not sure it's possible to implement atomic test and clear for
HBitmap.  But I can look into removing unlocked bdrv_get_dirty, the only
user is block migration.

>> diff --git a/block/mirror.c b/block/mirror.c
>> index dc227a2..6a5b0f8 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -344,10 +344,12 @@ static uint64_t coroutine_fn 
>> mirror_iteration(MirrorBlockJob *s)
>>  
>>      sector_num = bdrv_dirty_iter_next(s->dbi);
>>      if (sector_num < 0) {
>> +        bdrv_dirty_bitmap_lock(s->dirty_bitmap);
> 
> bdrv_dirty_iter_next() is listed under "functions that require manual
> locking" but it's being called outside of the lock.

Thanks, will fix.

Paolo