[Qemu-devel] [PATCH 0/4] bitmaps: some refactoring

[Qemu-devel] [PATCH 0/4] bitmaps: some refactoring

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here I suggest some refactoring patches for bitmaps.

Vladimir Sementsov-Ogievskiy (4):
  block/dirty-bitmap: drop meta
  block/dirty-bitmap: add bs link
  block/dirty-bitmap: drop BdrvDirtyBitmap.mutex
  block/dirty-bitmap: refactor bdrv_dirty_bitmap_next

 include/block/dirty-bitmap.h   |  28 +++---
 block.c                        |   4 +-
 block/backup.c                 |  14 ++-
 block/dirty-bitmap.c           | 165 ++++++++++++---------------------
 block/mirror.c                 |   4 +-
 block/qcow2-bitmap.c           |  14 +--
 blockdev.c                     |   6 +-
 migration/block-dirty-bitmap.c |  11 +--
 migration/block.c              |   4 +-
 9 files changed, 95 insertions(+), 155 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH 1/4] block/dirty-bitmap: drop meta

[Vladimir Sementsov-Ogievskiy]

Drop meta bitmaps, as they are unused.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/dirty-bitmap.h |  5 ----
 block/dirty-bitmap.c         | 46 ------------------------------------
 2 files changed, 51 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 4b4b731b46..848dfc6590 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -18,9 +18,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
                                           uint32_t granularity,
                                           const char *name,
                                           Error **errp);
-void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
-                                   int chunk_size);
-void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
                                        BdrvDirtyBitmap *bitmap,
                                        Error **errp);
@@ -55,7 +52,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                            int64_t offset, int64_t bytes);
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                              int64_t offset, int64_t bytes);
-BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
 
@@ -97,7 +93,6 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
 int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
 void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
-int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
 bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
 bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 134e0c9a0c..acfc3077f1 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -30,7 +30,6 @@
 struct BdrvDirtyBitmap {
     QemuMutex *mutex;
     HBitmap *bitmap;            /* Dirty bitmap implementation */
-    HBitmap *meta;              /* Meta dirty bitmap */
     bool busy;                  /* Bitmap is busy, it can't be used via QMP */
     BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
     char *name;                 /* Optional non-empty unique ID */
@@ -126,36 +125,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
     return bitmap;
 }
 
-/* bdrv_create_meta_dirty_bitmap
- *
- * Create a meta dirty bitmap that tracks the changes of bits in @bitmap. I.e.
- * when a dirty status bit in @bitmap is changed (either from reset to set or
- * the other way around), its respective meta dirty bitmap bit will be marked
- * dirty as well.
- *
- * @bitmap: the block dirty bitmap for which to create a meta dirty bitmap.
- * @chunk_size: how many bytes of bitmap data does each bit in the meta bitmap
- * track.
- */
-void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
-                                   int chunk_size)
-{
-    assert(!bitmap->meta);
-    qemu_mutex_lock(bitmap->mutex);
-    bitmap->meta = hbitmap_create_meta(bitmap->bitmap,
-                                       chunk_size * BITS_PER_BYTE);
-    qemu_mutex_unlock(bitmap->mutex);
-}
-
-void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
-{
-    assert(bitmap->meta);
-    qemu_mutex_lock(bitmap->mutex);
-    hbitmap_free_meta(bitmap->bitmap);
-    bitmap->meta = NULL;
-    qemu_mutex_unlock(bitmap->mutex);
-}
-
 int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap)
 {
     return bitmap->size;
@@ -319,7 +288,6 @@ static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
     assert(!bitmap->active_iterators);
     assert(!bdrv_dirty_bitmap_busy(bitmap));
     assert(!bdrv_dirty_bitmap_has_successor(bitmap));
-    assert(!bitmap->meta);
     QLIST_REMOVE(bitmap, list);
     hbitmap_free(bitmap->bitmap);
     g_free(bitmap->name);
@@ -557,15 +525,6 @@ BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)
     return iter;
 }
 
-BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap)
-{
-    BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
-    hbitmap_iter_init(&iter->hbi, bitmap->meta, 0);
-    iter->bitmap = bitmap;
-    bitmap->active_iterators++;
-    return iter;
-}
-
 void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)
 {
     if (!iter) {
@@ -712,11 +671,6 @@ int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
     return hbitmap_count(bitmap->bitmap);
 }
 
-int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
-{
-    return hbitmap_count(bitmap->meta);
-}
-
 bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
 {
     return bitmap->readonly;
-- 
2.21.0

[Qemu-devel] [PATCH 2/4] block/dirty-bitmap: add bs link

[Vladimir Sementsov-Ogievskiy]

Add bs field to BdrvDirtyBitmap structure. Drop BlockDriverState
parameter from bitmap APIs where possible.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/dirty-bitmap.h   | 14 +++++---------
 block/backup.c                 | 14 ++++++--------
 block/dirty-bitmap.c           | 24 ++++++++++++------------
 block/mirror.c                 |  4 ++--
 block/qcow2-bitmap.c           |  6 +++---
 blockdev.c                     |  6 +++---
 migration/block-dirty-bitmap.c |  7 +++----
 migration/block.c              |  4 ++--
 8 files changed, 36 insertions(+), 43 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 848dfc6590..4c58d922e4 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -18,21 +18,18 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
                                           uint32_t granularity,
                                           const char *name,
                                           Error **errp);
-int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
-                                       BdrvDirtyBitmap *bitmap,
+int bdrv_dirty_bitmap_create_successor(BdrvDirtyBitmap *bitmap,
                                        Error **errp);
-BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
-                                            BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BdrvDirtyBitmap *bitmap,
                                             Error **errp);
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
-                                           BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                                            Error **errp);
 void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
 BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
                                         const char *name);
 int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
                             Error **errp);
