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 <f...@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;
>
