From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45707) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZvtYu-0005nO-Up for qemu-devel@nongnu.org; Mon, 09 Nov 2015 16:01:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZvtYr-0001Bn-Nb for qemu-devel@nongnu.org; Mon, 09 Nov 2015 16:01:12 -0500 References: <1446663467-22485-1-git-send-email-mreitz@redhat.com> <1446663467-22485-4-git-send-email-mreitz@redhat.com> <563CF891.8070008@redhat.com> <5640C7FF.8070203@redhat.com> From: Max Reitz Message-ID: <56410983.5000406@redhat.com> Date: Mon, 9 Nov 2015 22:00:51 +0100 MIME-Version: 1.0 In-Reply-To: <5640C7FF.8070203@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Tq4WtUEGUTGcPlRNWTmaBfxVjHS1CtVwB" 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) --Tq4WtUEGUTGcPlRNWTmaBfxVjHS1CtVwB Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 09.11.2015 17:21, Max Reitz wrote: > On 06.11.2015 19:59, John Snow wrote: >> >> >> 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(), a= nd >>> 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(BlockDriverStat= e *bs, BdrvDirtyBitmap *bitmap) >>> } >>> } >>> =20 >>> +/** >>> + * Release all dirty bitmaps attached to a BDS, independently of whe= ther they >>> + * are frozen or not (for use in bdrv_close()). >>> + */ >> >> This comment caught my attention ... >> >>> +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)); >>> >> >> 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. >=20 > 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). :-) >=20 > (At least that's how it's supposed to be; also, there is an > assert(!bs->job) in bdrv_close()) >=20 > 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...). >=20 >> I am assuming by now that it's actually likely that you've canceled an= y >> jobs attached to this node before you go through the motions of deleti= ng >> it, and it should be safe to just call bdrv_release_dirty_bitmap ... >=20 > Well, yes, but then I'd have to iterate through all the dirty bitmaps t= o > 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. >=20 >> 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_bitm= ap >> can share. You can leave the frozen assertion >> in just bdrv_release_dirty_bitmap before it invokes the helper, so tha= t >> bdrv_delete always succeeds in case something gets out-of-sync in the >> internals. >=20 > Hm, yes, that should do just fine, thanks. >=20 >> (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 mak= e >> the helper recursive.) >=20 > 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(). OK, I can't actually do that yet. There is a bdrv_close() in one of the fail paths of bdrv_open_inherit(), and the refcount of that BDS will probably be 1. There is generally no way that BDS has any bitmaps attached to it, so we're fine in that regard. I'll revisit this after this series and its follow-up. Max --Tq4WtUEGUTGcPlRNWTmaBfxVjHS1CtVwB 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 iQEcBAEBCAAGBQJWQQmDAAoJEDuxQgLoOKytrMYIAInTOLpEXsyCbM2jzbSz6sMC cOeM/zD3nrXy6pRK+lYn8SoVeySr8MFTTcYTbIHZx5ofLSUHk/krYb9MqOasvSqv pRHBeabdLmJXHqxGHCUs80LkPLLSycdLZ+umnH+hRqOQ/q/H1a/inrO7Vaz7uy6d 4BCYlucw5WrtjhEh5XXgTDJRZWwrYd/CSNceHygzuaj8jezcXkFI8Z5Zkk+yY1tT 3drnLCaHLqcncDc4YfWjg4h68CxP6aRvV3HlZBIIw0+W4uoxRWHAF5jWfDletV1X QhAexEA1mAZP2hSDhroqt8eFIjx73vyA+uYxURpUPJXUFcDlTLNNrJbolFHagH8= =Z/Tt -----END PGP SIGNATURE----- --Tq4WtUEGUTGcPlRNWTmaBfxVjHS1CtVwB--