[Qemu-devel] [PATCH v3 0/7] Implement reference count for BlockDriverState

[Qemu-devel] [PATCH v3 0/7] Implement reference count for BlockDriverState

[Fam Zheng]

BlockDriverState lifecycle management is needed by future features such as
image fleecing and blockdev-add. This series adds reference count to
BlockDriverState.

The first two patches clean up two odd BlockDriverState use cases, so all code
uses bdrv_new() to create BlockDriverState instance.

Then implemented bdrv_ref() and bdrv_unref() to operate on refcnt: Initially,
refcnt is 1, which means bdrv_unref is effectively a bdrv_delete() here. So
patch 04 has a search and replace to convert bdrv_delete to bdrv_unref, before
bdrv_ref is used anywhere. 05~08 patches calls bdrv_ref for device attach,
block-migration and nbd.

The rule is: Either bdrv_ref() or bdrv_new() must have a matching
bdrv_unref() call, and the last matching bdrv_unref deletes the bs.

v3:
    03: Removed unnecessary bdrv_close() call.

v2:
    05: Removed: "block: use BlockDriverState refcnt for device attach/detach"
    07: Fix xen_disk blk_disconnect() as it depended on device attach refcnt.

Fam Zheng (7):
  vvfat: use bdrv_new() to allocate BlockDriverState
  iscsi: use bdrv_new() instead of stack structure
  block: implement reference count for BlockDriverState
  block: make bdrv_delete() static
  migration: omit drive ref as we have bdrv_ref now
  xen_disk: simplify blk_disconnect with refcnt
  nbd: use BlockDriverState refcnt

 block-migration.c         |  4 ++--
 block.c                   | 44 +++++++++++++++++++++++++++++++++-----------
 block/backup.c            |  2 +-
 block/blkverify.c         |  4 ++--
 block/cow.c               |  2 +-
 block/iscsi.c             | 14 +++++++-------
 block/mirror.c            |  2 +-
 block/qcow.c              |  2 +-
 block/qcow2.c             |  2 +-
 block/qed.c               |  2 +-
 block/sheepdog.c          |  6 +++---
 block/snapshot.c          |  2 +-
 block/stream.c            |  2 +-
 block/vmdk.c              | 10 +++++-----
 block/vvfat.c             |  6 +++---
 blockdev-nbd.c            | 10 +---------
 blockdev.c                | 14 +++++++-------
 hw/block/xen_disk.c       | 13 ++++++-------
 include/block/block.h     |  3 ++-
 include/block/block_int.h |  1 +
 nbd.c                     |  5 +++++
 qemu-img.c                | 26 +++++++++++++-------------
 qemu-io.c                 |  6 +++---
 23 files changed, 101 insertions(+), 81 deletions(-)

-- 
1.8.3.4

[Qemu-devel] [PATCH v3 1/7] vvfat: use bdrv_new() to allocate BlockDriverState

[Fam Zheng]

we need bdrv_new() to properly initialize BDS, don't allocate memory
manually.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vvfat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index cd3b8ed..a827d91 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2943,7 +2943,7 @@ static int enable_write_target(BDRVVVFATState *s)
     unlink(s->qcow_filename);
 #endif
 
-    s->bs->backing_hd = calloc(sizeof(BlockDriverState), 1);
+    s->bs->backing_hd = bdrv_new("");
     s->bs->backing_hd->drv = &vvfat_write_target;
     s->bs->backing_hd->opaque = g_malloc(sizeof(void*));
     *(void**)s->bs->backing_hd->opaque = s;
-- 
1.8.3.4

[Qemu-devel] [PATCH v3 2/7] iscsi: use bdrv_new() instead of stack structure

[Fam Zheng]

BlockDriverState structure needs bdrv_new() to initialize refcnt, don't
allocate a local structure variable and memset to 0, becasue with coming
refcnt implementation, bdrv_unref will crash if bs->refcnt not
initialized to 1.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/iscsi.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 5f28c6a..db8a699 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1247,11 +1247,11 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
 {
     int ret = 0;
     int64_t total_size = 0;
-    BlockDriverState bs;
+    BlockDriverState *bs;
     IscsiLun *iscsilun = NULL;
     QDict *bs_options;
 
-    memset(&bs, 0, sizeof(BlockDriverState));
+    bs = bdrv_new("");
 
     /* Read out options */
     while (options && options->name) {
@@ -1261,12 +1261,12 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
         options++;
     }
 
-    bs.opaque = g_malloc0(sizeof(struct IscsiLun));
-    iscsilun = bs.opaque;
+    bs->opaque = g_malloc0(sizeof(struct IscsiLun));
+    iscsilun = bs->opaque;
 
     bs_options = qdict_new();
     qdict_put(bs_options, "filename", qstring_from_str(filename));
-    ret = iscsi_open(&bs, bs_options, 0);
+    ret = iscsi_open(bs, bs_options, 0);
     QDECREF(bs_options);
 
     if (ret != 0) {
@@ -1280,7 +1280,7 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
         ret = -ENODEV;
         goto out;
     }
-    if (bs.total_sectors < total_size) {
+    if (bs->total_sectors < total_size) {
         ret = -ENOSPC;
         goto out;
     }
@@ -1290,7 +1290,7 @@ out:
     if (iscsilun->iscsi != NULL) {
         iscsi_destroy_context(iscsilun->iscsi);
     }
-    g_free(bs.opaque);
+    bdrv_delete(bs);
     return ret;
 }
 
-- 
1.8.3.4

[Qemu-devel] [PATCH v3 3/7] block: implement reference count for BlockDriverState

[Fam Zheng]

Introduce bdrv_ref/bdrv_unref to manage the lifecycle of
BlockDriverState. They are unused for now but will used to replace
bdrv_delete() later.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c                   | 21 +++++++++++++++++++++
 include/block/block.h     |  2 ++
 include/block/block_int.h |  1 +
 3 files changed, 24 insertions(+)

diff --git a/block.c b/block.c
index c77cfd1..8f0d646 100644
--- a/block.c
+++ b/block.c
@@ -306,6 +306,7 @@ BlockDriverState *bdrv_new(const char *device_name)
     bdrv_iostatus_disable(bs);
     notifier_list_init(&bs->close_notifiers);
     notifier_with_return_list_init(&bs->before_write_notifiers);
+    bs->refcnt = 1;
 
     return bs;
 }