-void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_release_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
 void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
                                          const char *name,
@@ -107,8 +104,7 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset,
                                     uint64_t bytes);
 bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
                                        uint64_t *offset, uint64_t *bytes);
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
-                                                  BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
                                                   Error **errp);
 
 #endif
diff --git a/block/backup.c b/block/backup.c
index 763f0d7ff6..acb67da3a7 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -352,7 +352,6 @@ static int coroutine_fn backup_before_write_notify(
 static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
 {
     BdrvDirtyBitmap *bm;
-    BlockDriverState *bs = blk_bs(job->common.blk);
     bool sync = (((ret == 0) || (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS)) \
                  && (job->bitmap_mode != BITMAP_SYNC_MODE_NEVER));
 
@@ -361,13 +360,13 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
          * We succeeded, or we always intended to sync the bitmap.
          * Delete this bitmap and install the child.
          */
-        bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL);
+        bm = bdrv_dirty_bitmap_abdicate(job->sync_bitmap, NULL);
     } else {
         /*
          * We failed, or we never intended to sync the bitmap anyway.
          * Merge the successor back into the parent, keeping all data.
          */
-        bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
+        bm = bdrv_reclaim_dirty_bitmap(job->sync_bitmap, NULL);
     }
 
     assert(bm);
@@ -398,10 +397,9 @@ static void backup_abort(Job *job)
 static void backup_clean(Job *job)
 {
     BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
-    BlockDriverState *bs = blk_bs(s->common.blk);
 
     if (s->copy_bitmap) {
-        bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
+        bdrv_release_dirty_bitmap(s->copy_bitmap);
         s->copy_bitmap = NULL;
     }
 
@@ -679,7 +677,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         }
 
         /* Create a new bitmap, and freeze/disable this one. */
-        if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) {
+        if (bdrv_dirty_bitmap_create_successor(sync_bitmap, errp) < 0) {
             return NULL;
         }
     }
@@ -758,10 +756,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
  error:
     if (copy_bitmap) {
         assert(!job || !job->copy_bitmap);
-        bdrv_release_dirty_bitmap(bs, copy_bitmap);
+        bdrv_release_dirty_bitmap(copy_bitmap);
     }
     if (sync_bitmap) {
-        bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
+        bdrv_reclaim_dirty_bitmap(sync_bitmap, NULL);
     }
     if (job) {
         backup_clean(&job->common.job);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index acfc3077f1..f3dc7b3ca5 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -29,6 +29,7 @@
 
 struct BdrvDirtyBitmap {
     QemuMutex *mutex;
+    BlockDriverState *bs;
     HBitmap *bitmap;            /* Dirty bitmap implementation */
     bool busy;                  /* Bitmap is busy, it can't be used via QMP */
     BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
@@ -114,6 +115,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
         return NULL;
     }
     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;
@@ -236,8 +238,7 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
  * The successor will be enabled if the parent bitmap was.
  * Called with BQL taken.
  */
-int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
-                                       BdrvDirtyBitmap *bitmap, Error **errp)
+int bdrv_dirty_bitmap_create_successor(BdrvDirtyBitmap *bitmap, Error **errp)
 {
     uint64_t granularity;
     BdrvDirtyBitmap *child;
@@ -253,7 +254,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
 
     /* Create an anonymous successor */
     granularity = bdrv_dirty_bitmap_granularity(bitmap);
-    child = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
+    child = bdrv_create_dirty_bitmap(bitmap->bs, granularity, NULL, errp);
     if (!child) {
         return -1;
     }
@@ -299,8 +300,7 @@ static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
  * delete the old bitmap, and return a handle to the new bitmap.
  * Called with BQL taken.
  */
-BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
-                                            BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BdrvDirtyBitmap *bitmap,
                                             Error **errp)
 {
     char *name;
@@ -319,7 +319,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
     successor->persistent = bitmap->persistent;
     bitmap->persistent = false;
     bitmap->busy = false;
-    bdrv_release_dirty_bitmap(bs, bitmap);
+    bdrv_release_dirty_bitmap(bitmap);
 
     return successor;
 }
@@ -331,8 +331,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
  * The marged parent will be enabled if and only if the successor was enabled.
  * Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.
  */
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
-                                                  BdrvDirtyBitmap *parent,
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *parent,
                                                   Error **errp)
 {
     BdrvDirtyBitmap *successor = parent->successor;
@@ -356,14 +355,13 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
 }
 
 /* Called with BQL taken. */
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
-                                           BdrvDirtyBitmap *parent,
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap *parent,
                                            Error **errp)
 {
     BdrvDirtyBitmap *ret;
 
     qemu_mutex_lock(parent->mutex);
-    ret = bdrv_reclaim_dirty_bitmap_locked(bs, parent, errp);
+    ret = bdrv_reclaim_dirty_bitmap_locked(parent, errp);
     qemu_mutex_unlock(parent->mutex);
 
     return ret;
@@ -389,8 +387,10 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes)
 }
 
 /* Called with BQL taken.  */
