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

[Qemu-devel] [PATCH 0/8] 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.


Fam Zheng (8):
  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
  block: use BlockDriverState refcnt for device attach/detach
  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                   | 52 ++++++++++++++++++++++++++++++++++++-----------
 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       |  9 ++------
 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, 104 insertions(+), 82 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH 1/8] 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.2

[Qemu-devel] [PATCH 2/8] 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.2

[Qemu-devel] [PATCH 3/8] 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                   | 22 ++++++++++++++++++++++
 include/block/block.h     |  2 ++
 include/block/block_int.h |  1 +
 3 files changed, 25 insertions(+)

diff --git a/block.c b/block.c
index 6cd39fa..6f7ad7f 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;
 }
@@ -1511,6 +1512,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;
@@ -4385,6 +4389,24 @@ 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_close(bs);
+        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 c6ac871..a282d56 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.2

[Qemu-devel] [PATCH 4/8] 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 6f7ad7f..dfa4be0 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;
@@ -1002,12 +1002,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) {
@@ -1075,7 +1075,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     }
 
     if (bs->file != file) {
-        bdrv_delete(file);
+        bdrv_unref(file);
         file = NULL;
     }
 
@@ -1115,7 +1115,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);
@@ -1376,7 +1376,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);
@@ -1400,7 +1400,7 @@ void bdrv_close(BlockDriverState *bs)
         bs->options = NULL;
 
         if (bs->file != NULL) {
-            bdrv_delete(bs->file);
+            bdrv_unref(bs->file);
             bs->file = NULL;
         }
     }
@@ -1598,11 +1598,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);
@@ -2118,7 +2119,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;
 
@@ -4620,7 +4621,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 16105d4..b6b615b 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_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 0eceefe..3f9e8ef 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 c5abd65..34a9a6a 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);
@@ -719,7 +719,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
 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);
@@ -915,7 +915,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);
     }
 }
 
@@ -1503,7 +1503,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;
     }
@@ -1511,7 +1511,7 @@ void qmp_drive_backup(const char *device, const char *target,
     backup_start(bs, target_bs, speed, 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;
     }
@@ -1643,7 +1643,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;
     }
@@ -1652,7 +1652,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.2

[Qemu-devel] [PATCH 5/8] block: use BlockDriverState refcnt for device attach/detach

[Fam Zheng]

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

diff --git a/block.c b/block.c
index dfa4be0..ce4d94b 100644
--- a/block.c
+++ b/block.c
@@ -1620,11 +1620,13 @@ int bdrv_attach_dev(BlockDriverState *bs, void *dev)
         return -EBUSY;
     }
     bs->dev = dev;
+    bdrv_ref(bs);
     bdrv_iostatus_reset(bs);
     return 0;
 }
 
-/* TODO qdevified devices don't use this, remove when devices are qdevified */
+/* Attach a bs to dev, and increase its refcnt.
+ * TODO qdevified devices don't use this, remove when devices are qdevified */
 void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev)
 {
     if (bdrv_attach_dev(bs, dev) < 0) {
@@ -1632,10 +1634,13 @@ void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev)
     }
 }
 
+/* Detach bs from device. This decreases its refcnt, and may consequently
+ * deletes it make bs an invalid pointer */
 void bdrv_detach_dev(BlockDriverState *bs, void *dev)
 /* TODO change to DeviceState *dev when all users are qdevified */
 {
     assert(bs->dev == dev);
+    bdrv_unref(bs);
     bs->dev = NULL;
     bs->dev_ops = NULL;
     bs->dev_opaque = NULL;
-- 
1.8.3.2

[Qemu-devel] [PATCH 6/8] 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.2

[Qemu-devel] [PATCH 7/8] 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 | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 99537e8..3ec4bd2 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_unref(blkdev->bs);
-        }
+        bdrv_detach_dev(blkdev->bs, blkdev);
         blkdev->bs = NULL;
     }
     xen_be_unbind_evtchn(&blkdev->xendev);
-- 
1.8.3.2

[Qemu-devel] [PATCH 8/8] 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.2

Re: [Qemu-devel] [PATCH 5/8] block: use BlockDriverState refcnt for device attach/detach

[Jeff Cody]