@@ -1518,6 +1519,9 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
     /* dirty bitmap */
     bs_dest->dirty_bitmap       = bs_src->dirty_bitmap;
 
+    /* reference count */
+    bs_dest->refcnt             = bs_src->refcnt;
+
     /* job */
     bs_dest->in_use             = bs_src->in_use;
     bs_dest->job                = bs_src->job;
@@ -4392,6 +4396,23 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs)
     }
 }
 
+/* Get a reference to bs */
+void bdrv_ref(BlockDriverState *bs)
+{
+    bs->refcnt++;
+}
+
+/* Release a previously grabbed reference to bs.
+ * If after releasing, reference count is zero, the BlockDriverState is
+ * deleted. */
+void bdrv_unref(BlockDriverState *bs)
+{
+    assert(bs->refcnt > 0);
+    if (--bs->refcnt == 0) {
+        bdrv_delete(bs);
+    }
+}
+
 void bdrv_set_in_use(BlockDriverState *bs, int in_use)
 {
     assert(bs->in_use != in_use);
diff --git a/include/block/block.h b/include/block/block.h
index 742fce5..b33ef62 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -356,6 +356,8 @@ 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_ref(BlockDriverState *bs);
+void bdrv_unref(BlockDriverState *bs);
 void bdrv_set_in_use(BlockDriverState *bs, int in_use);
 int bdrv_in_use(BlockDriverState *bs);
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index e45f2a0..1f85cfb 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -294,6 +294,7 @@ struct BlockDriverState {
     BlockDeviceIoStatus iostatus;
     char device_name[32];
     HBitmap *dirty_bitmap;
+    int refcnt;
     int in_use; /* users other than guest access, eg. block migration */
     QTAILQ_ENTRY(BlockDriverState) list;
 
-- 
1.8.3.4

[Qemu-devel] [PATCH v3 4/7] block: make bdrv_delete() static

[Fam Zheng]

Manage BlockDriverState lifecycle with refcnt, so bdrv_delete() is no
longer public and should be called by bdrv_unref() if refcnt is
decreased to 0.

This is an identical change because effectively, there's no multiple
reference of BDS now: no caller of bdrv_ref() yet, only bdrv_new() sets
bs->refcnt to 1, so all bdrv_unref() now actually delete the BDS.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c               | 23 ++++++++++++-----------
 block/backup.c        |  2 +-
 block/blkverify.c     |  4 ++--
 block/cow.c           |  2 +-
 block/iscsi.c         |  2 +-
 block/mirror.c        |  2 +-
 block/qcow.c          |  2 +-
 block/qcow2.c         |  2 +-
 block/qed.c           |  2 +-
 block/sheepdog.c      |  6 +++---
 block/snapshot.c      |  2 +-
 block/stream.c        |  2 +-
 block/vmdk.c          | 10 +++++-----
 block/vvfat.c         |  4 ++--
 blockdev.c            | 14 +++++++-------
 hw/block/xen_disk.c   |  4 ++--
 include/block/block.h |  1 -
 qemu-img.c            | 26 +++++++++++++-------------
 qemu-io.c             |  6 +++---
 19 files changed, 58 insertions(+), 58 deletions(-)

diff --git a/block.c b/block.c
index 8f0d646..285c065 100644
--- a/block.c
+++ b/block.c
@@ -877,7 +877,7 @@ fail:
     if (!bs->drv) {
         QDECREF(bs->options);
     }
-    bdrv_delete(bs);
+    bdrv_unref(bs);
     return ret;
 }
 
@@ -928,7 +928,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options)
                     *backing_filename ? backing_filename : NULL, options,
                     back_flags, back_drv);
     if (ret < 0) {
-        bdrv_delete(bs->backing_hd);
+        bdrv_unref(bs->backing_hd);
         bs->backing_hd = NULL;
         bs->open_flags |= BDRV_O_NO_BACKING;
         return ret;
@@ -1003,12 +1003,12 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
         bs1 = bdrv_new("");
         ret = bdrv_open(bs1, filename, NULL, 0, drv);
         if (ret < 0) {
-            bdrv_delete(bs1);
+            bdrv_unref(bs1);
             goto fail;
         }
         total_size = bdrv_getlength(bs1) & BDRV_SECTOR_MASK;
 
-        bdrv_delete(bs1);
+        bdrv_unref(bs1);
 
         ret = get_tmp_filename(tmp_filename, sizeof(tmp_filename));
         if (ret < 0) {
@@ -1082,7 +1082,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     }
 
     if (bs->file != file) {
-        bdrv_delete(file);
+        bdrv_unref(file);
         file = NULL;
     }
 
@@ -1122,7 +1122,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
 
 unlink_and_fail:
     if (file != NULL) {
-        bdrv_delete(file);
+        bdrv_unref(file);
     }
     if (bs->is_temporary) {
         unlink(filename);
@@ -1383,7 +1383,7 @@ void bdrv_close(BlockDriverState *bs)
 
     if (bs->drv) {
         if (bs->backing_hd) {
-            bdrv_delete(bs->backing_hd);
+            bdrv_unref(bs->backing_hd);
             bs->backing_hd = NULL;
         }
         bs->drv->bdrv_close(bs);
@@ -1407,7 +1407,7 @@ void bdrv_close(BlockDriverState *bs)
         bs->options = NULL;
 
         if (bs->file != NULL) {
-            bdrv_delete(bs->file);
+            bdrv_unref(bs->file);
             bs->file = NULL;
         }
     }
@@ -1605,11 +1605,12 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
             bs_new->drv ? bs_new->drv->format_name : "");
 }
 