-void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+void bdrv_release_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
+    BlockDriverState *bs = bitmap->bs;
+
     bdrv_dirty_bitmaps_lock(bs);
     bdrv_release_dirty_bitmap_locked(bitmap);
     bdrv_dirty_bitmaps_unlock(bs);
diff --git a/block/mirror.c b/block/mirror.c
index fe984efb90..a6c50caea4 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -638,7 +638,7 @@ static int mirror_exit_common(Job *job)
         bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs);
     }
 
-    bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
+    bdrv_release_dirty_bitmap(s->dirty_bitmap);
 
     /* Make sure that the source BDS doesn't go away during bdrv_replace_node,
      * before we can call bdrv_drained_end */
@@ -1709,7 +1709,7 @@ fail:
         blk_unref(s->target);
         bs_opaque->job = NULL;
         if (s->dirty_bitmap) {
-            bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
+            bdrv_release_dirty_bitmap(s->dirty_bitmap);
         }
         job_early_fail(&s->common.job);
     }
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b2487101ed..6d795a2255 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -374,7 +374,7 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
 fail:
     g_free(bitmap_table);
     if (bitmap != NULL) {
-        bdrv_release_dirty_bitmap(bs, bitmap);
+        bdrv_release_dirty_bitmap(bitmap);
     }
 
     return NULL;
@@ -941,7 +941,7 @@ fail:
 static void release_dirty_bitmap_helper(gpointer bitmap,
                                         gpointer bs)
 {
-    bdrv_release_dirty_bitmap(bs, bitmap);
+    bdrv_release_dirty_bitmap(bitmap);
 }
 
 /* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */
@@ -1569,7 +1569,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
             continue;
         }
 
-        bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
+        bdrv_release_dirty_bitmap(bm->dirty_bitmap);
     }
 
     bitmap_list_free(bm_list);
diff --git a/blockdev.c b/blockdev.c
index fbef6845c8..a8593fa0c1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2178,7 +2178,7 @@ static void block_dirty_bitmap_remove_commit(BlkActionState *common)
                                              common, common);
 
     bdrv_dirty_bitmap_set_busy(state->bitmap, false);
-    bdrv_release_dirty_bitmap(state->bs, state->bitmap);
+    bdrv_release_dirty_bitmap(state->bitmap);
 }
 
 static void abort_prepare(BlkActionState *common, Error **errp)
