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 <vsement...@virtuozzo.com>
Reviewed-by: John Snow <js...@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);
> }
> }
>
>
--
—js