-void bdrv_delete(BlockDriverState *bs)
+static void bdrv_delete(BlockDriverState *bs)
 {
     assert(!bs->dev);
     assert(!bs->job);
     assert(!bs->in_use);
+    assert(!bs->refcnt);
 
     /* remove from list, if necessary */
     bdrv_make_anon(bs);
@@ -2125,7 +2126,7 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
     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_unref(intermediate_state->bs);
     }
     ret = 0;
 
@@ -4626,7 +4627,7 @@ out:
     free_option_parameters(param);
 
     if (bs) {
-        bdrv_delete(bs);
+        bdrv_unref(bs);
     }
 }
 
diff --git a/block/backup.c b/block/backup.c
index 6ae8a05..586d941 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -338,7 +338,7 @@ static void coroutine_fn backup_run(void *opaque)
     hbitmap_free(job->bitmap);
 
     bdrv_iostatus_disable(target);
-    bdrv_delete(target);
+    bdrv_unref(target);
 
     block_job_completed(&job->common, ret);
 }
diff --git a/block/blkverify.c b/block/blkverify.c
index 1d58cc3..c4e961e 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -155,7 +155,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags)
     s->test_file = bdrv_new("");
     ret = bdrv_open(s->test_file, filename, NULL, flags, NULL);
     if (ret < 0) {
-        bdrv_delete(s->test_file);
+        bdrv_unref(s->test_file);
         s->test_file = NULL;
         goto fail;
     }
@@ -169,7 +169,7 @@ static void blkverify_close(BlockDriverState *bs)
 {
     BDRVBlkverifyState *s = bs->opaque;
 
-    bdrv_delete(s->test_file);
+    bdrv_unref(s->test_file);
     s->test_file = NULL;
 }
 
diff --git a/block/cow.c b/block/cow.c
index 1cc2e89..767639c 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -314,7 +314,7 @@ static int cow_create(const char *filename, QEMUOptionParameter *options)
     }
 
 exit:
-    bdrv_delete(cow_bs);
+    bdrv_unref(cow_bs);
     return ret;
 }
 
diff --git a/block/iscsi.c b/block/iscsi.c
index db8a699..cf7da49 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1290,7 +1290,7 @@ out:
     if (iscsilun->iscsi != NULL) {
         iscsi_destroy_context(iscsilun->iscsi);
     }
-    bdrv_delete(bs);
+    bdrv_unref(bs);
     return ret;
 }
 
diff --git a/block/mirror.c b/block/mirror.c
index bed4a7e..6662f55 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -480,7 +480,7 @@ immediate_exit:
         bdrv_swap(s->target, s->common.bs);
     }
     bdrv_close(s->target);
-    bdrv_delete(s->target);
+    bdrv_unref(s->target);
     block_job_completed(&s->common, ret);
 }
 
diff --git a/block/qcow.c b/block/qcow.c
index 5239bd6..6b891ac 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -751,7 +751,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options)
     g_free(tmp);
     ret = 0;
 exit:
-    bdrv_delete(qcow_bs);
+    bdrv_unref(qcow_bs);
     return ret;
 }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 3376901..c236505 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1390,7 +1390,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
 
     ret = 0;
 out:
-    bdrv_delete(bs);
+    bdrv_unref(bs);
     return ret;
 }
 
diff --git a/block/qed.c b/block/qed.c
index f767b05..0e1b767 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -599,7 +599,7 @@ static int qed_create(const char *filename, uint32_t cluster_size,
     ret = 0; /* success */
 out:
     g_free(l1_table);
-    bdrv_delete(bs);
+    bdrv_unref(bs);
     return ret;
 }
 
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 6a41ad9..3ac925b 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1447,7 +1447,7 @@ static int sd_prealloc(const char *filename)
     }
 out:
     if (bs) {
-        bdrv_delete(bs);
+        bdrv_unref(bs);
     }
     g_free(buf);
 
@@ -1526,13 +1526,13 @@ static int sd_create(const char *filename, QEMUOptionParameter *options)
 
         if (!is_snapshot(&s->inode)) {
             error_report("cannot clone from a non snapshot vdi");
-            bdrv_delete(bs);
+            bdrv_unref(bs);
             ret = -EINVAL;
             goto out;
         }
 
         base_vid = s->inode.vdi_id;
-        bdrv_delete(bs);
+        bdrv_unref(bs);
     }
 
     ret = do_sd_create(s, vdi, vdi_size, base_vid, &vid, 0);
diff --git a/block/snapshot.c b/block/snapshot.c
index 6c6d9de..8f61cc0 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -99,7 +99,7 @@ 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_unref(bs->file);
             bs->drv = NULL;
             return open_ret;
         }
diff --git a/block/stream.c b/block/stream.c
index 7fe9e48..97cc14b 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_unref(unused);
     }
     top->backing_hd = base;
 }
diff --git a/block/vmdk.c b/block/vmdk.c
index 3756333..cc34708 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -215,7 +215,7 @@ static void vmdk_free_extents(BlockDriverState *bs)
         g_free(e->l2_cache);
         g_free(e->l1_backup_table);
         if (e->file != bs->file) {
-            bdrv_delete(e->file);
+            bdrv_unref(e->file);
         }
     }
     g_free(s->extents);
