Patchwork [v3,5/7] migration: omit drive ref as we have bdrv_ref now

login
register
mail settings
Submitter Fam Zheng
Date July 31, 2013, 10:13 a.m.
Message ID <1375265640-27307-6-git-send-email-famz@redhat.com>
Download mbox | patch
Permalink /patch/263668/
State New
Headers show

Comments

Fam Zheng - July 31, 2013, 10:13 a.m.
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(-)
Wayne Xia - Aug. 2, 2013, 8:13 a.m.
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);
>       }
>
Stefan Hajnoczi - Aug. 2, 2013, 3:04 p.m.
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

Patch

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