From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37587) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aeO2s-0008SX-JH for qemu-devel@nongnu.org; Fri, 11 Mar 2016 09:28:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aeO2r-0002E3-9j for qemu-devel@nongnu.org; Fri, 11 Mar 2016 09:28:02 -0500 References: <1457412306-18940-1-git-send-email-famz@redhat.com> <1457412306-18940-7-git-send-email-famz@redhat.com> <56E2CE1D.4080502@redhat.com> From: Max Reitz Message-ID: <56E2D5E7.4000105@redhat.com> Date: Fri, 11 Mar 2016 15:27:51 +0100 MIME-Version: 1.0 In-Reply-To: <56E2CE1D.4080502@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="OoOxpHWxkBlUdCr0ia5Ik3N40KOHPD4cB" Subject: Re: [Qemu-devel] [PATCH v4 06/15] block: Hide HBitmap in block dirty bitmap interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: kwolf@redhat.com, Vladimir Sementsov-Ogievskiy , jsnow@redhat.com, qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --OoOxpHWxkBlUdCr0ia5Ik3N40KOHPD4cB Content-Type: multipart/mixed; boundary="4gxI9vQ9Ng6HUK7MtTumcMqaOJD735LVn" From: Max Reitz To: Fam Zheng , qemu-devel@nongnu.org Cc: qemu-block@nongnu.org, Vladimir Sementsov-Ogievskiy , jsnow@redhat.com, kwolf@redhat.com Message-ID: <56E2D5E7.4000105@redhat.com> Subject: Re: [PATCH v4 06/15] block: Hide HBitmap in block dirty bitmap interface References: <1457412306-18940-1-git-send-email-famz@redhat.com> <1457412306-18940-7-git-send-email-famz@redhat.com> <56E2CE1D.4080502@redhat.com> In-Reply-To: <56E2CE1D.4080502@redhat.com> --4gxI9vQ9Ng6HUK7MtTumcMqaOJD735LVn Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 11.03.2016 14:54, Max Reitz wrote: > On 08.03.2016 05:44, Fam Zheng wrote: >> HBitmap is an implementation detail of block dirty bitmap that should = be hidden >> from users. Introduce a BdrvDirtyBitmapIter to encapsulate the underly= ing >> HBitmapIter. >> >> A small difference in the interface is, before, an HBitmapIter is init= ialized >> in place, now the new BdrvDirtyBitmapIter must be dynamically allocate= d because >> the structure definition is in block.c. >=20 > It's block/dirty-bitmap.c now. :-) >=20 >> Two current users are converted too. >> >> Signed-off-by: Fam Zheng >> --- >> block/backup.c | 14 ++++++++------ >> block/dirty-bitmap.c | 39 +++++++++++++++++++++++++++++++++--= ---- >> block/mirror.c | 15 +++++++++------ >> include/block/dirty-bitmap.h | 7 +++++-- >> include/qemu/typedefs.h | 1 + >> 5 files changed, 56 insertions(+), 20 deletions(-) >> >=20 > [...] >=20 >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index 556e1d1..16f73b2 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c >=20 > [...] >=20 >> @@ -373,12 +401,11 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_= t cur_sector, >> } >> =20 >> /** >> - * Advance an HBitmapIter to an arbitrary offset. >> + * Advance an BdrvDirtyBitmapIter to an arbitrary offset. >=20 > This should be "a BdrvDirtyBitmapIter". >=20 >> */ >> -void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset) >> +void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t sector_nu= m) >> { >> - assert(hbi->hb); >> - hbitmap_iter_init(hbi, hbi->hb, offset); >> + hbitmap_iter_init(&iter->hbi, iter->hbi.hb, sector_num); >> } >> =20 >> int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap) >> diff --git a/block/mirror.c b/block/mirror.c >> index 9635fa8..87a5a86 100644 >> --- a/block/mirror.c >> +++ b/block/mirror.c >=20 > [...] >=20 >> @@ -304,10 +304,11 @@ static uint64_t coroutine_fn mirror_iteration(Mi= rrorBlockJob *s) >> int64_t end =3D s->bdev_length / BDRV_SECTOR_SIZE; >> int sectors_per_chunk =3D s->granularity >> BDRV_SECTOR_BITS; >> =20 >> - sector_num =3D hbitmap_iter_next(&s->hbi); >> + sector_num =3D bdrv_dirty_iter_next(s->dbi); >> if (sector_num < 0) { >> - bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi); >> - sector_num =3D hbitmap_iter_next(&s->hbi); >> + bdrv_dirty_iter_free(s->dbi); >> + s->dbi =3D bdrv_dirty_iter_new(s->dirty_bitmap, 0); >=20 > Works, but wouldn't bdrv_set_dirty_iter(s->dbi, 0); suffice? >=20 >> + sector_num =3D bdrv_dirty_iter_next(s->dbi); >> trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bi= tmap)); >> assert(sector_num >=3D 0); >> } >> @@ -330,7 +331,7 @@ static uint64_t coroutine_fn mirror_iteration(Mirr= orBlockJob *s) >> mirror_wait_for_io(s); >> /* Now retry. */ >> } else { >> - hbitmap_next =3D hbitmap_iter_next(&s->hbi); >> + hbitmap_next =3D bdrv_dirty_iter_next(s->dbi); >=20 > It would make sense to change this variable's name from hbitmap_next to= > e.g. bdrv_dirty_next or bs_next_dirty. >=20 >> assert(hbitmap_next =3D=3D next_sector); >> nb_chunks++; >> } >> @@ -577,7 +578,8 @@ static void coroutine_fn mirror_run(void *opaque) >> } >> } >> =20 >> - bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi); >> + bdrv_dirty_iter_free(s->dbi); >> + s->dbi =3D bdrv_dirty_iter_new(s->dirty_bitmap, 0); >=20 > Again, bdrv_set_dirty_iter(s->dbi, 0); should work just fine. OK, here it wouldn't because s->dbi is still NULL. Why do we need the bdrv_dirty_iter_free(), then? It wasn't present in v3. Max > Since this patch is technically correct, I could still imagine giving a= n > R-b, but I'm really reluctant to do so because of the > bdrv_dirty_iter_free()/bdrv_dirty_iter_new() pairs. >=20 > Max >=20 >> for (;;) { >> uint64_t delay_ns =3D 0; >> int64_t cnt; >> @@ -688,6 +690,7 @@ immediate_exit: >> qemu_vfree(s->buf); >> g_free(s->cow_bitmap); >> g_free(s->in_flight_bitmap); >> + bdrv_dirty_iter_free(s->dbi); >> bdrv_release_dirty_bitmap(bs, s->dirty_bitmap); >> if (s->target->blk) { >> blk_iostatus_disable(s->target->blk); >> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap= =2Eh >> index 80afe60..2ea601b 100644 >> --- a/include/block/dirty-bitmap.h >> +++ b/include/block/dirty-bitmap.h >> @@ -36,8 +36,11 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,= >> int64_t cur_sector, int nr_sectors); >> void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap, >> int64_t cur_sector, int nr_sectors); >> -void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, struct HBitmapIter= *hbi); >> -void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset); >> +BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap, >> + uint64_t first_sector); >> +void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter); >> +int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter); >> +void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num= ); >> int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap); >> void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); >> =20 >> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h >> index fd039e0..7c39f0f 100644 >> --- a/include/qemu/typedefs.h >> +++ b/include/qemu/typedefs.h >> @@ -11,6 +11,7 @@ typedef struct AioContext AioContext; >> typedef struct AllwinnerAHCIState AllwinnerAHCIState; >> typedef struct AudioState AudioState; >> typedef struct BdrvDirtyBitmap BdrvDirtyBitmap; >> +typedef struct BdrvDirtyBitmapIter BdrvDirtyBitmapIter; >> typedef struct BlockBackend BlockBackend; >> typedef struct BlockBackendRootState BlockBackendRootState; >> typedef struct BlockDriverState BlockDriverState; >> >=20 >=20 --4gxI9vQ9Ng6HUK7MtTumcMqaOJD735LVn-- --OoOxpHWxkBlUdCr0ia5Ik3N40KOHPD4cB 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 iQEcBAEBCAAGBQJW4tXoAAoJEDuxQgLoOKytq64H/0jzzF2JTmtdALVDXmyOWxqy oYfMLasrCUmgN+qqpRXA9k+GxyFvl7btSHbatw8OEsxgJYtna16lTTL0KF6rIbAw r5jT+m/3mBubHQK4pcEu867MQcrWioxT9II1biU+6GMmCku48APJKoUY/J5uvu0A QTzaUSBHR/W3/LymKbHK9clYG04kgUvsLBHed3I/xlmJEDEp9jjXEi/k2gpD6AgI TywnPQ4zo2URJ1lD4PQ6ZCiPtg/QN71kFJ89bsvXcBX3tNlTbnKdlH5lpljs1+h6 A6uvGqvL6kCxLxmAKd4AgoY3DE4APsWDrVvgSHkux9U8Zv0Ag0Ww211Fty94pJg= =CRpG -----END PGP SIGNATURE----- --OoOxpHWxkBlUdCr0ia5Ik3N40KOHPD4cB--