From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41699) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZvpCH-0000vV-W9 for qemu-devel@nongnu.org; Mon, 09 Nov 2015 11:21:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZvpCE-0003Hv-0d for qemu-devel@nongnu.org; Mon, 09 Nov 2015 11:21:34 -0500 References: <1446663467-22485-1-git-send-email-mreitz@redhat.com> <1446663467-22485-4-git-send-email-mreitz@redhat.com> <563CF891.8070008@redhat.com> From: Max Reitz Message-ID: <5640C7FF.8070203@redhat.com> Date: Mon, 9 Nov 2015 17:21:19 +0100 MIME-Version: 1.0 In-Reply-To: <563CF891.8070008@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="IlTV82udneStDAsI29fSvp1kQE1ecmpAB" Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v6 03/15] block: Release dirty bitmaps in bdrv_close() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-block@nongnu.org Cc: Kevin Wolf , Paolo Bonzini , Markus Armbruster , Stefan Hajnoczi , qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --IlTV82udneStDAsI29fSvp1kQE1ecmpAB Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 06.11.2015 19:59, John Snow wrote: >=20 >=20 > On 11/04/2015 01:57 PM, 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 >> --- >> block.c | 20 +++++++++++++++++++- >> 1 file changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/block.c b/block.c >> index 3493501..23448ed 100644 >> --- a/block.c >> +++ b/block.c >> @@ -87,6 +87,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 >> @@ -1916,6 +1918,8 @@ void bdrv_close(BlockDriverState *bs) >> bdrv_drain(bs); /* in case flush left pending I/O */ >> 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); >> } >> @@ -2123,7 +2127,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 >> @@ -3318,6 +3321,21 @@ void bdrv_release_dirty_bitmap(BlockDriverState= *bs, BdrvDirtyBitmap *bitmap) >> } >> } >> =20 >> +/** >> + * Release all dirty bitmaps attached to a BDS, independently of whet= her they >> + * are frozen or not (for use in bdrv_close()). >> + */ >=20 > This comment caught my attention ... >=20 >> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs) >> +{ >> + BdrvDirtyBitmap *bm, *next; >> + QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { >> + QLIST_REMOVE(bm, list); >> + hbitmap_free(bm->bitmap); >> + g_free(bm->name); >> + g_free(bm); >> + } >> +} >> + >> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) >> { >> assert(!bdrv_dirty_bitmap_frozen(bitmap)); >> >=20 > Currently, the only way a bitmap could/should be frozen is if it is in > use by a job. If a job is running, you probably shouldn't delete stuff > it is using out from under it. Right. Good thing I'm fixing everything and bdrv_close() will no longer be called unless there are no references to the BDS anymore (and each block job holds an own reference to its BDS). :-) (At least that's how it's supposed to be; also, there is an assert(!bs->job) in bdrv_close()) Also, I wanted to mirror current behavior. Right now, we are just leaking all dirty bitmaps on bdrv_close() (bdrv_delete() asserts that there are none, but if you invoke bdrv_close() directly...). > I am assuming by now that it's actually likely that you've canceled any= > jobs attached to this node before you go through the motions of deletin= g > it, and it should be safe to just call bdrv_release_dirty_bitmap ... Well, yes, but then I'd have to iterate through all the dirty bitmaps to call bdrv_release_dirty_bitmap() for each of them, and then bdrv_release_dirty_bitmap() iterates through all of them again to check whether it really belongs to the BDS. That seemed a bit like a waste. > We don't want the two foreach loops though, so maybe just factor out a > helper that bdrv_release_all_dirty_bitmaps and bdrv_release_dirty_bitma= p > can share. You can leave the frozen assertion > in just bdrv_release_dirty_bitmap before it invokes the helper, so that= > bdrv_delete always succeeds in case something gets out-of-sync in the > internals. Hm, yes, that should do just fine, thanks. > (Hmm, to prevent leaks, if you delete a bitmap that is frozen, you > should probably call the helper on its child object too... or just make= > the helper recursive.) I think it'll be actually better to just keep the assertion in and thus not allow releasing frozen dirty bitmaps in bdrv_close(). In addition I'll add an assertion that !bs->refcount to bdrv_close(). Max --IlTV82udneStDAsI29fSvp1kQE1ecmpAB 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 iQEcBAEBCAAGBQJWQMf/AAoJEDuxQgLoOKytOa4IAIgUzITMx5QFV4D6MFFsNdRG wG9kMqizQ4OxfYbXkuT0ZnYTJfHUlaoK1McryXmFhiRaBR2YAJ8xQZGkdOkvE1jy 8A6TRMWne0Lmv9DKSKeHZD98g/jxXk2L8+odUqYn6u8PY5AUh0ySvCrPpYF0EYi7 yA9QIeFDNts0NWZNS/AvUMOO4cL9gqx5m/UunO3YnbgBrKXSFZqiYB8JA7o+i+LK cQ1XKPAJxuQt8nIHvtpqbA5Vcy61OgTmlZuzCVDyfZ/WV/irAHKKy/L7mKYj0yRM oWT6SCY1EyUY1izfgPYhRomTE12o/6un8fX2h8+KdRE6RFsaIXXetfIJhcGgW8s= =8g4g -----END PGP SIGNATURE----- --IlTV82udneStDAsI29fSvp1kQE1ecmpAB--