Message ID | 20190916141911.5255-4-vsementsov@virtuozzo.com |
---|---|
State | New |
Headers | show |
Series | bitmaps: some refactoring | expand |
On 9/16/19 10:19 AM, Vladimir Sementsov-Ogievskiy wrote: > mutex field is just a pointer to bs->dirty_bitmap_mutex, so no needs > to store it in BdrvDirtyBitmap when we have bs pointer in it (since > previous patch). > > Drop mutex field. Constantly use bdrv_dirty_bitmaps_lock/unlock in > block/dirty-bitmap.c to make it more obvious that it's not per-bitmap > lock. Still, for simplicity, leave bdrv_dirty_bitmap_lock/unlock > functions as an external API. > It's convenient, but I wish it had a better name because it still implies a per-bitmap lock, which we've never had. It's fine for now, this patch doesn't make it worse, of course. > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: John Snow <jsnow@redhat.com> > --- > block/dirty-bitmap.c | 84 +++++++++++++++++++++----------------------- > 1 file changed, 41 insertions(+), 43 deletions(-) > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index f3dc7b3ca5..76a8e59e61 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -28,7 +28,6 @@ > #include "block/blockjob.h" > > struct BdrvDirtyBitmap { > - QemuMutex *mutex; > BlockDriverState *bs; > HBitmap *bitmap; /* Dirty bitmap implementation */ > bool busy; /* Bitmap is busy, it can't be used via QMP */ > @@ -71,12 +70,12 @@ static inline void bdrv_dirty_bitmaps_unlock(BlockDriverState *bs) > > void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap) > { > - qemu_mutex_lock(bitmap->mutex); > + bdrv_dirty_bitmaps_lock(bitmap->bs); > } > > void bdrv_dirty_bitmap_unlock(BdrvDirtyBitmap *bitmap) > { > - qemu_mutex_unlock(bitmap->mutex); > + bdrv_dirty_bitmaps_unlock(bitmap->bs); > } > > /* Called with BQL or dirty_bitmap lock taken. */ > @@ -116,7 +115,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, > } > bitmap = g_new0(BdrvDirtyBitmap, 1); > bitmap->bs = bs; > - bitmap->mutex = &bs->dirty_bitmap_mutex; > bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(granularity)); > bitmap->size = bitmap_size; > bitmap->name = g_strdup(name); > @@ -150,9 +148,9 @@ static bool bdrv_dirty_bitmap_busy(const BdrvDirtyBitmap *bitmap) > > void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy) > { > - qemu_mutex_lock(bitmap->mutex); > + bdrv_dirty_bitmaps_lock(bitmap->bs); > bitmap->busy = busy; > - qemu_mutex_unlock(bitmap->mutex); > + bdrv_dirty_bitmaps_unlock(bitmap->bs); > } > > /* Called with BQL taken. */ > @@ -277,10 +275,10 @@ void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap) > /* Called with BQL taken. */ > void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap) > { > - assert(bitmap->mutex == bitmap->successor->mutex); > - qemu_mutex_lock(bitmap->mutex); > + assert(bitmap->bs == bitmap->successor->bs); > + bdrv_dirty_bitmaps_lock(bitmap->bs); > bdrv_enable_dirty_bitmap_locked(bitmap->successor); > - qemu_mutex_unlock(bitmap->mutex); > + bdrv_dirty_bitmaps_unlock(bitmap->bs); > } > > /* Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken. */ > @@ -360,9 +358,9 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap *parent, > { > BdrvDirtyBitmap *ret; > > - qemu_mutex_lock(parent->mutex); > + bdrv_dirty_bitmaps_lock(parent->bs); > ret = bdrv_reclaim_dirty_bitmap_locked(parent, errp); > - qemu_mutex_unlock(parent->mutex); > + bdrv_dirty_bitmaps_unlock(parent->bs); > > return ret; > } > @@ -434,16 +432,16 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, > > void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) > { > - bdrv_dirty_bitmap_lock(bitmap); > + bdrv_dirty_bitmaps_lock(bitmap->bs); > bitmap->disabled = true; > - bdrv_dirty_bitmap_unlock(bitmap); > + bdrv_dirty_bitmaps_unlock(bitmap->bs); > } > > void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap) > { > - bdrv_dirty_bitmap_lock(bitmap); > + bdrv_dirty_bitmaps_lock(bitmap->bs); > bdrv_enable_dirty_bitmap_locked(bitmap); > - bdrv_dirty_bitmap_unlock(bitmap); > + bdrv_dirty_bitmaps_unlock(bitmap->bs); > } > > BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs) > @@ -484,9 +482,9 @@ bool bdrv_dirty_bitmap_get_locked(BdrvDirtyBitmap *bitmap, int64_t offset) > bool bdrv_dirty_bitmap_get(BdrvDirtyBitmap *bitmap, int64_t offset) > { > bool ret; > - bdrv_dirty_bitmap_lock(bitmap); > + bdrv_dirty_bitmaps_lock(bitmap->bs); > ret = bdrv_dirty_bitmap_get_locked(bitmap, offset); > - bdrv_dirty_bitmap_unlock(bitmap); > + bdrv_dirty_bitmaps_unlock(bitmap->bs); > > return ret; > } > @@ -551,9 +549,9 @@ void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap, > void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap, > int64_t offset, int64_t bytes) > { > - bdrv_dirty_bitmap_lock(bitmap); > + bdrv_dirty_bitmaps_lock(bitmap->bs); > bdrv_set_dirty_bitmap_locked(bitmap, offset, bytes); > - bdrv_dirty_bitmap_unlock(bitmap); > + bdrv_dirty_bitmaps_unlock(bitmap->bs); > } > > /* Called within bdrv_dirty_bitmap_lock..unlock */ > @@ -567,15 +565,15 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap, > void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap, > int64_t offset, int64_t bytes) > { > - bdrv_dirty_bitmap_lock(bitmap); > + bdrv_dirty_bitmaps_lock(bitmap->bs); > bdrv_reset_dirty_bitmap_locked(bitmap, offset, bytes); > - bdrv_dirty_bitmap_unlock(bitmap); > + bdrv_dirty_bitmaps_unlock(bitmap->bs); > } > > void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out) > { > assert(!bdrv_dirty_bitmap_readonly(bitmap)); > - bdrv_dirty_bitmap_lock(bitmap); > + bdrv_dirty_bitmaps_lock(bitmap->bs); > if (!out) { > hbitmap_reset_all(bitmap->bitmap); > } else { > @@ -584,7 +582,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out) > hbitmap_granularity(backup)); > *out = backup; > } > - bdrv_dirty_bitmap_unlock(bitmap); > + bdrv_dirty_bitmaps_unlock(bitmap->bs); > } > > void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup) > @@ -679,9 +677,9 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap) > /* Called with BQL taken. */ > void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value) > { > - qemu_mutex_lock(bitmap->mutex); > + bdrv_dirty_bitmaps_lock(bitmap->bs); > bitmap->readonly = value; > - qemu_mutex_unlock(bitmap->mutex); > + bdrv_dirty_bitmaps_unlock(bitmap->bs); > } > > bool bdrv_has_readonly_bitmaps(BlockDriverState *bs) > @@ -699,27 +697,27 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs) > /* Called with BQL taken. */ > void bdrv_dirty_bitmap_set_persistence(BdrvDirtyBitmap *bitmap, bool persistent) > { > - qemu_mutex_lock(bitmap->mutex); > + bdrv_dirty_bitmaps_lock(bitmap->bs); > bitmap->persistent = persistent; > - qemu_mutex_unlock(bitmap->mutex); > + bdrv_dirty_bitmaps_unlock(bitmap->bs); > } > > /* Called with BQL taken. */ > void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap) > { > - qemu_mutex_lock(bitmap->mutex); > + bdrv_dirty_bitmaps_lock(bitmap->bs); > assert(bitmap->persistent == true); > bitmap->inconsistent = true; > bitmap->disabled = true; > - qemu_mutex_unlock(bitmap->mutex); > + bdrv_dirty_bitmaps_unlock(bitmap->bs); > } > > /* Called with BQL taken. */ > void bdrv_dirty_bitmap_skip_store(BdrvDirtyBitmap *bitmap, bool skip) > { > - qemu_mutex_lock(bitmap->mutex); > + bdrv_dirty_bitmaps_lock(bitmap->bs); > bitmap->skip_store = skip; > - qemu_mutex_unlock(bitmap->mutex); > + bdrv_dirty_bitmaps_unlock(bitmap->bs); > } > > bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap) > @@ -779,9 +777,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, > { > bool ret; > > - qemu_mutex_lock(dest->mutex); > - if (src->mutex != dest->mutex) { > - qemu_mutex_lock(src->mutex); > + bdrv_dirty_bitmaps_lock(dest->bs); > + if (src->bs != dest->bs) { > + bdrv_dirty_bitmaps_lock(src->bs); > } > > if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) { > @@ -801,9 +799,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, > assert(ret); > > out: > - qemu_mutex_unlock(dest->mutex); > - if (src->mutex != dest->mutex) { > - qemu_mutex_unlock(src->mutex); > + bdrv_dirty_bitmaps_unlock(dest->bs); > + if (src->bs != dest->bs) { > + bdrv_dirty_bitmaps_unlock(src->bs); > } > } > > @@ -827,9 +825,9 @@ bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest, > assert(!bdrv_dirty_bitmap_inconsistent(src)); > > if (lock) { > - qemu_mutex_lock(dest->mutex); > - if (src->mutex != dest->mutex) { > - qemu_mutex_lock(src->mutex); > + bdrv_dirty_bitmaps_lock(dest->bs); > + if (src->bs != dest->bs) { > + bdrv_dirty_bitmaps_lock(src->bs); > } > } > > @@ -842,9 +840,9 @@ bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest, > } > > if (lock) { > - qemu_mutex_unlock(dest->mutex); > - if (src->mutex != dest->mutex) { > - qemu_mutex_unlock(src->mutex); > + bdrv_dirty_bitmaps_unlock(dest->bs); > + if (src->bs != dest->bs) { > + bdrv_dirty_bitmaps_unlock(src->bs); > } > } > >
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index f3dc7b3ca5..76a8e59e61 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -28,7 +28,6 @@ #include "block/blockjob.h" struct BdrvDirtyBitmap { - QemuMutex *mutex; BlockDriverState *bs; HBitmap *bitmap; /* Dirty bitmap implementation */ bool busy; /* Bitmap is busy, it can't be used via QMP */ @@ -71,12 +70,12 @@ static inline void bdrv_dirty_bitmaps_unlock(BlockDriverState *bs) void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap) { - qemu_mutex_lock(bitmap->mutex); + bdrv_dirty_bitmaps_lock(bitmap->bs); } void bdrv_dirty_bitmap_unlock(BdrvDirtyBitmap *bitmap) { - qemu_mutex_unlock(bitmap->mutex); + bdrv_dirty_bitmaps_unlock(bitmap->bs); } /* Called with BQL or dirty_bitmap lock taken. */ @@ -116,7 +115,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, } bitmap = g_new0(BdrvDirtyBitmap, 1); bitmap->bs = bs; - bitmap->mutex = &bs->dirty_bitmap_mutex; bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(granularity)); bitmap->size = bitmap_size; bitmap->name = g_strdup(name); @@ -150,9 +148,9 @@ static bool bdrv_dirty_bitmap_busy(const BdrvDirtyBitmap *bitmap) void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy) { - qemu_mutex_lock(bitmap->mutex); + bdrv_dirty_bitmaps_lock(bitmap->bs); bitmap->busy = busy; - qemu_mutex_unlock(bitmap->mutex); + bdrv_dirty_bitmaps_unlock(bitmap->bs); } /* Called with BQL taken. */ @@ -277,10 +275,10 @@ void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap) /* Called with BQL taken. */ void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap) { - assert(bitmap->mutex == bitmap->successor->mutex); - qemu_mutex_lock(bitmap->mutex); + assert(bitmap->bs == bitmap->successor->bs); + bdrv_dirty_bitmaps_lock(bitmap->bs); bdrv_enable_dirty_bitmap_locked(bitmap->successor); - qemu_mutex_unlock(bitmap->mutex); + bdrv_dirty_bitmaps_unlock(bitmap->bs); } /* Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken. */ @@ -360,9 +358,9 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap *parent, { BdrvDirtyBitmap *ret; - qemu_mutex_lock(parent->mutex); + bdrv_dirty_bitmaps_lock(parent->bs); ret = bdrv_reclaim_dirty_bitmap_locked(parent, errp); - qemu_mutex_unlock(parent->mutex); + bdrv_dirty_bitmaps_unlock(parent->bs); return ret; } @@ -434,16 +432,16 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) { - bdrv_dirty_bitmap_lock(bitmap); + bdrv_dirty_bitmaps_lock(bitmap->bs); bitmap->disabled = true; - bdrv_dirty_bitmap_unlock(bitmap); + bdrv_dirty_bitmaps_unlock(bitmap->bs); } void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap) { - bdrv_dirty_bitmap_lock(bitmap); + bdrv_dirty_bitmaps_lock(bitmap->bs); bdrv_enable_dirty_bitmap_locked(bitmap); - bdrv_dirty_bitmap_unlock(bitmap); + bdrv_dirty_bitmaps_unlock(bitmap->bs); } BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs) @@ -484,9 +482,9 @@ bool bdrv_dirty_bitmap_get_locked(BdrvDirtyBitmap *bitmap, int64_t offset) bool bdrv_dirty_bitmap_get(BdrvDirtyBitmap *bitmap, int64_t offset) { bool ret; - bdrv_dirty_bitmap_lock(bitmap); + bdrv_dirty_bitmaps_lock(bitmap->bs); ret = bdrv_dirty_bitmap_get_locked(bitmap, offset); - bdrv_dirty_bitmap_unlock(bitmap); + bdrv_dirty_bitmaps_unlock(bitmap->bs); return ret; } @@ -551,9 +549,9 @@ void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap, void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap, int64_t offset, int64_t bytes) { - bdrv_dirty_bitmap_lock(bitmap); + bdrv_dirty_bitmaps_lock(bitmap->bs); bdrv_set_dirty_bitmap_locked(bitmap, offset, bytes); - bdrv_dirty_bitmap_unlock(bitmap); + bdrv_dirty_bitmaps_unlock(bitmap->bs); } /* Called within bdrv_dirty_bitmap_lock..unlock */ @@ -567,15 +565,15 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap, void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap, int64_t offset, int64_t bytes) { - bdrv_dirty_bitmap_lock(bitmap); + bdrv_dirty_bitmaps_lock(bitmap->bs); bdrv_reset_dirty_bitmap_locked(bitmap, offset, bytes); - bdrv_dirty_bitmap_unlock(bitmap); + bdrv_dirty_bitmaps_unlock(bitmap->bs); } void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out) { assert(!bdrv_dirty_bitmap_readonly(bitmap)); - bdrv_dirty_bitmap_lock(bitmap); + bdrv_dirty_bitmaps_lock(bitmap->bs); if (!out) { hbitmap_reset_all(bitmap->bitmap); } else { @@ -584,7 +582,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out) hbitmap_granularity(backup)); *out = backup; } - bdrv_dirty_bitmap_unlock(bitmap); + bdrv_dirty_bitmaps_unlock(bitmap->bs); } void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup) @@ -679,9 +677,9 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap) /* Called with BQL taken. */ void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value) { - qemu_mutex_lock(bitmap->mutex); + bdrv_dirty_bitmaps_lock(bitmap->bs); bitmap->readonly = value; - qemu_mutex_unlock(bitmap->mutex); + bdrv_dirty_bitmaps_unlock(bitmap->bs); } bool bdrv_has_readonly_bitmaps(BlockDriverState *bs) @@ -699,27 +697,27 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs) /* Called with BQL taken. */ void bdrv_dirty_bitmap_set_persistence(BdrvDirtyBitmap *bitmap, bool persistent) { - qemu_mutex_lock(bitmap->mutex); + bdrv_dirty_bitmaps_lock(bitmap->bs); bitmap->persistent = persistent; - qemu_mutex_unlock(bitmap->mutex); + bdrv_dirty_bitmaps_unlock(bitmap->bs); } /* Called with BQL taken. */ void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap) { - qemu_mutex_lock(bitmap->mutex); + bdrv_dirty_bitmaps_lock(bitmap->bs); assert(bitmap->persistent == true); bitmap->inconsistent = true; bitmap->disabled = true; - qemu_mutex_unlock(bitmap->mutex); + bdrv_dirty_bitmaps_unlock(bitmap->bs); } /* Called with BQL taken. */ void bdrv_dirty_bitmap_skip_store(BdrvDirtyBitmap *bitmap, bool skip) { - qemu_mutex_lock(bitmap->mutex); + bdrv_dirty_bitmaps_lock(bitmap->bs); bitmap->skip_store = skip; - qemu_mutex_unlock(bitmap->mutex); + bdrv_dirty_bitmaps_unlock(bitmap->bs); } bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap) @@ -779,9 +777,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, { bool ret; - qemu_mutex_lock(dest->mutex); - if (src->mutex != dest->mutex) { - qemu_mutex_lock(src->mutex); + bdrv_dirty_bitmaps_lock(dest->bs); + if (src->bs != dest->bs) { + bdrv_dirty_bitmaps_lock(src->bs); } if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) { @@ -801,9 +799,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, assert(ret); out: - qemu_mutex_unlock(dest->mutex); - if (src->mutex != dest->mutex) { - qemu_mutex_unlock(src->mutex); + bdrv_dirty_bitmaps_unlock(dest->bs); + if (src->bs != dest->bs) { + bdrv_dirty_bitmaps_unlock(src->bs); } } @@ -827,9 +825,9 @@ bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest, assert(!bdrv_dirty_bitmap_inconsistent(src)); if (lock) { - qemu_mutex_lock(dest->mutex); - if (src->mutex != dest->mutex) { - qemu_mutex_lock(src->mutex); + bdrv_dirty_bitmaps_lock(dest->bs); + if (src->bs != dest->bs) { + bdrv_dirty_bitmaps_lock(src->bs); } } @@ -842,9 +840,9 @@ bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest, } if (lock) { - qemu_mutex_unlock(dest->mutex); - if (src->mutex != dest->mutex) { - qemu_mutex_unlock(src->mutex); + bdrv_dirty_bitmaps_unlock(dest->bs); + if (src->bs != dest->bs) { + bdrv_dirty_bitmaps_unlock(src->bs); } }
mutex field is just a pointer to bs->dirty_bitmap_mutex, so no needs to store it in BdrvDirtyBitmap when we have bs pointer in it (since previous patch). Drop mutex field. Constantly use bdrv_dirty_bitmaps_lock/unlock in block/dirty-bitmap.c to make it more obvious that it's not per-bitmap lock. Still, for simplicity, leave bdrv_dirty_bitmap_lock/unlock functions as an external API. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/dirty-bitmap.c | 84 +++++++++++++++++++++----------------------- 1 file changed, 41 insertions(+), 43 deletions(-)