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

[Qemu-devel] [PATCH v2 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.

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                   | 45 ++++++++++++++++++++++++++++++++++-----------
 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, 102 insertions(+), 81 deletions(-)

-- 
1.8.3.4

[Qemu-devel] [PATCH v2 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 v2 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 v2 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                   | 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 c77cfd1..f86f14c 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,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 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 v2 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 f86f14c..6afc025 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;
 
@@ -4627,7 +4628,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 v2 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 v2 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 v2 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 v2 3/7] block: implement reference count for BlockDriverState

[Andreas Färber]

Am 30.07.2013 09:52, schrieb 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(+)

Didn't Kevin and Markus look into turning BlockDriverState into QOM
objects a while back? That would give you reference counting for free,
even atomic unlike this patch.

MemoryRegion resorted to reference counting on the owner object that was
added for the purpose.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Stefan Hajnoczi]

On Tue, Jul 30, 2013 at 02:16:08PM +0200, Andreas Färber wrote:
> Am 30.07.2013 09:52, schrieb 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(+)
> 
> Didn't Kevin and Markus look into turning BlockDriverState into QOM
> objects a while back? That would give you reference counting for free,
> even atomic unlike this patch.

Splitting BlockDriverState into QOM objects has proven hard because
there is more than one class in today's BlockDriverState struct.  I
think handling refcounts separate is actually a good idea.

It tackles the lifecycle work without getting caught up in all the other
considerations (which QOM objects to have, -drive -> -blockdev
transition, etc).

So I guess the answer is that the QOM work is not ready.  This series
may actually help clean things up for the QOM work, which can then
switch to the objects' own refcount.

Stefan

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

[Jeff Cody]

On Tue, Jul 30, 2013 at 03:52:53PM +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 c77cfd1..f86f14c 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,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 bdrv_close() here is redundant.

> +    }
> +}
> +
>  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
> 

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

[Stefan Hajnoczi]

On Tue, Jul 30, 2013 at 03:52:53PM +0800, Fam Zheng wrote:
> @@ -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;

Not sure this is correct, but then bdrv_swap() is hard to reason
about... :)

Imagine an emulated storage controller holds a reference to the
BlockDriverState.  When we create an external snapshot we'll
bdrv_swap(old_top, new_top).

We must not move new_top's refcount into old_top since the old_top
object is still being referenced by the emulated storage controller.
When the emulated storage controller does bdrv_unref() we'll hit the
recount < 0 assertion and be accessing freed memory.

> @@ -4392,6 +4396,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);

bdrv_delete() already calls bdrv_close() internally, no need to call
bdrv_close() here.

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

[Fam Zheng]

On Tue, 07/30 16:58, Stefan Hajnoczi wrote:
> On Tue, Jul 30, 2013 at 03:52:53PM +0800, Fam Zheng wrote:
> > @@ -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;
> 
> Not sure this is correct, but then bdrv_swap() is hard to reason
> about... :)
> 
> Imagine an emulated storage controller holds a reference to the
> BlockDriverState.  When we create an external snapshot we'll
> bdrv_swap(old_top, new_top).
> 
> We must not move new_top's refcount into old_top since the old_top
> object is still being referenced by the emulated storage controller.
> When the emulated storage controller does bdrv_unref() we'll hit the
> recount < 0 assertion and be accessing freed memory.
> 
When we swap old_top and new_top, we want to swap all fields except for
these, so we use bdrv_move_feature_fields() to move them back
(bdrv_swap):

    tmp = *bs_new;
    *bs_new = *bs_old;
    *bs_old = tmp;

    /* there are some fields that should not be swapped, move them back */
    bdrv_move_feature_fields(&tmp, bs_old);
    bdrv_move_feature_fields(bs_old, bs_new);
    bdrv_move_feature_fields(bs_new, &tmp);

And I agree that refcnt is one of the fields that shouldn't be moved, so
it's in bdrv_move_feature_fields(). So isn't above right? Without these
lines, it *is* swapped.

> > @@ -4392,6 +4396,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);
> 
> bdrv_delete() already calls bdrv_close() internally, no need to call
> bdrv_close() here.

OK!

-- 
Fam

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

[Stefan Hajnoczi]

On Wed, Jul 31, 2013 at 05:51:17PM +0800, Fam Zheng wrote:
> On Tue, 07/30 16:58, Stefan Hajnoczi wrote:
> > On Tue, Jul 30, 2013 at 03:52:53PM +0800, Fam Zheng wrote:
> > > @@ -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;
> > 
> > Not sure this is correct, but then bdrv_swap() is hard to reason
> > about... :)
> > 
> > Imagine an emulated storage controller holds a reference to the
> > BlockDriverState.  When we create an external snapshot we'll
> > bdrv_swap(old_top, new_top).
> > 
> > We must not move new_top's refcount into old_top since the old_top
> > object is still being referenced by the emulated storage controller.
> > When the emulated storage controller does bdrv_unref() we'll hit the
> > recount < 0 assertion and be accessing freed memory.
> > 
> When we swap old_top and new_top, we want to swap all fields except for
> these, so we use bdrv_move_feature_fields() to move them back
> (bdrv_swap):
> 
>     tmp = *bs_new;
>     *bs_new = *bs_old;
>     *bs_old = tmp;
> 
>     /* there are some fields that should not be swapped, move them back */
>     bdrv_move_feature_fields(&tmp, bs_old);
>     bdrv_move_feature_fields(bs_old, bs_new);
>     bdrv_move_feature_fields(bs_new, &tmp);
> 
> And I agree that refcnt is one of the fields that shouldn't be moved, so
> it's in bdrv_move_feature_fields(). So isn't above right? Without these
> lines, it *is* swapped.

Yes, you are right.  I should have looked at the calling function.  We
want to swap the refcount field back into the original struct where is
belongs.

Stefan