[1/7] block: Convert BlockDriverState.in_use to refcount
diff mbox

Message ID 1372744789-997-2-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng July 2, 2013, 5:59 a.m. UTC
Use numeric value to replace in_use flag in BDS, this will make
lifecycle management with ref count possible. This patch only replaces
existing uses of bdrv_set_in_use, so no logic change here.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block-migration.c               |  4 ++--
 block.c                         | 23 +++++++++++++++--------
 blockjob.c                      |  6 +++---
 hw/block/dataplane/virtio-blk.c |  4 ++--
 include/block/block.h           |  3 ++-
 include/block/block_int.h       |  2 +-
 6 files changed, 25 insertions(+), 17 deletions(-)

Comments

Paolo Bonzini July 2, 2013, 10:21 a.m. UTC | #1
Il 02/07/2013 07:59, Fam Zheng ha scritto:
> Use numeric value to replace in_use flag in BDS, this will make
> lifecycle management with ref count possible. This patch only replaces
> existing uses of bdrv_set_in_use, so no logic change here.

This still does not entirely explain the rules for who sets in_use (or
gets a reference) and who checks in_use.

Why should offline commit worry about the disk being shared, for
example?  The reason "of course" is that a background job might modify
bs->backing_hd while commit is running (or might expect bs->backing_hd
to not change).  However, this is in no way related to a reference count.

So I think your series is doing two things right (setting the in_use
flag for BDSes in the ->file and ->backing_hd chains; turning the in_use
flag into a counter) and one wrong (tying bdrv_in_use checks to the
lifecycle).  I wonder thus if we need two counters: the "in use" counter
and the reference count for the lifecycle.

One further nit is inline.

> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block-migration.c               |  4 ++--
>  block.c                         | 23 +++++++++++++++--------
>  blockjob.c                      |  6 +++---
>  hw/block/dataplane/virtio-blk.c |  4 ++--
>  include/block/block.h           |  3 ++-
>  include/block/block_int.h       |  2 +-
>  6 files changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/block-migration.c b/block-migration.c
> index 2fd7699..2efb6c0 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -321,7 +321,7 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
>          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_get_ref(bs);
>  
>          block_mig_state.total_sector_sum += sectors;
>  
> @@ -557,7 +557,7 @@ static void blk_mig_cleanup(void)
>      blk_mig_lock();
>      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);
> +        bdrv_get_ref(bmds->bs);
>          drive_put_ref(drive_get_by_blockdev(bmds->bs));
>          g_free(bmds->aio_bitmap);
>          g_free(bmds);
> diff --git a/block.c b/block.c
> index 6c493ad..84c3181 100644
> --- a/block.c
> +++ b/block.c
> @@ -1503,8 +1503,10 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
>      /* dirty bitmap */
>      bs_dest->dirty_bitmap       = bs_src->dirty_bitmap;
>  
> +    /* ref count */
> +    bs_dest->refcount           = bs_src->refcount;
> +
>      /* job */
> -    bs_dest->in_use             = bs_src->in_use;
>      bs_dest->job                = bs_src->job;
>  
>      /* keep the same entry in bdrv_states */
> @@ -1534,7 +1536,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
>      assert(bs_new->dirty_bitmap == NULL);
>      assert(bs_new->job == NULL);
>      assert(bs_new->dev == NULL);
> -    assert(bs_new->in_use == 0);
> +    assert(bs_new->refcount == 0);
>      assert(bs_new->io_limits_enabled == false);
>      assert(bs_new->block_timer == NULL);
>  
> @@ -1553,7 +1555,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
>      /* Check a few fields that should remain attached to the device */
>      assert(bs_new->dev == NULL);
>      assert(bs_new->job == NULL);
> -    assert(bs_new->in_use == 0);
> +    assert(bs_new->refcount == 0);
>      assert(bs_new->io_limits_enabled == false);
>      assert(bs_new->block_timer == NULL);
>  
> @@ -1590,7 +1592,7 @@ void bdrv_delete(BlockDriverState *bs)
>  {
>      assert(!bs->dev);
>      assert(!bs->job);
> -    assert(!bs->in_use);
> +    assert(!bs->refcount);
>  
>      /* remove from list, if necessary */
>      bdrv_make_anon(bs);
> @@ -4360,15 +4362,20 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs)
>      }
>  }
>  
> -void bdrv_set_in_use(BlockDriverState *bs, int in_use)
> +void bdrv_put_ref(BlockDriverState *bs)
> +{
> +    assert(bs->refcount > 0);
> +    bs->refcount--;
> +}
> +
> +void bdrv_get_ref(BlockDriverState *bs)
>  {
> -    assert(bs->in_use != in_use);
> -    bs->in_use = in_use;
> +    bs->refcount++;
>  }