On Thu, Jul 25, 2013 at 05:01:43PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index dfa4be0..ce4d94b 100644
> --- a/block.c
> +++ b/block.c
> @@ -1620,11 +1620,13 @@ int bdrv_attach_dev(BlockDriverState *bs, void *dev)
>          return -EBUSY;
>      }
>      bs->dev = dev;
> +    bdrv_ref(bs);
>      bdrv_iostatus_reset(bs);
>      return 0;
>  }
>  
> -/* TODO qdevified devices don't use this, remove when devices are qdevified */
> +/* Attach a bs to dev, and increase its refcnt.
> + * TODO qdevified devices don't use this, remove when devices are qdevified */
>  void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev)
>  {
>      if (bdrv_attach_dev(bs, dev) < 0) {
> @@ -1632,10 +1634,13 @@ void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev)
>      }
>  }
>  
> +/* Detach bs from device. This decreases its refcnt, and may consequently
> + * deletes it make bs an invalid pointer */
>  void bdrv_detach_dev(BlockDriverState *bs, void *dev)
>  /* TODO change to DeviceState *dev when all users are qdevified */
>  {
>      assert(bs->dev == dev);
> +    bdrv_unref(bs);
>      bs->dev = NULL;
>      bs->dev_ops = NULL;
>      bs->dev_opaque = NULL;

This won't work, since we are dereferencing bs shortly after
(potentially) freeing it.  I would say just move the bdrv_unref() to
the end of the function, but I have another concern as well.  

If bs is freed, then BDS pointer is now invalid, but not NULL.  So
there is no way for callers of bdrv_detach_dev() to know if the BDS
pointer they passed into bdrv_detach_dev() is still valid; in fact, I
think some call bdrv_close(bs) afterwards (piix).  Qdev also still
uses it, although just for pointer comparison and not dereferencing.

Jeff

> -- 
> 1.8.3.2
> 
> 

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

[Jeff Cody]

On Thu, Jul 25, 2013 at 05:01:45PM +0800, Fam Zheng wrote:
> 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 | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 99537e8..3ec4bd2 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_unref(blkdev->bs);
> -        }
> +        bdrv_detach_dev(blkdev->bs, blkdev);
>          blkdev->bs = NULL;

If the refcnt for blkdev->bs is > 1, then this leaks blkdev->bs, and
doesn't call bdrv_close().

>      }
>      xen_be_unbind_evtchn(&blkdev->xendev);
> -- 
> 1.8.3.2
> 
> 

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

[Jeff Cody]

On Thu, Jul 25, 2013 at 05:01:46PM +0800, Fam Zheng wrote:
> 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;
> +    }

Same thing here - if the refcnt is ever > 1, then this leaks exp->bs.

>  }
>  
>  void nbd_export_get(NBDExport *exp)
> -- 
> 1.8.3.2
> 
> 

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

[Jeff Cody]

On Thu, Jul 25, 2013 at 05:01:41PM +0800, Fam Zheng wrote:
> 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                   | 22 ++++++++++++++++++++++
>  include/block/block.h     |  2 ++
>  include/block/block_int.h |  1 +
>  3 files changed, 25 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 6cd39fa..6f7ad7f 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;
>  }
> @@ -1511,6 +1512,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;
> @@ -4385,6 +4389,24 @@ 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_close(bs);
> +        bdrv_delete(bs);
> +    }
> +}

The problem with this is that a caller to bdrv_unref() has no
way of knowing after calling bdrv_unref() if bs is still valid.  We
can't just set bs to NULL after calling bdrv_unref() as with
bdrv_delete(), because now it may not have been freed.

Maybe bdrv_unref should either return the current bs pointer, or
alternatively accept as its argument a pointer to the BDS pointer:

void bdrv_unref(BlockDriverState **bs)
{
    assert(*bs->refcnt > 0);
    if (--*bs->refcnt == 0) {
        bdrv_close(*bs);
        bdrv_delete(*bs);
        *bs = NULL;
    }
}

Of course, all callers would need to then check for NULL.

Also, do we need to call bdrv_close() in here?  In bdrv_delete(),
bdrv_close() is called prior to the free.

> +
>  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 c6ac871..a282d56 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.2
> 
> 

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

[Fam Zheng]

