From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45988) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZhIdB-0003dB-Ui for qemu-devel@nongnu.org; Wed, 30 Sep 2015 10:45:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZhId6-00077c-0l for qemu-devel@nongnu.org; Wed, 30 Sep 2015 10:45:17 -0400 References: <1442497700-2536-1-git-send-email-kwolf@redhat.com> <1442497700-2536-14-git-send-email-kwolf@redhat.com> <5602D522.5030905@redhat.com> <20150929135151.GK3930@noname.str.redhat.com> From: Max Reitz Message-ID: <560BF56D.9060201@redhat.com> Date: Wed, 30 Sep 2015 16:45:01 +0200 MIME-Version: 1.0 In-Reply-To: <20150929135151.GK3930@noname.str.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="KLpOhbg4Wo3FDWX509ROSGeh0uioHOp9T" Subject: Re: [Qemu-devel] [PATCH 13/16] block: Implement bdrv_append() without bdrv_swap() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: berto@igalia.com, qemu-block@nongnu.org, jcody@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, stefanha@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --KLpOhbg4Wo3FDWX509ROSGeh0uioHOp9T Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 29.09.2015 15:51, Kevin Wolf wrote: > Am 23.09.2015 um 18:36 hat Max Reitz geschrieben: >> On 17.09.2015 15:48, Kevin Wolf wrote: >>> Remember all parent nodes and just change the pointers there instead = of >>> swapping the contents of the BlockDriverState. >>> >>> Handling of snapshot=3Don must be moved further down in bdrv_open() >>> because *pbs (which is the bs pointer in the BlockBackend) must alrea= dy >>> be set before bdrv_append() is called. Otherwise bdrv_append() change= s >>> the BB's pointer to the temporary snapshot, but bdrv_open() overwrite= s >>> it with the read-only original image. >>> >>> Signed-off-by: Kevin Wolf >=20 >>> @@ -2155,6 +2157,42 @@ void bdrv_swap(BlockDriverState *bs_new, Block= DriverState *bs_old) >>> bdrv_rebind(bs_old); >>> } >>> =20 >>> +static void change_parent_backing_link(BlockDriverState *from, >>> + BlockDriverState *to) >>> +{ >>> + BdrvChild *c, *next; >>> + >>> + QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) { >>> + assert(c->role !=3D &child_backing); >>> + c->bs =3D to; >>> + QLIST_REMOVE(c, next_parent); >>> + QLIST_INSERT_HEAD(&to->parents, c, next_parent); >> >> This drops a reference from the parent BDS to @from, and adds a new on= e >> from the parent BDS to @to. However, this is not reflected here. >=20 > You mean bdrv_ref(to); bdrv_unref(from); ? Yes. >>> + } >>> + if (from->blk) { >>> + blk_set_bs(from->blk, to); >>> + if (!to->device_list.tqe_prev) { >>> + QTAILQ_INSERT_BEFORE(from, to, device_list); >>> + } >>> + QTAILQ_REMOVE(&bdrv_states, from, device_list); >>> + } >>> +} >>> + >>> +static void swap_feature_fields(BlockDriverState *bs_top, >>> + BlockDriverState *bs_new) >>> +{ >>> + BlockDriverState tmp; >>> + >>> + bdrv_move_feature_fields(&tmp, bs_top); >>> + bdrv_move_feature_fields(bs_top, bs_new); >>> + bdrv_move_feature_fields(bs_new, &tmp); >>> + >>> + assert(!bs_new->io_limits_enabled); >>> + if (bs_top->io_limits_enabled) { >>> + bdrv_io_limits_enable(bs_new, throttle_group_get_name(bs_top= )); >>> + bdrv_io_limits_disable(bs_top); >>> + } >>> +} >>> + >>> /* >>> * Add new bs contents at the top of an image chain while the chain = is >>> * live, while keeping required fields on the top layer. >>> @@ -2165,14 +2203,29 @@ void bdrv_swap(BlockDriverState *bs_new, Bloc= kDriverState *bs_old) >>> * bs_new must not be attached to a BlockBackend. >>> * >>> * This function does not create any image files. >>> + * >>> + * bdrv_append() takes ownership of a bs_new reference and unrefs it= because >>> + * that's what the callers commonly need. bs_new will be referenced = by the old >>> + * parents of bs_top after bdrv_append() returns. If the caller need= s to keep a >>> + * reference of its own, it must call bdrv_ref(). >>> */ >>> void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)= >>> { >>> - bdrv_swap(bs_new, bs_top); >>> + assert(!bdrv_requests_pending(bs_top)); >>> + assert(!bdrv_requests_pending(bs_new)); >>> + >>> + bdrv_ref(bs_top); >>> + change_parent_backing_link(bs_top, bs_new); >>> + >>> + /* Some fields always stay on top of the backing file chain */ >>> + swap_feature_fields(bs_top, bs_new); >>> + >>> + bdrv_set_backing_hd(bs_new, bs_top); >>> + bdrv_unref(bs_top); >>> =20 >>> - /* The contents of 'tmp' will become bs_top, as we are >>> - * swapping bs_new and bs_top contents. */ >>> - bdrv_set_backing_hd(bs_top, bs_new); >>> + /* bs_new is now referenced by its new parents, we don't need th= e >>> + * additional reference any more. */ >>> + bdrv_unref(bs_new); >>> } >> >> Before, all pointers to @bs_new were moved to @bs_top. Now, they stay = at >> @bs_new. I suppose we are assuming there are no pointers to @bs_new, >> should we assert that, and/or point it out in the documentation? >=20 > How would you assert something like this? Of course, I have no idea. > Also, I think it's currently > true, but there's no reason why it should stay so. The important part i= s > just that it's true while applying the patch because the semantics > changes. Once it's applied, we have sane behaviour and can make use of > it. The thing is that this is exactly the reason for the bug Berto found. external_snapshot_commit() keeps a reference to state->new_bs (@bs_new) and uses it afterwards for bdrv_reopen(), whereas it should be using state->old_bs (@bs_top) after this series. Max --KLpOhbg4Wo3FDWX509ROSGeh0uioHOp9T Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWC/VtAAoJEDuxQgLoOKytpHYIAKfcGS6Z2CA83ZmzS/6UzPVB 3pC8En6XuUgyP5DebhK6W/AkT1+fyyRIAbXibqs8zV9I1Z39Eo6jmH5ElQXrodcg PXI5U83m1bgI+RsXlvm/KI218khIrPB+kEY2bB8VBzSumzBxTrT09PaCudyL0NSP kmYX2x6jURvNDvKeDReDX4jWw3CDBd5VdECtWcqIhiKFJ4Y46/2XMgdSkFvoslVM 0qBpzXvUAGdoAA1pRgpWVdXKK4GulTjqPWTqK8L9kLY9u8Mfm48ZBhlICtN3De/h Z2ErFuQqTDJA+cRWEiUENRolrqZnAXVo+jXl7fKcodkN5Oxj2OtmzKF8T00AfKY= =JyHq -----END PGP SIGNATURE----- --KLpOhbg4Wo3FDWX509ROSGeh0uioHOp9T--