The convention that we use in QEMU is bdrv_ref/bdrv_unref.

Paolo

>  int bdrv_in_use(BlockDriverState *bs)
>  {
> -    return bs->in_use;
> +    return bs->refcount > 0;
>  }
>  
>  void bdrv_iostatus_enable(BlockDriverState *bs)
> diff --git a/blockjob.c b/blockjob.c
> index ca80df1..a841a66 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -45,7 +45,7 @@ void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
>          error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
>          return NULL;
>      }
> -    bdrv_set_in_use(bs, 1);
> +    bdrv_get_ref(bs);
>  
>      job = g_malloc0(job_type->instance_size);
>      job->job_type      = job_type;
> @@ -63,7 +63,7 @@ void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
>          if (error_is_set(&local_err)) {
>              bs->job = NULL;
>              g_free(job);
> -            bdrv_set_in_use(bs, 0);
> +            bdrv_put_ref(bs);
>              error_propagate(errp, local_err);
>              return NULL;
>          }
> @@ -79,7 +79,7 @@ void block_job_completed(BlockJob *job, int ret)
>      job->cb(job->opaque, ret);
>      bs->job = NULL;
>      g_free(job);
> -    bdrv_set_in_use(bs, 0);
> +    bdrv_put_ref(bs);
>  }
>  
>  void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 0356665..9893a11 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -431,7 +431,7 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
>      s->blk = blk;
>  
>      /* Prevent block operations that conflict with data plane thread */
> -    bdrv_set_in_use(blk->conf.bs, 1);
> +    bdrv_get_ref(blk->conf.bs);
>  
>      error_setg(&s->migration_blocker,
>              "x-data-plane does not support migration");
> @@ -450,7 +450,7 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
>      virtio_blk_data_plane_stop(s);
>      migrate_del_blocker(s->migration_blocker);
>      error_free(s->migration_blocker);
> -    bdrv_set_in_use(s->blk->conf.bs, 0);
> +    bdrv_put_ref(s->blk->conf.bs);
>      g_free(s);
>  }
>  
> diff --git a/include/block/block.h b/include/block/block.h
> index dd8eca1..77f0f0d 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -353,7 +353,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_set_in_use(BlockDriverState *bs, int in_use);
> +void bdrv_put_ref(BlockDriverState *bs);
> +void bdrv_get_ref(BlockDriverState *bs);
>  int bdrv_in_use(BlockDriverState *bs);
>  
>  #ifdef CONFIG_LINUX_AIO
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index c6ac871..1ea80e7 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -294,7 +294,7 @@ struct BlockDriverState {
>      BlockDeviceIoStatus iostatus;
>      char device_name[32];
>      HBitmap *dirty_bitmap;
> -    int in_use; /* users other than guest access, eg. block migration */
> +    int refcount;
>      QTAILQ_ENTRY(BlockDriverState) list;
>  
>      QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
>
Eric Blake July 2, 2013, 7:41 p.m. UTC | #2
On 07/01/2013 11:59 PM, Fam Zheng wrote:
> Use numeric value to replace in_use flag in BDS, this will make
> lifecycle management with ref count possible. This patch only replaces
> existing uses of bdrv_set_in_use, so no logic change here.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block-migration.c               |  4 ++--
>  block.c                         | 23 +++++++++++++++--------
>  blockjob.c                      |  6 +++---
>  hw/block/dataplane/virtio-blk.c |  4 ++--
>  include/block/block.h           |  3 ++-
>  include/block/block_int.h       |  2 +-
>  6 files changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/block-migration.c b/block-migration.c
> index 2fd7699..2efb6c0 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -321,7 +321,7 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
>          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_get_ref(bs);

The old code sets the flag, the new code gets a ref.

>  
>          block_mig_state.total_sector_sum += sectors;
>  
> @@ -557,7 +557,7 @@ static void blk_mig_cleanup(void)
>      blk_mig_lock();
>      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);
> +        bdrv_get_ref(bmds->bs);

