[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v1 1/1] add incremental mode on dri
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v1 1/1] add incremental mode on drive-mirror |
Date: |
Wed, 26 Dec 2018 09:35:12 +0000 |
Hi!
it's a very brief review, but hope it will help.
20.12.2018 11:38, mahaocong wrote:
> From: mahaocong <address@hidden>
>
> Signed-off-by: mahaocong <address@hidden>
> ---
> block/dirty-bitmap.c | 14 ++++++++++
> block/mirror.c | 63
> +++++++++++++++++++++++++++++++++++---------
> blockdev.c | 36 +++++++++++++++++++++++--
> include/block/block_int.h | 3 ++-
> include/block/dirty-bitmap.h | 3 +++
> include/qemu/hbitmap.h | 2 ++
> qapi/block-core.json | 9 ++++++-
> util/hbitmap.c | 28 ++++++++++++++++++++
> 8 files changed, 141 insertions(+), 17 deletions(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 89fd1d7f8b..118447d0db 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -348,6 +348,20 @@ BdrvDirtyBitmap
> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> return ret;
> }
>
> +BdrvDirtyBitmap *bdrv_copy_dirty_bitmap_locked(BdrvDirtyBitmap *src,
> + BdrvDirtyBitmap *dest,
> + Error **errp)
common notation in the file is opposite: _locked for functions, which need
external locking
> +{
> + qemu_mutex_lock(src->mutex);
> + if (!hbitmap_copy(dest->bitmap, src->bitmap)) {
> + error_setg(errp, "Error: copy src bitmap failed");
> + return NULL;
> + }
> + qemu_mutex_unlock(src->mutex);
> +
> + return dest;
> +}
please, keep new bitmap related feature (copy) in a separate patch
> +
> /**
> * Truncates _all_ bitmaps attached to a BDS.
> * Called with BQL taken.
> diff --git a/block/mirror.c b/block/mirror.c
> index ab59ad77e8..a860fe75a7 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -50,6 +50,8 @@ typedef struct MirrorBlockJob {
> /* Used to block operations on the drive-mirror-replace target */
> Error *replace_blocker;
> bool is_none_mode;
> + bool is_inc_mode;
> + char *bitmap_name;
> BlockMirrorBackingMode backing_mode;
> MirrorCopyMode copy_mode;
> BlockdevOnError on_source_error, on_target_error;
> @@ -814,6 +816,28 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob
> *s)
> return 0;
> }
>
> +/*
> + * init dirty bitmap by using user bitmap. usr->hbitmap will be copy to
> + * mirror bitmap->hbitmap instead of reuse it.
> + */
> +static int coroutine_fn mirror_dirty_init_incremental(MirrorBlockJob *s)
> +{
> + BlockDriverState *bs = s->mirror_top_bs->backing->bs;
> + BdrvDirtyBitmap *src, *dest;
> +
> + dest = s->dirty_bitmap;
> + src = bdrv_find_dirty_bitmap(bs, s->bitmap_name);
> + if (!src) {
> + return -1;
> + }
> +
> + if (!bdrv_copy_dirty_bitmap_locked(src, dest, NULL)) {
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> /* Called when going out of the streaming phase to flush the bulk of the
> * data to the medium, or just before completing.
> */
> @@ -913,9 +937,17 @@ static int coroutine_fn mirror_run(Job *job, Error
> **errp)
>
> s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> if (!s->is_none_mode) {
> - ret = mirror_dirty_init(s);
> - if (ret < 0 || job_is_cancelled(&s->common.job)) {
> - goto immediate_exit;
> + /* incremental mode */
> + if (s->is_inc_mode) {
> + ret = mirror_dirty_init_incremental(s);
> + if (ret < 0 || job_is_cancelled(&s->common.job)) {
bitmap copying don't have yield point, so checking for cancelled is not needed
here
> + goto immediate_exit;
> + }
> + } else {
> + ret = mirror_dirty_init(s);
> + if (ret < 0 || job_is_cancelled(&s->common.job)) {
> + goto immediate_exit;
> + }
> }
> }
>
> @@ -1484,7 +1516,9 @@ static void mirror_start_job(const char *job_id,
> BlockDriverState *bs,
> BlockCompletionFunc *cb,
> void *opaque,
> const BlockJobDriver *driver,
> - bool is_none_mode, BlockDriverState *base,
> + bool is_none_mode, bool is_inc_mode,
> + const char *bitmap_name,
> + BlockDriverState *base,
> bool auto_complete, const char
> *filter_node_name,
> bool is_mirror, MirrorCopyMode copy_mode,
> Error **errp)
> @@ -1598,6 +1632,8 @@ static void mirror_start_job(const char *job_id,
> BlockDriverState *bs,
> s->on_source_error = on_source_error;
> s->on_target_error = on_target_error;
> s->is_none_mode = is_none_mode;
> + s->is_inc_mode = is_inc_mode;
> + s->bitmap_name = g_strdup(bitmap_name);
> s->backing_mode = backing_mode;
> s->copy_mode = copy_mode;
> s->base = base;
> @@ -1648,6 +1684,7 @@ fail:
> bdrv_ref(mirror_top_bs);
>
> g_free(s->replaces);
> + g_free(s->bitmap_name);
> blk_unref(s->target);
> bs_opaque->job = NULL;
> job_early_fail(&s->common.job);
> @@ -1664,26 +1701,25 @@ void mirror_start(const char *job_id,
> BlockDriverState *bs,
> BlockDriverState *target, const char *replaces,
> int creation_flags, int64_t speed,
> uint32_t granularity, int64_t buf_size,
> - MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
> + MirrorSyncMode mode, const char *bitmap_name,
> + BlockMirrorBackingMode backing_mode,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> bool unmap, const char *filter_node_name,
> MirrorCopyMode copy_mode, Error **errp)
> {
> - bool is_none_mode;
> + bool is_none_mode, is_inc_mode;
> BlockDriverState *base;
>
> - if (mode == MIRROR_SYNC_MODE_INCREMENTAL) {
> - error_setg(errp, "Sync mode 'incremental' not supported");
> - return;
> - }
> is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
> + is_inc_mode = mode == MIRROR_SYNC_MODE_INCREMENTAL;
> base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
> mirror_start_job(job_id, bs, creation_flags, target, replaces,
> speed, granularity, buf_size, backing_mode,
> on_source_error, on_target_error, unmap, NULL, NULL,
> - &mirror_job_driver, is_none_mode, base, false,
> - filter_node_name, true, copy_mode, errp);
> + &mirror_job_driver, is_none_mode, is_inc_mode,
semantics becomes strange: why not to just pass mode instead of two bools?
On the other hand, even bitmap_name != NULL is enough to understand that mode
is incremental, so we don't need new paramter
> + bitmap_name, base, false, filter_node_name, true,
> + copy_mode, errp);
> }
>
> void commit_active_start(const char *job_id, BlockDriverState *bs,
> @@ -1707,7 +1743,8 @@ void commit_active_start(const char *job_id,
> BlockDriverState *bs,
> mirror_start_job(job_id, bs, creation_flags, base, NULL, speed, 0, 0,
> MIRROR_LEAVE_BACKING_CHAIN,
> on_error, on_error, true, cb, opaque,
> - &commit_active_job_driver, false, base, auto_complete,
> + &commit_active_job_driver, false, false,
> + NULL, base, auto_complete,
> filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
> &local_err);
> if (local_err) {
> diff --git a/blockdev.c b/blockdev.c
> index e6c8349409..4ac36f33dc 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3663,6 +3663,8 @@ static void blockdev_mirror_common(const char *job_id,
> BlockDriverState *bs,
> BlockDriverState *target,
> bool has_replaces, const char *replaces,
> enum MirrorSyncMode sync,
> + bool has_bitmap_name,
> + const char *bitmap_name,
> BlockMirrorBackingMode backing_mode,
> bool has_speed, int64_t speed,
> bool has_granularity, uint32_t
> granularity,
> @@ -3680,6 +3682,7 @@ static void blockdev_mirror_common(const char *job_id,
> BlockDriverState *bs,
> Error **errp)
> {
> int job_flags = JOB_DEFAULT;
> + BdrvDirtyBitmap *src_bitmap;
>
> if (!has_speed) {
> speed = 0;
> @@ -3702,6 +3705,23 @@ static void blockdev_mirror_common(const char *job_id,
> BlockDriverState *bs,
> if (!has_filter_node_name) {
> filter_node_name = NULL;
> }
> + if (!has_bitmap_name) {
> + bitmap_name = NULL;
> + }
> + /*
> + * In incremental mode, we should create null name bitmap by
> + * using user bitmap's granularity.
> + */
> + if (sync == MIRROR_SYNC_MODE_INCREMENTAL && bitmap_name != NULL) {
I think, these two ^^^ conditions must be equal, so, better assert it than
check both
> + src_bitmap = bdrv_find_dirty_bitmap(bs, bitmap_name);
and if we have to find bitmap here, why to ...
> + if (!src_bitmap) {
> + error_setg(errp, "Error: can't find dirty bitmap "
> + "before start incremental drive-mirror");
> + return;
> + }
> + granularity = bdrv_dirty_bitmap_granularity(src_bitmap);
> + }
> +
> if (!has_copy_mode) {
> copy_mode = MIRROR_COPY_MODE_BACKGROUND;
> }
> @@ -3739,7 +3759,7 @@ static void blockdev_mirror_common(const char *job_id,
> BlockDriverState *bs,
> */
> mirror_start(job_id, bs, target,
> has_replaces ? replaces : NULL, job_flags,
> - speed, granularity, buf_size, sync, backing_mode,
> + speed, granularity, buf_size, sync, bitmap_name,
> backing_mode,
... still pass bitmap_name here, and then find bitmap again?
> on_source_error, on_target_error, unmap, filter_node_name,
> copy_mode, errp);
> }
> @@ -3786,6 +3806,16 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
> if (arg->sync == MIRROR_SYNC_MODE_NONE) {
> source = bs;
> }
> + if ((arg->sync == MIRROR_SYNC_MODE_INCREMENTAL) &&
> + (!arg->has_bitmap_name)) {
> + error_setg(errp, "incremental mode must specify the bitmap name");
> + goto out;
> + }
> + if ((arg->sync == MIRROR_SYNC_MODE_INCREMENTAL) &&
> + (arg->has_granularity)) {
> + error_setg(errp, "incremental mode can not set bitmap granularity");
> + goto out;
> + }
>
> size = bdrv_getlength(bs);
> if (size < 0) {
> @@ -3880,6 +3910,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>
> blockdev_mirror_common(arg->has_job_id ? arg->job_id : NULL, bs,
> target_bs,
> arg->has_replaces, arg->replaces, arg->sync,
> + arg->has_bitmap_name, arg->bitmap_name,
> backing_mode, arg->has_speed, arg->speed,
> arg->has_granularity, arg->granularity,
> arg->has_buf_size, arg->buf_size,
> @@ -3937,7 +3968,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char
> *job_id,
> bdrv_set_aio_context(target_bs, aio_context);
>
> blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs,
> - has_replaces, replaces, sync, backing_mode,
> + has_replaces, replaces, sync, false, NULL,
> + backing_mode,
> has_speed, speed,
> has_granularity, granularity,
> has_buf_size, buf_size,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f605622216..2f4725e1c5 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1054,7 +1054,8 @@ void mirror_start(const char *job_id, BlockDriverState
> *bs,
> BlockDriverState *target, const char *replaces,
> int creation_flags, int64_t speed,
> uint32_t granularity, int64_t buf_size,
> - MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
> + MirrorSyncMode mode, const char *bitmap_name,
> + BlockMirrorBackingMode backing_mode,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> bool unmap, const char *filter_node_name,
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 8f38a3dec1..d354d8f950 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -21,6 +21,9 @@ BdrvDirtyBitmap
> *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
> BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap,
> Error **errp);
> +BdrvDirtyBitmap *bdrv_copy_dirty_bitmap_locked(BdrvDirtyBitmap *src,
> + BdrvDirtyBitmap *dest,
> + Error **errp);
> void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
> BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
> const char *name);
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index a7cb780592..6de3b4eb7e 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -91,6 +91,8 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b,
> HBitmap *result);
> */
> bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b);
>
> +bool hbitmap_copy(HBitmap *dst, const HBitmap *src);
> +
> /**
> * hbitmap_empty:
> * @hb: HBitmap to operate on.
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 762000f31f..7230dfb40b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1727,6 +1727,13 @@
> # (all the disk, only the sectors allocated in the topmost image, or
> # only new I/O).
> #
> +# @bitmap-name: the name of user-created-bitmap which is used to initialize
> the
> +# unnamed-bitmap used on incremental drive-mirror. It's hbitmap
> +# will be copy to mirror bitmap. If user select incremental
> mode,
> +# bitmap-name should not be null, and can not set granularity
> for
> +# the mirror bitmap should have the same granularity with
> +# user-created-bitmap.
1. agree with Eric, that it should be just @bitmap.
2. I don't sure in common semantics of new incremental mode: you implement it
in the way
you describe, just init mirror bitmap from user chosen bitmap. But if guest
writes to
the non-dirty range of given bitmap, this data will be mirrored. However,
as I understand
from your cover letter, you don't need such behavior.
> +#
> # @granularity: granularity of the dirty bitmap, default is 64K
> # if the image format doesn't have clusters, 4K if the
> clusters
> # are smaller than that, else the cluster size. Must be a
> @@ -1768,7 +1775,7 @@
> { 'struct': 'DriveMirror',
> 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
> '*format': 'str', '*node-name': 'str', '*replaces': 'str',
> - 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> + 'sync': 'MirrorSyncMode', '*bitmap-name': 'str', '*mode':
> 'NewImageMode',
> '*speed': 'int', '*granularity': 'uint32',
> '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
> '*on-target-error': 'BlockdevOnError',
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 8d402c59d9..7ae2fc270c 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -765,6 +765,34 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b,
> HBitmap *result)
> return true;
> }
>
> +/*
> + * Copy HBitmaps form src to dst.
> + *
> + * @return true if the copy was successful,
> + * false if it was not attempted.
> + */
> +bool hbitmap_copy(HBitmap *dst, const HBitmap *src)
> +{
> + int i;
> +
> + if ((dst->size != src->size) || (dst->granularity != src->granularity)) {
> + return false;
> + }
> +
> + if (hbitmap_count(src) == 0) {
> + return true;
> + }
> +
> + for (i = HBITMAP_LEVELS - 1; i >= 0; i--) {
> + memcpy(dst->levels[i], src->levels[i],
> + src->sizes[i] * sizeof(unsigned long));
> + }
> +
> + dst->count = src->count;
> + return true;
> +}
> +
> +
> HBitmap *hbitmap_create_meta(HBitmap *hb, int chunk_size)
> {
> assert(!(chunk_size & (chunk_size - 1)));
>
--
Best regards,
Vladimir