| Message ID | 1375265640-27307-6-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
There should be a section of code in device hot unplug, checking DriverInfo's ref, fail or do nothing when ref != 1. But I haven't found that code, so not sure whether this patch will change the behavior in device hot unplug. > block-migration.c does not actually use DriveInfo anywhere. Hence it's > safe to drive ref code, we really only care about referencing BDS. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block-migration.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/block-migration.c b/block-migration.c > index f803f20..daf9ec1 100644 > --- a/block-migration.c > +++ b/block-migration.c > @@ -336,8 +336,8 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs) > bmds->completed_sectors = 0; > 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_ref(bs); > > block_mig_state.total_sector_sum += sectors; > > @@ -575,7 +575,7 @@ static void blk_mig_cleanup(void) > 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); > - drive_put_ref(drive_get_by_blockdev(bmds->bs)); > + bdrv_unref(bmds->bs); > g_free(bmds->aio_bitmap); > g_free(bmds); > } >
On Fri, Aug 02, 2013 at 04:13:55PM +0800, Wenchao Xia wrote: > There should be a section of code in device hot unplug, checking > DriverInfo's ref, fail or do nothing when ref != 1. But I haven't found > that code, so not sure whether this patch will change the behavior in > device hot unplug. It is not necessary to refuse device hot unplug when bs.ref > 0. But a couple of fixes are required to make it safe: Background ---------- DriveInfo always holds a BDS refcount, so BDS can never be deleted while the DriveInfo exists. DriveInfo is the metadata that connects an emulated storage controller with its BDS. Therefore, hot unplugging an emulated storage controller may release the last DriveInfo reference and delete it. Anything that still holds the BDS reference when DriveInfo is deleted will keep the BDS alive. It turns out that a lot of commands only use BDS with bdrv_find(), not DriveInfo, so you can continue to do useful things with the BDS after its DriveInfo is deleted. The problem ----------- A couple of places have not been converted to use bdrv_ref() yet, so they still go through drive_get_ref(drive_get_by_blockdev(bs)). These cases will now fail! I pointed out the blockjob cases but please grep to make sure there are no others. You need to fix them before this series is safe, otherwise drive_put_ref(NULL) will segfault! Stefan
diff --git a/block-migration.c b/block-migration.c index f803f20..daf9ec1 100644 --- a/block-migration.c +++ b/block-migration.c @@ -336,8 +336,8 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs) bmds->completed_sectors = 0; 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_ref(bs); block_mig_state.total_sector_sum += sectors; @@ -575,7 +575,7 @@ static void blk_mig_cleanup(void) 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); - drive_put_ref(drive_get_by_blockdev(bmds->bs)); + bdrv_unref(bmds->bs); g_free(bmds->aio_bitmap); g_free(bmds); }
block-migration.c does not actually use DriveInfo anywhere. Hence it's safe to drive ref code, we really only care about referencing BDS. Signed-off-by: Fam Zheng <famz@redhat.com> --- block-migration.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)