On Thu, 04/20 14:00, Paolo Bonzini wrote: > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 519737c..e13718e 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -52,6 +52,24 @@ struct BdrvDirtyBitmapIter { > BdrvDirtyBitmap *bitmap; > }; > > +static QemuMutex dirty_bitmap_mutex; > + > +static void __attribute__((__constructor__)) > bdrv_dirty_bitmaps_init_lock(void) > +{ > + qemu_mutex_init(&dirty_bitmap_mutex); > +} > + > +static inline void bdrv_dirty_bitmaps_lock(BlockDriverState *bs) > +{ > + qemu_mutex_lock(&dirty_bitmap_mutex); > +} > + > +static inline void bdrv_dirty_bitmaps_unlock(BlockDriverState *bs) > +{ > + qemu_mutex_unlock(&dirty_bitmap_mutex); > +}
Why a global lock instead of a per-BDS one? The contention can be heavy if a block job is made to run on a different thread than the one processing guest I/O. > @@ -508,12 +550,19 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t > cur_sector, > int64_t nr_sectors) > { > BdrvDirtyBitmap *bitmap; > + > + if (QLIST_EMPTY(&bs->dirty_bitmaps)) { > + return; > + } Should this check be protected by lock/unlock? Or simply removed? > + > + bdrv_dirty_bitmaps_lock(bs); > QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { > if (!bdrv_dirty_bitmap_enabled(bitmap)) { > continue; > } > hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); > } > + bdrv_dirty_bitmaps_unlock(bs); > } > > /** Fam