From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58787) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZgvK6-0007UJ-Nj for qemu-devel@nongnu.org; Tue, 29 Sep 2015 09:52:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZgvK5-0007XB-6Q for qemu-devel@nongnu.org; Tue, 29 Sep 2015 09:52:02 -0400 Date: Tue, 29 Sep 2015 15:51:51 +0200 From: Kevin Wolf Message-ID: <20150929135151.GK3930@noname.str.redhat.com> References: <1442497700-2536-1-git-send-email-kwolf@redhat.com> <1442497700-2536-14-git-send-email-kwolf@redhat.com> <5602D522.5030905@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="f61P+fpdnY2FZS1u" Content-Disposition: inline In-Reply-To: <5602D522.5030905@redhat.com> 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: Max Reitz Cc: berto@igalia.com, qemu-block@nongnu.org, jcody@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, stefanha@redhat.com --f61P+fpdnY2FZS1u Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. > >=20 > > Handling of snapshot=3Don must be moved further down in bdrv_open() > > because *pbs (which is the bs pointer in the BlockBackend) must already > > be set before bdrv_append() is called. Otherwise bdrv_append() changes > > the BB's pointer to the temporary snapshot, but bdrv_open() overwrites > > it with the read-only original image. > >=20 > > Signed-off-by: Kevin Wolf > > @@ -2155,6 +2157,42 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDr= iverState *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); >=20 > This drops a reference from the parent BDS to @from, and adds a new one > from the parent BDS to @to. However, this is not reflected here. You mean bdrv_ref(to); bdrv_unref(from); ? > > + } > > + 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, BlockD= riverState *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 b= ecause > > + * 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 needs = 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 the > > + * additional reference any more. */ > > + bdrv_unref(bs_new); > > } >=20 > 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? How would you assert something like this? Also, I think it's currently true, but there's no reason why it should stay so. The important part is 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. Kevin --f61P+fpdnY2FZS1u Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJWCpd3AAoJEH8JsnLIjy/WEw8QAIarrKFKWgd5zpe5CRGa0zk2 4T5gZJZnsw5jd1e+WSEzdGJ1B7g5EW6e2GEUL/PpEmbT/UxXtxAh3BscFebw/j5P 6XzrKrFTnJLxcaO8HJoqfWZeAcC2pux55IZsHBLCR7J8BlL06g6QYjDacnNhSAhe PnXerDnrFQIrviN9FEZbtUE09ZkwsCbC418A1NG7cs/SgTExzenuUzeCDST6USFB 51Yz3mhmh4pXAMJ8n0wSCl1eIUgxfoT/a+7eG3io3gX31lomjVYQhZHmWVnPw8mZ jTx0Ikp6oZT5q6y6te3NDJYZvyivsAwr356BjhH3/BAdEqT4z6iSqov3ZCt2J9sZ UmRJ4wKt5vTaRTepodjeoeUqqJN9yMtrkPXMV/aBz6mW2L1VUAtf2GxbBUkqeNUZ CZvOeha3BOjcGYRqQkPBY2dJACK/DfAMcb9eDxvhKNoNPHlX0kWq1twGXhcw2/61 d+TFDUrZRXI8RcJqgBG8++tzEwlsJ7KAg390x5WeFHG+snXUYoM+WoZzk4OUNuIs sH4hAnKIlHEhKE2q3WWPf0jNamN620+F23Z21EiIyDE7NXahaYOc7QbI1VyWGIQO Ph5IycbY8QTawfJRC12fOvyoiLHGEYSRyizDQbUjbfC4czxwFuqyeSpaP7iAKL8P bLcyhCv7ZFa4DJbI2xbI =srwv -----END PGP SIGNATURE----- --f61P+fpdnY2FZS1u--