@@ -709,7 +709,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
             /* SPARSE extent */
             ret = vmdk_open_sparse(bs, extent_file, bs->open_flags);
             if (ret) {
-                bdrv_delete(extent_file);
+                bdrv_unref(extent_file);
                 return ret;
             }
         } else {
@@ -1589,15 +1589,15 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
         BlockDriverState *bs = bdrv_new("");
         ret = bdrv_open(bs, backing_file, NULL, 0, NULL);
         if (ret != 0) {
-            bdrv_delete(bs);
+            bdrv_unref(bs);
             return ret;
         }
         if (strcmp(bs->drv->format_name, "vmdk")) {
-            bdrv_delete(bs);
+            bdrv_unref(bs);
             return -EINVAL;
         }
         parent_cid = vmdk_read_cid(bs, 0);
-        bdrv_delete(bs);
+        bdrv_unref(bs);
         snprintf(parent_desc_line, sizeof(parent_desc_line),
                 "parentFileNameHint=\"%s\"", backing_file);
     }
diff --git a/block/vvfat.c b/block/vvfat.c
index a827d91..2178a13 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2894,7 +2894,7 @@ static int write_target_commit(BlockDriverState *bs, int64_t sector_num,
 
 static void write_target_close(BlockDriverState *bs) {
     BDRVVVFATState* s = *((BDRVVVFATState**) bs->opaque);
-    bdrv_delete(s->qcow);
+    bdrv_unref(s->qcow);
     g_free(s->qcow_filename);
 }
 
@@ -2935,7 +2935,7 @@ 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) {
-        bdrv_delete(s->qcow);
+        bdrv_unref(s->qcow);
         goto err;
     }
 
diff --git a/blockdev.c b/blockdev.c
index 7879e85..1f32087 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -212,7 +212,7 @@ 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);
+    bdrv_unref(dinfo->bdrv);
     g_free(dinfo->id);
     QTAILQ_REMOVE(&drives, dinfo, next);
     g_free(dinfo->serial);
@@ -723,7 +723,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
 err:
     qemu_opts_del(opts);
     QDECREF(bs_opts);
-    bdrv_delete(dinfo->bdrv);
+    bdrv_unref(dinfo->bdrv);
     g_free(dinfo->id);
     QTAILQ_REMOVE(&drives, dinfo, next);
     g_free(dinfo);
@@ -973,7 +973,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_unref(state->new_bs);
     }
 }
 
@@ -1576,7 +1576,7 @@ void qmp_drive_backup(const char *device, const char *target,
     target_bs = bdrv_new("");
     ret = bdrv_open(target_bs, target, NULL, flags, drv);
     if (ret < 0) {
-        bdrv_delete(target_bs);
+        bdrv_unref(target_bs);
         error_setg_file_open(errp, -ret, target);
         return;
     }
@@ -1584,7 +1584,7 @@ void qmp_drive_backup(const char *device, const char *target,
     backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
                  block_job_cb, bs, &local_err);
     if (local_err != NULL) {
-        bdrv_delete(target_bs);
+        bdrv_unref(target_bs);
         error_propagate(errp, local_err);
         return;
     }
@@ -1716,7 +1716,7 @@ void qmp_drive_mirror(const char *device, const char *target,
     target_bs = bdrv_new("");
     ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv);
     if (ret < 0) {
-        bdrv_delete(target_bs);
+        bdrv_unref(target_bs);
         error_setg_file_open(errp, -ret, target);
         return;
     }
@@ -1725,7 +1725,7 @@ void qmp_drive_mirror(const char *device, const char *target,
                  on_source_error, on_target_error,
                  block_job_cb, bs, &local_err);
     if (local_err != NULL) {
-        bdrv_delete(target_bs);
+        bdrv_unref(target_bs);
         error_propagate(errp, local_err);
         return;
     }
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 247f32f..99537e8 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -801,7 +801,7 @@ static int blk_connect(struct XenDevice *xendev)
                                                            readonly);
             if (bdrv_open(blkdev->bs,
                           blkdev->filename, NULL, qflags, drv) != 0) {
-                bdrv_delete(blkdev->bs);
+                bdrv_unref(blkdev->bs);
                 blkdev->bs = NULL;
             }
         }
@@ -914,7 +914,7 @@ static void blk_disconnect(struct XenDevice *xendev)
             /* close/delete only if we created it ourself */
             bdrv_close(blkdev->bs);
             bdrv_detach_dev(blkdev->bs, blkdev);
-            bdrv_delete(blkdev->bs);
+            bdrv_unref(blkdev->bs);
         }
         blkdev->bs = NULL;
     }
diff --git a/include/block/block.h b/include/block/block.h
index b33ef62..1929355 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -123,7 +123,6 @@ BlockDriverState *bdrv_new(const char *device_name);
 void bdrv_make_anon(BlockDriverState *bs);
 void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
 void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
-void bdrv_delete(BlockDriverState *bs);
 int bdrv_parse_cache_flags(const char *mode, int *flags);
 int bdrv_parse_discard_flags(const char *mode, int *flags);
 int bdrv_file_open(BlockDriverState **pbs, const char *filename,
diff --git a/qemu-img.c b/qemu-img.c
index c55ca5c..463e04e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -298,7 +298,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
     return bs;
 fail:
     if (bs) {
-        bdrv_delete(bs);
+        bdrv_unref(bs);
     }
     return NULL;
 }
@@ -649,7 +649,7 @@ static int img_check(int argc, char **argv)
 
 fail:
     qapi_free_ImageCheck(check);
-    bdrv_delete(bs);
+    bdrv_unref(bs);
 
     return ret;
 }
@@ -719,7 +719,7 @@ static int img_commit(int argc, char **argv)
         break;
     }
 
-    bdrv_delete(bs);
+    bdrv_unref(bs);
     if (ret) {
         return 1;
     }
@@ -1101,11 +1101,11 @@ static int img_compare(int argc, char **argv)
     ret = 0;
 
 out:
