Re: [Qemu-devel] [PATCH 1/7] block: Convert BlockDriverState.in_use to refcount

[Paolo Bonzini <pbonzini@redhat.com>]

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;
>