* [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
* 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 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
* [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
* 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
* [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
* 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 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
* [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 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 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
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.