-    bdrv_delete(bs2);
+    bdrv_unref(bs2);
     qemu_vfree(buf1);
     qemu_vfree(buf2);
 out2:
-    bdrv_delete(bs1);
+    bdrv_unref(bs1);
 out3:
     qemu_progress_end();
     return ret;
@@ -1535,12 +1535,12 @@ out:
     free_option_parameters(param);
     qemu_vfree(buf);
     if (out_bs) {
-        bdrv_delete(out_bs);
+        bdrv_unref(out_bs);
     }
     if (bs) {
         for (bs_i = 0; bs_i < bs_n; bs_i++) {
             if (bs[bs_i]) {
-                bdrv_delete(bs[bs_i]);
+                bdrv_unref(bs[bs_i]);
             }
         }
         g_free(bs);
@@ -1678,7 +1678,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
         *last = elem;
         last = &elem->next;
 
-        bdrv_delete(bs);
+        bdrv_unref(bs);
 
         filename = fmt = NULL;
         if (chain) {
@@ -1892,7 +1892,7 @@ static int img_snapshot(int argc, char **argv)
     }
 
     /* Cleanup */
-    bdrv_delete(bs);
+    bdrv_unref(bs);
     if (ret) {
         return 1;
     }
@@ -2167,14 +2167,14 @@ out:
     /* Cleanup */
     if (!unsafe) {
         if (bs_old_backing != NULL) {
-            bdrv_delete(bs_old_backing);
+            bdrv_unref(bs_old_backing);
         }
         if (bs_new_backing != NULL) {
-            bdrv_delete(bs_new_backing);
+            bdrv_unref(bs_new_backing);
         }
     }
 
-    bdrv_delete(bs);
+    bdrv_unref(bs);
     if (ret) {
         return 1;
     }
@@ -2297,7 +2297,7 @@ static int img_resize(int argc, char **argv)
     }
 out:
     if (bs) {
-        bdrv_delete(bs);
+        bdrv_unref(bs);
     }
     if (ret) {
         return 1;
diff --git a/qemu-io.c b/qemu-io.c
index cb9def5..1de70b4 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -32,7 +32,7 @@ static char **cmdline;
 
 static int close_f(BlockDriverState *bs, int argc, char **argv)
 {
-    bdrv_delete(bs);
+    bdrv_unref(bs);
     qemuio_bs = NULL;
     return 0;
 }
@@ -61,7 +61,7 @@ static int openfile(char *name, int flags, int growable)
 
         if (bdrv_open(qemuio_bs, name, NULL, flags, NULL) < 0) {
             fprintf(stderr, "%s: can't open device %s\n", progname, name);
-            bdrv_delete(qemuio_bs);
+            bdrv_unref(qemuio_bs);
             qemuio_bs = NULL;
             return 1;
         }
@@ -418,7 +418,7 @@ int main(int argc, char **argv)
     bdrv_drain_all();
 
     if (qemuio_bs) {
-        bdrv_delete(qemuio_bs);
+        bdrv_unref(qemuio_bs);
     }
     return 0;
 }
-- 
1.8.3.4

[Qemu-devel] [PATCH v3 5/7] migration: omit drive ref as we have bdrv_ref now

[Fam Zheng]

block-migration.c does not actually use DriveInfo anywhere.  Hence it's
safe to drive ref code, we really only care about referencing BDS.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block-migration.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index f803f20..daf9ec1 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -336,8 +336,8 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
         bmds->completed_sectors = 0;
         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);
 
         block_mig_state.total_sector_sum += sectors;
 
@@ -575,7 +575,7 @@ 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_set_in_use(bmds->bs, 0);
-        drive_put_ref(drive_get_by_blockdev(bmds->bs));
+        bdrv_unref(bmds->bs);
         g_free(bmds->aio_bitmap);
         g_free(bmds);
     }
-- 
1.8.3.4

[Qemu-devel] [PATCH v3 6/7] xen_disk: simplify blk_disconnect with refcnt

[Fam Zheng]

We call bdrv_attach_dev when initializing whether or not bs is created
locally, so call bdrv_detach_dev and let the refcnt handle the
lifecycle.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/xen_disk.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 99537e8..39757d9 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -812,6 +812,9 @@ static int blk_connect(struct XenDevice *xendev)
         /* setup via qemu cmdline -> already setup for us */
         xen_be_printf(&blkdev->xendev, 2, "get configured bdrv (cmdline setup)\n");
         blkdev->bs = blkdev->dinfo->bdrv;
+        /* blkdev->bs is not create by us, we get a reference
+         * so we can bdrv_unref() unconditionally */
+        bdrv_ref(blkdev->bs);
     }
     bdrv_attach_dev_nofail(blkdev->bs, blkdev);
     blkdev->file_size = bdrv_getlength(blkdev->bs);
@@ -910,12 +913,8 @@ 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_unref(blkdev->bs);
-        }
+        bdrv_detach_dev(blkdev->bs, blkdev);
+        bdrv_unref(blkdev->bs);
         blkdev->bs = NULL;
     }
     xen_be_unbind_evtchn(&blkdev->xendev);
-- 
1.8.3.4

[Qemu-devel] [PATCH v3 7/7] nbd: use BlockDriverState refcnt

[Fam Zheng]

Previously, nbd calls drive_get_ref() on the drive of bs. A BDS doesn't
always have associated dinfo, which nbd doesn't care either. We already
have BDS ref count, so use it to make it safe for a BDS w/o blockdev.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev-nbd.c | 10 +---------
 nbd.c          |  5 +++++
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 95f10c8..922cf56 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -69,12 +69,6 @@ static void nbd_close_notifier(Notifier *n, void *data)
     g_free(cn);
 }
 
-static void nbd_server_put_ref(NBDExport *exp)
-{
-    BlockDriverState *bs = nbd_export_get_blockdev(exp);
-    drive_put_ref(drive_get_by_blockdev(bs));
-}
-
 void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
                         Error **errp)
 {
@@ -105,11 +99,9 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
         writable = false;
     }
 
