* [Qemu-devel] [PATCH 0/8] Implement reference count for BlockDriverState
@ 2013-07-25 9:01 Fam Zheng
2013-07-25 9:01 ` [Qemu-devel] [PATCH 1/8] vvfat: use bdrv_new() to allocate BlockDriverState Fam Zheng
` (7 more replies)
0 siblings, 8 replies; 18+ messages in thread
From: Fam Zheng @ 2013-07-25 9:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, famz, stefanha
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH 1/8] vvfat: use bdrv_new() to allocate BlockDriverState
2013-07-25 9:01 [Qemu-devel] [PATCH 0/8] Implement reference count for BlockDriverState Fam Zheng
@ 2013-07-25 9:01 ` Fam Zheng
2013-07-25 9:01 ` [Qemu-devel] [PATCH 2/8] iscsi: use bdrv_new() instead of stack structure Fam Zheng
` (6 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Fam Zheng @ 2013-07-25 9:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, famz, stefanha
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH 2/8] iscsi: use bdrv_new() instead of stack structure
2013-07-25 9:01 [Qemu-devel] [PATCH 0/8] Implement reference count for BlockDriverState Fam Zheng
2013-07-25 9:01 ` [Qemu-devel] [PATCH 1/8] vvfat: use bdrv_new() to allocate BlockDriverState Fam Zheng
@ 2013-07-25 9:01 ` Fam Zheng
2013-07-25 9:01 ` [Qemu-devel] [PATCH 3/8] block: implement reference count for BlockDriverState Fam Zheng
` (5 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Fam Zheng @ 2013-07-25 9:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, famz, stefanha
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH 3/8] block: implement reference count for BlockDriverState
2013-07-25 9:01 [Qemu-devel] [PATCH 0/8] Implement reference count for BlockDriverState Fam Zheng
2013-07-25 9:01 ` [Qemu-devel] [PATCH 1/8] vvfat: use bdrv_new() to allocate BlockDriverState Fam Zheng
2013-07-25 9:01 ` [Qemu-devel] [PATCH 2/8] iscsi: use bdrv_new() instead of stack structure Fam Zheng
@ 2013-07-25 9:01 ` Fam Zheng
2013-07-25 13:15 ` Jeff Cody
2013-07-25 9:01 ` [Qemu-devel] [PATCH 4/8] block: make bdrv_delete() static Fam Zheng
` (4 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Fam Zheng @ 2013-07-25 9:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, famz, stefanha
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH 4/8] block: make bdrv_delete() static
2013-07-25 9:01 [Qemu-devel] [PATCH 0/8] Implement reference count for BlockDriverState Fam Zheng
` (2 preceding siblings ...)
2013-07-25 9:01 ` [Qemu-devel] [PATCH 3/8] block: implement reference count for BlockDriverState Fam Zheng
@ 2013-07-25 9:01 ` Fam Zheng
2013-07-25 9:01 ` [Qemu-devel] [PATCH 5/8] block: use BlockDriverState refcnt for device attach/detach Fam Zheng
` (3 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Fam Zheng @ 2013-07-25 9:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, famz, stefanha
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH 5/8] block: use BlockDriverState refcnt for device attach/detach
2013-07-25 9:01 [Qemu-devel] [PATCH 0/8] Implement reference count for BlockDriverState Fam Zheng
` (3 preceding siblings ...)
2013-07-25 9:01 ` [Qemu-devel] [PATCH 4/8] block: make bdrv_delete() static Fam Zheng
@ 2013-07-25 9:01 ` Fam Zheng
2013-07-25 12:49 ` Jeff Cody
2013-07-25 9:01 ` [Qemu-devel] [PATCH 6/8] migration: omit drive ref as we have bdrv_ref now Fam Zheng
` (2 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Fam Zheng @ 2013-07-25 9:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, famz, stefanha
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH 6/8] migration: omit drive ref as we have bdrv_ref now
2013-07-25 9:01 [Qemu-devel] [PATCH 0/8] Implement reference count for BlockDriverState Fam Zheng
` (4 preceding siblings ...)
2013-07-25 9:01 ` [Qemu-devel] [PATCH 5/8] block: use BlockDriverState refcnt for device attach/detach Fam Zheng
@ 2013-07-25 9:01 ` Fam Zheng
2013-07-25 9:01 ` [Qemu-devel] [PATCH 7/8] xen_disk: simplify blk_disconnect with refcnt Fam Zheng
2013-07-25 9:01 ` [Qemu-devel] [PATCH 8/8] nbd: use BlockDriverState refcnt Fam Zheng
7 siblings, 0 replies; 18+ messages in thread
From: Fam Zheng @ 2013-07-25 9:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, famz, stefanha
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH 7/8] xen_disk: simplify blk_disconnect with refcnt
2013-07-25 9:01 [Qemu-devel] [PATCH 0/8] Implement reference count for BlockDriverState Fam Zheng
` (5 preceding siblings ...)
2013-07-25 9:01 ` [Qemu-devel] [PATCH 6/8] migration: omit drive ref as we have bdrv_ref now Fam Zheng
@ 2013-07-25 9:01 ` Fam Zheng
2013-07-25 12:56 ` Jeff Cody
2013-07-25 9:01 ` [Qemu-devel] [PATCH 8/8] nbd: use BlockDriverState refcnt Fam Zheng
7 siblings, 1 reply; 18+ messages in thread
From: Fam Zheng @ 2013-07-25 9:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, famz, stefanha
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH 8/8] nbd: use BlockDriverState refcnt
2013-07-25 9:01 [Qemu-devel] [PATCH 0/8] Implement reference count for BlockDriverState Fam Zheng
` (6 preceding siblings ...)
2013-07-25 9:01 ` [Qemu-devel] [PATCH 7/8] xen_disk: simplify blk_disconnect with refcnt Fam Zheng
@ 2013-07-25 9:01 ` Fam Zheng
2013-07-25 13:01 ` Jeff Cody
7 siblings, 1 reply; 18+ messages in thread
From: Fam Zheng @ 2013-07-25 9:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, famz, stefanha
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH 5/8] block: use BlockDriverState refcnt for device attach/detach
2013-07-25 9:01 ` [Qemu-devel] [PATCH 5/8] block: use BlockDriverState refcnt for device attach/detach Fam Zheng
@ 2013-07-25 12:49 ` Jeff Cody
0 siblings, 0 replies; 18+ messages in thread
From: Jeff Cody @ 2013-07-25 12:49 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, pbonzini, qemu-devel, stefanha
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
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH 7/8] xen_disk: simplify blk_disconnect with refcnt
2013-07-25 9:01 ` [Qemu-devel] [PATCH 7/8] xen_disk: simplify blk_disconnect with refcnt Fam Zheng
@ 2013-07-25 12:56 ` Jeff Cody
2013-07-26 1:30 ` Jeff Cody
0 siblings, 1 reply; 18+ messages in thread
From: Jeff Cody @ 2013-07-25 12:56 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, pbonzini, qemu-devel, stefanha
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
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH 8/8] nbd: use BlockDriverState refcnt
2013-07-25 9:01 ` [Qemu-devel] [PATCH 8/8] nbd: use BlockDriverState refcnt Fam Zheng
@ 2013-07-25 13:01 ` Jeff Cody
2013-07-26 1:29 ` Jeff Cody
0 siblings, 1 reply; 18+ messages in thread
From: Jeff Cody @ 2013-07-25 13:01 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, pbonzini, qemu-devel, stefanha
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
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH 3/8] block: implement reference count for BlockDriverState
2013-07-25 9:01 ` [Qemu-devel] [PATCH 3/8] block: implement reference count for BlockDriverState Fam Zheng
@ 2013-07-25 13:15 ` Jeff Cody
2013-07-26 1:13 ` Fam Zheng
0 siblings, 1 reply; 18+ messages in thread
From: Jeff Cody @ 2013-07-25 13:15 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, pbonzini, qemu-devel, stefanha
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
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH 3/8] block: implement reference count for BlockDriverState
2013-07-25 13:15 ` Jeff Cody
@ 2013-07-26 1:13 ` Fam Zheng
2013-07-26 1:50 ` Jeff Cody
0 siblings, 1 reply; 18+ messages in thread
From: Fam Zheng @ 2013-07-26 1:13 UTC (permalink / raw)
To: Jeff Cody; +Cc: kwolf, pbonzini, qemu-devel, stefanha
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH 8/8] nbd: use BlockDriverState refcnt
2013-07-25 13:01 ` Jeff Cody
@ 2013-07-26 1:29 ` Jeff Cody
0 siblings, 0 replies; 18+ messages in thread
From: Jeff Cody @ 2013-07-26 1:29 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, pbonzini, qemu-devel, stefanha
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
> >
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH 7/8] xen_disk: simplify blk_disconnect with refcnt
2013-07-25 12:56 ` Jeff Cody
@ 2013-07-26 1:30 ` Jeff Cody
0 siblings, 0 replies; 18+ messages in thread
From: Jeff Cody @ 2013-07-26 1:30 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, pbonzini, qemu-devel, stefanha
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
> >
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH 3/8] block: implement reference count for BlockDriverState
2013-07-26 1:13 ` Fam Zheng
@ 2013-07-26 1:50 ` Jeff Cody
2013-07-26 1:56 ` Fam Zheng
0 siblings, 1 reply; 18+ messages in thread
From: Jeff Cody @ 2013-07-26 1:50 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, pbonzini, qemu-devel, stefanha
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH 3/8] block: implement reference count for BlockDriverState
2013-07-26 1:50 ` Jeff Cody
@ 2013-07-26 1:56 ` Fam Zheng
0 siblings, 0 replies; 18+ messages in thread
From: Fam Zheng @ 2013-07-26 1:56 UTC (permalink / raw)
To: Jeff Cody; +Cc: kwolf, pbonzini, qemu-devel, stefanha
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
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2013-07-26 1:56 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-25 9:01 [Qemu-devel] [PATCH 0/8] Implement reference count for BlockDriverState Fam Zheng
2013-07-25 9:01 ` [Qemu-devel] [PATCH 1/8] vvfat: use bdrv_new() to allocate BlockDriverState Fam Zheng
2013-07-25 9:01 ` [Qemu-devel] [PATCH 2/8] iscsi: use bdrv_new() instead of stack structure Fam Zheng
2013-07-25 9:01 ` [Qemu-devel] [PATCH 3/8] block: implement reference count for BlockDriverState Fam Zheng
2013-07-25 13:15 ` Jeff Cody
2013-07-26 1:13 ` Fam Zheng
2013-07-26 1:50 ` Jeff Cody
2013-07-26 1:56 ` Fam Zheng
2013-07-25 9:01 ` [Qemu-devel] [PATCH 4/8] block: make bdrv_delete() static Fam Zheng
2013-07-25 9:01 ` [Qemu-devel] [PATCH 5/8] block: use BlockDriverState refcnt for device attach/detach Fam Zheng
2013-07-25 12:49 ` Jeff Cody
2013-07-25 9:01 ` [Qemu-devel] [PATCH 6/8] migration: omit drive ref as we have bdrv_ref now Fam Zheng
2013-07-25 9:01 ` [Qemu-devel] [PATCH 7/8] xen_disk: simplify blk_disconnect with refcnt Fam Zheng
2013-07-25 12:56 ` Jeff Cody
2013-07-26 1:30 ` Jeff Cody
2013-07-25 9:01 ` [Qemu-devel] [PATCH 8/8] nbd: use BlockDriverState refcnt Fam Zheng
2013-07-25 13:01 ` Jeff Cody
2013-07-26 1:29 ` Jeff Cody
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.