On Mon, Dec 01, 2014 at 03:30:13PM -0500, John Snow wrote: > From: Fam Zheng <f...@redhat.com> > > 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. > > There are two bitmap use modes for sync=dirty-bitmap: > > - 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 <f...@redhat.com>
> Signed-off-by: John Snow <js...@redhat.com>
> Reviewed-by: Max Reitz <mre...@redhat.com>
> ---
> 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(-)
>
> 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);
> }
>
> +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=dirty-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 *opaque)
> g_free(data);
> }
>
> +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.)
pgpQGkbPNVSky.pgp
Description: PGP signature