-    exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY,
-                         nbd_server_put_ref);
+    exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, NULL);
 
     nbd_export_set_name(exp, device);
-    drive_get_ref(drive_get_by_blockdev(bs));
 
     n = g_malloc0(sizeof(NBDCloseNotifier));
     n->n.notify = nbd_close_notifier;
diff --git a/nbd.c b/nbd.c
index 2606403..f258cdd 100644
--- a/nbd.c
+++ b/nbd.c
@@ -881,6 +881,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset,
     exp->nbdflags = nbdflags;
     exp->size = size == -1 ? bdrv_getlength(bs) : size;
     exp->close = close;
+    bdrv_ref(bs);
     return exp;
 }
 
@@ -927,6 +928,10 @@ void nbd_export_close(NBDExport *exp)
     }
     nbd_export_set_name(exp, NULL);
     nbd_export_put(exp);
+    if (exp->bs) {
+        bdrv_unref(exp->bs);
+        exp->bs = NULL;
+    }
 }
 
 void nbd_export_get(NBDExport *exp)
-- 
1.8.3.4

Re: [Qemu-devel] [PATCH v3 0/7] Implement reference count for BlockDriverState

[Stefan Hajnoczi]

On Wed, Jul 31, 2013 at 06:13:53PM +0800, Fam Zheng wrote:
> BlockDriverState lifecycle management is needed by future features such as
> image fleecing and blockdev-add. This series adds reference count to
> BlockDriverState.
> 
> The first two patches clean up two odd BlockDriverState use cases, so all code
> uses bdrv_new() to create BlockDriverState instance.
> 
> Then implemented bdrv_ref() and bdrv_unref() to operate on refcnt: Initially,
> refcnt is 1, which means bdrv_unref is effectively a bdrv_delete() here. So
> patch 04 has a search and replace to convert bdrv_delete to bdrv_unref, before
> bdrv_ref is used anywhere. 05~08 patches calls bdrv_ref for device attach,
> block-migration and nbd.
> 
> The rule is: Either bdrv_ref() or bdrv_new() must have a matching
> bdrv_unref() call, and the last matching bdrv_unref deletes the bs.
> 
> v3:
>     03: Removed unnecessary bdrv_close() call.
> 
> v2:
>     05: Removed: "block: use BlockDriverState refcnt for device attach/detach"
>     07: Fix xen_disk blk_disconnect() as it depended on device attach refcnt.
> 
> Fam Zheng (7):
>   vvfat: use bdrv_new() to allocate BlockDriverState
>   iscsi: use bdrv_new() instead of stack structure
>   block: implement reference count for BlockDriverState
>   block: make bdrv_delete() static
>   migration: omit drive ref as we have bdrv_ref now
>   xen_disk: simplify blk_disconnect with refcnt
>   nbd: use BlockDriverState refcnt
> 
>  block-migration.c         |  4 ++--
>  block.c                   | 44 +++++++++++++++++++++++++++++++++-----------
>  block/backup.c            |  2 +-
>  block/blkverify.c         |  4 ++--
>  block/cow.c               |  2 +-
>  block/iscsi.c             | 14 +++++++-------
>  block/mirror.c            |  2 +-
>  block/qcow.c              |  2 +-
>  block/qcow2.c             |  2 +-
>  block/qed.c               |  2 +-
>  block/sheepdog.c          |  6 +++---
>  block/snapshot.c          |  2 +-
>  block/stream.c            |  2 +-
>  block/vmdk.c              | 10 +++++-----
>  block/vvfat.c             |  6 +++---
>  blockdev-nbd.c            | 10 +---------
>  blockdev.c                | 14 +++++++-------
>  hw/block/xen_disk.c       | 13 ++++++-------
>  include/block/block.h     |  3 ++-
>  include/block/block_int.h |  1 +
>  nbd.c                     |  5 +++++
>  qemu-img.c                | 26 +++++++++++++-------------
>  qemu-io.c                 |  6 +++---
>  23 files changed, 101 insertions(+), 81 deletions(-)

One interesting thing is the interaction between the DriveInfo and BDS
lifecycle.  Both now have refcounts but drive_init()/drive_uninit() do
bdrv_ref()/bdrv_unref() so you can be sure that the BDS will not go away
if you hold a DriveInfo reference.

Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan

Re: [Qemu-devel] [PATCH v3 0/7] Implement reference count for BlockDriverState

[Stefan Hajnoczi]

On Wed, Jul 31, 2013 at 06:13:53PM +0800, Fam Zheng wrote:
> BlockDriverState lifecycle management is needed by future features such as
> image fleecing and blockdev-add. This series adds reference count to
> BlockDriverState.
> 
> The first two patches clean up two odd BlockDriverState use cases, so all code
> uses bdrv_new() to create BlockDriverState instance.
> 
> Then implemented bdrv_ref() and bdrv_unref() to operate on refcnt: Initially,
> refcnt is 1, which means bdrv_unref is effectively a bdrv_delete() here. So
> patch 04 has a search and replace to convert bdrv_delete to bdrv_unref, before
> bdrv_ref is used anywhere. 05~08 patches calls bdrv_ref for device attach,
> block-migration and nbd.
> 
> The rule is: Either bdrv_ref() or bdrv_new() must have a matching
> bdrv_unref() call, and the last matching bdrv_unref deletes the bs.
> 
> v3:
>     03: Removed unnecessary bdrv_close() call.
> 
> v2:
>     05: Removed: "block: use BlockDriverState refcnt for device attach/detach"
>     07: Fix xen_disk blk_disconnect() as it depended on device attach refcnt.
> 
> Fam Zheng (7):
>   vvfat: use bdrv_new() to allocate BlockDriverState
>   iscsi: use bdrv_new() instead of stack structure
>   block: implement reference count for BlockDriverState
>   block: make bdrv_delete() static
>   migration: omit drive ref as we have bdrv_ref now
>   xen_disk: simplify blk_disconnect with refcnt
>   nbd: use BlockDriverState refcnt

