[v2,3/7] block: implement reference count for BlockDriverState
diff mbox

Message ID 1375170777-31457-4-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng July 30, 2013, 7:52 a.m. UTC
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(+)

Comments

Andreas Färber July 30, 2013, 12:16 p.m. UTC | #1
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
Stefan Hajnoczi July 30, 2013, 2:50 p.m. UTC | #2
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
Jeff Cody July 30, 2013, 2:51 p.m. UTC | #3
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
>
Stefan Hajnoczi July 30, 2013, 2:58 p.m. UTC | #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.
Fam Zheng July 31, 2013, 9:51 a.m. UTC | #5
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!
Stefan Hajnoczi Aug. 1, 2013, 9:30 a.m. UTC | #6
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

Patch
diff mbox

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;