[Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard

[Fam Zheng]

Introduce refcnt_soft (soft reference) and refcnt_hard (hard reference)
to BlockDriverState, since in_use mechanism cannot provide proper
management of lifecycle when a BDS is referenced in multiple places
(e.g. pointed to by another bs's backing_hd while also used as a block
job device, in the use case of image fleecing).

The original in_use case is considered a "hard reference" in this patch,
where the bs is busy and should not be used in other tasks that require
a hard reference. (However the interface doesn't force this, caller
still need to call bdrv_in_use() to check by itself.).

A soft reference is implemented but not used yet. It will be used in
following patches to manage the lifecycle together with hard reference.

If bdrv_ref() is called on a BDS, it must be released by exactly the
same numbers of bdrv_unref() with the same "soft/hard" type, and never
call bdrv_delete() directly. If the BDS is only used locally (unnamed),
bdrv_ref/bdrv_unref can be skipped and just use bdrv_delete().

Signed-off-by: Fam Zheng <address@hidden>
---
 block-migration.c               |  4 ++--
 block.c                         | 40 +++++++++++++++++++++++++++++++---------
 blockjob.c                      |  6 +++---
 hw/block/dataplane/virtio-blk.c |  4 ++--
 include/block/block.h           |  5 +++--
 include/block/block_int.h       |  3 ++-
 6 files changed, 43 insertions(+), 19 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index 2fd7699..d558410 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -321,7 +321,7 @@ static void init_blk_migration_it(void *opaque, 
BlockDriverState *bs)
         bmds->shared_base = block_mig_state.shared_base;
         alloc_aio_bitmap(bmds);
         drive_get_ref(drive_get_by_blockdev(bs));
-        bdrv_set_in_use(bs, 1);
+        bdrv_ref(bs, true);
 
         block_mig_state.total_sector_sum += sectors;
 
@@ -557,7 +557,7 @@ static void blk_mig_cleanup(void)
     blk_mig_lock();
     while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
         QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);
-        bdrv_set_in_use(bmds->bs, 0);
+        bdrv_unref(bmds->bs, true);
         drive_put_ref(drive_get_by_blockdev(bmds->bs));
         g_free(bmds->aio_bitmap);
         g_free(bmds);
diff --git a/block.c b/block.c
index b560241..4170ff6 100644
--- a/block.c
+++ b/block.c
@@ -1511,8 +1511,11 @@ static void bdrv_move_feature_fields(BlockDriverState 
*bs_dest,
     /* dirty bitmap */
     bs_dest->dirty_bitmap       = bs_src->dirty_bitmap;
 
+    /* reference count */
+    bs_dest->refcnt_soft        = bs_src->refcnt_soft;
+    bs_dest->refcnt_hard        = bs_src->refcnt_hard;
+
     /* job */
-    bs_dest->in_use             = bs_src->in_use;
     bs_dest->job                = bs_src->job;
 
     /* keep the same entry in bdrv_states */
@@ -1542,7 +1545,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState 
*bs_old)
     assert(bs_new->dirty_bitmap == NULL);
     assert(bs_new->job == NULL);
     assert(bs_new->dev == NULL);
-    assert(bs_new->in_use == 0);
     assert(bs_new->io_limits_enabled == false);
     assert(bs_new->block_timer == NULL);
 
@@ -1561,7 +1563,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState 
*bs_old)
     /* Check a few fields that should remain attached to the device */
     assert(bs_new->dev == NULL);
     assert(bs_new->job == NULL);
-    assert(bs_new->in_use == 0);
     assert(bs_new->io_limits_enabled == false);
     assert(bs_new->block_timer == NULL);
 
@@ -1598,7 +1599,6 @@ void bdrv_delete(BlockDriverState *bs)
 {
     assert(!bs->dev);
     assert(!bs->job);
-    assert(!bs->in_use);
 
     /* remove from list, if necessary */
     bdrv_make_anon(bs);
@@ -4374,15 +4374,37 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs)
     }
 }
 