Follow-up question:

Did you look at using bdrv_ref() for the BDS <- BlockJob relationship
too?

blockdev.c block job code still uses the DriveInfo refcount after your
series.  The BDS reference would be sufficient since the DriveInfo
fields are not used by block jobs.

Stefan

Re: [Qemu-devel] [PATCH v3 0/7] Implement reference count for BlockDriverState

[Fam Zheng]

On Thu, 08/01 14:28, Stefan Hajnoczi wrote:
> On Wed, Jul 31, 2013 at 06:13:53PM +0800, Fam Zheng wrote:
> > BlockDriverState lifecycle management is needed by future features such as
> > image fleecing and blockdev-add. This series adds reference count to
> > BlockDriverState.
> > 
> > The first two patches clean up two odd BlockDriverState use cases, so all code
> > uses bdrv_new() to create BlockDriverState instance.
> > 
> > Then implemented bdrv_ref() and bdrv_unref() to operate on refcnt: Initially,
> > refcnt is 1, which means bdrv_unref is effectively a bdrv_delete() here. So
> > patch 04 has a search and replace to convert bdrv_delete to bdrv_unref, before
> > bdrv_ref is used anywhere. 05~08 patches calls bdrv_ref for device attach,
> > block-migration and nbd.
> > 
> > The rule is: Either bdrv_ref() or bdrv_new() must have a matching
> > bdrv_unref() call, and the last matching bdrv_unref deletes the bs.
> > 
> > v3:
> >     03: Removed unnecessary bdrv_close() call.
> > 
> > v2:
> >     05: Removed: "block: use BlockDriverState refcnt for device attach/detach"
> >     07: Fix xen_disk blk_disconnect() as it depended on device attach refcnt.
> > 
> > Fam Zheng (7):
> >   vvfat: use bdrv_new() to allocate BlockDriverState
> >   iscsi: use bdrv_new() instead of stack structure
> >   block: implement reference count for BlockDriverState
> >   block: make bdrv_delete() static
> >   migration: omit drive ref as we have bdrv_ref now
> >   xen_disk: simplify blk_disconnect with refcnt
> >   nbd: use BlockDriverState refcnt
> 
> Follow-up question:
> 
> Did you look at using bdrv_ref() for the BDS <- BlockJob relationship
> too?
> 
> blockdev.c block job code still uses the DriveInfo refcount after your
> series.  The BDS reference would be sufficient since the DriveInfo
> fields are not used by block jobs.
> 
For now they coexist, which works but with redundancy. I will send
follow-up patch(es) to clean up block job to only use BDS refcount.

-- 
Fam

Re: [Qemu-devel] [PATCH v3 1/7] vvfat: use bdrv_new() to allocate BlockDriverState

[Wenchao Xia]

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

> we need bdrv_new() to properly initialize BDS, don't allocate memory
> manually.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block/vvfat.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index cd3b8ed..a827d91 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2943,7 +2943,7 @@ static int enable_write_target(BDRVVVFATState *s)
>       unlink(s->qcow_filename);
>   #endif
> 
> -    s->bs->backing_hd = calloc(sizeof(BlockDriverState), 1);
> +    s->bs->backing_hd = bdrv_new("");
>       s->bs->backing_hd->drv = &vvfat_write_target;
>       s->bs->backing_hd->opaque = g_malloc(sizeof(void*));
>       *(void**)s->bs->backing_hd->opaque = s;
> 


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH v3 2/7] iscsi: use bdrv_new() instead of stack structure

[Wenchao Xia]

于 2013-7-31 18:13, Fam Zheng 写道:
> BlockDriverState structure needs bdrv_new() to initialize refcnt, don't
> allocate a local structure variable and memset to 0, becasue with coming
> refcnt implementation, bdrv_unref will crash if bs->refcnt not
> initialized to 1.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block/iscsi.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 5f28c6a..db8a699 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1247,11 +1247,11 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
>   {
>       int ret = 0;
>       int64_t total_size = 0;
> -    BlockDriverState bs;
> +    BlockDriverState *bs;
>       IscsiLun *iscsilun = NULL;
>       QDict *bs_options;
> 
> -    memset(&bs, 0, sizeof(BlockDriverState));
> +    bs = bdrv_new("");
> 
>       /* Read out options */
>       while (options && options->name) {
> @@ -1261,12 +1261,12 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
>           options++;
>       }
> 
> -    bs.opaque = g_malloc0(sizeof(struct IscsiLun));
> -    iscsilun = bs.opaque;
> +    bs->opaque = g_malloc0(sizeof(struct IscsiLun));
> +    iscsilun = bs->opaque;
> 
>       bs_options = qdict_new();
>       qdict_put(bs_options, "filename", qstring_from_str(filename));
> -    ret = iscsi_open(&bs, bs_options, 0);
> +    ret = iscsi_open(bs, bs_options, 0);
>       QDECREF(bs_options);
> 
>       if (ret != 0) {
> @@ -1280,7 +1280,7 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
>           ret = -ENODEV;
>           goto out;
>       }
> -    if (bs.total_sectors < total_size) {
> +    if (bs->total_sectors < total_size) {
>           ret = -ENOSPC;
>           goto out;
>       }
> @@ -1290,7 +1290,7 @@ out:
>       if (iscsilun->iscsi != NULL) {
>           iscsi_destroy_context(iscsilun->iscsi);
>       }
> -    g_free(bs.opaque);
  bs,opaque seems leaked. bdrv_delete() will not free it unless bs->drv
!= NULL.


> +    bdrv_delete(bs);
>       return ret;
>   }
> 


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH v3 3/7] block: implement reference count for BlockDriverState