The old code sets the flag, the new code gets a ref.  Shouldn't this be
clearing a ref, with bdrv_put_ref?
Fam Zheng July 3, 2013, 12:59 a.m. UTC | #3
On Tue, 07/02 13:41, Eric Blake wrote:
> On 07/01/2013 11:59 PM, Fam Zheng wrote:
> > Use numeric value to replace in_use flag in BDS, this will make
> > lifecycle management with ref count possible. This patch only replaces
> > existing uses of bdrv_set_in_use, so no logic change here.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block-migration.c               |  4 ++--
> >  block.c                         | 23 +++++++++++++++--------
> >  blockjob.c                      |  6 +++---
> >  hw/block/dataplane/virtio-blk.c |  4 ++--
> >  include/block/block.h           |  3 ++-
> >  include/block/block_int.h       |  2 +-
> >  6 files changed, 25 insertions(+), 17 deletions(-)
> > 
> > diff --git a/block-migration.c b/block-migration.c
> > index 2fd7699..2efb6c0 100644
> > --- a/block-migration.c
> > +++ b/block-migration.c
> > @@ -321,7 +321,7 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
> >          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_get_ref(bs);
> 
> The old code sets the flag, the new code gets a ref.
> 
> >  
> >          block_mig_state.total_sector_sum += sectors;
> >  
> > @@ -557,7 +557,7 @@ static void blk_mig_cleanup(void)
> >      blk_mig_lock();
> >      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);
> > +        bdrv_get_ref(bmds->bs);
> 
> The old code sets the flag, the new code gets a ref.  Shouldn't this be
> clearing a ref, with bdrv_put_ref?
Yes, thanks for pointing out.
Fam Zheng July 8, 2013, 8:37 a.m. UTC | #4
On Tue, 07/02 12:21, Paolo Bonzini wrote:
> Il 02/07/2013 07:59, Fam Zheng ha scritto:
> > Use numeric value to replace in_use flag in BDS, this will make
> > lifecycle management with ref count possible. This patch only replaces
> > existing uses of bdrv_set_in_use, so no logic change here.
> 
> This still does not entirely explain the rules for who sets in_use (or
> gets a reference) and who checks in_use.
> 
> Why should offline commit worry about the disk being shared, for
> example?  The reason "of course" is that a background job might modify
> bs->backing_hd while commit is running (or might expect bs->backing_hd
> to not change).  However, this is in no way related to a reference count.
> 
> So I think your series is doing two things right (setting the in_use
> flag for BDSes in the ->file and ->backing_hd chains; turning the in_use
> flag into a counter) and one wrong (tying bdrv_in_use checks to the
> lifecycle).  I wonder thus if we need two counters: the "in use" counter
> and the reference count for the lifecycle.

But the two are not orthognal, it's somehow like rlock and wlock.  What
I try is to simplify the (usual) logic that a BDS user calls
bdrv_get_ref() for lifecycle as well as bdrv_set_in_use() to secure the
state. With refcount as a sign for in_use, they just need inference if
its ref count is more than one: it's shared, don't modify it, and expect
it to change, like rwlock implies.

I think commit should worry about the disk being shared: We eventually
need the target of drive-backup to be exported through NBD, which means
it has a device id, and its backing_hd is also attached to guest device,
so we can't commit it. That said, I think a BDS being shared is an
evidence for not to make any change. Do we have any exception on this
rule?

For NBD there is close notifier, so I want to drop the refcount (PATCH
3/7).

> > -void bdrv_set_in_use(BlockDriverState *bs, int in_use)
> > +void bdrv_put_ref(BlockDriverState *bs)
> > +{
> > +    assert(bs->refcount > 0);
> > +    bs->refcount--;
> > +}
> > +
> > +void bdrv_get_ref(BlockDriverState *bs)
> >  {
> > -    assert(bs->in_use != in_use);
> > -    bs->in_use = in_use;
> > +    bs->refcount++;
> >  }
> 
> The convention that we use in QEMU is bdrv_ref/bdrv_unref.

I'm following drive_get_ref() here.

Thanks.

Fam

Patch
diff mbox

diff --git a/block-migration.c b/block-migration.c
index 2fd7699..2efb6c0 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -321,7 +321,7 @@  static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
         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_get_ref(bs);
 
         block_mig_state.total_sector_sum += sectors;
 
@@ -557,7 +557,7 @@  static void blk_mig_cleanup(void)
     blk_mig_lock();
     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);
+        bdrv_get_ref(bmds->bs);
         drive_put_ref(drive_get_by_blockdev(bmds->bs));
         g_free(bmds->aio_bitmap);
         g_free(bmds);
diff --git a/block.c b/block.c
index 6c493ad..84c3181 100644
--- a/block.c
+++ b/block.c
@@ -1503,8 +1503,10 @@  static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
     /* dirty bitmap */
     bs_dest->dirty_bitmap       = bs_src->dirty_bitmap;
 
