From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48681) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZhIiE-0004Pp-52 for qemu-devel@nongnu.org; Wed, 30 Sep 2015 10:50:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZhIi9-0001gg-87 for qemu-devel@nongnu.org; Wed, 30 Sep 2015 10:50:30 -0400 References: <1442497700-2536-1-git-send-email-kwolf@redhat.com> <1442497700-2536-16-git-send-email-kwolf@redhat.com> <5602DC99.5040204@redhat.com> <20150929152235.GL3930@noname.str.redhat.com> From: Max Reitz Message-ID: <560BF6A6.1080709@redhat.com> Date: Wed, 30 Sep 2015 16:50:14 +0200 MIME-Version: 1.0 In-Reply-To: <20150929152235.GL3930@noname.str.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Qb6pIrvdKMUfvXDmc5VMvW4c0hDb5MAkN" 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: 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) --Qb6pIrvdKMUfvXDmc5VMvW4c0hDb5MAkN Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 29.09.2015 17:22, Kevin Wolf wrote: > 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. >>> >>> The interface change of the mirror job that callers must consider is >>> that after job completion, their local BDS pointers still point to th= e >>> same node now. qemu-img must change its code accordingly (which makes= it >>> easier to understand); the other callers stays unchanged because afte= r >>> completion they don't do anything with the BDS, but just with the job= , >>> and the job is still owned by the source BDS. >>> >>> 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(-) >>> >>> 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(BlockDriver= State *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, Blo= ckDriverState *bs_top) >>> bdrv_unref(bs_new); >>> } >>> =20 >>> +void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDrive= rState *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 th= e top-level >>> + * BlockDriverState, it's not possible for a BDS to have two= BBs. >>> + * >>> + * We really want to copy the fields from old to new, but we= go for a >>> + * swap instead so that pointers aren't duplicated and cause= trouble. >>> + * (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 adde= d 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(= old), new)) { >>> + bdrv_set_backing_hd(new, backing_bs(old)); >>> + bdrv_set_backing_hd(old, NULL); >> >> Wouldn't we want @old to keep its backing file? >=20 > 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: >=20 > A <- B <- C <- D >=20 > X >=20 > becomes >=20 > A <- X <- C <- D >=20 > B >=20 > Of course, you could define a different operation, but this seemed to b= e > the obvious one that the mirror completion needs. Oops, apparently I missed the backing_bs(old) in bdrv_set_backing_hd(new, ...). >> Then bdrv_append() would basically be a special case of this function,= >> with it additionally decrementing the refcount of @bs_new. >=20 > Hm, less duplication sounds nice, but as long as the current way is > technically correct, can we leave this for a cleanup on top? And with the backing_bs() taken into account, it is not so duplicated after all. Max > Kevin >=20 >> Max >> >>> + } >>> + >>> + bdrv_unref(old); >>> +} >>> + >>> static void bdrv_delete(BlockDriverState *bs) >>> { >>> assert(!bs->job); >> >=20 >=20 --Qb6pIrvdKMUfvXDmc5VMvW4c0hDb5MAkN 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/amAAoJEDuxQgLoOKytpnQH/iH5k8RPqGZQXzYmF5nYErX0 BCyEvV9B5/95s4a1hVSR1bRHcghoXpXeXCqoo6Eyv562tuS68MTnADqikwPqmKfR XClCtBJbPwJ9IQ2SSFtgx+jW/JcLIKggZtEX/3X8vfVk35cOJWZRkNYijxtm62Py pO9/okxy/nIshi5lcZgRIHktr4uYZNqDg9hn/NEYWZxDF71v23A5Zxw22eBXQ29+ cj+72AC6+j+iw6u5Fx4sFI2zOxgBsqSILbBedVt5lF2fIPoVCz0EuleAt6uEgdui tsDt5vc6igHcaoSbvyjNEHOASGgYmU8oOWCC2CwUhfoThHMWKywRBwAkxngDZhc= =K/j+ -----END PGP SIGNATURE----- --Qb6pIrvdKMUfvXDmc5VMvW4c0hDb5MAkN--