[Wenchao Xia]

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

> Introduce bdrv_ref/bdrv_unref to manage the lifecycle of
> BlockDriverState. They are unused for now but will used to replace
> bdrv_delete() later.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block.c                   | 21 +++++++++++++++++++++
>   include/block/block.h     |  2 ++
>   include/block/block_int.h |  1 +
>   3 files changed, 24 insertions(+)
> 

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH v3 4/7] block: make bdrv_delete() static

[Wenchao Xia]

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH v3 5/7] migration: omit drive ref as we have bdrv_ref now

[Wenchao Xia]

There should be a section of code in device hot unplug, checking
DriverInfo's ref, fail or do nothing when ref != 1. But I haven't found
that code, so not sure whether this patch will change the behavior in
device hot unplug.

> block-migration.c does not actually use DriveInfo anywhere.  Hence it's
> safe to drive ref code, we really only care about referencing BDS.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block-migration.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block-migration.c b/block-migration.c
> index f803f20..daf9ec1 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -336,8 +336,8 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
>           bmds->completed_sectors = 0;
>           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);
> 
>           block_mig_state.total_sector_sum += sectors;
> 
> @@ -575,7 +575,7 @@ 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_set_in_use(bmds->bs, 0);
> -        drive_put_ref(drive_get_by_blockdev(bmds->bs));
> +        bdrv_unref(bmds->bs);
>           g_free(bmds->aio_bitmap);
>           g_free(bmds);
>       }
> 


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH v3 6/7] xen_disk: simplify blk_disconnect with refcnt

[Wenchao Xia]

Better to split it into two patches:
1 bugfix: always call bdrv_detach_dev().
2 use refcnt to manage lifecycle.

> We call bdrv_attach_dev when initializing whether or not bs is created
> locally, so call bdrv_detach_dev and let the refcnt handle the
> lifecycle.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   hw/block/xen_disk.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 99537e8..39757d9 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -812,6 +812,9 @@ static int blk_connect(struct XenDevice *xendev)
>           /* setup via qemu cmdline -> already setup for us */
>           xen_be_printf(&blkdev->xendev, 2, "get configured bdrv (cmdline setup)\n");
>           blkdev->bs = blkdev->dinfo->bdrv;
> +        /* blkdev->bs is not create by us, we get a reference
> +         * so we can bdrv_unref() unconditionally */
> +        bdrv_ref(blkdev->bs);
>       }
>       bdrv_attach_dev_nofail(blkdev->bs, blkdev);
>       blkdev->file_size = bdrv_getlength(blkdev->bs);
> @@ -910,12 +913,8 @@ 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_unref(blkdev->bs);
> -        }
> +        bdrv_detach_dev(blkdev->bs, blkdev);
> +        bdrv_unref(blkdev->bs);
>           blkdev->bs = NULL;
>       }
>       xen_be_unbind_evtchn(&blkdev->xendev);
> 


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH v3 7/7] nbd: use BlockDriverState refcnt

[Wenchao Xia]

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

> Previously, nbd calls drive_get_ref() on the drive of bs. A BDS doesn't
> always have associated dinfo, which nbd doesn't care either. We already
> have BDS ref count, so use it to make it safe for a BDS w/o blockdev.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>



-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH v3 5/7] migration: omit drive ref as we have bdrv_ref now

[Stefan Hajnoczi]

On Fri, Aug 02, 2013 at 04:13:55PM +0800, Wenchao Xia wrote:
> There should be a section of code in device hot unplug, checking
> DriverInfo's ref, fail or do nothing when ref != 1. But I haven't found
> that code, so not sure whether this patch will change the behavior in
> device hot unplug.

It is not necessary to refuse device hot unplug when bs.ref > 0.  But a
couple of fixes are required to make it safe:

Background
----------
DriveInfo always holds a BDS refcount, so BDS can never be deleted while
the DriveInfo exists.

DriveInfo is the metadata that connects an emulated storage controller
with its BDS.  Therefore, hot unplugging an emulated storage controller
may release the last DriveInfo reference and delete it.

Anything that still holds the BDS reference when DriveInfo is deleted
will keep the BDS alive.  It turns out that a lot of commands only use
BDS with bdrv_find(), not DriveInfo, so you can continue to do useful
things with the BDS after its DriveInfo is deleted.

The problem
-----------
A couple of places have not been converted to use bdrv_ref() yet, so
they still go through drive_get_ref(drive_get_by_blockdev(bs)).  These
cases will now fail!  I pointed out the blockjob cases but please grep
to make sure there are no others.

You need to fix them before this series is safe, otherwise
drive_put_ref(NULL) will segfault!

Stefan

Re: [Qemu-devel] [PATCH v3 0/7] Implement reference count for BlockDriverState

[Stefan Hajnoczi]

On Fri, Aug 02, 2013 at 10:22:13AM +0800, Fam Zheng wrote:
> On Thu, 08/01 14:28, Stefan Hajnoczi wrote:
> > On Wed, Jul 31, 2013 at 06:13:53PM +0800, Fam Zheng wrote:
> > Follow-up question:
> > 
> > Did you look at using bdrv_ref() for the BDS <- BlockJob relationship
> > too?
> > 
> > blockdev.c block job code still uses the DriveInfo refcount after your
> > series.  The BDS reference would be sufficient since the DriveInfo
> > fields are not used by block jobs.
> > 
> For now they coexist, which works but with redundancy. I will send
> follow-up patch(es) to clean up block job to only use BDS refcount.

Wenchao's review comment pointed out the case where block job
termination will segfault if DriveInfo has been deleted.  I think this
needs to be address together with these patches instead of later.

Stefan