From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43959) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XyOpa-0002fI-NF for qemu-devel@nongnu.org; Tue, 09 Dec 2014 12:44:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XyOpR-0005Xu-LP for qemu-devel@nongnu.org; Tue, 09 Dec 2014 12:44:14 -0500 Received: from mail-wi0-x22d.google.com ([2a00:1450:400c:c05::22d]:37850) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XyOpR-0005Xq-A5 for qemu-devel@nongnu.org; Tue, 09 Dec 2014 12:44:05 -0500 Received: by mail-wi0-f173.google.com with SMTP id r20so8695682wiv.6 for ; Tue, 09 Dec 2014 09:44:04 -0800 (PST) Date: Tue, 9 Dec 2014 17:44:01 +0000 From: Stefan Hajnoczi Message-ID: <20141209174401.GH27053@stefanha-thinkpad.redhat.com> References: <1417465816-19345-1-git-send-email-jsnow@redhat.com> <1417465816-19345-8-git-send-email-jsnow@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="svZFHVx8/dhPCe52" Content-Disposition: inline In-Reply-To: <1417465816-19345-8-git-send-email-jsnow@redhat.com> Subject: Re: [Qemu-devel] [PATCH v9 07/10] qmp: Add support of "dirty-bitmap" sync mode for drive-backup List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: kwolf@redhat.com, Fam Zheng , armbru@redhat.com, qemu-devel@nongnu.org, mreitz@redhat.com, vsementsov@parallels.com, stefanha@redhat.com, pbonzini@redhat.com --svZFHVx8/dhPCe52 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Dec 01, 2014 at 03:30:13PM -0500, John Snow wrote: > From: Fam Zheng >=20 > For "dirty-bitmap" sync mode, the block job will iterate through the > given dirty bitmap to decide if a sector needs backup (backup all the > dirty clusters and skip clean ones), just as allocation conditions of > "top" sync mode. >=20 > There are two bitmap use modes for sync=3Ddirty-bitmap: >=20 > - reset: backup job makes a copy of bitmap and resets the original > one. > - consume: backup job makes the original anonymous (invisible to user) > and releases it after use. It's not obvious to me that we need both modes. Can you explain why the choice between reset and consume is necessary? > Signed-off-by: Fam Zheng > Signed-off-by: John Snow > Reviewed-by: Max Reitz > --- > block.c | 5 ++ > block/backup.c | 130 ++++++++++++++++++++++++++++++++++++++--= ------ > block/mirror.c | 4 ++ > blockdev.c | 17 +++++- > hmp.c | 4 +- > include/block/block.h | 1 + > include/block/block_int.h | 6 +++ > qapi/block-core.json | 30 +++++++++-- > qmp-commands.hx | 7 +-- > 9 files changed, 174 insertions(+), 30 deletions(-) >=20 > diff --git a/block.c b/block.c > index 85215b3..42244f6 100644 > --- a/block.c > +++ b/block.c > @@ -5489,6 +5489,11 @@ void bdrv_dirty_iter_init(BlockDriverState *bs, > hbitmap_iter_init(hbi, bitmap->bitmap, 0); > } > =20 > +void bdrv_dirty_iter_set(HBitmapIter *hbi, int64_t offset) > +{ > + hbitmap_iter_init(hbi, hbi->hb, offset); > +} > + > void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, > int nr_sectors) > { > diff --git a/block/backup.c b/block/backup.c > index 792e655..2aab68f 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -37,6 +37,10 @@ typedef struct CowRequest { > typedef struct BackupBlockJob { > BlockJob common; > BlockDriverState *target; > + /* bitmap for sync=3Ddirty-bitmap */ > + BdrvDirtyBitmap *sync_bitmap; > + /* dirty bitmap granularity */ > + int64_t sync_bitmap_gran; What is the point of this field? I think we've duplicated granularity twice now (originally stored in HBitmap, once in BdrvDirtyBitmap, and now again). Maybe the one user of this field should just call bdrv_dirty_bitmap_granularity(). > MirrorSyncMode sync_mode; > RateLimit limit; > BlockdevOnError on_source_error; > @@ -242,6 +246,31 @@ static void backup_complete(BlockJob *job, void *opa= que) > g_free(data); > } > =20 > +static bool yield_and_check(BackupBlockJob *job) missing coroutine_fn annotation. coroutine_fn tells the reader that this function may only be called from coroutine context. (In this case the function and contains "yield" so that's a big hint but please still always mark coroutine functions coroutine_fn so that we can enforce static checking in the future.) --svZFHVx8/dhPCe52 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJUhzThAAoJEJykq7OBq3PIhf4H/1bm/2yUQrmMccRs0sDeNr5L rdYaYNyws2pw86TgzokuWOJq4n3Rjl+GrYGIQC1KAUILm8yrbFFOHjY52QF1uLhW 0cFVbd4qZBnsMwY0utJ1+ypDlI5KubswHOl372EEJsK1jnEfzHvw1hpM7CQgDHDO tv0Vx189EzHm95IiDIjQCSl5vTele7Rqw/WIJ24v2neVODPGUZramu2jeeQHOZXK zxNXZ9xylpVyZkIYqb1aDMBJW9rpRLTNsje3P1tEFTqGkzBl36RkD0BNqJrWCvCR UvhnJx5qPOXqbTpytCSCyxWGfLuqgjH6yIDIVBzNp5eLtq9c9prfoh1uI2adaGM= =dUsO -----END PGP SIGNATURE----- --svZFHVx8/dhPCe52--