From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56794) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aeNWn-0005PS-Lg for qemu-devel@nongnu.org; Fri, 11 Mar 2016 08:54:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aeNWi-0000nS-8l for qemu-devel@nongnu.org; Fri, 11 Mar 2016 08:54:53 -0500 References: <1457412306-18940-1-git-send-email-famz@redhat.com> <1457412306-18940-7-git-send-email-famz@redhat.com> From: Max Reitz Message-ID: <56E2CE1D.4080502@redhat.com> Date: Fri, 11 Mar 2016 14:54:37 +0100 MIME-Version: 1.0 In-Reply-To: <1457412306-18940-7-git-send-email-famz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="OBeq05H4WmqjJhhpVM8fbf67vTI4p76Sp" 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) --OBeq05H4WmqjJhhpVM8fbf67vTI4p76Sp Content-Type: multipart/mixed; boundary="5rOq5cXORWwc6tA5KU2BCdF3KsooGkhnU" 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: <56E2CE1D.4080502@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> In-Reply-To: <1457412306-18940-7-git-send-email-famz@redhat.com> --5rOq5cXORWwc6tA5KU2BCdF3KsooGkhnU Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 08.03.2016 05:44, Fam Zheng wrote: > HBitmap is an implementation detail of block dirty bitmap that should b= e hidden > from users. Introduce a BdrvDirtyBitmapIter to encapsulate the underlyi= ng > HBitmapIter. >=20 > A small difference in the interface is, before, an HBitmapIter is initi= alized > in place, now the new BdrvDirtyBitmapIter must be dynamically allocated= because > the structure definition is in block.c. It's block/dirty-bitmap.c now. :-) > Two current users are converted too. >=20 > 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 [...] > 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 [...] > @@ -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. This should be "a BdrvDirtyBitmapIter". > */ > -void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset) > +void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t sector_num= ) > { > - 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 [...] > @@ -304,10 +304,11 @@ static uint64_t coroutine_fn mirror_iteration(Mir= rorBlockJob *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); Works, but wouldn't bdrv_set_dirty_iter(s->dbi, 0); suffice? > + sector_num =3D bdrv_dirty_iter_next(s->dbi); > trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bit= map)); > assert(sector_num >=3D 0); > } > @@ -330,7 +331,7 @@ static uint64_t coroutine_fn mirror_iteration(Mirro= rBlockJob *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); It would make sense to change this variable's name from hbitmap_next to e.g. bdrv_dirty_next or bs_next_dirty. > 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); Again, bdrv_set_dirty_iter(s->dbi, 0); should work just fine. Since this patch is technically correct, I could still imagine giving an R-b, but I'm really reluctant to do so because of the bdrv_dirty_iter_free()/bdrv_dirty_iter_new() pairs. Max > 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.= h > 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 --5rOq5cXORWwc6tA5KU2BCdF3KsooGkhnU-- --OBeq05H4WmqjJhhpVM8fbf67vTI4p76Sp 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 iQEcBAEBCAAGBQJW4s4dAAoJEDuxQgLoOKytMjUH+gOb7OiG140iqq99jezzixqG luh6QxcOTVhhCgm7knt1DonACeM95/go4vvI+A5dZAvtjncLHzA3ZZdlFNfPEbx0 1MhOQPIlUN+uKjRQK9BFAQ2evA0VVcrWrURzyFfnBgg852xZieNhexDSV9Uy83q5 WPHqB5radtONfFutUtcjJ2Wx1pguR9a+toHgm1Rsawu4YZR58PCRnJB1o5fAYmGm 5cvsXPoxCxVg0XUH4mm/n5V0eV0Od/Xh3QjA9jaqaa94E89oZuGq9wRPROe8bFSx t17mcYaGVuJzrQdXdFsNFbnIJLklHnSuBUW4dv3e81NZEzK1y14BLK5luDdngTw= =gL8T -----END PGP SIGNATURE----- --OBeq05H4WmqjJhhpVM8fbf67vTI4p76Sp--