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