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
