From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55763) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zen2Y-0002jw-66 for qemu-devel@nongnu.org; Wed, 23 Sep 2015 12:37:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zen2W-0000hN-HG for qemu-devel@nongnu.org; Wed, 23 Sep 2015 12:37:06 -0400 References: <1442497700-2536-1-git-send-email-kwolf@redhat.com> <1442497700-2536-14-git-send-email-kwolf@redhat.com> From: Max Reitz Message-ID: <5602D522.5030905@redhat.com> Date: Wed, 23 Sep 2015 18:36:50 +0200 MIME-Version: 1.0 In-Reply-To: <1442497700-2536-14-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="HFJ0xrC6n6BTq1JrQIjrU7iGB4AfkaL6F" 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: 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) --HFJ0xrC6n6BTq1JrQIjrU7iGB4AfkaL6F Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable 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 > --- > block.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++---------= -------- > 1 file changed, 81 insertions(+), 28 deletions(-) >=20 > diff --git a/block.c b/block.c > index c196f83..98fc17c 100644 > --- a/block.c > +++ b/block.c > @@ -1516,15 +1516,6 @@ static int bdrv_open_inherit(BlockDriverState **= pbs, const char *filename, > =20 > bdrv_refresh_filename(bs); > =20 > - /* For snapshot=3Don, create a temporary qcow2 overlay. bs points = to the > - * temporary snapshot afterwards. */ > - if (snapshot_flags) { > - ret =3D bdrv_append_temp_snapshot(bs, snapshot_flags, &local_e= rr); > - if (local_err) { > - goto close_and_fail; > - } > - } > - > /* Check if any unknown options were used */ > if (options && (qdict_size(options) !=3D 0)) { > const QDictEntry *entry =3D qdict_first(options); > @@ -1556,6 +1547,16 @@ static int bdrv_open_inherit(BlockDriverState **= pbs, const char *filename, > =20 > QDECREF(options); > *pbs =3D bs; > + > + /* For snapshot=3Don, create a temporary qcow2 overlay. bs points = to the > + * temporary snapshot afterwards. */ > + if (snapshot_flags) { > + ret =3D bdrv_append_temp_snapshot(bs, snapshot_flags, &local_e= rr); > + if (local_err) { > + goto close_and_fail; > + } > + } > + > return 0; > =20 > fail: > @@ -1999,20 +2000,6 @@ static void bdrv_move_feature_fields(BlockDriver= State *bs_dest, > =20 > bs_dest->enable_write_cache =3D bs_src->enable_write_cache; > =20 > - /* i/o throttled req */ > - bs_dest->throttle_state =3D bs_src->throttle_state, > - bs_dest->io_limits_enabled =3D bs_src->io_limits_enabled; > - bs_dest->pending_reqs[0] =3D bs_src->pending_reqs[0]; > - bs_dest->pending_reqs[1] =3D bs_src->pending_reqs[1]; > - bs_dest->throttled_reqs[0] =3D bs_src->throttled_reqs[0]; > - bs_dest->throttled_reqs[1] =3D bs_src->throttled_reqs[1]; > - memcpy(&bs_dest->round_robin, > - &bs_src->round_robin, > - sizeof(bs_dest->round_robin)); > - memcpy(&bs_dest->throttle_timers, > - &bs_src->throttle_timers, > - sizeof(ThrottleTimers)); > - > /* r/w error */ > bs_dest->on_read_error =3D bs_src->on_read_error; > bs_dest->on_write_error =3D bs_src->on_write_error; > @@ -2026,10 +2013,25 @@ static void bdrv_move_feature_fields(BlockDrive= rState *bs_dest, > } > =20 > /* Fields that only need to be swapped if the contents of BDSes is swa= pped > - * rather than pointers being changed in the parents. */ > + * rather than pointers being changed in the parents, and throttling f= ields > + * because only bdrv_swap() messes with internals of throttling. */ > static void bdrv_move_reference_fields(BlockDriverState *bs_dest, > BlockDriverState *bs_src) > { > + /* i/o throttled req */ > + bs_dest->throttle_state =3D bs_src->throttle_state, > + bs_dest->io_limits_enabled =3D bs_src->io_limits_enabled; > + bs_dest->pending_reqs[0] =3D bs_src->pending_reqs[0]; > + bs_dest->pending_reqs[1] =3D bs_src->pending_reqs[1]; > + bs_dest->throttled_reqs[0] =3D bs_src->throttled_reqs[0]; > + bs_dest->throttled_reqs[1] =3D bs_src->throttled_reqs[1]; > + memcpy(&bs_dest->round_robin, > + &bs_src->round_robin, > + sizeof(bs_dest->round_robin)); > + memcpy(&bs_dest->throttle_timers, > + &bs_src->throttle_timers, > + sizeof(ThrottleTimers)); > + > /* reference count */ > bs_dest->refcnt =3D bs_src->refcnt; > =20 > @@ -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); 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. > + } > + 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); > } 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? Max > =20 > static void bdrv_delete(BlockDriverState *bs) >=20 --HFJ0xrC6n6BTq1JrQIjrU7iGB4AfkaL6F 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 iQEcBAEBCAAGBQJWAtUiAAoJEDuxQgLoOKytlQkH/1GMKkJsu4Q8rw/UTv1o1mpV pUFVqKOo0P240z/Ur4DUpyUBhP0Be69nDoZ+7hW0Klji9620xE6+iMmbI0dXdhX5 8v3A2X2Q+pZbYqz9mAa7618MMasngbvV/yffmmxtUS0UYl2XdBBQqpgdNtFefvf/ 0Hsb3XrP6ZPYW31aMpBiqnee0ssqzITxkhan1weEIJudUuX9JluLX3NEoYUwDfcy h9g8lFMJKZYAXTiLhOZ58TqCVYhHf53zUgRAZdJ1wtFJJwe/WgbVVcjgFenSg9JL oJlYWi3SkGs0RcEubz9j1mlo6+Lskc9R1uDGu0sVXGyAUkLjmb4+5JVrrAPgZJc= =InRD -----END PGP SIGNATURE----- --HFJ0xrC6n6BTq1JrQIjrU7iGB4AfkaL6F--