@@ -2954,7 +2954,7 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
     }
 
     if (release) {
-        bdrv_release_dirty_bitmap(bs, bitmap);
+        bdrv_release_dirty_bitmap(bitmap);
     }
 
     if (bitmap_bs) {
@@ -3086,7 +3086,7 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(
     bdrv_merge_dirty_bitmap(dst, anon, backup, errp);
 
  out:
-    bdrv_release_dirty_bitmap(bs, anon);
+    bdrv_release_dirty_bitmap(anon);
     return dst;
 }
 
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 5121f86d73..793f249aa5 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -474,7 +474,7 @@ static int dirty_bitmap_load_start(QEMUFile *f, DirtyBitmapLoadState *s)
     if (flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED) {
         DirtyBitmapLoadBitmapState *b;
 
-        bdrv_dirty_bitmap_create_successor(s->bs, s->bitmap, &local_err);
+        bdrv_dirty_bitmap_create_successor(s->bitmap, &local_err);
         if (local_err) {
             error_report_err(local_err);
             return -EINVAL;
@@ -535,13 +535,12 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DirtyBitmapLoadState *s)
         bdrv_dirty_bitmap_lock(s->bitmap);
         if (enabled_bitmaps == NULL) {
             /* in postcopy */
-            bdrv_reclaim_dirty_bitmap_locked(s->bs, s->bitmap, &error_abort);
+            bdrv_reclaim_dirty_bitmap_locked(s->bitmap, &error_abort);
             bdrv_enable_dirty_bitmap_locked(s->bitmap);
         } else {
             /* target not started, successor must be empty */
             int64_t count = bdrv_get_dirty_count(s->bitmap);
-            BdrvDirtyBitmap *ret = bdrv_reclaim_dirty_bitmap_locked(s->bs,
-                                                                    s->bitmap,
+            BdrvDirtyBitmap *ret = bdrv_reclaim_dirty_bitmap_locked(s->bitmap,
                                                                     NULL);
             /* bdrv_reclaim_dirty_bitmap can fail only on no successor (it
              * must be) or on merge fail, but merge can't fail when second
diff --git a/migration/block.c b/migration/block.c
index 0de9d84198..0496b9b66e 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -361,7 +361,7 @@ static int set_dirty_tracking(void)
 fail:
     QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
         if (bmds->dirty_bitmap) {
-            bdrv_release_dirty_bitmap(blk_bs(bmds->blk), bmds->dirty_bitmap);
+            bdrv_release_dirty_bitmap(bmds->dirty_bitmap);
         }
     }
     return ret;
@@ -374,7 +374,7 @@ static void unset_dirty_tracking(void)
     BlkMigDevState *bmds;
 
     QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
-        bdrv_release_dirty_bitmap(blk_bs(bmds->blk), bmds->dirty_bitmap);
+        bdrv_release_dirty_bitmap(bmds->dirty_bitmap);
     }
 }
 
-- 
2.21.0

[Qemu-devel] [PATCH 3/4] block/dirty-bitmap: drop BdrvDirtyBitmap.mutex

[Vladimir Sementsov-Ogievskiy]

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(-)

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);
         }
     }
 
-- 
2.21.0

[Qemu-devel] [PATCH 4/4] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next

[Vladimir Sementsov-Ogievskiy]

bdrv_dirty_bitmap_next is always used in same pattern. So, split it
into _next and _first, instead of combining two functions into one and
add FOR_EACH_DIRTY_BITMAP macro.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/dirty-bitmap.h   |  9 +++++++--
 block.c                        |  4 +---
 block/dirty-bitmap.c           | 11 +++++++----
 block/qcow2-bitmap.c           |  8 ++------
 migration/block-dirty-bitmap.c |  4 +---
 5 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 4c58d922e4..89e52db7ec 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -97,8 +97,13 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
-BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
-                                        BdrvDirtyBitmap *bitmap);
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs);
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap);
+#define FOR_EACH_DIRTY_BITMAP(bs, bitmap) \
+for (bitmap = bdrv_dirty_bitmap_first(bs); bitmap; \
+     bitmap = bdrv_dirty_bitmap_next(bitmap))
+
 char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
 int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset,
                                     uint64_t bytes);
diff --git a/block.c b/block.c
index 5944124845..96c2c5ae2d 100644
--- a/block.c
+++ b/block.c
@@ -5363,9 +5363,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
         }
     }
 
-    for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm;
-         bm = bdrv_dirty_bitmap_next(bs, bm))
-    {
+    FOR_EACH_DIRTY_BITMAP(bs, bm) {
         bdrv_dirty_bitmap_skip_store(bm, false);
     }
 
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 76a8e59e61..e2df82af01 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -742,11 +742,14 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
     return false;
 }
 
-BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
-                                        BdrvDirtyBitmap *bitmap)
+BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs)
 {
-    return bitmap == NULL ? QLIST_FIRST(&bs->dirty_bitmaps) :
-                            QLIST_NEXT(bitmap, list);
+    return QLIST_FIRST(&bs->dirty_bitmaps);
+}
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap)
+{
+    return QLIST_NEXT(bitmap, list);
 }
 
 char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 6d795a2255..73ebd2ff6a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1480,9 +1480,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
     }
 
     /* check constraints and names */
-    for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
-         bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
-    {
+    FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
         const char *name = bdrv_dirty_bitmap_name(bitmap);
         uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
         Qcow2Bitmap *bm;
@@ -1602,9 +1600,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
         return -EINVAL;
     }
 
-    for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
-         bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
-    {
+    FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
         if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
             bdrv_dirty_bitmap_set_readonly(bitmap, true);
         }
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 793f249aa5..7eafface61 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -283,9 +283,7 @@ static int init_dirty_bitmap_migration(void)
     for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) {
         const char *name = bdrv_get_device_or_node_name(bs);
 
-        for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
-             bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
-        {
+        FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
             if (!bdrv_dirty_bitmap_name(bitmap)) {
                 continue;
             }
-- 
2.21.0

Re: [Qemu-devel] [PATCH 1/4] block/dirty-bitmap: drop meta

[John Snow]

On 9/16/19 10:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> Drop meta bitmaps, as they are unused.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

believe it or not, I had a local patch that does the same :)

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 2/4] block/dirty-bitmap: add bs link

[John Snow]

On 9/16/19 10:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add bs field to BdrvDirtyBitmap structure. Drop BlockDriverState
> parameter from bitmap APIs where possible.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

I've thought about doing this before, but couldn't figure out if it was
worth it.

So, let's do it.

makes any functions that take (bs, bitmap) pairs less error-prone.

Reviewed-by: John Snow <jsnow@redhat.com>

> ---
>  include/block/dirty-bitmap.h   | 14 +++++---------
>  block/backup.c                 | 14 ++++++--------
>  block/dirty-bitmap.c           | 24 ++++++++++++------------
>  block/mirror.c                 |  4 ++--
>  block/qcow2-bitmap.c           |  6 +++---
>  blockdev.c                     |  6 +++---
>  migration/block-dirty-bitmap.c |  7 +++----
>  migration/block.c              |  4 ++--
>  8 files changed, 36 insertions(+), 43 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 848dfc6590..4c58d922e4 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -18,21 +18,18 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>                                            uint32_t granularity,
>                                            const char *name,
>                                            Error **errp);
> -int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> -                                       BdrvDirtyBitmap *bitmap,
> +int bdrv_dirty_bitmap_create_successor(BdrvDirtyBitmap *bitmap,
>                                         Error **errp);
> -BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
> -                                            BdrvDirtyBitmap *bitmap,
> +BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BdrvDirtyBitmap *bitmap,
>                                              Error **errp);
> -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> -                                           BdrvDirtyBitmap *bitmap,
> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>                                             Error **errp);
>  void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
>  BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
>                                          const char *name);
>  int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
>                              Error **errp);
> -void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
> +void bdrv_release_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>  void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
>  void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>                                           const char *name,
> @@ -107,8 +104,7 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset,
>                                      uint64_t bytes);
>  bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
>                                         uint64_t *offset, uint64_t *bytes);
> -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
> -                                                  BdrvDirtyBitmap *bitmap,
> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
>                                                    Error **errp);
>  
>  #endif
> diff --git a/block/backup.c b/block/backup.c
> index 763f0d7ff6..acb67da3a7 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -352,7 +352,6 @@ static int coroutine_fn backup_before_write_notify(
>  static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
>  {
>      BdrvDirtyBitmap *bm;
> -    BlockDriverState *bs = blk_bs(job->common.blk);
>      bool sync = (((ret == 0) || (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS)) \
>                   && (job->bitmap_mode != BITMAP_SYNC_MODE_NEVER));
>  
> @@ -361,13 +360,13 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
>           * We succeeded, or we always intended to sync the bitmap.
>           * Delete this bitmap and install the child.
>           */
> -        bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL);
> +        bm = bdrv_dirty_bitmap_abdicate(job->sync_bitmap, NULL);
>      } else {
>          /*
>           * We failed, or we never intended to sync the bitmap anyway.
>           * Merge the successor back into the parent, keeping all data.
>           */
> -        bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
> +        bm = bdrv_reclaim_dirty_bitmap(job->sync_bitmap, NULL);
>      }
>  
>      assert(bm);
> @@ -398,10 +397,9 @@ static void backup_abort(Job *job)
>  static void backup_clean(Job *job)
>  {
>      BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
> -    BlockDriverState *bs = blk_bs(s->common.blk);
>  
>      if (s->copy_bitmap) {
> -        bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
> +        bdrv_release_dirty_bitmap(s->copy_bitmap);
>          s->copy_bitmap = NULL;
>      }
>  
> @@ -679,7 +677,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>          }
>  
>          /* Create a new bitmap, and freeze/disable this one. */
> -        if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) {
> +        if (bdrv_dirty_bitmap_create_successor(sync_bitmap, errp) < 0) {
>              return NULL;
>          }
>      }
> @@ -758,10 +756,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>   error:
>      if (copy_bitmap) {
>          assert(!job || !job->copy_bitmap);
> -        bdrv_release_dirty_bitmap(bs, copy_bitmap);
> +        bdrv_release_dirty_bitmap(copy_bitmap);
>      }
>      if (sync_bitmap) {
> -        bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
> +        bdrv_reclaim_dirty_bitmap(sync_bitmap, NULL);
>      }
>      if (job) {
>          backup_clean(&job->common.job);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index acfc3077f1..f3dc7b3ca5 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -29,6 +29,7 @@
>  
>  struct BdrvDirtyBitmap {
>      QemuMutex *mutex;
> +    BlockDriverState *bs;
>      HBitmap *bitmap;            /* Dirty bitmap implementation */
>      bool busy;                  /* Bitmap is busy, it can't be used via QMP */
>      BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
> @@ -114,6 +115,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>          return NULL;
>      }
>      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;
> @@ -236,8 +238,7 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
>   * The successor will be enabled if the parent bitmap was.
>   * Called with BQL taken.
>   */
> -int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> -                                       BdrvDirtyBitmap *bitmap, Error **errp)
> +int bdrv_dirty_bitmap_create_successor(BdrvDirtyBitmap *bitmap, Error **errp)
>  {
>      uint64_t granularity;
>      BdrvDirtyBitmap *child;
> @@ -253,7 +254,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>  
>      /* Create an anonymous successor */
>      granularity = bdrv_dirty_bitmap_granularity(bitmap);
> -    child = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
> +    child = bdrv_create_dirty_bitmap(bitmap->bs, granularity, NULL, errp);
>      if (!child) {
>          return -1;
>      }
> @@ -299,8 +300,7 @@ static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
>   * delete the old bitmap, and return a handle to the new bitmap.
>   * Called with BQL taken.
>   */
> -BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
> -                                            BdrvDirtyBitmap *bitmap,
> +BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BdrvDirtyBitmap *bitmap,
>                                              Error **errp)
>  {
>      char *name;
> @@ -319,7 +319,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>      successor->persistent = bitmap->persistent;
>      bitmap->persistent = false;
>      bitmap->busy = false;
> -    bdrv_release_dirty_bitmap(bs, bitmap);
> +    bdrv_release_dirty_bitmap(bitmap);
>  
>      return successor;
>  }
> @@ -331,8 +331,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>   * The marged parent will be enabled if and only if the successor was enabled.
>   * Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.
>   */
> -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
> -                                                  BdrvDirtyBitmap *parent,
> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *parent,
>                                                    Error **errp)
>  {
>      BdrvDirtyBitmap *successor = parent->successor;
> @@ -356,14 +355,13 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
>  }
>  
>  /* Called with BQL taken. */
> -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> -                                           BdrvDirtyBitmap *parent,
> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap *parent,
>                                             Error **errp)
>  {
>      BdrvDirtyBitmap *ret;
>  
>      qemu_mutex_lock(parent->mutex);
> -    ret = bdrv_reclaim_dirty_bitmap_locked(bs, parent, errp);
> +    ret = bdrv_reclaim_dirty_bitmap_locked(parent, errp);
>      qemu_mutex_unlock(parent->mutex);
>  
>      return ret;
> @@ -389,8 +387,10 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes)
>  }
>  
>  /* Called with BQL taken.  */
> -void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +void bdrv_release_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>  {
> +    BlockDriverState *bs = bitmap->bs;
> +
>      bdrv_dirty_bitmaps_lock(bs);
>      bdrv_release_dirty_bitmap_locked(bitmap);
>      bdrv_dirty_bitmaps_unlock(bs);
> diff --git a/block/mirror.c b/block/mirror.c
> index fe984efb90..a6c50caea4 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -638,7 +638,7 @@ static int mirror_exit_common(Job *job)
>          bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs);
>      }
>  
> -    bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
> +    bdrv_release_dirty_bitmap(s->dirty_bitmap);
>  
>      /* Make sure that the source BDS doesn't go away during bdrv_replace_node,
>       * before we can call bdrv_drained_end */
> @@ -1709,7 +1709,7 @@ fail:
>          blk_unref(s->target);
>          bs_opaque->job = NULL;
>          if (s->dirty_bitmap) {
> -            bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
> +            bdrv_release_dirty_bitmap(s->dirty_bitmap);
>          }
>          job_early_fail(&s->common.job);
>      }
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index b2487101ed..6d795a2255 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -374,7 +374,7 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
>  fail:
>      g_free(bitmap_table);
>      if (bitmap != NULL) {
> -        bdrv_release_dirty_bitmap(bs, bitmap);
> +        bdrv_release_dirty_bitmap(bitmap);
>      }
>  
>      return NULL;
> @@ -941,7 +941,7 @@ fail:
>  static void release_dirty_bitmap_helper(gpointer bitmap,
>                                          gpointer bs)
>  {
> -    bdrv_release_dirty_bitmap(bs, bitmap);
> +    bdrv_release_dirty_bitmap(bitmap);
>  }
>  
>  /* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */
> @@ -1569,7 +1569,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>              continue;
>          }
>  
> -        bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
> +        bdrv_release_dirty_bitmap(bm->dirty_bitmap);
>      }
>  
>      bitmap_list_free(bm_list);
> diff --git a/blockdev.c b/blockdev.c
> index fbef6845c8..a8593fa0c1 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2178,7 +2178,7 @@ static void block_dirty_bitmap_remove_commit(BlkActionState *common)
>                                               common, common);
>  
>      bdrv_dirty_bitmap_set_busy(state->bitmap, false);
> -    bdrv_release_dirty_bitmap(state->bs, state->bitmap);
> +    bdrv_release_dirty_bitmap(state->bitmap);
>  }
>  
>  static void abort_prepare(BlkActionState *common, Error **errp)
> @@ -2954,7 +2954,7 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
>      }
>  
>      if (release) {
> -        bdrv_release_dirty_bitmap(bs, bitmap);
> +        bdrv_release_dirty_bitmap(bitmap);
>      }
>  
>      if (bitmap_bs) {
> @@ -3086,7 +3086,7 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(
>      bdrv_merge_dirty_bitmap(dst, anon, backup, errp);
>  
>   out:
> -    bdrv_release_dirty_bitmap(bs, anon);
> +    bdrv_release_dirty_bitmap(anon);
>      return dst;
>  }
>  
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 5121f86d73..793f249aa5 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -474,7 +474,7 @@ static int dirty_bitmap_load_start(QEMUFile *f, DirtyBitmapLoadState *s)
>      if (flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED) {
>          DirtyBitmapLoadBitmapState *b;
>  
> -        bdrv_dirty_bitmap_create_successor(s->bs, s->bitmap, &local_err);
> +        bdrv_dirty_bitmap_create_successor(s->bitmap, &local_err);
>          if (local_err) {
>              error_report_err(local_err);
>              return -EINVAL;
> @@ -535,13 +535,12 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DirtyBitmapLoadState *s)
>          bdrv_dirty_bitmap_lock(s->bitmap);
>          if (enabled_bitmaps == NULL) {
>              /* in postcopy */
> -            bdrv_reclaim_dirty_bitmap_locked(s->bs, s->bitmap, &error_abort);
> +            bdrv_reclaim_dirty_bitmap_locked(s->bitmap, &error_abort);
>              bdrv_enable_dirty_bitmap_locked(s->bitmap);
>          } else {
>              /* target not started, successor must be empty */
>              int64_t count = bdrv_get_dirty_count(s->bitmap);
> -            BdrvDirtyBitmap *ret = bdrv_reclaim_dirty_bitmap_locked(s->bs,
> -                                                                    s->bitmap,
> +            BdrvDirtyBitmap *ret = bdrv_reclaim_dirty_bitmap_locked(s->bitmap,
>                                                                      NULL);
>              /* bdrv_reclaim_dirty_bitmap can fail only on no successor (it
>               * must be) or on merge fail, but merge can't fail when second
> diff --git a/migration/block.c b/migration/block.c
> index 0de9d84198..0496b9b66e 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -361,7 +361,7 @@ static int set_dirty_tracking(void)
>  fail:
>      QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
>          if (bmds->dirty_bitmap) {
> -            bdrv_release_dirty_bitmap(blk_bs(bmds->blk), bmds->dirty_bitmap);
> +            bdrv_release_dirty_bitmap(bmds->dirty_bitmap);
>          }
>      }
>      return ret;
> @@ -374,7 +374,7 @@ static void unset_dirty_tracking(void)
>      BlkMigDevState *bmds;
>  
>      QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
> -        bdrv_release_dirty_bitmap(blk_bs(bmds->blk), bmds->dirty_bitmap);
> +        bdrv_release_dirty_bitmap(bmds->dirty_bitmap);
>      }
>  }
>  
> 

-- 
—js

Re: [Qemu-devel] [PATCH 3/4] block/dirty-bitmap: drop BdrvDirtyBitmap.mutex

[John Snow]

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);
>          }
>      }
>  
> 

-- 
—js

Re: [Qemu-devel] [PATCH 4/4] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next

[John Snow]

On 9/16/19 10:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> bdrv_dirty_bitmap_next is always used in same pattern. So, split it
> into _next and _first, instead of combining two functions into one and
> add FOR_EACH_DIRTY_BITMAP macro.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/dirty-bitmap.h   |  9 +++++++--
>  block.c                        |  4 +---
>  block/dirty-bitmap.c           | 11 +++++++----
>  block/qcow2-bitmap.c           |  8 ++------
>  migration/block-dirty-bitmap.c |  4 +---
>  5 files changed, 18 insertions(+), 18 deletions(-)

I'm not as sure that this is an improvement.

> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 4c58d922e4..89e52db7ec 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -97,8 +97,13 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
>  bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
> -BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
> -                                        BdrvDirtyBitmap *bitmap);
> +
> +BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs);
> +BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap);
> +#define FOR_EACH_DIRTY_BITMAP(bs, bitmap) \
> +for (bitmap = bdrv_dirty_bitmap_first(bs); bitmap; \
> +     bitmap = bdrv_dirty_bitmap_next(bitmap))
> +
>  char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
>  int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset,
>                                      uint64_t bytes);
> diff --git a/block.c b/block.c
> index 5944124845..96c2c5ae2d 100644
> --- a/block.c
> +++ b/block.c
> @@ -5363,9 +5363,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
>          }
>      }
>  
> -    for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm;
> -         bm = bdrv_dirty_bitmap_next(bs, bm))
> -    {
> +    FOR_EACH_DIRTY_BITMAP(bs, bm) {
>          bdrv_dirty_bitmap_skip_store(bm, false);
>      }

... and I kind of prefer loops with explicit function calls more than I
like macro-loops.

>  
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 76a8e59e61..e2df82af01 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -742,11 +742,14 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
>      return false;
>  }
>  
> -BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
> -                                        BdrvDirtyBitmap *bitmap)
> +BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs)
>  {
> -    return bitmap == NULL ? QLIST_FIRST(&bs->dirty_bitmaps) :
> -                            QLIST_NEXT(bitmap, list);
> +    return QLIST_FIRST(&bs->dirty_bitmaps);
> +}
> +
> +BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap)
> +{
> +    return QLIST_NEXT(bitmap, list);
>  }
>  
>  char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp)
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 6d795a2255..73ebd2ff6a 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1480,9 +1480,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>      }
>  
>      /* check constraints and names */
> -    for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
> -         bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
> -    {
> +    FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
>          const char *name = bdrv_dirty_bitmap_name(bitmap);
>          uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
>          Qcow2Bitmap *bm;
> @@ -1602,9 +1600,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
>          return -EINVAL;
>      }
>  
> -    for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
> -         bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
> -    {
> +    FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
>          if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
>              bdrv_dirty_bitmap_set_readonly(bitmap, true);
>          }
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 793f249aa5..7eafface61 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -283,9 +283,7 @@ static int init_dirty_bitmap_migration(void)
>      for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) {
>          const char *name = bdrv_get_device_or_node_name(bs);
>  
> -        for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
> -             bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
> -        {
> +        FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
>              if (!bdrv_dirty_bitmap_name(bitmap)) {
>                  continue;
>              }
> 

Well, I guess explicit first and next functions is harder to mess up,
anyway.

Reviewed-by: John Snow <jsnow@redhat.com>

(Any other thoughts?)

Re: [Qemu-devel] [PATCH 4/4] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next

[Eric Blake]

On 9/26/19 1:54 PM, John Snow wrote:
> 
> 
> On 9/16/19 10:19 AM, Vladimir Sementsov-Ogievskiy wrote:
>> bdrv_dirty_bitmap_next is always used in same pattern. So, split it
>> into _next and _first, instead of combining two functions into one and
>> add FOR_EACH_DIRTY_BITMAP macro.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/dirty-bitmap.h   |  9 +++++++--
>>   block.c                        |  4 +---
>>   block/dirty-bitmap.c           | 11 +++++++----
>>   block/qcow2-bitmap.c           |  8 ++------
>>   migration/block-dirty-bitmap.c |  4 +---
>>   5 files changed, 18 insertions(+), 18 deletions(-)
> 
> I'm not as sure that this is an improvement.
> 

>>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>> -BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>> -                                        BdrvDirtyBitmap *bitmap);
>> +
>> +BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs);
>> +BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap);
>> +#define FOR_EACH_DIRTY_BITMAP(bs, bitmap) \
>> +for (bitmap = bdrv_dirty_bitmap_first(bs); bitmap; \
>> +     bitmap = bdrv_dirty_bitmap_next(bitmap))
>> +

If you want the macro, you can do that without splitting the function in 
two:

#define FOR_EACH_DIRTY_BITMAP(bs, bitmap) \
for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap; \
      bitmap = bdrv_dirty_bitmap_next(bs, bitmap))


> 
> Well, I guess explicit first and next functions is harder to mess up,
> anyway.
> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> 
> (Any other thoughts?)

I don't mind the macro as much (since we already use iteration macros 
for QTAILQ_FOREACH and such throughout the codebase, and since it 
somewhat isolates you from the calling conventions of passing NULL to 
prime the iteration), but introducing the macro does not imply that we 
need two functions.  So I think this is, to some extent, a question of 
the maintainer's sense of aesthetics, and not an actual problem in the code.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 4/4] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next

