| Message ID | 1455646106-2047-9-git-send-email-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 16.02.2016 um 19:08 hat Max Reitz geschrieben: > This function first removed the BlockBackend from the blk_backends list > and cleared its name so it would no longer be found by blk_name(); since > blk_next() now iterates through monitor_block_backends (which the BB is > removed from in hmp_drive_del()), this is no longer necessary. Not clearing the name any more means that you can't create a new BlockBackend with the same ID until the hidden BB finally goes away. As it happens, you already can't do that with drive_add, because we also keep the ID reserved in the QemuOpts, so it's already broken today. However, before this patch, you can still use blockdev-add to create a new BB with the ID of a BB that drive_del removed; after it, you get an error: {"error": {"class": "GenericError", "desc": "Device with id 'disk' already exists"}} In order to fix this, I suggested on IRC that we view the BB name as belonging to the monitor reference logically, i.e. we add a name with monitor_add_blk() (which would get a new ID parameter) and clear it again in monitor_remove_blk(). > Second, bdrv_make_anon() was called on the BDS. This was intended for > cases where the BDS was owned by that BB alone; in which case the BDS > will no longer exist at this point thanks to the blk_remove_bs() in > hmp_drive_del(). > > Therefore, this function does nothing useful anymore. Remove it. > > Signed-off-by: Max Reitz <mreitz@redhat.com> Kevin
On 17.02.2016 15:18, Kevin Wolf wrote: > Am 16.02.2016 um 19:08 hat Max Reitz geschrieben: >> This function first removed the BlockBackend from the blk_backends list >> and cleared its name so it would no longer be found by blk_name(); since >> blk_next() now iterates through monitor_block_backends (which the BB is >> removed from in hmp_drive_del()), this is no longer necessary. > > Not clearing the name any more means that you can't create a new > BlockBackend with the same ID until the hidden BB finally goes away. > > As it happens, you already can't do that with drive_add, because we also > keep the ID reserved in the QemuOpts, so it's already broken today. > However, before this patch, you can still use blockdev-add to create a > new BB with the ID of a BB that drive_del removed; after it, you get an > error: > > {"error": {"class": "GenericError", "desc": "Device with id 'disk' > already exists"}} > > In order to fix this, I suggested on IRC that we view the BB name as > belonging to the monitor reference logically, i.e. we add a name with > monitor_add_blk() (which would get a new ID parameter) and clear it > again in monitor_remove_blk(). Moving the @name parameter from blk_new() to monitor_add_blk() seems reasonable but would necessitate pulling in two patches from the follow-up series to this (there are places which try to emit the BB's name while its BDS is created, which these two patches change so they no longer do this). Alternatively, as I said, we might want to call monitor_add_blk() in blk_new() if the name is non-NULL (and allow NULL names). Max >> Second, bdrv_make_anon() was called on the BDS. This was intended for >> cases where the BDS was owned by that BB alone; in which case the BDS >> will no longer exist at this point thanks to the blk_remove_bs() in >> hmp_drive_del(). >> >> Therefore, this function does nothing useful anymore. Remove it. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> > > Kevin >
Am 17.02.2016 um 16:47 hat Max Reitz geschrieben: > On 17.02.2016 15:18, Kevin Wolf wrote: > > Am 16.02.2016 um 19:08 hat Max Reitz geschrieben: > >> This function first removed the BlockBackend from the blk_backends list > >> and cleared its name so it would no longer be found by blk_name(); since > >> blk_next() now iterates through monitor_block_backends (which the BB is > >> removed from in hmp_drive_del()), this is no longer necessary. > > > > Not clearing the name any more means that you can't create a new > > BlockBackend with the same ID until the hidden BB finally goes away. > > > > As it happens, you already can't do that with drive_add, because we also > > keep the ID reserved in the QemuOpts, so it's already broken today. > > However, before this patch, you can still use blockdev-add to create a > > new BB with the ID of a BB that drive_del removed; after it, you get an > > error: > > > > {"error": {"class": "GenericError", "desc": "Device with id 'disk' > > already exists"}} > > > > In order to fix this, I suggested on IRC that we view the BB name as > > belonging to the monitor reference logically, i.e. we add a name with > > monitor_add_blk() (which would get a new ID parameter) and clear it > > again in monitor_remove_blk(). > > Moving the @name parameter from blk_new() to monitor_add_blk() seems > reasonable but would necessitate pulling in two patches from the > follow-up series to this (there are places which try to emit the BB's > name while its BDS is created, which these two patches change so they no > longer do this). > > Alternatively, as I said, we might want to call monitor_add_blk() in > blk_new() if the name is non-NULL (and allow NULL names). But if you allow NULL names, you need to fix those places, too. If the two patches aren't too bad, let's include them here. Kevin
diff --git a/block/block-backend.c b/block/block-backend.c index 430d7d5..a10fe44 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -69,7 +69,7 @@ static const AIOCBInfo block_backend_aiocb_info = { static void drive_info_del(DriveInfo *dinfo); -/* All the BlockBackends (except for hidden ones) */ +/* All the BlockBackends */ static QTAILQ_HEAD(, BlockBackend) blk_backends = QTAILQ_HEAD_INITIALIZER(blk_backends); @@ -181,10 +181,7 @@ static void blk_delete(BlockBackend *blk) g_free(blk->root_state.throttle_group); throttle_group_unref(blk->root_state.throttle_state); } - /* Avoid double-remove after blk_hide_on_behalf_of_hmp_drive_del() */ - if (blk->name[0]) { - QTAILQ_REMOVE(&blk_backends, blk, link); - } + QTAILQ_REMOVE(&blk_backends, blk, link); g_free(blk->name); drive_info_del(blk->legacy_dinfo); block_acct_cleanup(&blk->stats); @@ -400,24 +397,6 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo) } /* - * Hide @blk. - * @blk must not have been hidden already. - * Make attached BlockDriverState, if any, anonymous. - * Once hidden, @blk is invisible to all functions that don't receive - * it as argument. For example, blk_by_name() won't return it. - * Strictly for use by do_drive_del(). - * TODO get rid of it! - */ -void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk) -{ - QTAILQ_REMOVE(&blk_backends, blk, link); - blk->name[0] = 0; - if (blk->bs) { - bdrv_make_anon(blk->bs); - } -} - -/* * Disassociates the currently associated BlockDriverState from @blk. */ void blk_remove_bs(BlockBackend *blk) diff --git a/blockdev.c b/blockdev.c index ba1f648..6b929a9 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2818,13 +2818,10 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict) monitor_remove_blk(blk); - /* if we have a device attached to this BlockDriverState - * then we need to make the drive anonymous until the device - * can be removed. If this is a drive with no device backing - * then we can just get rid of the block driver state right here. + /* If this BlockBackend has a device attached to it, its refcount will be + * decremented when the device is removed; otherwise we have to do so here. */ if (blk_get_attached_dev(blk)) { - blk_hide_on_behalf_of_hmp_drive_del(blk); /* Further I/O must not pause the guest */ blk_set_on_error(blk, BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT); diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index 078690a..3a47982 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -80,8 +80,6 @@ BlockDriverState *blk_bs(BlockBackend *blk); void blk_remove_bs(BlockBackend *blk); void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs); -void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk); - void blk_iostatus_enable(BlockBackend *blk); bool blk_iostatus_is_enabled(const BlockBackend *blk); BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk);
This function first removed the BlockBackend from the blk_backends list and cleared its name so it would no longer be found by blk_name(); since blk_next() now iterates through monitor_block_backends (which the BB is removed from in hmp_drive_del()), this is no longer necessary. Second, bdrv_make_anon() was called on the BDS. This was intended for cases where the BDS was owned by that BB alone; in which case the BDS will no longer exist at this point thanks to the blk_remove_bs() in hmp_drive_del(). Therefore, this function does nothing useful anymore. Remove it. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/block-backend.c | 25 ++----------------------- blockdev.c | 7 ++----- include/sysemu/block-backend.h | 2 -- 3 files changed, 4 insertions(+), 30 deletions(-)