From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:52363) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UhcxQ-0008Sj-Rf for qemu-devel@nongnu.org; Wed, 29 May 2013 05:46:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UhcxO-0007fa-Lh for qemu-devel@nongnu.org; Wed, 29 May 2013 05:46:12 -0400 Received: from e23smtp02.au.ibm.com ([202.81.31.144]:40715) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UhcxO-0007f9-1z for qemu-devel@nongnu.org; Wed, 29 May 2013 05:46:10 -0400 Received: from /spool/local by e23smtp02.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 29 May 2013 19:37:14 +1000 Received: from d23relay04.au.ibm.com (d23relay04.au.ibm.com [9.190.234.120]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id 1125A357804A for ; Wed, 29 May 2013 19:46:03 +1000 (EST) Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r4T9Vj0O21889182 for ; Wed, 29 May 2013 19:31:46 +1000 Received: from d23av02.au.ibm.com (loopback [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r4T9k1KD009401 for ; Wed, 29 May 2013 19:46:02 +1000 Message-ID: <51A5CE42.6000903@linux.vnet.ibm.com> Date: Wed, 29 May 2013 17:45:38 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1369451385-23452-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1369451385-23452-2-git-send-email-xiawenc@linux.vnet.ibm.com> <20130527152516.GF2373@dhcp-200-207.str.redhat.com> <20130528074649.GC13368@stefanha-thinkpad.redhat.com> <51A5B446.7080905@linux.vnet.ibm.com> <20130529090927.GA24346@stefanha-thinkpad.redhat.com> In-Reply-To: <20130529090927.GA24346@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , phrdina@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com, armbru@redhat.com 于 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 >>>>> >>>>> 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 >>>>> Reviewed-by: Eric Blake >>>>> Reviewed-by: Wenchao Xia >>>>> Signed-off-by: Wenchao Xia >>>>> --- >>>>> 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 > 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