[John Snow]

On 9/26/19 3:28 PM, Eric Blake wrote:
> On 9/26/19 1:54 PM, John Snow wrote:
>>
>>
>> On 9/16/19 10:19 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> bdrv_dirty_bitmap_next is always used in same pattern. So, split it
>>> into _next and _first, instead of combining two functions into one and
>>> add FOR_EACH_DIRTY_BITMAP macro.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   include/block/dirty-bitmap.h   |  9 +++++++--
>>>   block.c                        |  4 +---
>>>   block/dirty-bitmap.c           | 11 +++++++----
>>>   block/qcow2-bitmap.c           |  8 ++------
>>>   migration/block-dirty-bitmap.c |  4 +---
>>>   5 files changed, 18 insertions(+), 18 deletions(-)
>>
>> I'm not as sure that this is an improvement.
>>
> 
>>>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>>> -BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>>> -                                        BdrvDirtyBitmap *bitmap);
>>> +
>>> +BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs);
>>> +BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap);
>>> +#define FOR_EACH_DIRTY_BITMAP(bs, bitmap) \
>>> +for (bitmap = bdrv_dirty_bitmap_first(bs); bitmap; \
>>> +     bitmap = bdrv_dirty_bitmap_next(bitmap))
>>> +
> 
> If you want the macro, you can do that without splitting the function in
> two:
> 
> #define FOR_EACH_DIRTY_BITMAP(bs, bitmap) \
> for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap; \
>      bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
> 
> 
>>
>> Well, I guess explicit first and next functions is harder to mess up,
>> anyway.
>>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>>
>> (Any other thoughts?)
> 
> I don't mind the macro as much (since we already use iteration macros
> for QTAILQ_FOREACH and such throughout the codebase, and since it
> somewhat isolates you from the calling conventions of passing NULL to
> prime the iteration), but introducing the macro does not imply that we
> need two functions.  So I think this is, to some extent, a question of
> the maintainer's sense of aesthetics, and not an actual problem in the
> code.
> 
No harm in taking it and it's easier than not taking it.