+    /* ref count */
+    bs_dest->refcount           = bs_src->refcount;
+
     /* job */
-    bs_dest->in_use             = bs_src->in_use;
     bs_dest->job                = bs_src->job;
 
     /* keep the same entry in bdrv_states */
@@ -1534,7 +1536,7 @@  void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
     assert(bs_new->dirty_bitmap == NULL);
     assert(bs_new->job == NULL);
     assert(bs_new->dev == NULL);
-    assert(bs_new->in_use == 0);
+    assert(bs_new->refcount == 0);
     assert(bs_new->io_limits_enabled == false);
     assert(bs_new->block_timer == NULL);
 
@@ -1553,7 +1555,7 @@  void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
     /* Check a few fields that should remain attached to the device */
     assert(bs_new->dev == NULL);
     assert(bs_new->job == NULL);
-    assert(bs_new->in_use == 0);
+    assert(bs_new->refcount == 0);
     assert(bs_new->io_limits_enabled == false);
     assert(bs_new->block_timer == NULL);
 
@@ -1590,7 +1592,7 @@  void bdrv_delete(BlockDriverState *bs)
 {
     assert(!bs->dev);
     assert(!bs->job);
-    assert(!bs->in_use);
+    assert(!bs->refcount);
 
     /* remove from list, if necessary */
     bdrv_make_anon(bs);
@@ -4360,15 +4362,20 @@  int64_t bdrv_get_dirty_count(BlockDriverState *bs)
     }
 }
 
-void bdrv_set_in_use(BlockDriverState *bs, int in_use)
+void bdrv_put_ref(BlockDriverState *bs)
+{
+    assert(bs->refcount > 0);
+    bs->refcount--;
+}
+
+void bdrv_get_ref(BlockDriverState *bs)
 {
-    assert(bs->in_use != in_use);
-    bs->in_use = in_use;
+    bs->refcount++;
 }
 
 int bdrv_in_use(BlockDriverState *bs)
 {
-    return bs->in_use;
+    return bs->refcount > 0;
 }
 
 void bdrv_iostatus_enable(BlockDriverState *bs)
diff --git a/blockjob.c b/blockjob.c
index ca80df1..a841a66 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -45,7 +45,7 @@  void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
         error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
         return NULL;
     }
-    bdrv_set_in_use(bs, 1);
+    bdrv_get_ref(bs);
 
     job = g_malloc0(job_type->instance_size);
     job->job_type      = job_type;
@@ -63,7 +63,7 @@  void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
         if (error_is_set(&local_err)) {
             bs->job = NULL;
             g_free(job);
-            bdrv_set_in_use(bs, 0);
+            bdrv_put_ref(bs);
             error_propagate(errp, local_err);
             return NULL;
         }
@@ -79,7 +79,7 @@  void block_job_completed(BlockJob *job, int ret)
     job->cb(job->opaque, ret);
     bs->job = NULL;
     g_free(job);
-    bdrv_set_in_use(bs, 0);
+    bdrv_put_ref(bs);
 }
 
 void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 0356665..9893a11 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -431,7 +431,7 @@  bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
     s->blk = blk;
 
     /* Prevent block operations that conflict with data plane thread */
-    bdrv_set_in_use(blk->conf.bs, 1);
+    bdrv_get_ref(blk->conf.bs);
 
     error_setg(&s->migration_blocker,
             "x-data-plane does not support migration");
@@ -450,7 +450,7 @@  void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
     virtio_blk_data_plane_stop(s);
     migrate_del_blocker(s->migration_blocker);
     error_free(s->migration_blocker);
-    bdrv_set_in_use(s->blk->conf.bs, 0);
+    bdrv_put_ref(s->blk->conf.bs);
     g_free(s);
 }
 
diff --git a/include/block/block.h b/include/block/block.h
index dd8eca1..77f0f0d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -353,7 +353,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_set_in_use(BlockDriverState *bs, int in_use);
+void bdrv_put_ref(BlockDriverState *bs);
+void bdrv_get_ref(BlockDriverState *bs);
 int bdrv_in_use(BlockDriverState *bs);
 
 #ifdef CONFIG_LINUX_AIO
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c6ac871..1ea80e7 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -294,7 +294,7 @@  struct BlockDriverState {
     BlockDeviceIoStatus iostatus;
     char device_name[32];
     HBitmap *dirty_bitmap;
-    int in_use; /* users other than guest access, eg. block migration */
+    int refcount;
     QTAILQ_ENTRY(BlockDriverState) list;
 
     QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;