On Thu, 07/25 09:15, Jeff Cody wrote:
> On Thu, Jul 25, 2013 at 05:01:41PM +0800, Fam Zheng wrote:
> > 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                   | 22 ++++++++++++++++++++++
> >  include/block/block.h     |  2 ++
> >  include/block/block_int.h |  1 +
> >  3 files changed, 25 insertions(+)
> > 
> > diff --git a/block.c b/block.c
> > index 6cd39fa..6f7ad7f 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;
> >  }
> > @@ -1511,6 +1512,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;
> > @@ -4385,6 +4389,24 @@ 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_close(bs);
> > +        bdrv_delete(bs);
> > +    }
> > +}
> 
> The problem with this is that a caller to bdrv_unref() has no
> way of knowing after calling bdrv_unref() if bs is still valid.  We
> can't just set bs to NULL after calling bdrv_unref() as with
> bdrv_delete(), because now it may not have been freed.
> 
By calling bdrv_unref, it means the caller is not going to use bs any
more.  In other words, bdrv_unref() is a bdrv_delete() as seen by the
caller, if bs is still valid pointer after unref, it's no longer safe
for the caller: it can be freed by other code, in any time, but the
caller can't know.

> Maybe bdrv_unref should either return the current bs pointer, or
> alternatively accept as its argument a pointer to the BDS pointer:
> 
> void bdrv_unref(BlockDriverState **bs)
> {
>     assert(*bs->refcnt > 0);
>     if (--*bs->refcnt == 0) {
>         bdrv_close(*bs);
>         bdrv_delete(*bs);
>         *bs = NULL;
>     }
> }
> 
> Of course, all callers would need to then check for NULL.
> 
> Also, do we need to call bdrv_close() in here?  In bdrv_delete(),
> bdrv_close() is called prior to the free.
> 
Yes, it can be omited.

> > +
> >  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 c6ac871..a282d56 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.2
> > 
> > 

-- 
Fam

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

[Jeff Cody]

On Thu, Jul 25, 2013 at 09:01:41AM -0400, Jeff Cody wrote:
> On Thu, Jul 25, 2013 at 05:01:46PM +0800, Fam Zheng wrote:
> > 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;
> > +    }
> 
> Same thing here - if the refcnt is ever > 1, then this leaks exp->bs.
>

Nevermind - since bs continues to exist elsewhere (and would
presumably be unrefed there), then this would not leak exp->bs.

> >  }
> >  
> >  void nbd_export_get(NBDExport *exp)
> > -- 
> > 1.8.3.2
> > 
> > 

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

[Jeff Cody]

On Thu, Jul 25, 2013 at 08:56:41AM -0400, Jeff Cody wrote:
> On Thu, Jul 25, 2013 at 05:01:45PM +0800, Fam Zheng wrote:
> > 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 | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> > 
> > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> > index 99537e8..3ec4bd2 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_unref(blkdev->bs);
> > -        }
> > +        bdrv_detach_dev(blkdev->bs, blkdev);
> >          blkdev->bs = NULL;
> 
> If the refcnt for blkdev->bs is > 1, then this leaks blkdev->bs, and
> doesn't call bdrv_close().
> 

Same with this, I don't think it will leak this either.

> >      }
> >      xen_be_unbind_evtchn(&blkdev->xendev);
> > -- 
> > 1.8.3.2
> > 
> > 

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

[Jeff Cody]

On Fri, Jul 26, 2013 at 09:13:32AM +0800, Fam Zheng wrote:
> On Thu, 07/25 09:15, Jeff Cody wrote:
> > On Thu, Jul 25, 2013 at 05:01:41PM +0800, Fam Zheng wrote:
> > > 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                   | 22 ++++++++++++++++++++++
> > >  include/block/block.h     |  2 ++
> > >  include/block/block_int.h |  1 +
> > >  3 files changed, 25 insertions(+)
> > > 
> > > diff --git a/block.c b/block.c
> > > index 6cd39fa..6f7ad7f 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;
> > >  }
> > > @@ -1511,6 +1512,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;
> > > @@ -4385,6 +4389,24 @@ 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_close(bs);
> > > +        bdrv_delete(bs);
> > > +    }
> > > +}
> > 
> > The problem with this is that a caller to bdrv_unref() has no
> > way of knowing after calling bdrv_unref() if bs is still valid.  We
> > can't just set bs to NULL after calling bdrv_unref() as with
> > bdrv_delete(), because now it may not have been freed.
> > 
> By calling bdrv_unref, it means the caller is not going to use bs any
> more.  In other words, bdrv_unref() is a bdrv_delete() as seen by the
> caller, if bs is still valid pointer after unref, it's no longer safe
> for the caller: it can be freed by other code, in any time, but the
> caller can't know.