-void bdrv_set_in_use(BlockDriverState *bs, int in_use)
+/* Get a soft or hard reference to bs */
+void bdrv_ref(BlockDriverState *bs, bool is_hard)
+{
+    if (is_hard) {
+        bs->refcnt_hard++;
+    } else {
+        bs->refcnt_soft++;
+    }
+}
+
+/* Release a previously grabbed reference to bs, need to specify if it is hard
+ * or soft. If after this releasing, both soft and hard reference counts are
+ * zero, the BlockDriverState is deleted. */
+void bdrv_unref(BlockDriverState *bs, bool is_hard)
 {
-    assert(bs->in_use != in_use);
-    bs->in_use = in_use;
+    if (is_hard) {
+        assert(bs->refcnt_hard > 0);
+        bs->refcnt_hard--;
+    } else {
+        assert(bs->refcnt_soft > 0);
+        bs->refcnt_soft--;
+    }
+    if (bs->refcnt_hard == 0 && bs->refcnt_soft == 0) {
+        bdrv_close(bs);
+        bdrv_delete(bs);
+    }
 }
 
-int bdrv_in_use(BlockDriverState *bs)
+bool bdrv_in_use(BlockDriverState *bs)
 {
-    return bs->in_use;
+    return bs->refcnt_hard > 0;
 }
 
 void bdrv_iostatus_enable(BlockDriverState *bs)
diff --git a/blockjob.c b/blockjob.c
index ca80df1..24f07f9 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -45,7 +45,7 @@ void *block_job_create(const BlockJobType *job_type, 
BlockDriverState *bs,
         error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
         return NULL;
     }
-    bdrv_set_in_use(bs, 1);
+    bdrv_ref(bs, true);
 
     job = g_malloc0(job_type->instance_size);
     job->job_type      = job_type;
@@ -63,7 +63,7 @@ void *block_job_create(const BlockJobType *job_type, 
BlockDriverState *bs,
         if (error_is_set(&local_err)) {
             bs->job = NULL;
             g_free(job);
-            bdrv_set_in_use(bs, 0);
+            bdrv_unref(bs, true);
             error_propagate(errp, local_err);
             return NULL;
         }
@@ -79,7 +79,7 @@ void block_job_completed(BlockJob *job, int ret)
     job->cb(job->opaque, ret);
     bs->job = NULL;
     g_free(job);
-    bdrv_set_in_use(bs, 0);
+    bdrv_unref(bs, true);
 }
 
 void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 0356665..3075e84 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -431,7 +431,7 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, 
VirtIOBlkConf *blk,
     s->blk = blk;
 
     /* Prevent block operations that conflict with data plane thread */
-    bdrv_set_in_use(blk->conf.bs, 1);
+    bdrv_ref(blk->conf.bs, true);
 
     error_setg(&s->migration_blocker,
             "x-data-plane does not support migration");
@@ -450,7 +450,7 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
     virtio_blk_data_plane_stop(s);
     migrate_del_blocker(s->migration_blocker);
     error_free(s->migration_blocker);
-    bdrv_set_in_use(s->blk->conf.bs, 0);
+    bdrv_unref(s->blk->conf.bs, true);
     g_free(s);
 }
 
diff --git a/include/block/block.h b/include/block/block.h
index b6b9014..8df8794 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -354,8 +354,9 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs);
 void bdrv_enable_copy_on_read(BlockDriverState *bs);
 void bdrv_disable_copy_on_read(BlockDriverState *bs);
 
-void bdrv_set_in_use(BlockDriverState *bs, int in_use);
-int bdrv_in_use(BlockDriverState *bs);
+void bdrv_ref(BlockDriverState *bs, bool is_hard);
+void bdrv_unref(BlockDriverState *bs, bool is_hard);
+bool bdrv_in_use(BlockDriverState *bs);
 
 #ifdef CONFIG_LINUX_AIO
 int raw_get_aio_fd(BlockDriverState *bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c6ac871..9de5a8f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -294,7 +294,8 @@ struct BlockDriverState {
     BlockDeviceIoStatus iostatus;
     char device_name[32];
     HBitmap *dirty_bitmap;
-    int in_use; /* users other than guest access, eg. block migration */
+    int refcnt_soft;
+    int refcnt_hard;
     QTAILQ_ENTRY(BlockDriverState) list;
 
     QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
-- 
1.8.3.2