|
From: | Wenchao Xia |
Subject: | Re: [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable |
Date: | Wed, 29 May 2013 17:45:38 +0800 |
User-agent: | Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/20130509 Thunderbird/17.0.6 |
于 2013-5-29 17:09, Stefan Hajnoczi 写道:
On Wed, May 29, 2013 at 03:54:46PM +0800, Wenchao Xia wrote:于 2013-5-28 15:46, Stefan Hajnoczi 写道:On Mon, May 27, 2013 at 05:25:16PM +0200, Kevin Wolf wrote:Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:From: Stefan Hajnoczi <address@hidden> The bs_snapshots global variable points to the BlockDriverState which will be used to save vmstate. This is really a savevm.c concept but was moved into block.c:bdrv_snapshots() when it became clear that hotplug could result in a dangling pointer. While auditing the block layer's global state I came upon bs_snapshots and realized that a variable is not necessary here. Simply find the first BlockDriverState capable of internal snapshots each time this is needed. The behavior of bdrv_snapshots() is preserved across hotplug because new drives are always appended to the bdrv_states list. This means that calling the new find_vmstate_bs() function is idempotent - it returns the same BlockDriverState unless it was hot-unplugged. Signed-off-by: Stefan Hajnoczi <address@hidden> Reviewed-by: Eric Blake <address@hidden> Reviewed-by: Wenchao Xia <address@hidden> Signed-off-by: Wenchao Xia <address@hidden> --- block.c | 28 ---------------------------- include/block/block.h | 1 - savevm.c | 19 +++++++++++++++---- 3 files changed, 15 insertions(+), 33 deletions(-) diff --git a/block.c b/block.c index 3f87489..478a3b2 100644 --- a/block.c +++ b/block.c @@ -99,9 +99,6 @@ static QTAILQ_HEAD(, BlockDriverState) bdrv_states = static QLIST_HEAD(, BlockDriver) bdrv_drivers = QLIST_HEAD_INITIALIZER(bdrv_drivers); -/* The device to use for VM snapshots */ -static BlockDriverState *bs_snapshots; - /* If non-zero, use only whitelisted block drivers */ static int use_bdrv_whitelist; @@ -1357,9 +1354,6 @@ void bdrv_close(BlockDriverState *bs) notifier_list_notify(&bs->close_notifiers, bs); if (bs->drv) { - if (bs == bs_snapshots) { - bs_snapshots = NULL; - } if (bs->backing_hd) { bdrv_delete(bs->backing_hd); bs->backing_hd = NULL; @@ -1591,7 +1585,6 @@ void bdrv_delete(BlockDriverState *bs) bdrv_close(bs); - assert(bs != bs_snapshots); g_free(bs); } @@ -1635,9 +1628,6 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops, { bs->dev_ops = ops; bs->dev_opaque = opaque; - if (bdrv_dev_has_removable_media(bs) && bs == bs_snapshots) { - bs_snapshots = NULL; - }This hunk isn't replaced by any other code. If I understand correctly what it's doing, it prevented you from saving the VM state to a removable device, which would be allowed after this patch. Is this a change we want to make? Why isn't it described in the commit message?My understanding of this change is different. Markus is on CC so maybe he can confirm. The point of bs_snapshots = NULL is not to prevent you from saving snapshots. It's simply to reset the pointer to the next snapshottable device (used by bdrv_snapshots()). See the bdrv_close() hunk above which does the same thing, as well as bdrv_snapshots() which iterates bdrv_states and updates bs_snapshots. So what this hunk does is to reset the bdrv_snapshots() iterator when a removable device is hooked up to an emulated storage controller. It's no longer necessary since this patch drops the global state (bs_snapshots) and users will always iterate from scratch. The whole stateful approach was not necessary. StefanReading the code, original logic actually forbidded saving vmstate into a removable device, now it is possible since find_vmstate_bs() doesn't check it. How about forbid again in find_vmstate_bs()?I don't follow. The hunk that Kevin quoted ensures that we restart bs_snapshots iteration - this prevents us from choosing a device that has no medium inserted (also see bdrv_can_snapshot() which checks for an inserted medium). This behavior is preserved in this patch because we now always restart iteration and it's no longer necessary to reset global iterator state. But I don't see any code that forbids/skips inserted, read-write removable devices in the orginal code. Please point out the specific piece of code you think has been dropped. Stefan
OK, original code make sure following code is executed before savm_vm(), after this patch following code will be always executed before save_vm(), nothing need to be added. while ((bs = bdrv_next(bs))) { if (bdrv_can_snapshot(bs)) { bs_snapshots = bs; return bs; } } -- Best Regards Wenchao Xia
[Prev in Thread] | Current Thread | [Next in Thread] |