From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47128) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YAxuz-0004Hk-Ab for qemu-devel@nongnu.org; Tue, 13 Jan 2015 04:37:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YAxuv-0000E4-Pm for qemu-devel@nongnu.org; Tue, 13 Jan 2015 04:37:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55674) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YAxuv-0000Dh-GP for qemu-devel@nongnu.org; Tue, 13 Jan 2015 04:37:41 -0500 Date: Tue, 13 Jan 2015 17:37:36 +0800 From: Fam Zheng Message-ID: <20150113093736.GD31703@fam-t430.nay.redhat.com> References: <1421080265-2228-1-git-send-email-jsnow@redhat.com> <1421080265-2228-10-git-send-email-jsnow@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1421080265-2228-10-git-send-email-jsnow@redhat.com> Subject: Re: [Qemu-devel] [PATCH v11 09/13] 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, armbru@redhat.com, qemu-devel@nongnu.org, mreitz@redhat.com, vsementsov@parallels.com, stefanha@redhat.com On Mon, 01/12 11:31, John Snow wrote: > 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. > > Signed-off-by: Fam Zheng > Signed-off-by: John Snow > --- > block.c | 5 ++ > block/backup.c | 120 ++++++++++++++++++++++++++++++++++++++-------- > block/mirror.c | 4 ++ > blockdev.c | 14 +++++- > hmp.c | 3 +- > include/block/block.h | 1 + > include/block/block_int.h | 2 + > qapi/block-core.json | 11 +++-- > qmp-commands.hx | 7 +-- > 9 files changed, 137 insertions(+), 30 deletions(-) > > diff --git a/block.c b/block.c > index 3f33b9d..5eb6788 100644 > --- a/block.c > +++ b/block.c > @@ -5633,6 +5633,11 @@ static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, > } > } > > +void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset) > +{ > + hbitmap_iter_init(hbi, hbi->hb, offset); What's the reason for this indirection? Can caller just use hbitmap_iter_init? > +} > + > int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) > { > return hbitmap_count(bitmap->bitmap); > diff --git a/block/backup.c b/block/backup.c > index 792e655..0626a3e 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -37,6 +37,8 @@ typedef struct CowRequest { > typedef struct BackupBlockJob { > BlockJob common; > BlockDriverState *target; > + /* bitmap for sync=dirty-bitmap */ > + BdrvDirtyBitmap *sync_bitmap; > MirrorSyncMode sync_mode; > RateLimit limit; > BlockdevOnError on_source_error; > @@ -242,6 +244,31 @@ static void backup_complete(BlockJob *job, void *opaque) > g_free(data); > } > > +static bool coroutine_fn yield_and_check(BackupBlockJob *job) > +{ > + if (block_job_is_cancelled(&job->common)) { > + return true; > + } > + > + /* we need to yield so that qemu_aio_flush() returns. > + * (without, VM does not reboot) > + */ > + if (job->common.speed) { > + uint64_t delay_ns = ratelimit_calculate_delay(&job->limit, > + job->sectors_read); > + job->sectors_read = 0; > + block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, delay_ns); > + } else { > + block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, 0); > + } > + > + if (block_job_is_cancelled(&job->common)) { > + return true; > + } > + > + return false; > +} > + > static void coroutine_fn backup_run(void *opaque) > { > BackupBlockJob *job = opaque; > @@ -254,13 +281,13 @@ static void coroutine_fn backup_run(void *opaque) > }; > int64_t start, end; > int ret = 0; > + bool error_is_read; > > QLIST_INIT(&job->inflight_reqs); > qemu_co_rwlock_init(&job->flush_rwlock); > > start = 0; > - end = DIV_ROUND_UP(job->common.len / BDRV_SECTOR_SIZE, > - BACKUP_SECTORS_PER_CLUSTER); > + end = DIV_ROUND_UP(job->common.len, BACKUP_CLUSTER_SIZE); > > job->bitmap = hbitmap_alloc(end, 0); > > @@ -278,28 +305,45 @@ static void coroutine_fn backup_run(void *opaque) > qemu_coroutine_yield(); > job->common.busy = true; > } > + } else if (job->sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) { > + /* Dirty Bitmap sync has a slightly different iteration method */ > + HBitmapIter hbi; > + int64_t sector; > + int64_t cluster; > + bool polyrhythmic; > + > + bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi); > + /* Does the granularity happen to match our backup cluster size? */ > + polyrhythmic = (bdrv_dirty_bitmap_granularity(bs, job->sync_bitmap) != > + BACKUP_CLUSTER_SIZE); > + > + /* Find the next dirty /sector/ and copy that /cluster/ */ > + while ((sector = hbitmap_iter_next(&hbi)) != -1) { > + if (yield_and_check(job)) { > + goto leave; > + } > + cluster = sector / BACKUP_SECTORS_PER_CLUSTER; > + > + do { > + ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER, > + BACKUP_SECTORS_PER_CLUSTER, &error_is_read); > + if ((ret < 0) && > + backup_error_action(job, error_is_read, -ret) == > + BLOCK_ERROR_ACTION_REPORT) { > + goto leave; > + } > + } while (ret < 0); > + > + /* Advance (or rewind) our iterator if we need to. */ > + if (polyrhythmic) { > + bdrv_set_dirty_iter(&hbi, > + (cluster + 1) * BACKUP_SECTORS_PER_CLUSTER); > + } > + } > } else { > /* Both FULL and TOP SYNC_MODE's require copying.. */ > for (; start < end; start++) { > - bool error_is_read; > - > - if (block_job_is_cancelled(&job->common)) { > - break; > - } > - > - /* we need to yield so that qemu_aio_flush() returns. > - * (without, VM does not reboot) > - */ > - if (job->common.speed) { > - uint64_t delay_ns = ratelimit_calculate_delay( > - &job->limit, job->sectors_read); > - job->sectors_read = 0; > - block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, delay_ns); > - } else { > - block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, 0); > - } > - > - if (block_job_is_cancelled(&job->common)) { > + if (yield_and_check(job)) { > break; > } > > @@ -351,12 +395,23 @@ static void coroutine_fn backup_run(void *opaque) > } > } > > +leave: > notifier_with_return_remove(&before_write); > > /* wait until pending backup_do_cow() calls have completed */ > qemu_co_rwlock_wrlock(&job->flush_rwlock); > qemu_co_rwlock_unlock(&job->flush_rwlock); > > + if (job->sync_bitmap) { > + if (ret < 0) { > + /* Merge the successor back into the parent, delete nothing. */ > + assert(bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL)); Why can't reclaim fail here? If we canassert, please move the expression out because it has a side effect. > + bdrv_enable_dirty_bitmap(job->sync_bitmap); > + } else { > + /* Everything is fine, delete this bitmap and install the backup. */ > + assert(bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL)); > + } > + } > hbitmap_free(job->bitmap); > > bdrv_iostatus_disable(target); > @@ -368,6 +423,7 @@ static void coroutine_fn backup_run(void *opaque) > > void backup_start(BlockDriverState *bs, BlockDriverState *target, > int64_t speed, MirrorSyncMode sync_mode, > + BdrvDirtyBitmap *sync_bitmap, > BlockdevOnError on_source_error, > BlockdevOnError on_target_error, > BlockCompletionFunc *cb, void *opaque, > @@ -386,6 +442,26 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, > return; > } > > + if (sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) { > + if (!sync_bitmap) { > + error_setg(errp, "must provide a valid bitmap name for " > + "\"dirty-bitmap\" sync mode"); > + return; > + } > + > + /* Create a new bitmap, and freeze/disable this one. */ > + if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) { > + return; > + } There are two error paths in this function after this creation, need to release it there? > + > + } else if (sync_bitmap) { > + error_setg(errp, > + "a sync_bitmap was provided to backup_run, " > + "but received an incompatible sync_mode (%s)", > + MirrorSyncMode_lookup[sync_mode]); > + return; > + } > + > len = bdrv_getlength(bs); > if (len < 0) { > error_setg_errno(errp, -len, "unable to get length for '%s'", > @@ -403,6 +479,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, > job->on_target_error = on_target_error; > job->target = target; > job->sync_mode = sync_mode; > + job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP ? > + sync_bitmap : NULL; > job->common.len = len; > job->common.co = qemu_coroutine_create(backup_run); > qemu_coroutine_enter(job->common.co, job); > diff --git a/block/mirror.c b/block/mirror.c > index fc545f1..427c688 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -717,6 +717,10 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, > bool is_none_mode; > BlockDriverState *base; > > + if (mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) { > + error_setg(errp, "Sync mode 'dirty-bitmap' not supported"); > + return; > + } > is_none_mode = mode == MIRROR_SYNC_MODE_NONE; > base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL; > mirror_start_job(bs, target, replaces, > diff --git a/blockdev.c b/blockdev.c > index 8dde72a..114a94c 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1569,6 +1569,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp) > backup->sync, > backup->has_mode, backup->mode, > backup->has_speed, backup->speed, > + backup->has_bitmap, backup->bitmap, > backup->has_on_source_error, backup->on_source_error, > backup->has_on_target_error, backup->on_target_error, > &local_err); > @@ -2358,6 +2359,7 @@ void qmp_drive_backup(const char *device, const char *target, > enum MirrorSyncMode sync, > bool has_mode, enum NewImageMode mode, > bool has_speed, int64_t speed, > + bool has_bitmap, const char *bitmap, > bool has_on_source_error, BlockdevOnError on_source_error, > bool has_on_target_error, BlockdevOnError on_target_error, > Error **errp) > @@ -2365,6 +2367,7 @@ void qmp_drive_backup(const char *device, const char *target, > BlockDriverState *bs; > BlockDriverState *target_bs; > BlockDriverState *source = NULL; > + BdrvDirtyBitmap *bmap = NULL; > AioContext *aio_context; > BlockDriver *drv = NULL; > Error *local_err = NULL; > @@ -2460,7 +2463,16 @@ void qmp_drive_backup(const char *device, const char *target, > > bdrv_set_aio_context(target_bs, aio_context); > > - backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error, > + if (has_bitmap) { > + bmap = bdrv_find_dirty_bitmap(bs, bitmap); > + if (!bmap) { > + error_setg(errp, "Bitmap '%s' could not be found", bitmap); > + goto out; > + } > + } > + > + backup_start(bs, target_bs, speed, sync, bmap, > + on_source_error, on_target_error, > block_job_cb, bs, &local_err); > if (local_err != NULL) { > bdrv_unref(target_bs); > diff --git a/hmp.c b/hmp.c > index 481be80..63b19c7 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1021,7 +1021,8 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict) > > qmp_drive_backup(device, filename, !!format, format, > full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP, > - true, mode, false, 0, false, 0, false, 0, &err); > + true, mode, false, 0, false, NULL, > + false, 0, false, 0, &err); > hmp_handle_error(mon, &err); > } > > diff --git a/include/block/block.h b/include/block/block.h > index 6138544..99ed679 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -462,6 +462,7 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, > void bdrv_clear_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); > void bdrv_dirty_iter_init(BlockDriverState *bs, > BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi); > +void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset); > int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); > > void bdrv_enable_copy_on_read(BlockDriverState *bs); > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 06a21dd..cb1e4a1 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -584,6 +584,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, > * @target: Block device to write to. > * @speed: The maximum speed, in bytes per second, or 0 for unlimited. > * @sync_mode: What parts of the disk image should be copied to the destination. > + * @sync_bitmap: The dirty bitmap if sync_mode is MIRROR_SYNC_MODE_DIRTY_BITMAP. > * @on_source_error: The action to take upon error reading from the source. > * @on_target_error: The action to take upon error writing to the target. > * @cb: Completion function for the job. > @@ -594,6 +595,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, > */ > void backup_start(BlockDriverState *bs, BlockDriverState *target, > int64_t speed, MirrorSyncMode sync_mode, > + BdrvDirtyBitmap *sync_bitmap, > BlockdevOnError on_source_error, > BlockdevOnError on_target_error, > BlockCompletionFunc *cb, void *opaque, > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 3e863b9..92c3a84 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -500,7 +500,7 @@ > # Since: 1.3 > ## > { 'enum': 'MirrorSyncMode', > - 'data': ['top', 'full', 'none'] } > + 'data': ['top', 'full', 'none', 'dirty-bitmap'] } Maybe document version info for 'dirty-bitmap'? Eric? Fam