|
| From: | Daniel Kučera |
| Subject: | Re: [Qemu-block] [Qemu-devel] migration: add incremental drive-mirror and blockdev-mirror with dirtymap |
| Date: | Tue, 9 May 2017 09:35:23 +0200 |
On 05/04/2017 03:45 AM, Daniel Kučera wrote:
>
> 2017-05-04 1:44 GMT+02:00 John Snow <address@hidden
> <mailto:address@hidden>>:
>
>
>
> On 05/03/2017 03:56 AM, Daniel Kučera wrote:
> > Hi all,
> >
> > this patch adds possibility to start mirroring since specific dirtyblock
> > bitmap.
> > The use-case is, for live migrations with ZFS volume used as block device:
> > 1. make dirtyblock bitmap in qemu
>
> A "block dirty bitmap," I assume you mean. Through which interface?
> "block dirty bitmap add" via QMP?
>
>
> Yes.
>
>
> > 2. make ZFS volume snapshot
>
> ZFS Volume Snapshot is going to be a filesystem-level operation, isn't
> it? That is, creating this snapshot will necessarily create new dirty
> sectors, yes?
>
>
> No, we are using "zfs volumes" which are block devices (similar to LVM)
>
> # blockdev --report /dev/zstore/storage4
> RO RA SSZ BSZ StartSec Size Device
> rw 256 512 4096 0 42949672960 /dev/zstore/storage4
>
> -drive
> file=/dev/zstore/storage4,format=raw,if=none,id=drive- Ah, I see. Clearly I don't know much about ZFS in practice.scsi0-0-0-0,cache=none, discard=unmap
>
>
> > 3. zfs send/receive the snapshot to target machine
>
> Why? Is this an attempt to make the process faster?
>
>
> This preserves and transfers the whole chain of snapshots to destination
> host, not only current state as it would be with drive-mirror sync: full
>
>
>
> > 4. start mirroring to destination with block map from step 1
>
> This makes me a little nervous: what guarantees do you have that the
> bitmap and the ZFS snapshot were synchronous?
>
>
> It doesn't have to be synchronous (or atomic). The "block dirty bitmap"
> just needs to be done prior to ZFS snapshot. Those few writes done in
> the meantime don't hurt to be done twice.
>
Hm, I suppose that's right, pending cache issues, perhaps?
(1) Write occurs; cached
(2) Bitmap is added
(3) Write occurs, cached
(4) ZFS snapshot is taken
(5) Data is flushed to backing storage.
Now, the ZFS snapshot is migrated, but is missing the writes that
occurred in (1) and (3).
Next, we mirror the data in the bitmap, but it only includes the data
from (3).
The target now appears to be missing the write from (1) -- maybe,
depending on how the volume snapshot occurs.
>
>
> > 5. live migrate VM state to destination
> >
> > The point is, that I'm not able to live stream zfs changed data to
> > destination
> > to ensure same volume state in the moment of switchover of migrated VM
> > to the new hypervisor.
>
> I'm a little concerned about the mixing of filesystem and block level
> snapshots...
>
>
> As I explained above, ZFS snapshots are also block level.
>
>
>
> >
> >
> > From 7317d731d51c5d743d7a4081b368f0a6862856d7 Mon Sep 17 00:00:00 2001 > > From: Daniel Kucera <address@hidden <mailto:address@hidden
>
> What happened to your timestamp?
>
com >>
> > Date: Tue, 2 May 2017 15:00:39 +0000
> > Subject: [PATCH] migration: add incremental drive-mirror and blockdev-mirror
>
> Your actual email subject here, however, is missing the [PATCH] tag,
> which is useful for it getting picked up by the patchew build bot.
>
> > with dirtymap added parameter bitmap which will be used instead
> of newly
> > created dirtymap in mirror_start_job
> >
> > Signed-off-by: Daniel Kucera <address@hidden
> <mailto:address@hiddencom >>
Thanks for explaining the use-case, I'll take a look at the newer> > ---
> > block/mirror.c | 41
> ++++++++++++++++++++++++-----------------
> > blockdev.c | 6 +++++-
> > include/block/block_int.h | 4 +++-
> > qapi/block-core.json | 12 ++++++++++--
> > 4 files changed, 42 insertions(+), 21 deletions(-)
> >
> > diff --git a/block/mirror.c b/block/mirror.c
> > index 9f5eb69..02b2f69 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -49,7 +49,7 @@ typedef struct MirrorBlockJob {
> > BlockDriverState *to_replace;
> > /* Used to block operations on the drive-mirror-replace target */
> > Error *replace_blocker;
> > - bool is_none_mode;
> > + MirrorSyncMode sync_mode;
> > BlockMirrorBackingMode backing_mode;
> > BlockdevOnError on_source_error, on_target_error;
> > bool synced;
> > @@ -523,7 +523,9 @@ static void mirror_exit(BlockJob *job, void
> *opaque)
> > bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
> > &error_abort);
> > if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
> > - BlockDriverState *backing = s->is_none_mode ? src : s->base;
> > + BlockDriverState *backing =
> > + (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) ||
> > + (s->sync_mode == MIRROR_SYNC_MODE_NONE) ? src : s->base;
> > if (backing_bs(target_bs) != backing) {
> > bdrv_set_backing_hd(target_bs, backing, &local_err);
> > if (local_err) {
> > @@ -771,7 +773,8 @@ static void coroutine_fn mirror_run(void *opaque)
> > mirror_free_init(s);
> >
> > s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> > - if (!s->is_none_mode) {
> > + if ((s->sync_mode != MIRROR_SYNC_MODE_INCREMENTAL) &&
> > + (s->sync_mode != MIRROR_SYNC_MODE_NONE)) {
> > ret = mirror_dirty_init(s);
> > if (ret < 0 || block_job_is_cancelled(&s->common)) {
> > goto immediate_exit;
> > @@ -1114,7 +1117,8 @@ static void mirror_start_job(const char *job_id,
> > BlockDriverState *bs,
>
> Something appears to have corrupted your patch. Did you copy/paste this
> into gmail? I am unable to apply it.
>
> Please use "git send-email" as detailed in the wiki contributors guide.
>
>
> Okay, I'll try to fix these issues and send the patch again, ha
>
>
>
> > BlockCompletionFunc *cb,
> > void *opaque,
> > const BlockJobDriver *driver,
> > - bool is_none_mode, BlockDriverState
> *base,
> > + MirrorSyncMode sync_mode, const char
> *bitmap,
> > + BlockDriverState *base,
> > bool auto_complete, const char
> > *filter_node_name,
> > Error **errp)
> > {
> > @@ -1203,7 +1207,7 @@ static void mirror_start_job(const char *job_id,
> > BlockDriverState *bs,
> > s->replaces = g_strdup(replaces);
> > s->> > > s->> > > - s->is_none_mode = is_none_mode;
> > + s->sync_mode = sync_mode;
> > s->backing_mode = backing_mode;
> > s->base = base;
> > s->granularity = granularity;
> > @@ -1213,9 +1217,16 @@ static void mirror_start_job(const char
> *job_id,
> > BlockDriverState *bs,
> > s->should_complete = true;
> > }
> >
> > - s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL,
> > errp);
> > - if (!s->dirty_bitmap) {
> > - goto fail;
> > + if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
> > + s->dirty_bitmap = bdrv_find_dirty_bitmap(bs, bitmap);
>
> You're using a potentially undefined string here. You've also now split
> the error case for if we fail to start the coroutine -- one case has
> created a new bitmap, the other has merely picked up a handle to it. On
> failure in one case, we wish to free the bitmap -- in the other, we
> don't.
>
> Whoa, actually, it looks like the code as-is doesn't free the bitmap on
> failure! that's a problem. I think there should be a call to
> release_dirty_bitmap somewhere here. The only one I can see is in
> mirror_run.
>
> > + if (!s->dirty_bitmap) {
> > + goto fail;
>
> You can't jump straight to the failure label without setting an error
> message. Prior instances in this function do not because functions they
> are calling have set errp for them. Please use err_setg.
>
> > + }
> > + } else {
>
> You're not checking to see if a bitmap was provided but the sync mode
> wasn't incremental, which we also need to validate.
>
> > + s->dirty_bitmap = bdrv_create_dirty_bitmap(bs,
> granularity, NULL,
> > errp);
> > + if (!s->dirty_bitmap) {
> > + goto fail;
> > + }
> > }
> >
> > /* Required permissions are already taken with blk_new() */
> > @@ -1265,24 +1276,19 @@ fail:
> > void mirror_start(const char *job_id, BlockDriverState *bs,
> > BlockDriverState *target, const char *replaces,
> > int64_t speed, uint32_t granularity, int64_t
> buf_size,
> > - MirrorSyncMode mode, BlockMirrorBackingMode
> backing_mode,
> > + MirrorSyncMode mode, const char *bitmap,
> > + BlockMirrorBackingMode backing_mode,
> > BlockdevOnError on_source_error,
> > BlockdevOnError on_target_error,
> > bool unmap, const char *filter_node_name, Error
> **errp)
> > {
> > - bool is_none_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;
> > base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
> > mirror_start_job(job_id, bs, BLOCK_JOB_DEFAULT, 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,
> > + &mirror_job_driver, mode, bitmap, base, false,
> > filter_node_name, errp);
> > }
> >
> > @@ -1305,7 +1311,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,
> MIRROR_SYNC_MODE_FULL,
> > + NULL, base, auto_complete,
> > filter_node_name, &local_err);
> > if (local_err) {
> > error_propagate(errp, local_err);
> > diff --git a/blockdev.c b/blockdev.c
> > index 6428206..2569924 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -3379,6 +3379,7 @@ static void blockdev_mirror_common(const char
> > *job_id, BlockDriverState *bs,
> > enum MirrorSyncMode sync,
> > BlockMirrorBackingMode
> backing_mode,
> > bool has_speed, int64_t speed,
> > + bool has_bitmap, const char
> *bitmap,
> > bool has_granularity, uint32_t
> > granularity,
> > bool has_buf_size, int64_t
> buf_size,
> > bool has_on_source_error,
> > @@ -3440,7 +3441,7 @@ static void blockdev_mirror_common(const char
> > *job_id, BlockDriverState *bs,
> > */
> > mirror_start(job_id, bs, target,
> > has_replaces ? replaces : NULL,
> > - speed, granularity, buf_size, sync, backing_mode,
> > + speed, granularity, buf_size, sync, bitmap,
> backing_mode,
>
> You never validate the string passed in by the user before passing it on
> to internal routines; you don't check the value of "has_bitmap" at any
> point. You need to check has_bitmap and find that the user has provided
> a valid bitmap name before handing off to mirror_start.
>
> Take a look at do_drive_backup for how this is handled there.
>
> > on_source_error, on_target_error, unmap,
> filter_node_name,
> > errp);
> > }
> > @@ -3575,6 +3576,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,
> > backing_mode, arg->has_speed, arg->speed,
> > + arg->has_bitmap, arg->bitmap,
> > arg->has_granularity, arg->granularity,
> > arg->has_buf_size, arg->buf_size,
> > arg->has_on_source_error,
> arg->on_source_error,
> > @@ -3593,6 +3595,7 @@ void qmp_blockdev_mirror(bool has_job_id,
> const char
> > *job_id,
> > bool has_replaces, const char *replaces,
> > MirrorSyncMode sync,
> > bool has_speed, int64_t speed,
> > + bool has_bitmap, const char *bitmap,
> > bool has_granularity, uint32_t granularity,
> > bool has_buf_size, int64_t buf_size,
> > bool has_on_source_error,
> > @@ -3627,6 +3630,7 @@ void qmp_blockdev_mirror(bool has_job_id,
> const char
> > *job_id,
> > blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs,
> > has_replaces, replaces, sync,
> backing_mode,
> > has_speed, speed,
> > + has_bitmap, bitmap,
> > has_granularity, granularity,
> > has_buf_size, buf_size,
> > has_on_source_error, on_source_error,
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index 4f8cd29..3c59d69 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -827,6 +827,7 @@ void commit_active_start(const char *job_id,
> > BlockDriverState *bs,
> > * @granularity: The chosen granularity for the dirty bitmap.
> > * @buf_size: The amount of data that can be in flight at one time.
> > * @mode: Whether to collapse all images in the chain to the target.
> > + * @bitmap: The dirty bitmap if sync_mode is
> MIRROR_SYNC_MODE_INCREMENTAL.
>
> This doesn't match what this field is in practice, which is a
> user-string from the QMP monitor, which is never checked to see if it is
> valid or not (or present or not.)
>
> The documentation here reflects what it should be, but not what it is.
>
> > * @backing_mode: How to establish the target's backing chain after
> > completion.
> > * @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.
> > @@ -844,7 +845,8 @@ void commit_active_start(const char *job_id,
> > BlockDriverState *bs,
> > void mirror_start(const char *job_id, BlockDriverState *bs,
> > BlockDriverState *target, const char *replaces,
> > int64_t speed, uint32_t granularity, int64_t
> buf_size,
> > - MirrorSyncMode mode, BlockMirrorBackingMode
> backing_mode,
> > + MirrorSyncMode mode, const char *bitmap,
> > + BlockMirrorBackingMode backing_mode,
> > BlockdevOnError on_source_error,
> > BlockdevOnError on_target_error,
> > bool unmap, const char *filter_node_name, Error
> **errp);
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 87fb747..8e3d9b2 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1502,6 +1502,10 @@
> > #
> > # @speed: the maximum speed, in bytes per second
> > #
> > +# @bitmap: the name of dirty bitmap if sync is "incremental".
> > +# Must be present if sync is "incremental", must NOT be
> present
> > +# otherwise. (Since 2.10)
> > +#
> > # @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).
> > @@ -1533,7 +1537,7 @@
> > 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
> > '*format': 'str', '*node-name': 'str', '*replaces':
> 'str',
> > 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> > - '*speed': 'int', '*granularity': 'uint32',
> > + '*speed': 'int', '*bitmap': 'str', '*granularity':
> 'uint32',
> > '*buf-size': 'int', '*on-source-error':
> 'BlockdevOnError',
> > '*on-target-error': 'BlockdevOnError',
> > '*unmap': 'bool' } }
> > @@ -1652,6 +1656,10 @@
> > #
> > # @speed: the maximum speed, in bytes per second
> > #
> > +# @bitmap: the name of dirty bitmap if sync is "incremental".
> > +# Must be present if sync is "incremental", must NOT be
> present
> > +# otherwise. (Since 2.10)
> > +#
> > # @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).
> > @@ -1694,7 +1702,7 @@
> > 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
> > '*replaces': 'str',
> > 'sync': 'MirrorSyncMode',
> > - '*speed': 'int', '*granularity': 'uint32',
> > + '*speed': 'int', '*bitmap': 'str', '*granularity':
> 'uint32',
> > '*buf-size': 'int', '*on-source-error':
> 'BlockdevOnError',
> > '*on-target-error': 'BlockdevOnError',
> > '*filter-node-name': 'str' } }
> >
>
> --
> —js
>
>
> Thanks for the notes.
>
> D.
version shortly.
--js
| [Prev in Thread] | Current Thread | [Next in Thread] |