From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40874) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UhcO5-0000ub-Id for qemu-devel@nongnu.org; Wed, 29 May 2013 05:09:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UhcNw-0003FE-2E for qemu-devel@nongnu.org; Wed, 29 May 2013 05:09:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:5777) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UhcNv-0003F0-QG for qemu-devel@nongnu.org; Wed, 29 May 2013 05:09:31 -0400 Date: Wed, 29 May 2013 11:09:27 +0200 From: Stefan Hajnoczi Message-ID: <20130529090927.GA24346@stefanha-thinkpad.redhat.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <51A5B446.7080905@linux.vnet.ibm.com> Content-Transfer-Encoding: quoted-printable 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: Wenchao Xia Cc: Kevin Wolf , phrdina@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com, armbru@redhat.com On Wed, May 29, 2013 at 03:54:46PM +0800, Wenchao Xia wrote: > =E4=BA=8E 2013-5-28 15:46, Stefan Hajnoczi =E5=86=99=E9=81=93: > >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 whic= h > >>>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 hotplu= g > >>>could result in a dangling pointer. > >>> > >>>While auditing the block layer's global state I came upon bs_snapsho= ts > >>>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 return= s > >>>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= =3D > >>> static QLIST_HEAD(, BlockDriver) bdrv_drivers =3D > >>> 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 =3D=3D bs_snapshots) { > >>>- bs_snapshots =3D NULL; > >>>- } > >>> if (bs->backing_hd) { > >>> bdrv_delete(bs->backing_hd); > >>> bs->backing_hd =3D NULL; > >>>@@ -1591,7 +1585,6 @@ void bdrv_delete(BlockDriverState *bs) > >>> > >>> bdrv_close(bs); > >>> > >>>- assert(bs !=3D bs_snapshots); > >>> g_free(bs); > >>> } > >>> > >>>@@ -1635,9 +1628,6 @@ void bdrv_set_dev_ops(BlockDriverState *bs, co= nst BlockDevOps *ops, > >>> { > >>> bs->dev_ops =3D ops; > >>> bs->dev_opaque =3D opaque; > >>>- if (bdrv_dev_has_removable_media(bs) && bs =3D=3D bs_snapshots)= { > >>>- bs_snapshots =3D 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 commi= t > >>message? > > > >My understanding of this change is different. Markus is on CC so mayb= e > >he can confirm. > > > >The point of bs_snapshots =3D 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