On 04/05/2017 09:55, Fam Zheng wrote:
> 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.
Yes, I'll introduce bs->dirty_bitmap_mutex in this patch already.
>> @@ -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?
The check avoids taking the lock in the common case of having no dirty
bitmap.
Paolo
>> +
>> + 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
>
>
