[Qemu-devel] [PATCH 2/7] block: use refcount to manage BlockDriverState lifecycle

[Fam Zheng]

Device attach, NBD server and block job can share a BlockDriverState,
this patch makes them to use refcount to grab/release a bs so they don't
interfere each other with BDS lifecycle.

Local BDS allocation/releasing don't need to use refcount to manage it,
just use bdrv_new() and bdrv_delete().

If BDS is possibly shared with other code (e.g. NBD and guest device),
use bdrv_new to allocate, then bdrv_get_ref to grab it, when job is
done, call bdrv_put_ref() to release it. Don't call bdrv_delete(), since
it's deleted automatically by the last bdrv_put_ref().

Signed-off-by: Fam Zheng <address@hidden>
---
 block-migration.c         |  1 -
 block.c                   | 31 ++++++++++++++++++++-----------
 block/backup.c            |  3 ++-
 block/blkdebug.c          |  1 +
 block/blkverify.c         |  2 ++
 block/mirror.c            |  4 ++--
 block/snapshot.c          |  3 ++-
 block/stream.c            |  2 +-
 block/vvfat.c             |  4 +++-
 blockdev.c                |  5 +++--
 hw/block/xen_disk.c       |  7 +------
 include/block/block_int.h | 16 ++++++++++++++++
 12 files changed, 53 insertions(+), 26 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index 2efb6c0..2154b3a 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -558,7 +558,6 @@ static void blk_mig_cleanup(void)
     while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
         QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);
         bdrv_get_ref(bmds->bs);
-        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 84c3181..d1ce570 100644
--- a/block.c
+++ b/block.c
@@ -740,7 +740,6 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockDriverState *file,
             ret = -EINVAL;
             goto free_and_fail;
         }
-        assert(file != NULL);
         bs->file = file;
         ret = drv->bdrv_open(bs, options, open_flags);
     }
@@ -748,7 +747,6 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockDriverState *file,
     if (ret < 0) {
         goto free_and_fail;
     }
-
     ret = refresh_total_sectors(bs, bs->total_sectors);
     if (ret < 0) {
         goto free_and_fail;
@@ -925,6 +923,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*options)
         bs->open_flags |= BDRV_O_NO_BACKING;
         return ret;
     }
+    bdrv_get_ref(bs->backing_hd);
     return 0;
 }
 
@@ -1068,9 +1067,11 @@ int bdrv_open(BlockDriverState *bs, const char 
*filename, QDict *options,
 
     if (bs->file != file) {
         bdrv_delete(file);
-        file = NULL;
     }
 
+    file = NULL;
+    bdrv_get_ref(bs->file);
+
     /* If there is a backing file, use it */
     if ((flags & BDRV_O_NO_BACKING) == 0) {
         QDict *backing_options;
@@ -1367,7 +1368,7 @@ void bdrv_close(BlockDriverState *bs)
 
     if (bs->drv) {
         if (bs->backing_hd) {
-            bdrv_delete(bs->backing_hd);
+            bdrv_put_ref(bs->backing_hd);
             bs->backing_hd = NULL;
         }
         bs->drv->bdrv_close(bs);
@@ -1391,7 +1392,7 @@ void bdrv_close(BlockDriverState *bs)
         bs->options = NULL;
 
         if (bs->file != NULL) {
-            bdrv_delete(bs->file);
+            bdrv_put_ref(bs->file);
             bs->file = NULL;
         }
     }
@@ -1536,7 +1537,7 @@ 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->refcount == 0);
+    assert(bs_new->refcount <= 1);
     assert(bs_new->io_limits_enabled == false);
     assert(bs_new->block_timer == NULL);
 
@@ -1555,7 +1556,7 @@ 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->refcount == 0);
+    assert(bs_new->refcount <= 1);
     assert(bs_new->io_limits_enabled == false);
     assert(bs_new->block_timer == NULL);
 
@@ -1609,6 +1610,7 @@ int bdrv_attach_dev(BlockDriverState *bs, void *dev)
         return -EBUSY;
     }
     bs->dev = dev;
+    bdrv_get_ref(bs);
     bdrv_iostatus_reset(bs);
     return 0;
 }