OK, I can go with that.  I can't think off the top of my head where
this would cause a problem if it is just used in place of current
bdrv_delete(), so long as everyone refs it when they should.

But then going with a bdrv_delete() equivalency model, I don't know if
it is appropriate to call bdrv_unref() in bdrv_detach_dev() (in patch
5/8).  Maybe all that is really needed there is some more cleanup in
the places that call bdrv_detach_dev(), and then it would be OK.
> 
> > Maybe bdrv_unref should either return the current bs pointer, or
> > alternatively accept as its argument a pointer to the BDS pointer:
> > 
> > void bdrv_unref(BlockDriverState **bs)
> > {
> >     assert(*bs->refcnt > 0);
> >     if (--*bs->refcnt == 0) {
> >         bdrv_close(*bs);
> >         bdrv_delete(*bs);
> >         *bs = NULL;
> >     }
> > }
> > 
> > Of course, all callers would need to then check for NULL.
> > 
> > Also, do we need to call bdrv_close() in here?  In bdrv_delete(),
> > bdrv_close() is called prior to the free.
> > 
> Yes, it can be omited.
> 
> > > +
> > >  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 c6ac871..a282d56 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.2
> > > 
> > > 
> 
> -- 
> Fam

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

[Fam Zheng]

On Thu, 07/25 21:50, Jeff Cody wrote:
> On Fri, Jul 26, 2013 at 09:13:32AM +0800, Fam Zheng wrote:
> > On Thu, 07/25 09:15, Jeff Cody wrote:
> > > On Thu, Jul 25, 2013 at 05:01:41PM +0800, Fam Zheng wrote:
> > > > 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                   | 22 ++++++++++++++++++++++
> > > >  include/block/block.h     |  2 ++
> > > >  include/block/block_int.h |  1 +
> > > >  3 files changed, 25 insertions(+)
> > > > 
> > > > diff --git a/block.c b/block.c
> > > > index 6cd39fa..6f7ad7f 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;
> > > >  }
> > > > @@ -1511,6 +1512,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;
> > > > @@ -4385,6 +4389,24 @@ 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_close(bs);
> > > > +        bdrv_delete(bs);
> > > > +    }
> > > > +}
> > > 
> > > The problem with this is that a caller to bdrv_unref() has no
> > > way of knowing after calling bdrv_unref() if bs is still valid.  We
> > > can't just set bs to NULL after calling bdrv_unref() as with
> > > bdrv_delete(), because now it may not have been freed.
> > > 
> > By calling bdrv_unref, it means the caller is not going to use bs any
> > more.  In other words, bdrv_unref() is a bdrv_delete() as seen by the
> > caller, if bs is still valid pointer after unref, it's no longer safe
> > for the caller: it can be freed by other code, in any time, but the
> > caller can't know.
> 
> OK, I can go with that.  I can't think off the top of my head where
> this would cause a problem if it is just used in place of current
> bdrv_delete(), so long as everyone refs it when they should.
> 
> But then going with a bdrv_delete() equivalency model, I don't know if
> it is appropriate to call bdrv_unref() in bdrv_detach_dev() (in patch
> 5/8).  Maybe all that is really needed there is some more cleanup in
> the places that call bdrv_detach_dev(), and then it would be OK.

Yes, you're right. I need to audit the device code yet. Thanks for
pointing out.
> > 
> > > Maybe bdrv_unref should either return the current bs pointer, or
> > > alternatively accept as its argument a pointer to the BDS pointer:
> > > 
> > > void bdrv_unref(BlockDriverState **bs)
> > > {
> > >     assert(*bs->refcnt > 0);
> > >     if (--*bs->refcnt == 0) {
> > >         bdrv_close(*bs);
> > >         bdrv_delete(*bs);
> > >         *bs = NULL;
> > >     }
> > > }
> > > 
> > > Of course, all callers would need to then check for NULL.
> > > 
> > > Also, do we need to call bdrv_close() in here?  In bdrv_delete(),
> > > bdrv_close() is called prior to the free.
> > > 
> > Yes, it can be omited.
> > 
> > > > +
> > > >  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 c6ac871..a282d56 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.2
> > > > 
> > > > 
> > 
> > -- 
> > Fam

-- 
Fam