From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36897) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aP954-0007rC-KX for qemu-devel@nongnu.org; Fri, 29 Jan 2016 08:27:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aP953-0003US-Gw for qemu-devel@nongnu.org; Fri, 29 Jan 2016 08:27:18 -0500 References: <1453917600-2663-1-git-send-email-mreitz@redhat.com> <1453917600-2663-2-git-send-email-mreitz@redhat.com> <20160128030153.GD7877@ad.usersys.redhat.com> From: Max Reitz Message-ID: <56AB68AB.2040305@redhat.com> Date: Fri, 29 Jan 2016 14:27:07 +0100 MIME-Version: 1.0 In-Reply-To: <20160128030153.GD7877@ad.usersys.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="tlbjDSFUoMaeJSPUbQFNTK7mmXx6T74xx" Subject: Re: [Qemu-devel] [PATCH v8 01/16] block: Release dirty bitmaps in bdrv_close() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: Kevin Wolf , Alberto Garcia , qemu-block@nongnu.org, John Snow , qemu-devel@nongnu.org, Paolo Bonzini This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --tlbjDSFUoMaeJSPUbQFNTK7mmXx6T74xx Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 28.01.2016 04:01, Fam Zheng wrote: > On Wed, 01/27 18:59, Max Reitz wrote: >> bdrv_delete() is not very happy about deleting BlockDriverStates with >> dirty bitmaps still attached to them. In the past, we got around that >> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), an= d >> bdrv_close() simply ignoring that condition. We should fix that by >> releasing all dirty bitmaps in bdrv_close() and drop the assertion in >> bdrv_delete(). >> >> Signed-off-by: Max Reitz >> Reviewed-by: John Snow >> --- >> block.c | 37 +++++++++++++++++++++++++++++-------- >> 1 file changed, 29 insertions(+), 8 deletions(-) >> >> diff --git a/block.c b/block.c >> index 5709d3d..9a31e20 100644 >> --- a/block.c >> +++ b/block.c >> @@ -88,6 +88,8 @@ static int bdrv_open_inherit(BlockDriverState **pbs,= const char *filename, >> const BdrvChildRole *child_role, Error *= *errp); >> =20 >> static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); >> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs); >> + >> /* If non-zero, use only whitelisted block drivers */ >> static int use_bdrv_whitelist; >> =20 >> @@ -2157,6 +2159,8 @@ void bdrv_close(BlockDriverState *bs) >> =20 >> notifier_list_notify(&bs->close_notifiers, bs); >> =20 >> + bdrv_release_all_dirty_bitmaps(bs); >> + >> if (bs->blk) { >> blk_dev_change_media_cb(bs->blk, false); >> } >> @@ -2366,7 +2370,6 @@ static void bdrv_delete(BlockDriverState *bs) >> assert(!bs->job); >> assert(bdrv_op_blocker_is_empty(bs)); >> assert(!bs->refcnt); >> - assert(QLIST_EMPTY(&bs->dirty_bitmaps)); >> =20 >> bdrv_close(bs); >> =20 >> @@ -3582,21 +3585,39 @@ static void bdrv_dirty_bitmap_truncate(BlockDr= iverState *bs) >> } >> } >> =20 >> -void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap = *bitmap) >> +static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *b= s, >> + BdrvDirtyBitmap *bi= tmap) >> { >> BdrvDirtyBitmap *bm, *next; >> QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { >> - if (bm =3D=3D bitmap) { >> + if (!bitmap || bm =3D=3D bitmap) { >> assert(!bdrv_dirty_bitmap_frozen(bm)); >> - QLIST_REMOVE(bitmap, list); >> - hbitmap_free(bitmap->bitmap); >> - g_free(bitmap->name); >> - g_free(bitmap); >> - return; >> + QLIST_REMOVE(bm, list); >> + hbitmap_free(bm->bitmap); >> + g_free(bm->name); >> + g_free(bm); >> + >> + if (bitmap) { >> + return; >> + } >> } >> } >> } >> =20 >> +void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap = *bitmap) >> +{ >> + bdrv_do_release_matching_dirty_bitmap(bs, bitmap); >> +} >> + >> +/** >> + * Release all dirty bitmaps attached to a BDS (for use in bdrv_close= ()). There >> + * must not be any frozen bitmaps attached. >=20 > Should we assert that? Well, it is asserted in bdrv_do_release_matching_dirty_bitmap(). > And IIUC the intention of this function is to re= lease > all monitor owned (i.e. user created) dirty bitmaps, which must be name= d. If > so, can we assert that too? Probably we can. I don't really know if it would be worth it, though (even if it isn't much code). There's not much harm in releasing unnamed bitmaps here, the main issue would be the question "where did they come from?". If I do it, I'll rename the function bdrv_release_named_dirty_bitmaps() and then assert in bdrv_close() that no bitmaps are attached any more. Max >=20 > Fam >=20 >> + */ >> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs) >> +{ >> + bdrv_do_release_matching_dirty_bitmap(bs, NULL); >> +} >> + >> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) >> { >> assert(!bdrv_dirty_bitmap_frozen(bitmap)); >> --=20 >> 2.7.0 >> --tlbjDSFUoMaeJSPUbQFNTK7mmXx6T74xx 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 iQEcBAEBCAAGBQJWq2irAAoJEDuxQgLoOKytHXMH/AgSMc/XKWsZRx5k8p2dYHvt 2qpp3q/eVoJfIPv7SLUloW+jGXvmcPPvNBeY9Q7TXLLJMIaKkKJ3CplpTcTGqn9T N12MnDZ9dGYCDCaWkPtrfCI5eqdpMtXMlOm2KNUWG7jFEo8DyOmIHQSi8yTgxV7P upyEdPoGbwaAXlVhqQToKj7/itXZ26sIaFnsxvylKH8qe//OYFA9BcO5/TqXu53p pitJnTWdN91OBXVarPqTY1cjaNzk9nlavrJur31NePpUHHqF7DyNZH6ASp8DFepy IL1CYEGLFhyTry4z1j1xzS4vh00pnPPyRZ+LgqquPJmqumQklTrl41UeLb8FOOk= =g3sY -----END PGP SIGNATURE----- --tlbjDSFUoMaeJSPUbQFNTK7mmXx6T74xx--