| Message ID | 1375170777-31457-4-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 30.07.2013 09:52, schrieb Fam Zheng: > Introduce bdrv_ref/bdrv_unref to manage the lifecycle of > BlockDriverState. They are unused for now but will used to replace > bdrv_delete() later. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block.c | 22 ++++++++++++++++++++++ > include/block/block.h | 2 ++ > include/block/block_int.h | 1 + > 3 files changed, 25 insertions(+) Didn't Kevin and Markus look into turning BlockDriverState into QOM objects a while back? That would give you reference counting for free, even atomic unlike this patch. MemoryRegion resorted to reference counting on the owner object that was added for the purpose. Regards, Andreas
On Tue, Jul 30, 2013 at 02:16:08PM +0200, Andreas Färber wrote: > Am 30.07.2013 09:52, schrieb Fam Zheng: > > Introduce bdrv_ref/bdrv_unref to manage the lifecycle of > > BlockDriverState. They are unused for now but will used to replace > > bdrv_delete() later. > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > block.c | 22 ++++++++++++++++++++++ > > include/block/block.h | 2 ++ > > include/block/block_int.h | 1 + > > 3 files changed, 25 insertions(+) > > Didn't Kevin and Markus look into turning BlockDriverState into QOM > objects a while back? That would give you reference counting for free, > even atomic unlike this patch. Splitting BlockDriverState into QOM objects has proven hard because there is more than one class in today's BlockDriverState struct. I think handling refcounts separate is actually a good idea. It tackles the lifecycle work without getting caught up in all the other considerations (which QOM objects to have, -drive -> -blockdev transition, etc). So I guess the answer is that the QOM work is not ready. This series may actually help clean things up for the QOM work, which can then switch to the objects' own refcount. Stefan
On Tue, Jul 30, 2013 at 03:52:53PM +0800, Fam Zheng wrote: > Introduce bdrv_ref/bdrv_unref to manage the lifecycle of > BlockDriverState. They are unused for now but will used to replace > bdrv_delete() later. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block.c | 22 ++++++++++++++++++++++ > include/block/block.h | 2 ++ > include/block/block_int.h | 1 + > 3 files changed, 25 insertions(+) > > diff --git a/block.c b/block.c > index c77cfd1..f86f14c 100644 > --- a/block.c > +++ b/block.c > @@ -306,6 +306,7 @@ BlockDriverState *bdrv_new(const char *device_name) > bdrv_iostatus_disable(bs); > notifier_list_init(&bs->close_notifiers); > notifier_with_return_list_init(&bs->before_write_notifiers); > + bs->refcnt = 1; > > return bs; > } > @@ -1518,6 +1519,9 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, > /* dirty bitmap */ > bs_dest->dirty_bitmap = bs_src->dirty_bitmap; > > + /* reference count */ > + bs_dest->refcnt = bs_src->refcnt; > + > /* job */ > bs_dest->in_use = bs_src->in_use; > bs_dest->job = bs_src->job; > @@ -4392,6 +4396,24 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs) > } > } > > +/* Get a reference to bs */ > +void bdrv_ref(BlockDriverState *bs) > +{ > + bs->refcnt++; > +} > + > +/* Release a previously grabbed reference to bs. > + * If after releasing, reference count is zero, the BlockDriverState is > + * deleted. */ > +void bdrv_unref(BlockDriverState *bs) > +{ > + assert(bs->refcnt > 0); > + if (--bs->refcnt == 0) { > + bdrv_close(bs); > + bdrv_delete(bs); The bdrv_close() here is redundant. > + } > +} > + > void bdrv_set_in_use(BlockDriverState *bs, int in_use) > { > assert(bs->in_use != in_use); > diff --git a/include/block/block.h b/include/block/block.h > index 742fce5..b33ef62 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -356,6 +356,8 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs); > void bdrv_enable_copy_on_read(BlockDriverState *bs); > void bdrv_disable_copy_on_read(BlockDriverState *bs); > > +void bdrv_ref(BlockDriverState *bs); > +void bdrv_unref(BlockDriverState *bs); > void bdrv_set_in_use(BlockDriverState *bs, int in_use); > int bdrv_in_use(BlockDriverState *bs); > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index e45f2a0..1f85cfb 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -294,6 +294,7 @@ struct BlockDriverState { > BlockDeviceIoStatus iostatus; > char device_name[32]; > HBitmap *dirty_bitmap; > + int refcnt; > int in_use; /* users other than guest access, eg. block migration */ > QTAILQ_ENTRY(BlockDriverState) list; > > -- > 1.8.3.4 >
On Tue, Jul 30, 2013 at 03:52:53PM +0800, Fam Zheng wrote: > @@ -1518,6 +1519,9 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, > /* dirty bitmap */ > bs_dest->dirty_bitmap = bs_src->dirty_bitmap; > > + /* reference count */ > + bs_dest->refcnt = bs_src->refcnt; > + > /* job */ > bs_dest->in_use = bs_src->in_use; > bs_dest->job = bs_src->job; Not sure this is correct, but then bdrv_swap() is hard to reason about... :) Imagine an emulated storage controller holds a reference to the BlockDriverState. When we create an external snapshot we'll bdrv_swap(old_top, new_top). We must not move new_top's refcount into old_top since the old_top object is still being referenced by the emulated storage controller. When the emulated storage controller does bdrv_unref() we'll hit the recount < 0 assertion and be accessing freed memory. > @@ -4392,6 +4396,24 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs) > } > } > > +/* Get a reference to bs */ > +void bdrv_ref(BlockDriverState *bs) > +{ > + bs->refcnt++; > +} > + > +/* Release a previously grabbed reference to bs. > + * If after releasing, reference count is zero, the BlockDriverState is > + * deleted. */ > +void bdrv_unref(BlockDriverState *bs) > +{ > + assert(bs->refcnt > 0); > + if (--bs->refcnt == 0) { > + bdrv_close(bs); > + bdrv_delete(bs); bdrv_delete() already calls bdrv_close() internally, no need to call bdrv_close() here.
On Tue, 07/30 16:58, Stefan Hajnoczi wrote: > On Tue, Jul 30, 2013 at 03:52:53PM +0800, Fam Zheng wrote: > > @@ -1518,6 +1519,9 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, > > /* dirty bitmap */ > > bs_dest->dirty_bitmap = bs_src->dirty_bitmap; > > > > + /* reference count */ > > + bs_dest->refcnt = bs_src->refcnt; > > + > > /* job */ > > bs_dest->in_use = bs_src->in_use; > > bs_dest->job = bs_src->job; > > Not sure this is correct, but then bdrv_swap() is hard to reason > about... :) > > Imagine an emulated storage controller holds a reference to the > BlockDriverState. When we create an external snapshot we'll > bdrv_swap(old_top, new_top). > > We must not move new_top's refcount into old_top since the old_top > object is still being referenced by the emulated storage controller. > When the emulated storage controller does bdrv_unref() we'll hit the > recount < 0 assertion and be accessing freed memory. > When we swap old_top and new_top, we want to swap all fields except for these, so we use bdrv_move_feature_fields() to move them back (bdrv_swap): tmp = *bs_new; *bs_new = *bs_old; *bs_old = tmp; /* there are some fields that should not be swapped, move them back */ bdrv_move_feature_fields(&tmp, bs_old); bdrv_move_feature_fields(bs_old, bs_new); bdrv_move_feature_fields(bs_new, &tmp); And I agree that refcnt is one of the fields that shouldn't be moved, so it's in bdrv_move_feature_fields(). So isn't above right? Without these lines, it *is* swapped. > > @@ -4392,6 +4396,24 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs) > > } > > } > > > > +/* Get a reference to bs */ > > +void bdrv_ref(BlockDriverState *bs) > > +{ > > + bs->refcnt++; > > +} > > + > > +/* Release a previously grabbed reference to bs. > > + * If after releasing, reference count is zero, the BlockDriverState is > > + * deleted. */ > > +void bdrv_unref(BlockDriverState *bs) > > +{ > > + assert(bs->refcnt > 0); > > + if (--bs->refcnt == 0) { > > + bdrv_close(bs); > > + bdrv_delete(bs); > > bdrv_delete() already calls bdrv_close() internally, no need to call > bdrv_close() here. OK!
On Wed, Jul 31, 2013 at 05:51:17PM +0800, Fam Zheng wrote: > On Tue, 07/30 16:58, Stefan Hajnoczi wrote: > > On Tue, Jul 30, 2013 at 03:52:53PM +0800, Fam Zheng wrote: > > > @@ -1518,6 +1519,9 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, > > > /* dirty bitmap */ > > > bs_dest->dirty_bitmap = bs_src->dirty_bitmap; > > > > > > + /* reference count */ > > > + bs_dest->refcnt = bs_src->refcnt; > > > + > > > /* job */ > > > bs_dest->in_use = bs_src->in_use; > > > bs_dest->job = bs_src->job; > > > > Not sure this is correct, but then bdrv_swap() is hard to reason > > about... :) > > > > Imagine an emulated storage controller holds a reference to the > > BlockDriverState. When we create an external snapshot we'll > > bdrv_swap(old_top, new_top). > > > > We must not move new_top's refcount into old_top since the old_top > > object is still being referenced by the emulated storage controller. > > When the emulated storage controller does bdrv_unref() we'll hit the > > recount < 0 assertion and be accessing freed memory. > > > When we swap old_top and new_top, we want to swap all fields except for > these, so we use bdrv_move_feature_fields() to move them back > (bdrv_swap): > > tmp = *bs_new; > *bs_new = *bs_old; > *bs_old = tmp; > > /* there are some fields that should not be swapped, move them back */ > bdrv_move_feature_fields(&tmp, bs_old); > bdrv_move_feature_fields(bs_old, bs_new); > bdrv_move_feature_fields(bs_new, &tmp); > > And I agree that refcnt is one of the fields that shouldn't be moved, so > it's in bdrv_move_feature_fields(). So isn't above right? Without these > lines, it *is* swapped. Yes, you are right. I should have looked at the calling function. We want to swap the refcount field back into the original struct where is belongs. Stefan
diff --git a/block.c b/block.c index c77cfd1..f86f14c 100644 --- a/block.c +++ b/block.c @@ -306,6 +306,7 @@ BlockDriverState *bdrv_new(const char *device_name) bdrv_iostatus_disable(bs); notifier_list_init(&bs->close_notifiers); notifier_with_return_list_init(&bs->before_write_notifiers); + bs->refcnt = 1; return bs; } @@ -1518,6 +1519,9 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, /* dirty bitmap */ bs_dest->dirty_bitmap = bs_src->dirty_bitmap; + /* reference count */ + bs_dest->refcnt = bs_src->refcnt; + /* job */ bs_dest->in_use = bs_src->in_use; bs_dest->job = bs_src->job; @@ -4392,6 +4396,24 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs) } } +/* Get a reference to bs */ +void bdrv_ref(BlockDriverState *bs) +{ + bs->refcnt++; +} + +/* Release a previously grabbed reference to bs. + * If after releasing, reference count is zero, the BlockDriverState is + * deleted. */ +void bdrv_unref(BlockDriverState *bs) +{ + assert(bs->refcnt > 0); + if (--bs->refcnt == 0) { + bdrv_close(bs); + bdrv_delete(bs); + } +} + void bdrv_set_in_use(BlockDriverState *bs, int in_use) { assert(bs->in_use != in_use); diff --git a/include/block/block.h b/include/block/block.h index 742fce5..b33ef62 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -356,6 +356,8 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs); void bdrv_enable_copy_on_read(BlockDriverState *bs); void bdrv_disable_copy_on_read(BlockDriverState *bs); +void bdrv_ref(BlockDriverState *bs); +void bdrv_unref(BlockDriverState *bs); void bdrv_set_in_use(BlockDriverState *bs, int in_use); int bdrv_in_use(BlockDriverState *bs); diff --git a/include/block/block_int.h b/include/block/block_int.h index e45f2a0..1f85cfb 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -294,6 +294,7 @@ struct BlockDriverState { BlockDeviceIoStatus iostatus; char device_name[32]; HBitmap *dirty_bitmap; + int refcnt; int in_use; /* users other than guest access, eg. block migration */ QTAILQ_ENTRY(BlockDriverState) list;
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(+)