From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43178) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yj69X-0001sW-AE for qemu-devel@nongnu.org; Fri, 17 Apr 2015 09:17:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yj69S-00042a-8D for qemu-devel@nongnu.org; Fri, 17 Apr 2015 09:17:51 -0400 Message-ID: <553107F4.6040100@redhat.com> Date: Fri, 17 Apr 2015 15:17:40 +0200 From: Max Reitz MIME-Version: 1.0 References: <1428531604-9428-1-git-send-email-jsnow@redhat.com> <1428531604-9428-11-git-send-email-jsnow@redhat.com> In-Reply-To: <1428531604-9428-11-git-send-email-jsnow@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 10/21] 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 , qemu-block@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, vsementsov@parallels.com, stefanha@redhat.com On 09.04.2015 00:19, 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 | 9 +++ > block/backup.c | 156 +++++++++++++++++++++++++++++++++++++++------- > block/mirror.c | 4 ++ > blockdev.c | 18 +++++- > hmp.c | 3 +- > include/block/block.h | 1 + > include/block/block_int.h | 2 + > qapi/block-core.json | 13 ++-- > qmp-commands.hx | 7 ++- > 9 files changed, 180 insertions(+), 33 deletions(-) > > diff --git a/block.c b/block.c > index 9d30379..2367311 100644 > --- a/block.c > +++ b/block.c > @@ -5717,6 +5717,15 @@ static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, > } > } > > +/** > + * Advance an HBitmapIter to an arbitrary offset. > + */ > +void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset) > +{ > + assert(hbi->hb); > + hbitmap_iter_init(hbi, hbi->hb, offset); > +} > + > 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 1c535b1..8513917 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,92 @@ 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 int coroutine_fn backup_run_incremental(BackupBlockJob *job) > +{ > + bool error_is_read; > + int ret = 0; > + int clusters_per_iter; > + uint32_t granularity; > + int64_t sector; > + int64_t cluster; > + int64_t end; > + int64_t last_cluster = -1; > + BlockDriverState *bs = job->common.bs; > + HBitmapIter hbi; > + > + granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap); > + clusters_per_iter = MAX((granularity / BACKUP_CLUSTER_SIZE), 1); DIV_ROUND_UP(granularity, BACKUP_CLUSTER_SIZE) would've worked, too (instead of the MAX()), but since both are powers of two, this is equivalent. > + bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi); > + > + /* Find the next dirty sector(s) */ > + while ((sector = hbitmap_iter_next(&hbi)) != -1) { > + cluster = sector / BACKUP_SECTORS_PER_CLUSTER; > + > + /* Fake progress updates for any clusters we skipped */ > + if (cluster != last_cluster + 1) { > + job->common.offset += ((cluster - last_cluster - 1) * > + BACKUP_CLUSTER_SIZE); > + } > + > + for (end = cluster + clusters_per_iter; cluster < end; cluster++) { > + if (yield_and_check(job)) { > + return ret; > + } > + > + 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) { > + return ret; > + } Now that I'm reading this code again... The other backup implementation handles retries differently; it redoes the whole loop, with the effective difference being that it calls yield_and_check() between every retry. Would it make sense to move the yield_and_check() call into this loop? > + } while (ret < 0); > + } > + > + /* If the bitmap granularity is smaller than the backup granularity, > + * we need to advance the iterator pointer to the next cluster. */ > + if (granularity < BACKUP_CLUSTER_SIZE) { Actually, whenever BACKUP_CLUSTER_SIZE isn't a factor of granularity. Both are powers of two, though, so that's the case iff granularity < BACKUP_CLUSTER_SIZE. (thus, the condition is correct) > + bdrv_set_dirty_iter(&hbi, cluster * BACKUP_SECTORS_PER_CLUSTER); > + } > + > + last_cluster = cluster - 1; A bit awkward, but hey... So, what's preventing me from giving an R-b is whether or not yield_and_check() should be moved. Max > + } > + > + /* Play some final catchup with the progress meter */ > + end = DIV_ROUND_UP(job->common.len, BACKUP_CLUSTER_SIZE); > + if (last_cluster + 1 < end) { > + job->common.offset += ((end - last_cluster - 1) * BACKUP_CLUSTER_SIZE); > + } > + > + return ret; > +} > + > static void coroutine_fn backup_run(void *opaque) > { > BackupBlockJob *job = opaque; > @@ -259,8 +347,7 @@ static void coroutine_fn backup_run(void *opaque) > 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 +365,13 @@ 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) { > + ret = backup_run_incremental(job); > } 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; > } > > @@ -357,6 +429,18 @@ static void coroutine_fn backup_run(void *opaque) > qemu_co_rwlock_wrlock(&job->flush_rwlock); > qemu_co_rwlock_unlock(&job->flush_rwlock); > > + if (job->sync_bitmap) { > + BdrvDirtyBitmap *bm; > + if (ret < 0) { > + /* Merge the successor back into the parent, delete nothing. */ > + bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL); > + assert(bm); > + } else { > + /* Everything is fine, delete this bitmap and install the backup. */ > + bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL); > + assert(bm); > + } > + } > hbitmap_free(job->bitmap); > > bdrv_iostatus_disable(target); > @@ -369,6 +453,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, > @@ -412,17 +497,36 @@ 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; > + } > + } 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'", > bdrv_get_device_name(bs)); > - return; > + goto error; > } > > BackupBlockJob *job = block_job_create(&backup_job_driver, bs, speed, > cb, opaque, errp); > if (!job) { > - return; > + goto error; > } > > bdrv_op_block_all(target, job->common.blocker); > @@ -431,7 +535,15 @@ 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); > + return; > + > + error: > + if (sync_bitmap) { > + bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL); > + } > } > diff --git a/block/mirror.c b/block/mirror.c > index 1cb700e..f89eccf 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -718,6 +718,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 373d19a..90ba5b6 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1584,6 +1584,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); > @@ -2394,6 +2395,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) > @@ -2402,6 +2404,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; > @@ -2501,7 +2504,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); > @@ -2562,8 +2574,8 @@ void qmp_blockdev_backup(const char *device, const char *target, > > bdrv_ref(target_bs); > bdrv_set_aio_context(target_bs, aio_context); > - backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error, > - block_job_cb, bs, &local_err); > + backup_start(bs, target_bs, speed, sync, NULL, on_source_error, > + on_target_error, block_job_cb, bs, &local_err); > if (local_err != NULL) { > bdrv_unref(target_bs); > error_propagate(errp, local_err); > diff --git a/hmp.c b/hmp.c > index f31ae27..d85d913 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1061,7 +1061,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 b02c670..80ac2cc 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -480,6 +480,7 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, > int64_t cur_sector, int nr_sectors); > 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 fb9e100..e0d5561 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -602,6 +602,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. > @@ -612,6 +613,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 710be68..7f629e2 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -512,10 +512,12 @@ > # > # @none: only copy data written from now on > # > +# @dirty-bitmap: only copy data described by the dirty bitmap. Since: 2.4 > +# > # Since: 1.3 > ## > { 'enum': 'MirrorSyncMode', > - 'data': ['top', 'full', 'none'] } > + 'data': ['top', 'full', 'none', 'dirty-bitmap'] } > > ## > # @BlockJobType: > @@ -690,14 +692,17 @@ > # probe if @mode is 'existing', else the format of the source > # > # @sync: what parts of the disk image should be copied to the destination > -# (all the disk, only the sectors allocated in the topmost image, or > -# only new I/O). > +# (all the disk, only the sectors allocated in the topmost image, from a > +# dirty bitmap, or only new I/O). > # > # @mode: #optional whether and how QEMU should create a new image, default is > # 'absolute-paths'. > # > # @speed: #optional the maximum speed, in bytes per second > # > +# @bitmap: #optional the name of dirty bitmap if sync is "dirty-bitmap" > +# (Since 2.4) > +# > # @on-source-error: #optional the action to take on an error on the source, > # default 'report'. 'stop' and 'enospc' can only be used > # if the block device supports io-status (see BlockInfo). > @@ -715,7 +720,7 @@ > { 'type': 'DriveBackup', > 'data': { 'device': 'str', 'target': 'str', '*format': 'str', > 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', > - '*speed': 'int', > + '*speed': 'int', '*bitmap': 'str', > '*on-source-error': 'BlockdevOnError', > '*on-target-error': 'BlockdevOnError' } } > > diff --git a/qmp-commands.hx b/qmp-commands.hx > index b6bd455..da750f0 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -1074,7 +1074,7 @@ EQMP > { > .name = "drive-backup", > .args_type = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?," > - "on-source-error:s?,on-target-error:s?", > + "bitmap:s?,on-source-error:s?,on-target-error:s?", > .mhandler.cmd_new = qmp_marshal_input_drive_backup, > }, > > @@ -1101,8 +1101,9 @@ Arguments: > (json-string, optional) > - "sync": what parts of the disk image should be copied to the destination; > possibilities include "full" for all the disk, "top" for only the sectors > - allocated in the topmost image, or "none" to only replicate new I/O > - (MirrorSyncMode). > + allocated in the topmost image, "dirty-bitmap" for only the dirty sectors in > + the bitmap, or "none" to only replicate new I/O (MirrorSyncMode). > +- "bitmap": dirty bitmap name for sync==dirty-bitmap > - "mode": whether and how QEMU should create a new image > (NewImageMode, optional, default 'absolute-paths') > - "speed": the maximum speed, in bytes per second (json-int, optional)