Re: [Qemu-block] [Qemu-devel] [PATCH 15/17] block: introduce dirty_bitmap_mutex

[Fam Zheng]

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