Thanks, applied to my bitmaps tree:

https://github.com/jnsnow/qemu/commits/bitmaps
https://github.com/jnsnow/qemu.git

--js

Re: [Qemu-devel] [PATCH 4/4] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next

[Vladimir Sementsov-Ogievskiy]

27.09.2019 0:44, John Snow wrote:
> 
> 
> On 9/26/19 3:28 PM, Eric Blake wrote:
>> On 9/26/19 1:54 PM, John Snow wrote:
>>>
>>>
>>> On 9/16/19 10:19 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> bdrv_dirty_bitmap_next is always used in same pattern. So, split it
>>>> into _next and _first, instead of combining two functions into one and
>>>> add FOR_EACH_DIRTY_BITMAP macro.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    include/block/dirty-bitmap.h   |  9 +++++++--
>>>>    block.c                        |  4 +---
>>>>    block/dirty-bitmap.c           | 11 +++++++----
>>>>    block/qcow2-bitmap.c           |  8 ++------
>>>>    migration/block-dirty-bitmap.c |  4 +---
>>>>    5 files changed, 18 insertions(+), 18 deletions(-)
>>>
>>> I'm not as sure that this is an improvement.
>>>
>>
>>>>    bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>>>> -BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>>>> -                                        BdrvDirtyBitmap *bitmap);
>>>> +
>>>> +BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs);
>>>> +BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap);
>>>> +#define FOR_EACH_DIRTY_BITMAP(bs, bitmap) \
>>>> +for (bitmap = bdrv_dirty_bitmap_first(bs); bitmap; \
>>>> +     bitmap = bdrv_dirty_bitmap_next(bitmap))
>>>> +
>>
>> If you want the macro, you can do that without splitting the function in
>> two:
>>
>> #define FOR_EACH_DIRTY_BITMAP(bs, bitmap) \
>> for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap; \
>>       bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
>>
>>
>>>
>>> Well, I guess explicit first and next functions is harder to mess up,
>>> anyway.
>>>
>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>>
>>> (Any other thoughts?)
>>
>> I don't mind the macro as much (since we already use iteration macros
>> for QTAILQ_FOREACH and such throughout the codebase, and since it
>> somewhat isolates you from the calling conventions of passing NULL to
>> prime the iteration), but introducing the macro does not imply that we
>> need two functions.  So I think this is, to some extent, a question of
>> the maintainer's sense of aesthetics, and not an actual problem in the
>> code.
>>
> No harm in taking it and it's easier than not taking it.

I don't insist, consider last patch as optional.

> 
> Thanks, applied to my bitmaps tree:
> 
> https://github.com/jnsnow/qemu/commits/bitmaps
> https://github.com/jnsnow/qemu.git
> 

Thank you!


-- 
Best regards,
Vladimir