[Top][All Lists]

[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: 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

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 =

-/* 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) {
              bs->backing_hd = NULL;
@@ -1591,7 +1585,6 @@ void bdrv_delete(BlockDriverState *bs)


-    assert(bs != bs_snapshots);

@@ -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

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.


   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

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.


  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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]