From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52298) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zgv5v-00064V-JH for qemu-devel@nongnu.org; Tue, 29 Sep 2015 09:37:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zgv5q-0000b9-Pn for qemu-devel@nongnu.org; Tue, 29 Sep 2015 09:37:23 -0400 Date: Tue, 29 Sep 2015 15:37:04 +0200 From: Kevin Wolf Message-ID: <20150929133704.GJ3930@noname.str.redhat.com> References: <1442497700-2536-1-git-send-email-kwolf@redhat.com> <1442497700-2536-10-git-send-email-kwolf@redhat.com> <5602D520.3050303@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="7CZp05NP8/gJM8Cl" Content-Disposition: inline In-Reply-To: <5602D520.3050303@redhat.com> Subject: Re: [Qemu-devel] [PATCH 09/16] block: Split bdrv_move_feature_fields() 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 --7CZp05NP8/gJM8Cl 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: > > After bdrv_swap(), some fields must be moved back to their original BDS > > to compensate for the effects that a swap of the contents of the objects > > has while keeping the old addresses. Other fields must be moved back > > because they should logically be moved and must stay on top > >=20 > > When replacing bdrv_swap() with operations changing the pointers in the > > parents, we only need the latter and must avoid swapping the former. > > Split the function accordingly. > >=20 > > Signed-off-by: Kevin Wolf > > --- > > block.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > >=20 > > diff --git a/block.c b/block.c > > index 743f82e..7930f3c 100644 > > --- a/block.c > > +++ b/block.c > > @@ -1984,6 +1984,8 @@ static void bdrv_rebind(BlockDriverState *bs) > > } > > } > > =20 > > +/* Fields that need to stay with the top-level BDS, no matter whether = the > > + * address of the top-level BDS stays the same or not. */ > > static void bdrv_move_feature_fields(BlockDriverState *bs_dest, > > BlockDriverState *bs_src) > > { > > @@ -2019,7 +2021,13 @@ static void bdrv_move_feature_fields(BlockDriver= State *bs_dest, > > =20 > > /* dirty bitmap */ > > bs_dest->dirty_bitmaps =3D bs_src->dirty_bitmaps; > > +} > > =20 > > +/* Fields that only need to be swapped if the contents of BDSes is swa= pped > > + * rather than pointers being changed in the parents. */ > > +static void bdrv_move_reference_fields(BlockDriverState *bs_dest, > > + BlockDriverState *bs_src) > > +{ > > /* reference count */ > > bs_dest->refcnt =3D bs_src->refcnt; > > =20 >=20 > I'm not sure whether the op blockers should be moved in this function... > I think they should be moved in bdrv_move_feasture_fields(), because > they generally depend on the position within the node graph and not on > the BDS itself, don't they? Op blockers are a mess. I believe we have both kinds. In any case, we should be relatively safe here because the operation involving something like bdrv_append() or bdrv_replace_in_backing_chain() would have been blocked in the first place if there were an op blocker that needs to be moved. I seem to remember that there were two reasons for not moving op blockers: The first one is that the next action the block jobs want to do is to unblock (because the job has completed) and it's nicer to do this on the same BDS as they blocked initially. The other one is that bs->backing_blocker is already managed by bdrv_set_backing_hd() and swapping in addition broke things. Oh, and the third one is that swapping is plain wrong. We're not really performing a symmetrical swap any more. If BDS A is replaced by BDS B (or B is appended to a chain that had A as it's root before), all parents of _both_ A and B point to B afterwards. So swapping would move some op blockers to now completely unrelated BDSes. Kevin --7CZp05NP8/gJM8Cl Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJWCpQAAAoJEH8JsnLIjy/WyAMP/1BfNFYFzU0z0clSbe/U/olP 2lWuc3WKTrRy8BnxwyLnAEJpPQMpI5HVtMjQ8G3TATkzVotRm7sv4ki5XQ1r3xVr ce+7mDVcEybumR7XR949Iz8AE0lZcFIqqwh8FhRGcnbi5/1XJ2msqeKxSRW3Pazm FN+ZnmB3WvmvwWBscqrbBj/+V37a35wus4Jrw5oATsZ4sUKZlPkTHAUpij2YBEk6 Wu2MgEBjEyyByH0mksF+6Jyus4EDJ25sFiytCX23zdTPaV4DU+TREFifldj2SBAP Osmuw7l0mcdAyfJYC3OvaLkSAagw9z7/T2jjdVxZGB5ve2j9qfgHBv9Pt+02w9tz LZynSliMCDcbe7h4V5SPt/Nn5wxPLPTXVmTw4UnvFfrliiYKoyReR2VET/zg7JLr cfpT6hu+l8XIEIW4ok5fq2jPPDX8elYLjdglO2U4HKC/oAMjCTcBik2T3JVE0G6N 4LoMeI7aqIxAIO7R0RMTFC9t/AWSWsnVUwE0z25/SlHsVR3R8afuhYZnhRISk2gp x7ym4MdRYFVTift0giuz6QvJzNvH5o/BTKHa4DeGN7OfN1eTcs5bLymJ4BN2PMpM KfGMoZHv+7jJ6XpR/1KG8vv1XzEhnTq5gbzFl2+tnDDC87hPHlUAmnralEmIrsZs qIIk6f9pevL1e6Z/pon8 =wikv -----END PGP SIGNATURE----- --7CZp05NP8/gJM8Cl--