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

[Stefan Hajnoczi]

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.

> 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.

signature.asc
Description: PGP signature