| Message ID | 1372744789-997-2-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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; >
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?
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.
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
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;
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(-)