@@ -1626,6 +1628,7 @@ void bdrv_detach_dev(BlockDriverState *bs, void *dev)
 {
     assert(bs->dev == dev);
     bs->dev = NULL;
+    bdrv_put_ref(bs);
     bs->dev_ops = NULL;
     bs->dev_opaque = NULL;
     bs->buffer_alignment = 512;
@@ -2102,13 +2105,16 @@ int bdrv_drop_intermediate(BlockDriverState *active, 
BlockDriverState *top,
     if (ret) {
         goto exit;
     }
+    if (new_top_bs->backing_hd) {
+        bdrv_put_ref(new_top_bs->backing_hd);
+    }
     new_top_bs->backing_hd = base_bs;
-
+    bdrv_get_ref(base_bs);
 
     QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) {
         /* so that bdrv_close() does not recursively close the chain */
         intermediate_state->bs->backing_hd = NULL;
-        bdrv_delete(intermediate_state->bs);
+        bdrv_put_ref(intermediate_state->bs);
     }
     ret = 0;
 
@@ -4365,7 +4371,10 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs)
 void bdrv_put_ref(BlockDriverState *bs)
 {
     assert(bs->refcount > 0);
-    bs->refcount--;
+    if (--bs->refcount == 0) {
+        bdrv_close(bs);
+        bdrv_delete(bs);
+    }
 }
 
 void bdrv_get_ref(BlockDriverState *bs)
@@ -4375,7 +4384,7 @@ void bdrv_get_ref(BlockDriverState *bs)
 
 int bdrv_in_use(BlockDriverState *bs)
 {
-    return bs->refcount > 0;
+    return bs->refcount > 1;
 }
 
 void bdrv_iostatus_enable(BlockDriverState *bs)
diff --git a/block/backup.c b/block/backup.c
index 16105d4..4e9f927 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -294,7 +294,7 @@ static void coroutine_fn backup_run(void *opaque)
     hbitmap_free(job->bitmap);
 
     bdrv_iostatus_disable(target);
-    bdrv_delete(target);
+    bdrv_put_ref(target);
 
     block_job_completed(&job->common, ret);
 }
@@ -332,6 +332,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
*target,
         return;
     }
 
+    bdrv_get_ref(target);
     job->on_source_error = on_source_error;
     job->on_target_error = on_target_error;
     job->target = target;
diff --git a/block/blkdebug.c b/block/blkdebug.c
index ccb627a..83c4018 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -389,6 +389,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
*options, int flags)
     if (ret < 0) {
         goto fail;
     }
+    bdrv_get_ref(bs->file);
 
     ret = 0;
 fail:
diff --git a/block/blkverify.c b/block/blkverify.c
index 1d58cc3..fd63f2b 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -145,6 +145,8 @@ static int blkverify_open(BlockDriverState *bs, QDict 
*options, int flags)
         goto fail;
     }
 
+    bdrv_get_ref(bs->file);
+
     /* Open the test file */
     filename = qemu_opt_get(opts, "x-image");
     if (filename == NULL) {
diff --git a/block/mirror.c b/block/mirror.c
index bed4a7e..ed98313 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -300,6 +300,7 @@ static void coroutine_fn mirror_run(void *opaque)
     int ret = 0;
     int n;
 
+    bdrv_get_ref(s->target);
     if (block_job_is_cancelled(&s->common)) {
         goto immediate_exit;
     }
@@ -479,8 +480,7 @@ immediate_exit:
         }
         bdrv_swap(s->target, s->common.bs);
     }
-    bdrv_close(s->target);
-    bdrv_delete(s->target);
+    bdrv_put_ref(s->target);
     block_job_completed(&s->common, ret);
 }
 
diff --git a/block/snapshot.c b/block/snapshot.c
index 6c6d9de..2d864d4 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -99,7 +99,8 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
         ret = bdrv_snapshot_goto(bs->file, snapshot_id);
         open_ret = drv->bdrv_open(bs, NULL, bs->open_flags);
         if (open_ret < 0) {
-            bdrv_delete(bs->file);
+            bdrv_put_ref(bs->file);
+            bs->file = NULL;
             bs->drv = NULL;
             return open_ret;
         }
