From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48699) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zgwk2-0003iU-8Y for qemu-devel@nongnu.org; Tue, 29 Sep 2015 11:23:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zgwjy-0006A1-6D for qemu-devel@nongnu.org; Tue, 29 Sep 2015 11:22:54 -0400 Date: Tue, 29 Sep 2015 17:22:35 +0200 From: Kevin Wolf Message-ID: <20150929152235.GL3930@noname.str.redhat.com> References: <1442497700-2536-1-git-send-email-kwolf@redhat.com> <1442497700-2536-16-git-send-email-kwolf@redhat.com> <5602DC99.5040204@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="/i8j2F0k9BYX4qLc" Content-Disposition: inline In-Reply-To: <5602DC99.5040204@redhat.com> Subject: Re: [Qemu-devel] [PATCH 15/16] block: Add and use bdrv_replace_in_backing_chain() 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 --/i8j2F0k9BYX4qLc Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 23.09.2015 um 19:08 hat Max Reitz geschrieben: > On 17.09.2015 15:48, Kevin Wolf wrote: > > This cleans up the mess we left behind in the mirror code after the > > previous patch. Instead of using bdrv_swap(), just change pointers. > >=20 > > The interface change of the mirror job that callers must consider is > > that after job completion, their local BDS pointers still point to the > > same node now. qemu-img must change its code accordingly (which makes it > > easier to understand); the other callers stays unchanged because after > > completion they don't do anything with the BDS, but just with the job, > > and the job is still owned by the source BDS. > >=20 > > Signed-off-by: Kevin Wolf > > --- > > block.c | 32 +++++++++++++++++++++++++++++++- > > block/mirror.c | 23 +++++++---------------- > > include/block/block.h | 4 +++- > > qemu-img.c | 16 ++++++++-------- > > 4 files changed, 49 insertions(+), 26 deletions(-) > >=20 > > diff --git a/block.c b/block.c > > index 98fc17c..7c21659 100644 > > --- a/block.c > > +++ b/block.c > > @@ -1095,7 +1095,7 @@ static BdrvChild *bdrv_attach_child(BlockDriverSt= ate *parent_bs, > > return child; > > } > > =20 > > -void bdrv_detach_child(BdrvChild *child) > > +static void bdrv_detach_child(BdrvChild *child) > > { > > QLIST_REMOVE(child, next); > > QLIST_REMOVE(child, next_parent); > > @@ -2228,6 +2228,36 @@ void bdrv_append(BlockDriverState *bs_new, Block= DriverState *bs_top) > > bdrv_unref(bs_new); > > } > > =20 > > +void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverS= tate *new) > > +{ > > + assert(!bdrv_requests_pending(old)); > > + assert(!bdrv_requests_pending(new)); > > + > > + bdrv_ref(old); > > + > > + if (old->blk) { > > + /* As long as these fields aren't in BlockBackend, but in the = top-level > > + * BlockDriverState, it's not possible for a BDS to have two B= Bs. > > + * > > + * We really want to copy the fields from old to new, but we g= o for a > > + * swap instead so that pointers aren't duplicated and cause t= rouble. > > + * (Also, bdrv_swap() used to do the same.) */ > > + assert(!new->blk); > > + swap_feature_fields(old, new); > > + } > > + change_parent_backing_link(old, new); > > + > > + /* Change backing files if a previously independent node is added = to the > > + * chain. For active commit, we replace top by its own (indirect) = backing > > + * file and don't do anything here so we don't build a loop. */ > > + if (new->backing =3D=3D NULL && !bdrv_chain_contains(backing_bs(ol= d), new)) { > > + bdrv_set_backing_hd(new, backing_bs(old)); > > + bdrv_set_backing_hd(old, NULL); >=20 > Wouldn't we want @old to keep its backing file? Would we? The operation I had in mind was: Given a backing file chain, one node in that chain and an independent node, replace the node in the chain. That is: A <- B <- C <- D X becomes A <- X <- C <- D B Of course, you could define a different operation, but this seemed to be the obvious one that the mirror completion needs. > Then bdrv_append() would basically be a special case of this function, > with it additionally decrementing the refcount of @bs_new. Hm, less duplication sounds nice, but as long as the current way is technically correct, can we leave this for a cleanup on top? Kevin > Max >=20 > > + } > > + > > + bdrv_unref(old); > > +} > > + > > static void bdrv_delete(BlockDriverState *bs) > > { > > assert(!bs->job); >=20 --/i8j2F0k9BYX4qLc Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJWCqy7AAoJEH8JsnLIjy/WaPIP/3qQTXkR1j/jg4KEV6oeW1k5 LQpf7Ae0B6HQOHkXgETfK3xNwu252sG3EYdt6WNDFdLe8EbYVEAdjEfyUxVkawNw pXGvW1mstb0AqrGs+bQDXgzW4Pv2mNQ4UkcwmGnflUee3f0ufhpadlDDNr/TL9i7 rAJ5INIEx/afljdi9897OwvvAuR+dpZFxOQtLhxiRM06Nwvrl+JstlpLrVjiBnkO DXLU7vI/VE8wlQr9poe/oYQBJOxBDHha55MRdQ2j0xj81TXSLF0sonv4KDV9vLFA lPE81wiuXuFfF+ERodq2Qk2zBv9UQUr0L2W+QvSXQ2vDcfLI3dYFcMK1juIDJjud gx7a+I0xgbCEi93st8rAd2qsXrpu6I4Tm0xL5DQpGOdKl/c4MUJpTLE8djTHTjBR IJXUj9WxFiZdm7iTDU72WQJApAp2jd2GjeycW5Jzz/bUDZceWb+sNep2nxFFGEWb HQTCkYfoSXESrXq3GQyfPARPjswQhJuuy1Sy7liGNgXU4kyfJdKQJS9YyzhId0YG T7PMhBTSX8qsM7xjx0qsxlH87TseuJ0pB9BY1ZL+W0d9Mq18rXVRnhBNc8UxRjNp Q7demlPTblfKdtngLNv2G9s5U73EnXZTUelmgbPZ2Eg8PmkDhsCngS+K3X+BjmER fncAXRsQkufa6HJOoIB8 =IQOY -----END PGP SIGNATURE----- --/i8j2F0k9BYX4qLc--