From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59512) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zelse-0000NH-DF for qemu-devel@nongnu.org; Wed, 23 Sep 2015 11:22:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zelsd-00085e-3e for qemu-devel@nongnu.org; Wed, 23 Sep 2015 11:22:48 -0400 References: <1442497700-2536-1-git-send-email-kwolf@redhat.com> <1442497700-2536-9-git-send-email-kwolf@redhat.com> From: Max Reitz Message-ID: <5602C3BD.4060704@redhat.com> Date: Wed, 23 Sep 2015 17:22:37 +0200 MIME-Version: 1.0 In-Reply-To: <1442497700-2536-9-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="hf4LJLdGl710CUNoSvk7qggiUmk3mDkJc" Subject: Re: [Qemu-devel] [PATCH 08/16] block: Manage backing file references in bdrv_set_backing_hd() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: jcody@redhat.com, berto@igalia.com, armbru@redhat.com, stefanha@redhat.com, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --hf4LJLdGl710CUNoSvk7qggiUmk3mDkJc Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 17.09.2015 15:48, Kevin Wolf wrote: > This simplifies the code somewhat, especially when dropping whole > backing file subchains. >=20 > The exception is the mirroring code that does adventurous things with > bdrv_swap() and in order to keep it working, I had to duplicate most of= > bdrv_set_backing_hd() locally. We'll get rid again of this ugliness > shortly. >=20 > Signed-off-by: Kevin Wolf > --- > block.c | 35 ++++++++++++++--------------------- > block/mirror.c | 17 ++++++++++++----- > block/stream.c | 21 --------------------- > block/vvfat.c | 6 +++++- > include/block/block.h | 1 + > 5 files changed, 32 insertions(+), 48 deletions(-) >=20 > diff --git a/block.c b/block.c > index fb94567..743f82e 100644 > --- a/block.c > +++ b/block.c > @@ -1094,7 +1094,7 @@ static BdrvChild *bdrv_attach_child(BlockDriverSt= ate *parent_bs, > return child; > } > =20 > -static void bdrv_detach_child(BdrvChild *child) > +void bdrv_detach_child(BdrvChild *child) > { > QLIST_REMOVE(child, next); > g_free(child); > @@ -1112,13 +1112,20 @@ void bdrv_unref_child(BlockDriverState *parent,= BdrvChild *child) > bdrv_unref(child_bs); > } > =20 > +/* > + * Sets the backing file link of a BDS. A new reference is created; ca= llers > + * which don't need their own reference any more must call bdrv_unref(= ). > + */ > void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backi= ng_hd) > { > + if (backing_hd) { > + bdrv_ref(backing_hd); > + } > =20 > if (bs->backing) { > assert(bs->backing_blocker); > bdrv_op_unblock_all(bs->backing->bs, bs->backing_blocker); > - bdrv_detach_child(bs->backing); > + bdrv_unref_child(bs, bs->backing); > } else if (backing_hd) { > error_setg(&bs->backing_blocker, > "node is used as backing hd of '%s'", > @@ -1214,7 +1221,10 @@ int bdrv_open_backing_file(BlockDriverState *bs,= QDict *options, Error **errp) > goto free_exit; > } > =20 > + /* Hook up the backing file link; drop our reference, bs owns the > + * backing_hd reference now */ > bdrv_set_backing_hd(bs, backing_hd); > + bdrv_unref(backing_hd); > =20 > free_exit: > g_free(backing_filename); > @@ -1884,11 +1894,7 @@ void bdrv_close(BlockDriverState *bs) > =20 > bs->drv->bdrv_close(bs); > =20 > - if (bs->backing) { > - BlockDriverState *backing_hd =3D bs->backing->bs; > - bdrv_set_backing_hd(bs, NULL); > - bdrv_unref(backing_hd); > - } > + bdrv_set_backing_hd(bs, NULL); > =20 > if (bs->file !=3D NULL) { > bdrv_unref(bs->file->bs); > @@ -2427,12 +2433,9 @@ int bdrv_drop_intermediate(BlockDriverState *act= ive, BlockDriverState *top, > BlockDriverState *intermediate; > BlockDriverState *base_bs =3D NULL; > BlockDriverState *new_top_bs =3D NULL; > - BlkIntermediateStates *intermediate_state, *next; > + BlkIntermediateStates *intermediate_state; > int ret =3D -EIO; > =20 > - QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_d= elete; > - QSIMPLEQ_INIT(&states_to_delete); > - > if (!top->drv || !base->drv) { > goto exit; > } > @@ -2459,7 +2462,6 @@ int bdrv_drop_intermediate(BlockDriverState *acti= ve, BlockDriverState *top, > while (intermediate) { > intermediate_state =3D g_new0(BlkIntermediateStates, 1); > intermediate_state->bs =3D intermediate; > - QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, en= try); > =20 > if (backing_bs(intermediate) =3D=3D base) { > base_bs =3D backing_bs(intermediate); > @@ -2482,17 +2484,8 @@ int bdrv_drop_intermediate(BlockDriverState *act= ive, BlockDriverState *top, > } > bdrv_set_backing_hd(new_top_bs, base_bs); > =20 > - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry= , next) { > - /* so that bdrv_close() does not recursively close the chain *= / > - bdrv_set_backing_hd(intermediate_state->bs, NULL); > - bdrv_unref(intermediate_state->bs); > - } > ret =3D 0; > - > exit: > - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry= , next) { > - g_free(intermediate_state); > - } > return ret; > } > =20 To me that begs the question why before we didn't simply bdrv_ref(base_bs), and then bdrv_unref(top). Well, now it's even simpler, so that's good. > diff --git a/block/mirror.c b/block/mirror.c > index 259e11a..571da27 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -370,11 +370,18 @@ static void mirror_exit(BlockJob *job, void *opaq= ue) > bdrv_swap(s->target, to_replace); > if (s->common.driver->job_type =3D=3D BLOCK_JOB_TYPE_COMMIT) {= > /* drop the bs loop chain formed by the swap: break the lo= op then > - * trigger the unref from the top one */ > - BlockDriverState *p =3D s->base->backing > - ? s->base->backing->bs : NULL; > - bdrv_set_backing_hd(s->base, NULL); > - bdrv_unref(p); > + * trigger the unref */ > + /* FIXME This duplicates bdrv_set_backing_hd(), except for= the > + * actual detach/unref so that the loop can be broken. Whe= n > + * bdrv_swap() gets replaced, this will become sane again.= */ > + BlockDriverState *backing =3D s->base->backing->bs; > + assert(s->base->backing_blocker); > + bdrv_op_unblock_all(s->base->backing->bs, s->base->backing= _blocker); I know it's dropped later on anwyay, but I can't stop my fingers from suggesting s/s->base->backing->bs/backing/. Anyway, with or without that: Reviewed-by: Max Reitz --hf4LJLdGl710CUNoSvk7qggiUmk3mDkJc 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 iQEcBAEBCAAGBQJWAsO9AAoJEDuxQgLoOKyt5CcH/1sqDHCHL4qcN07QDbZ/bIb0 +v06SuG6DNbnwaxknrBBbKJ84Nn02HNT9mmCaMCNz3ApmIKPxR5yS8mfZwoTxWg4 bZEU+IpLYQ7BVimvFnH5U9KEXxP8DuYuFSm22dcAI/Blld1BobLGPorYldq9pAJk TNv1MkormuSq0ovozfwZnjo7+ByZqi2UsMToCmYPM5pJ2uPSxdETBKjzoasNUw2g UC5rTCOb3I4bS7VCEpn1z7acHlRs95+7JiLxu+SmCj46A41W2QN9/YR2sQH3d7ZQ qiBEeayAyZ6lolaJkwNzEL1fxrS1OnDQn60EQKC02vGYQnxmG+c0l3a4l91bpKE= =C9Zu -----END PGP SIGNATURE----- --hf4LJLdGl710CUNoSvk7qggiUmk3mDkJc--