diff --git a/block/stream.c b/block/stream.c
index 7fe9e48..e611f31 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -68,7 +68,7 @@ static void close_unused_images(BlockDriverState *top, 
BlockDriverState *base,
         unused = intermediate;
         intermediate = intermediate->backing_hd;
         unused->backing_hd = NULL;
-        bdrv_delete(unused);
+        bdrv_put_ref(unused);
     }
     top->backing_hd = base;
 }
diff --git a/block/vvfat.c b/block/vvfat.c
index 87b0279..a73d765 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2938,8 +2938,10 @@ static int enable_write_target(BDRVVVFATState *s)
     ret = bdrv_open(s->qcow, s->qcow_filename, NULL,
             BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow);
     if (ret < 0) {
-       return ret;
+        bdrv_delete(s->qcow);
+        return ret;
     }
+    bdrv_get_ref(s->qcow);
 
 #ifndef _WIN32
     unlink(s->qcow_filename);
diff --git a/blockdev.c b/blockdev.c
index b3a57e0..2c2ea59 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -212,7 +212,6 @@ static void bdrv_format_print(void *opaque, const char 
*name)
 static void drive_uninit(DriveInfo *dinfo)
 {
     qemu_opts_del(dinfo->opts);
-    bdrv_delete(dinfo->bdrv);
     g_free(dinfo->id);
     QTAILQ_REMOVE(&drives, dinfo, next);
     g_free(dinfo->serial);
@@ -894,6 +893,7 @@ static void external_snapshot_prepare(BlkTransactionState 
*common,
     if (ret != 0) {
         error_setg_file_open(errp, -ret, new_image_file);
     }
+    bdrv_get_ref(state->new_bs);
 }
 
 static void external_snapshot_commit(BlkTransactionState *common)
@@ -908,6 +908,7 @@ static void external_snapshot_commit(BlkTransactionState 
*common)
      * don't want to abort all of them if one of them fails the reopen */
     bdrv_reopen(state->new_bs, state->new_bs->open_flags & ~BDRV_O_RDWR,
                 NULL);
+    bdrv_put_ref(state->new_bs);
 }
 
 static void external_snapshot_abort(BlkTransactionState *common)
@@ -915,7 +916,7 @@ static void external_snapshot_abort(BlkTransactionState 
*common)
     ExternalSnapshotState *state =
                              DO_UPCAST(ExternalSnapshotState, common, common);
     if (state->new_bs) {
-        bdrv_delete(state->new_bs);
+        bdrv_put_ref(state->new_bs);
     }
 }
 
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 247f32f..ae17acc 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -910,12 +910,7 @@ static void blk_disconnect(struct XenDevice *xendev)
     struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev, xendev);
 
     if (blkdev->bs) {
-        if (!blkdev->dinfo) {
-            /* close/delete only if we created it ourself */
-            bdrv_close(blkdev->bs);
-            bdrv_detach_dev(blkdev->bs, blkdev);
-            bdrv_delete(blkdev->bs);
-        }
+        bdrv_detach_dev(blkdev->bs, blkdev);
         blkdev->bs = NULL;
     }
     xen_be_unbind_evtchn(&blkdev->xendev);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1ea80e7..92acddf 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -294,6 +294,22 @@ struct BlockDriverState {
     BlockDeviceIoStatus iostatus;
     char device_name[32];
     HBitmap *dirty_bitmap;
+
+    /* Number of global references of this BDS, increased each time when:
+     * - Attached to device
+     * - Used by block job (both source and target)
+     * - NBD exported
+     * - Referenced as backing_hd of other BDS
+     * - Shared by multiple parts of QEMU in any other way
+     * And decreased each time when one of these roles finished.
+     *
+     * Note: Local/temp BDS isn't required to get a refcount. Two ways to
+     * manage the BDS lifecycle:
+     * - If BDS is only accessable locally, use bdrv_new() and bdrv_delete(),
+     *   Just forget refcount.
+     * - bdrv_new() then bdrv_get_ref()/bdrv_put_ref. DON'T call bdrv_delete()
+     *   yourself.
+     */
     int refcount;
     QTAILQ_ENTRY(BlockDriverState) list;
 
-- 
1.8.3.1