[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable |
Date: |
Wed, 29 May 2013 11:09:27 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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.
> >
> >Stefan
> >
> Reading 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
[Qemu-devel] [PATCH V3 2/4] block: move snapshot code in block.c to block/snapshot.c, Wenchao Xia, 2013/05/24
[Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output, Wenchao Xia, 2013/05/24
[Qemu-devel] [PATCH V3 3/4] block: move qmp and info dump related code to block/qapi.c, Wenchao Xia, 2013/05/24