| Message ID | 1402912857-10509-4-git-send-email-benoit.canet@irqsave.net |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 16, 2014 at 12:00:56PM +0200, Benoît Canet wrote: > +BlockDriverState *check_to_replace_node(const char *node_name, Error **errp) > +{ > + BlockDriverState *to_replace_bs = bdrv_find_node(node_name); > + if (!to_replace_bs) { > + error_setg(errp, "Node name '%s' not found", > + node_name); > + return NULL; > + } > + > + if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, errp)) { > + return NULL; > + } Currently bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, errp) is not effective since child nodes never have op blockers. Only the root node/drive has op blockers. We discussed propagating blockers to child nodes. Can you send a patch to address this? I think Jeff will need to rebase his node-name series too. Stefan
Am 16.06.2014 um 12:00 hat Benoît Canet geschrieben: > drive-mirror will bdrv_swap the new BDS named node-name with the one > pointed by replaces when the mirroring is finished. > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > Reviewed-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 17 ++++++++++++++ > block/mirror.c | 60 +++++++++++++++++++++++++++++++++++++---------- > blockdev.c | 30 +++++++++++++++++++++++- > hmp.c | 2 +- > include/block/block.h | 4 ++++ > include/block/block_int.h | 3 +++ > qapi/block-core.json | 6 ++++- > qmp-commands.hx | 4 +++- > 8 files changed, 109 insertions(+), 17 deletions(-) > > diff --git a/block.c b/block.c > index 17f763d..318f1e6 100644 > --- a/block.c > +++ b/block.c > @@ -5795,3 +5795,20 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate) > > return false; > } > + > +BlockDriverState *check_to_replace_node(const char *node_name, Error **errp) > +{ > + BlockDriverState *to_replace_bs = bdrv_find_node(node_name); > + if (!to_replace_bs) { > + error_setg(errp, "Node name '%s' not found", > + node_name); Unnecessary line break. > + return NULL; > + } > + > + if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, errp)) { > + return NULL; > + } > + > + return to_replace_bs; > +} > + Empty line before EOF. > diff --git a/block/mirror.c b/block/mirror.c > index 94c8661..151167e 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -32,6 +32,12 @@ typedef struct MirrorBlockJob { > RateLimit limit; > BlockDriverState *target; > BlockDriverState *base; > + /* The name of the graph node to replace */ > + char *replaces; > + /* The BDS to replace */ > + BlockDriverState *to_replace; > + /* Used to block operations on the drive-mirror-replace target */ > + Error *replace_blocker; > bool is_none_mode; > BlockdevOnError on_source_error, on_target_error; > bool synced; > @@ -490,10 +496,14 @@ immediate_exit: > bdrv_release_dirty_bitmap(bs, s->dirty_bitmap); > bdrv_iostatus_disable(s->target); > if (s->should_complete && ret == 0) { > - if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) { > - bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL); > + BlockDriverState *to_replace = s->common.bs; > + if (s->to_replace) { > + to_replace = s->to_replace; > } > - bdrv_swap(s->target, s->common.bs); > + if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) { > + bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL); > + } > + bdrv_swap(s->target, to_replace); > if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) { > /* drop the bs loop chain formed by the swap: break the loop then > * trigger the unref from the top one */ > @@ -502,6 +512,12 @@ immediate_exit: > bdrv_unref(p); > } > } > + if (s->to_replace) { > + bdrv_op_unblock_all(s->to_replace, s->replace_blocker); > + error_free(s->replace_blocker); > + bdrv_unref(s->to_replace); > + } > + g_free(s->replaces); > bdrv_unref(s->target); > block_job_completed(&s->common, ret); > } > @@ -540,6 +556,20 @@ static void mirror_complete(BlockJob *job, Error **errp) > return; > } > > + /* check the target bs is not blocked and block all operations on it */ > + if (s->replaces) { > + s->to_replace = check_to_replace_node(s->replaces, errp); > + This empty line looks unusual. > + if (!s->to_replace) { > + return; > + } So here is the thing that I really wanted to comment on. In the case of a REPLACE blocker being set, this is a silent failure. The completion command will return success, but s->should_complete won't actually be set, so the completion doesn't happen. The only thing that actually happens is the bdrv_open_backing_file(s->target) (which looks somewhat questionable, too...) Now I would expect that the REPLACE blocker is actually set for any backing file, because that is what bdrv_set_backing_hd() does. For quorum it does work as expected because quorum children don't get any backing_blocker (we need to check whether they should get something similar from the quorum BDS), so this is probably why it escaped your testing. We'll need a test case that tries replacing some ordinary backing file. Now I think the (accidental?) restriction to only replacing quorum nodes actually makes this patch pretty safe, so maybe it would be nice to keep this behaviour; but we need to fix it to not fail silently but return an explicit error. > + error_setg(&s->replace_blocker, > + "block device is in use by block-job-complete"); > + bdrv_op_block_all(s->to_replace, s->replace_blocker); > + bdrv_ref(s->to_replace); > + } > + > s->should_complete = true; > block_job_resume(job); > } > @@ -562,14 +592,15 @@ static const BlockJobDriver commit_active_job_driver = { > }; > > static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, > - int64_t speed, int64_t granularity, > - int64_t buf_size, > - BlockdevOnError on_source_error, > - BlockdevOnError on_target_error, > - BlockDriverCompletionFunc *cb, > - void *opaque, Error **errp, > - const BlockJobDriver *driver, > - bool is_none_mode, BlockDriverState *base) > + const char *replaces, > + int64_t speed, int64_t granularity, > + int64_t buf_size, > + BlockdevOnError on_source_error, > + BlockdevOnError on_target_error, > + BlockDriverCompletionFunc *cb, > + void *opaque, Error **errp, > + const BlockJobDriver *driver, > + bool is_none_mode, BlockDriverState *base) > { > MirrorBlockJob *s; > > @@ -600,6 +631,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, > return; > } > > + s->replaces = g_strdup(replaces); > s->on_source_error = on_source_error; > s->on_target_error = on_target_error; > s->target = target; One design question that isn't quite clear to me yet is why you resolve the device name only in mirror_complete() and not here. This means that the drive-mirror QMP command can refer to one BDS with node-name foo, which then gets removed and another BDS with node-name foo is added, and then it would refer to the new BDS on completion time. I would find it less surprising if we took a reference to the old BDS here so that you can't remove it. Perhaps setting the replace_blocker here already would be safer, too. > @@ -621,6 +653,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, > } > > void mirror_start(BlockDriverState *bs, BlockDriverState *target, > + const char *replaces, > int64_t speed, int64_t granularity, int64_t buf_size, > MirrorSyncMode mode, BlockdevOnError on_source_error, > BlockdevOnError on_target_error, > @@ -632,7 +665,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, > > is_none_mode = mode == MIRROR_SYNC_MODE_NONE; > base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL; > - mirror_start_job(bs, target, speed, granularity, buf_size, > + mirror_start_job(bs, target, replaces, > + speed, granularity, buf_size, > on_source_error, on_target_error, cb, opaque, errp, > &mirror_job_driver, is_none_mode, base); > } > @@ -680,7 +714,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, > } > > bdrv_ref(base); > - mirror_start_job(bs, base, speed, 0, 0, > + mirror_start_job(bs, base, NULL, speed, 0, 0, > on_error, on_error, cb, opaque, &local_err, > &commit_active_job_driver, false, base); > if (local_err) { > diff --git a/blockdev.c b/blockdev.c > index 06b14f2..237a548 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2107,6 +2107,7 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp) > void qmp_drive_mirror(const char *device, const char *target, > bool has_format, const char *format, > bool has_node_name, const char *node_name, > + bool has_replaces, const char *replaces, > enum MirrorSyncMode sync, > bool has_mode, enum NewImageMode mode, > bool has_speed, int64_t speed, > @@ -2194,6 +2195,28 @@ void qmp_drive_mirror(const char *device, const char *target, > return; > } > > + if (has_replaces) { > + BlockDriverState *to_replace_bs; > + > + if (!has_node_name) { > + error_setg(errp, "a node-name must be provided when replacing a" > + " named node of the graph"); > + return; > + } > + > + to_replace_bs = check_to_replace_node(replaces, errp); > + > + if (!to_replace_bs) { > + return; > + } > + > + if (size != bdrv_getlength(to_replace_bs)) { > + error_setg(errp, "cannot replace image with a mirror image of " > + "different size"); > + return; > + } We may want to loosen some of these restrictions later, but it's good to start with more restrictions if in doubt. > + } > + > if ((sync == MIRROR_SYNC_MODE_FULL || !source) > && mode != NEW_IMAGE_MODE_EXISTING) > { Kevin
The Friday 27 Jun 2014 à 13:57:02 (+0200), Kevin Wolf wrote : > Am 16.06.2014 um 12:00 hat Benoît Canet geschrieben: > > drive-mirror will bdrv_swap the new BDS named node-name with the one > > pointed by replaces when the mirroring is finished. > > > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > --- > > block.c | 17 ++++++++++++++ > > block/mirror.c | 60 +++++++++++++++++++++++++++++++++++++---------- > > blockdev.c | 30 +++++++++++++++++++++++- > > hmp.c | 2 +- > > include/block/block.h | 4 ++++ > > include/block/block_int.h | 3 +++ > > qapi/block-core.json | 6 ++++- > > qmp-commands.hx | 4 +++- > > 8 files changed, 109 insertions(+), 17 deletions(-) > > > > diff --git a/block.c b/block.c > > index 17f763d..318f1e6 100644 > > --- a/block.c > > +++ b/block.c > > @@ -5795,3 +5795,20 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate) > > > > return false; > > } > > + > > +BlockDriverState *check_to_replace_node(const char *node_name, Error **errp) > > +{ > > + BlockDriverState *to_replace_bs = bdrv_find_node(node_name); > > + if (!to_replace_bs) { > > + error_setg(errp, "Node name '%s' not found", > > + node_name); > > Unnecessary line break. > > > + return NULL; > > + } > > + > > + if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, errp)) { > > + return NULL; > > + } > > + > > + return to_replace_bs; > > +} > > + > > Empty line before EOF. > > > diff --git a/block/mirror.c b/block/mirror.c > > index 94c8661..151167e 100644 > > --- a/block/mirror.c > > +++ b/block/mirror.c > > @@ -32,6 +32,12 @@ typedef struct MirrorBlockJob { > > RateLimit limit; > > BlockDriverState *target; > > BlockDriverState *base; > > + /* The name of the graph node to replace */ > > + char *replaces; > > + /* The BDS to replace */ > > + BlockDriverState *to_replace; > > + /* Used to block operations on the drive-mirror-replace target */ > > + Error *replace_blocker; > > bool is_none_mode; > > BlockdevOnError on_source_error, on_target_error; > > bool synced; > > @@ -490,10 +496,14 @@ immediate_exit: > > bdrv_release_dirty_bitmap(bs, s->dirty_bitmap); > > bdrv_iostatus_disable(s->target); > > if (s->should_complete && ret == 0) { > > - if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) { > > - bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL); > > + BlockDriverState *to_replace = s->common.bs; > > + if (s->to_replace) { > > + to_replace = s->to_replace; > > } > > - bdrv_swap(s->target, s->common.bs); > > + if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) { > > + bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL); > > + } > > + bdrv_swap(s->target, to_replace); > > if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) { > > /* drop the bs loop chain formed by the swap: break the loop then > > * trigger the unref from the top one */ > > @@ -502,6 +512,12 @@ immediate_exit: > > bdrv_unref(p); > > } > > } > > + if (s->to_replace) { > > + bdrv_op_unblock_all(s->to_replace, s->replace_blocker); > > + error_free(s->replace_blocker); > > + bdrv_unref(s->to_replace); > > + } > > + g_free(s->replaces); > > bdrv_unref(s->target); > > block_job_completed(&s->common, ret); > > } > > @@ -540,6 +556,20 @@ static void mirror_complete(BlockJob *job, Error **errp) > > return; > > } > > > > + /* check the target bs is not blocked and block all operations on it */ > > + if (s->replaces) { > > + s->to_replace = check_to_replace_node(s->replaces, errp); > > + > > This empty line looks unusual. > > > + if (!s->to_replace) { > > + return; > > + } > > So here is the thing that I really wanted to comment on. In the case of > a REPLACE blocker being set, this is a silent failure. The completion > command will return success, but s->should_complete won't actually be > set, so the completion doesn't happen. The only thing that actually > happens is the bdrv_open_backing_file(s->target) (which looks somewhat > questionable, too...) > > Now I would expect that the REPLACE blocker is actually set for any > backing file, because that is what bdrv_set_backing_hd() does. For > quorum it does work as expected because quorum children don't get any > backing_blocker (we need to check whether they should get something > similar from the quorum BDS), so this is probably why it escaped your > testing. We'll need a test case that tries replacing some ordinary > backing file. > > Now I think the (accidental?) restriction to only replacing quorum nodes > actually makes this patch pretty safe, so maybe it would be nice to keep > this behaviour; but we need to fix it to not fail silently but return an > explicit error. > > > + error_setg(&s->replace_blocker, > > + "block device is in use by block-job-complete"); > > + bdrv_op_block_all(s->to_replace, s->replace_blocker); > > + bdrv_ref(s->to_replace); > > + } > > + > > s->should_complete = true; > > block_job_resume(job); > > } > > @@ -562,14 +592,15 @@ static const BlockJobDriver commit_active_job_driver = { > > }; > > > > static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, > > - int64_t speed, int64_t granularity, > > - int64_t buf_size, > > - BlockdevOnError on_source_error, > > - BlockdevOnError on_target_error, > > - BlockDriverCompletionFunc *cb, > > - void *opaque, Error **errp, > > - const BlockJobDriver *driver, > > - bool is_none_mode, BlockDriverState *base) > > + const char *replaces, > > + int64_t speed, int64_t granularity, > > + int64_t buf_size, > > + BlockdevOnError on_source_error, > > + BlockdevOnError on_target_error, > > + BlockDriverCompletionFunc *cb, > > + void *opaque, Error **errp, > > + const BlockJobDriver *driver, > > + bool is_none_mode, BlockDriverState *base) > > { > > MirrorBlockJob *s; > > > > @@ -600,6 +631,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, > > return; > > } > > > > + s->replaces = g_strdup(replaces); > > s->on_source_error = on_source_error; > > s->on_target_error = on_target_error; > > s->target = target; > > One design question that isn't quite clear to me yet is why you resolve > the device name only in mirror_complete() and not here. This means that > the drive-mirror QMP command can refer to one BDS with node-name foo, > which then gets removed and another BDS with node-name foo is added, and > then it would refer to the new BDS on completion time. > > I would find it less surprising if we took a reference to the old BDS > here so that you can't remove it. Perhaps setting the replace_blocker > here already would be safer, too. I have done late binding because this allow the code to block the to replace BDS late. If I get a reference to it early the code must block it early. > > > @@ -621,6 +653,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, > > } > > > > void mirror_start(BlockDriverState *bs, BlockDriverState *target, > > + const char *replaces, > > int64_t speed, int64_t granularity, int64_t buf_size, > > MirrorSyncMode mode, BlockdevOnError on_source_error, > > BlockdevOnError on_target_error, > > @@ -632,7 +665,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, > > > > is_none_mode = mode == MIRROR_SYNC_MODE_NONE; > > base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL; > > - mirror_start_job(bs, target, speed, granularity, buf_size, > > + mirror_start_job(bs, target, replaces, > > + speed, granularity, buf_size, > > on_source_error, on_target_error, cb, opaque, errp, > > &mirror_job_driver, is_none_mode, base); > > } > > @@ -680,7 +714,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, > > } > > > > bdrv_ref(base); > > - mirror_start_job(bs, base, speed, 0, 0, > > + mirror_start_job(bs, base, NULL, speed, 0, 0, > > on_error, on_error, cb, opaque, &local_err, > > &commit_active_job_driver, false, base); > > if (local_err) { > > diff --git a/blockdev.c b/blockdev.c > > index 06b14f2..237a548 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -2107,6 +2107,7 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp) > > void qmp_drive_mirror(const char *device, const char *target, > > bool has_format, const char *format, > > bool has_node_name, const char *node_name, > > + bool has_replaces, const char *replaces, > > enum MirrorSyncMode sync, > > bool has_mode, enum NewImageMode mode, > > bool has_speed, int64_t speed, > > @@ -2194,6 +2195,28 @@ void qmp_drive_mirror(const char *device, const char *target, > > return; > > } > > > > + if (has_replaces) { > > + BlockDriverState *to_replace_bs; > > + > > + if (!has_node_name) { > > + error_setg(errp, "a node-name must be provided when replacing a" > > + " named node of the graph"); > > + return; > > + } > > + > > + to_replace_bs = check_to_replace_node(replaces, errp); > > + > > + if (!to_replace_bs) { > > + return; > > + } > > + > > + if (size != bdrv_getlength(to_replace_bs)) { > > + error_setg(errp, "cannot replace image with a mirror image of " > > + "different size"); > > + return; > > + } > > We may want to loosen some of these restrictions later, but it's good to > start with more restrictions if in doubt. > > > + } > > + > > if ((sync == MIRROR_SYNC_MODE_FULL || !source) > > && mode != NEW_IMAGE_MODE_EXISTING) > > { > > Kevin
The Friday 27 Jun 2014 à 13:57:02 (+0200), Kevin Wolf wrote : > Am 16.06.2014 um 12:00 hat Benoît Canet geschrieben: > > drive-mirror will bdrv_swap the new BDS named node-name with the one > > pointed by replaces when the mirroring is finished. > > > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > --- > > block.c | 17 ++++++++++++++ > > block/mirror.c | 60 +++++++++++++++++++++++++++++++++++++---------- > > blockdev.c | 30 +++++++++++++++++++++++- > > hmp.c | 2 +- > > include/block/block.h | 4 ++++ > > include/block/block_int.h | 3 +++ > > qapi/block-core.json | 6 ++++- > > qmp-commands.hx | 4 +++- > > 8 files changed, 109 insertions(+), 17 deletions(-) > > > > diff --git a/block.c b/block.c > > index 17f763d..318f1e6 100644 > > --- a/block.c > > +++ b/block.c > > @@ -5795,3 +5795,20 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate) > > > > return false; > > } > > + > > +BlockDriverState *check_to_replace_node(const char *node_name, Error **errp) > > +{ > > + BlockDriverState *to_replace_bs = bdrv_find_node(node_name); > > + if (!to_replace_bs) { > > + error_setg(errp, "Node name '%s' not found", > > + node_name); > > Unnecessary line break. > > > + return NULL; > > + } > > + > > + if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, errp)) { > > + return NULL; > > + } > > + > > + return to_replace_bs; > > +} > > + > > Empty line before EOF. > > > diff --git a/block/mirror.c b/block/mirror.c > > index 94c8661..151167e 100644 > > --- a/block/mirror.c > > +++ b/block/mirror.c > > @@ -32,6 +32,12 @@ typedef struct MirrorBlockJob { > > RateLimit limit; > > BlockDriverState *target; > > BlockDriverState *base; > > + /* The name of the graph node to replace */ > > + char *replaces; > > + /* The BDS to replace */ > > + BlockDriverState *to_replace; > > + /* Used to block operations on the drive-mirror-replace target */ > > + Error *replace_blocker; > > bool is_none_mode; > > BlockdevOnError on_source_error, on_target_error; > > bool synced; > > @@ -490,10 +496,14 @@ immediate_exit: > > bdrv_release_dirty_bitmap(bs, s->dirty_bitmap); > > bdrv_iostatus_disable(s->target); > > if (s->should_complete && ret == 0) { > > - if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) { > > - bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL); > > + BlockDriverState *to_replace = s->common.bs; > > + if (s->to_replace) { > > + to_replace = s->to_replace; > > } > > - bdrv_swap(s->target, s->common.bs); > > + if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) { > > + bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL); > > + } > > + bdrv_swap(s->target, to_replace); > > if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) { > > /* drop the bs loop chain formed by the swap: break the loop then > > * trigger the unref from the top one */ > > @@ -502,6 +512,12 @@ immediate_exit: > > bdrv_unref(p); > > } > > } > > + if (s->to_replace) { > > + bdrv_op_unblock_all(s->to_replace, s->replace_blocker); > > + error_free(s->replace_blocker); > > + bdrv_unref(s->to_replace); > > + } > > + g_free(s->replaces); > > bdrv_unref(s->target); > > block_job_completed(&s->common, ret); > > } > > @@ -540,6 +556,20 @@ static void mirror_complete(BlockJob *job, Error **errp) > > return; > > } > > > > + /* check the target bs is not blocked and block all operations on it */ > > + if (s->replaces) { > > + s->to_replace = check_to_replace_node(s->replaces, errp); > > + > > This empty line looks unusual. > > > + if (!s->to_replace) { > > + return; > > + } > > So here is the thing that I really wanted to comment on. In the case of > a REPLACE blocker being set, this is a silent failure. Why would it be silent ? errp is directly passed to check_to_replace_node. > The completion > command will return success, but s->should_complete won't actually be > set, so the completion doesn't happen. The only thing that actually > happens is the bdrv_open_backing_file(s->target) (which looks somewhat > questionable, too...) > > Now I would expect that the REPLACE blocker is actually set for any > backing file, because that is what bdrv_set_backing_hd() does. For > quorum it does work as expected because quorum children don't get any > backing_blocker (we need to check whether they should get something > similar from the quorum BDS), so this is probably why it escaped your > testing. We'll need a test case that tries replacing some ordinary > backing file. > > Now I think the (accidental?) restriction to only replacing quorum nodes > actually makes this patch pretty safe, so maybe it would be nice to keep > this behaviour; but we need to fix it to not fail silently but return an > explicit error. > > > + error_setg(&s->replace_blocker, > > + "block device is in use by block-job-complete"); > > + bdrv_op_block_all(s->to_replace, s->replace_blocker); > > + bdrv_ref(s->to_replace); > > + } > > + > > s->should_complete = true; > > block_job_resume(job); > > } > > @@ -562,14 +592,15 @@ static const BlockJobDriver commit_active_job_driver = { > > }; > > > > static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, > > - int64_t speed, int64_t granularity, > > - int64_t buf_size, > > - BlockdevOnError on_source_error, > > - BlockdevOnError on_target_error, > > - BlockDriverCompletionFunc *cb, > > - void *opaque, Error **errp, > > - const BlockJobDriver *driver, > > - bool is_none_mode, BlockDriverState *base) > > + const char *replaces, > > + int64_t speed, int64_t granularity, > > + int64_t buf_size, > > + BlockdevOnError on_source_error, > > + BlockdevOnError on_target_error, > > + BlockDriverCompletionFunc *cb, > > + void *opaque, Error **errp, > > + const BlockJobDriver *driver, > > + bool is_none_mode, BlockDriverState *base) > > { > > MirrorBlockJob *s; > > > > @@ -600,6 +631,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, > > return; > > } > > > > + s->replaces = g_strdup(replaces); > > s->on_source_error = on_source_error; > > s->on_target_error = on_target_error; > > s->target = target; > > One design question that isn't quite clear to me yet is why you resolve > the device name only in mirror_complete() and not here. This means that > the drive-mirror QMP command can refer to one BDS with node-name foo, > which then gets removed and another BDS with node-name foo is added, and > then it would refer to the new BDS on completion time. > > I would find it less surprising if we took a reference to the old BDS > here so that you can't remove it. Perhaps setting the replace_blocker > here already would be safer, too. > > > @@ -621,6 +653,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, > > } > > > > void mirror_start(BlockDriverState *bs, BlockDriverState *target, > > + const char *replaces, > > int64_t speed, int64_t granularity, int64_t buf_size, > > MirrorSyncMode mode, BlockdevOnError on_source_error, > > BlockdevOnError on_target_error, > > @@ -632,7 +665,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, > > > > is_none_mode = mode == MIRROR_SYNC_MODE_NONE; > > base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL; > > - mirror_start_job(bs, target, speed, granularity, buf_size, > > + mirror_start_job(bs, target, replaces, > > + speed, granularity, buf_size, > > on_source_error, on_target_error, cb, opaque, errp, > > &mirror_job_driver, is_none_mode, base); > > } > > @@ -680,7 +714,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, > > } > > > > bdrv_ref(base); > > - mirror_start_job(bs, base, speed, 0, 0, > > + mirror_start_job(bs, base, NULL, speed, 0, 0, > > on_error, on_error, cb, opaque, &local_err, > > &commit_active_job_driver, false, base); > > if (local_err) { > > diff --git a/blockdev.c b/blockdev.c > > index 06b14f2..237a548 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -2107,6 +2107,7 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp) > > void qmp_drive_mirror(const char *device, const char *target, > > bool has_format, const char *format, > > bool has_node_name, const char *node_name, > > + bool has_replaces, const char *replaces, > > enum MirrorSyncMode sync, > > bool has_mode, enum NewImageMode mode, > > bool has_speed, int64_t speed, > > @@ -2194,6 +2195,28 @@ void qmp_drive_mirror(const char *device, const char *target, > > return; > > } > > > > + if (has_replaces) { > > + BlockDriverState *to_replace_bs; > > + > > + if (!has_node_name) { > > + error_setg(errp, "a node-name must be provided when replacing a" > > + " named node of the graph"); > > + return; > > + } > > + > > + to_replace_bs = check_to_replace_node(replaces, errp); > > + > > + if (!to_replace_bs) { > > + return; > > + } > > + > > + if (size != bdrv_getlength(to_replace_bs)) { > > + error_setg(errp, "cannot replace image with a mirror image of " > > + "different size"); > > + return; > > + } > > We may want to loosen some of these restrictions later, but it's good to > start with more restrictions if in doubt. > > > + } > > + > > if ((sync == MIRROR_SYNC_MODE_FULL || !source) > > && mode != NEW_IMAGE_MODE_EXISTING) > > { > > Kevin
Am 27.06.2014 um 14:53 hat Benoît Canet geschrieben: > The Friday 27 Jun 2014 à 13:57:02 (+0200), Kevin Wolf wrote : > > Am 16.06.2014 um 12:00 hat Benoît Canet geschrieben: > > > drive-mirror will bdrv_swap the new BDS named node-name with the one > > > pointed by replaces when the mirroring is finished. > > > > > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > > --- > > > block.c | 17 ++++++++++++++ > > > block/mirror.c | 60 +++++++++++++++++++++++++++++++++++++---------- > > > blockdev.c | 30 +++++++++++++++++++++++- > > > hmp.c | 2 +- > > > include/block/block.h | 4 ++++ > > > include/block/block_int.h | 3 +++ > > > qapi/block-core.json | 6 ++++- > > > qmp-commands.hx | 4 +++- > > > 8 files changed, 109 insertions(+), 17 deletions(-) > > > > > > diff --git a/block.c b/block.c > > > index 17f763d..318f1e6 100644 > > > --- a/block.c > > > +++ b/block.c > > > @@ -5795,3 +5795,20 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate) > > > > > > return false; > > > } > > > + > > > +BlockDriverState *check_to_replace_node(const char *node_name, Error **errp) > > > +{ > > > + BlockDriverState *to_replace_bs = bdrv_find_node(node_name); > > > + if (!to_replace_bs) { > > > + error_setg(errp, "Node name '%s' not found", > > > + node_name); > > > > Unnecessary line break. > > > > > + return NULL; > > > + } > > > + > > > + if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, errp)) { > > > + return NULL; > > > + } > > > + > > > + return to_replace_bs; > > > +} > > > + > > > > Empty line before EOF. > > > > > diff --git a/block/mirror.c b/block/mirror.c > > > index 94c8661..151167e 100644 > > > --- a/block/mirror.c > > > +++ b/block/mirror.c > > > @@ -32,6 +32,12 @@ typedef struct MirrorBlockJob { > > > RateLimit limit; > > > BlockDriverState *target; > > > BlockDriverState *base; > > > + /* The name of the graph node to replace */ > > > + char *replaces; > > > + /* The BDS to replace */ > > > + BlockDriverState *to_replace; > > > + /* Used to block operations on the drive-mirror-replace target */ > > > + Error *replace_blocker; > > > bool is_none_mode; > > > BlockdevOnError on_source_error, on_target_error; > > > bool synced; > > > @@ -490,10 +496,14 @@ immediate_exit: > > > bdrv_release_dirty_bitmap(bs, s->dirty_bitmap); > > > bdrv_iostatus_disable(s->target); > > > if (s->should_complete && ret == 0) { > > > - if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) { > > > - bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL); > > > + BlockDriverState *to_replace = s->common.bs; > > > + if (s->to_replace) { > > > + to_replace = s->to_replace; > > > } > > > - bdrv_swap(s->target, s->common.bs); > > > + if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) { > > > + bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL); > > > + } > > > + bdrv_swap(s->target, to_replace); > > > if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) { > > > /* drop the bs loop chain formed by the swap: break the loop then > > > * trigger the unref from the top one */ > > > @@ -502,6 +512,12 @@ immediate_exit: > > > bdrv_unref(p); > > > } > > > } > > > + if (s->to_replace) { > > > + bdrv_op_unblock_all(s->to_replace, s->replace_blocker); > > > + error_free(s->replace_blocker); > > > + bdrv_unref(s->to_replace); > > > + } > > > + g_free(s->replaces); > > > bdrv_unref(s->target); > > > block_job_completed(&s->common, ret); > > > } > > > @@ -540,6 +556,20 @@ static void mirror_complete(BlockJob *job, Error **errp) > > > return; > > > } > > > > > > + /* check the target bs is not blocked and block all operations on it */ > > > + if (s->replaces) { > > > + s->to_replace = check_to_replace_node(s->replaces, errp); > > > + > > > > This empty line looks unusual. > > > > > + if (!s->to_replace) { > > > + return; > > > + } > > > > So here is the thing that I really wanted to comment on. In the case of > > a REPLACE blocker being set, this is a silent failure. > > Why would it be silent ? > errp is directly passed to check_to_replace_node. Ah, sorry, my bad. I missed that bdrv_op_is_blocked() sets errp. Which is a bit odd for a function with this name. I had expected that it simply returns true if the op is blocked and sets an error only if it can't answer the question. Not a problem of your patch, though. Kevin > > The completion > > command will return success, but s->should_complete won't actually be > > set, so the completion doesn't happen. The only thing that actually > > happens is the bdrv_open_backing_file(s->target) (which looks somewhat > > questionable, too...) > > > > Now I would expect that the REPLACE blocker is actually set for any > > backing file, because that is what bdrv_set_backing_hd() does. For > > quorum it does work as expected because quorum children don't get any > > backing_blocker (we need to check whether they should get something > > similar from the quorum BDS), so this is probably why it escaped your > > testing. We'll need a test case that tries replacing some ordinary > > backing file. > > > > Now I think the (accidental?) restriction to only replacing quorum nodes > > actually makes this patch pretty safe, so maybe it would be nice to keep > > this behaviour; but we need to fix it to not fail silently but return an > > explicit error.
The Friday 27 Jun 2014 à 15:37:00 (+0200), Kevin Wolf wrote : > Am 27.06.2014 um 14:53 hat Benoît Canet geschrieben: > > The Friday 27 Jun 2014 à 13:57:02 (+0200), Kevin Wolf wrote : > > > Am 16.06.2014 um 12:00 hat Benoît Canet geschrieben: > > > > drive-mirror will bdrv_swap the new BDS named node-name with the one > > > > pointed by replaces when the mirroring is finished. > > > > > > > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > > > --- > > > > block.c | 17 ++++++++++++++ > > > > block/mirror.c | 60 +++++++++++++++++++++++++++++++++++++---------- > > > > blockdev.c | 30 +++++++++++++++++++++++- > > > > hmp.c | 2 +- > > > > include/block/block.h | 4 ++++ > > > > include/block/block_int.h | 3 +++ > > > > qapi/block-core.json | 6 ++++- > > > > qmp-commands.hx | 4 +++- > > > > 8 files changed, 109 insertions(+), 17 deletions(-) > > > > > > > > diff --git a/block.c b/block.c > > > > index 17f763d..318f1e6 100644 > > > > --- a/block.c > > > > +++ b/block.c > > > > @@ -5795,3 +5795,20 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate) > > > > > > > > return false; > > > > } > > > > + > > > > +BlockDriverState *check_to_replace_node(const char *node_name, Error **errp) > > > > +{ > > > > + BlockDriverState *to_replace_bs = bdrv_find_node(node_name); > > > > + if (!to_replace_bs) { > > > > + error_setg(errp, "Node name '%s' not found", > > > > + node_name); > > > > > > Unnecessary line break. > > > > > > > + return NULL; > > > > + } > > > > + > > > > + if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, errp)) { > > > > + return NULL; > > > > + } > > > > + > > > > + return to_replace_bs; > > > > +} > > > > + > > > > > > Empty line before EOF. > > > > > > > diff --git a/block/mirror.c b/block/mirror.c > > > > index 94c8661..151167e 100644 > > > > --- a/block/mirror.c > > > > +++ b/block/mirror.c > > > > @@ -32,6 +32,12 @@ typedef struct MirrorBlockJob { > > > > RateLimit limit; > > > > BlockDriverState *target; > > > > BlockDriverState *base; > > > > + /* The name of the graph node to replace */ > > > > + char *replaces; > > > > + /* The BDS to replace */ > > > > + BlockDriverState *to_replace; > > > > + /* Used to block operations on the drive-mirror-replace target */ > > > > + Error *replace_blocker; > > > > bool is_none_mode; > > > > BlockdevOnError on_source_error, on_target_error; > > > > bool synced; > > > > @@ -490,10 +496,14 @@ immediate_exit: > > > > bdrv_release_dirty_bitmap(bs, s->dirty_bitmap); > > > > bdrv_iostatus_disable(s->target); > > > > if (s->should_complete && ret == 0) { > > > > - if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) { > > > > - bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL); > > > > + BlockDriverState *to_replace = s->common.bs; > > > > + if (s->to_replace) { > > > > + to_replace = s->to_replace; > > > > } > > > > - bdrv_swap(s->target, s->common.bs); > > > > + if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) { > > > > + bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL); > > > > + } > > > > + bdrv_swap(s->target, to_replace); > > > > if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) { > > > > /* drop the bs loop chain formed by the swap: break the loop then > > > > * trigger the unref from the top one */ > > > > @@ -502,6 +512,12 @@ immediate_exit: > > > > bdrv_unref(p); > > > > } > > > > } > > > > + if (s->to_replace) { > > > > + bdrv_op_unblock_all(s->to_replace, s->replace_blocker); > > > > + error_free(s->replace_blocker); > > > > + bdrv_unref(s->to_replace); > > > > + } > > > > + g_free(s->replaces); > > > > bdrv_unref(s->target); > > > > block_job_completed(&s->common, ret); > > > > } > > > > @@ -540,6 +556,20 @@ static void mirror_complete(BlockJob *job, Error **errp) > > > > return; > > > > } > > > > > > > > + /* check the target bs is not blocked and block all operations on it */ > > > > + if (s->replaces) { > > > > + s->to_replace = check_to_replace_node(s->replaces, errp); > > > > + > > > > > > This empty line looks unusual. > > > > > > > + if (!s->to_replace) { > > > > + return; > > > > + } > > > > > > So here is the thing that I really wanted to comment on. In the case of > > > a REPLACE blocker being set, this is a silent failure. > > > > Why would it be silent ? > > errp is directly passed to check_to_replace_node. > > Ah, sorry, my bad. I missed that bdrv_op_is_blocked() sets errp. Which > is a bit odd for a function with this name. I had expected that it > simply returns true if the op is blocked and sets an error only if it > can't answer the question. I passed &local_err and propagated it to errp to make it clearer in the new version. Best regards Benoît > > Not a problem of your patch, though. > > Kevin > > > > The completion > > > command will return success, but s->should_complete won't actually be > > > set, so the completion doesn't happen. The only thing that actually > > > happens is the bdrv_open_backing_file(s->target) (which looks somewhat > > > questionable, too...) > > > > > > Now I would expect that the REPLACE blocker is actually set for any > > > backing file, because that is what bdrv_set_backing_hd() does. For > > > quorum it does work as expected because quorum children don't get any > > > backing_blocker (we need to check whether they should get something > > > similar from the quorum BDS), so this is probably why it escaped your > > > testing. We'll need a test case that tries replacing some ordinary > > > backing file. > > > > > > Now I think the (accidental?) restriction to only replacing quorum nodes > > > actually makes this patch pretty safe, so maybe it would be nice to keep > > > this behaviour; but we need to fix it to not fail silently but return an > > > explicit error. >
diff --git a/block.c b/block.c index 17f763d..318f1e6 100644 --- a/block.c +++ b/block.c @@ -5795,3 +5795,20 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate) return false; } + +BlockDriverState *check_to_replace_node(const char *node_name, Error **errp) +{ + BlockDriverState *to_replace_bs = bdrv_find_node(node_name); + if (!to_replace_bs) { + error_setg(errp, "Node name '%s' not found", + node_name); + return NULL; + } + + if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, errp)) { + return NULL; + } + + return to_replace_bs; +} + diff --git a/block/mirror.c b/block/mirror.c index 94c8661..151167e 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -32,6 +32,12 @@ typedef struct MirrorBlockJob { RateLimit limit; BlockDriverState *target; BlockDriverState *base; + /* The name of the graph node to replace */ + char *replaces; + /* The BDS to replace */ + BlockDriverState *to_replace; + /* Used to block operations on the drive-mirror-replace target */ + Error *replace_blocker; bool is_none_mode; BlockdevOnError on_source_error, on_target_error; bool synced; @@ -490,10 +496,14 @@ immediate_exit: bdrv_release_dirty_bitmap(bs, s->dirty_bitmap); bdrv_iostatus_disable(s->target); if (s->should_complete && ret == 0) { - if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) { - bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL); + BlockDriverState *to_replace = s->common.bs; + if (s->to_replace) { + to_replace = s->to_replace; } - bdrv_swap(s->target, s->common.bs); + if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) { + bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL); + } + bdrv_swap(s->target, to_replace); if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) { /* drop the bs loop chain formed by the swap: break the loop then * trigger the unref from the top one */ @@ -502,6 +512,12 @@ immediate_exit: bdrv_unref(p); } } + if (s->to_replace) { + bdrv_op_unblock_all(s->to_replace, s->replace_blocker); + error_free(s->replace_blocker); + bdrv_unref(s->to_replace); + } + g_free(s->replaces); bdrv_unref(s->target); block_job_completed(&s->common, ret); } @@ -540,6 +556,20 @@ static void mirror_complete(BlockJob *job, Error **errp) return; } + /* check the target bs is not blocked and block all operations on it */ + if (s->replaces) { + s->to_replace = check_to_replace_node(s->replaces, errp); + + if (!s->to_replace) { + return; + } + + error_setg(&s->replace_blocker, + "block device is in use by block-job-complete"); + bdrv_op_block_all(s->to_replace, s->replace_blocker); + bdrv_ref(s->to_replace); + } + s->should_complete = true; block_job_resume(job); } @@ -562,14 +592,15 @@ static const BlockJobDriver commit_active_job_driver = { }; static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, - int64_t speed, int64_t granularity, - int64_t buf_size, - BlockdevOnError on_source_error, - BlockdevOnError on_target_error, - BlockDriverCompletionFunc *cb, - void *opaque, Error **errp, - const BlockJobDriver *driver, - bool is_none_mode, BlockDriverState *base) + const char *replaces, + int64_t speed, int64_t granularity, + int64_t buf_size, + BlockdevOnError on_source_error, + BlockdevOnError on_target_error, + BlockDriverCompletionFunc *cb, + void *opaque, Error **errp, + const BlockJobDriver *driver, + bool is_none_mode, BlockDriverState *base) { MirrorBlockJob *s; @@ -600,6 +631,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, return; } + s->replaces = g_strdup(replaces); s->on_source_error = on_source_error; s->on_target_error = on_target_error; s->target = target; @@ -621,6 +653,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, } void mirror_start(BlockDriverState *bs, BlockDriverState *target, + const char *replaces, int64_t speed, int64_t granularity, int64_t buf_size, MirrorSyncMode mode, BlockdevOnError on_source_error, BlockdevOnError on_target_error, @@ -632,7 +665,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, is_none_mode = mode == MIRROR_SYNC_MODE_NONE; base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL; - mirror_start_job(bs, target, speed, granularity, buf_size, + mirror_start_job(bs, target, replaces, + speed, granularity, buf_size, on_source_error, on_target_error, cb, opaque, errp, &mirror_job_driver, is_none_mode, base); } @@ -680,7 +714,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, } bdrv_ref(base); - mirror_start_job(bs, base, speed, 0, 0, + mirror_start_job(bs, base, NULL, speed, 0, 0, on_error, on_error, cb, opaque, &local_err, &commit_active_job_driver, false, base); if (local_err) { diff --git a/blockdev.c b/blockdev.c index 06b14f2..237a548 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2107,6 +2107,7 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp) void qmp_drive_mirror(const char *device, const char *target, bool has_format, const char *format, bool has_node_name, const char *node_name, + bool has_replaces, const char *replaces, enum MirrorSyncMode sync, bool has_mode, enum NewImageMode mode, bool has_speed, int64_t speed, @@ -2194,6 +2195,28 @@ void qmp_drive_mirror(const char *device, const char *target, return; } + if (has_replaces) { + BlockDriverState *to_replace_bs; + + if (!has_node_name) { + error_setg(errp, "a node-name must be provided when replacing a" + " named node of the graph"); + return; + } + + to_replace_bs = check_to_replace_node(replaces, errp); + + if (!to_replace_bs) { + return; + } + + if (size != bdrv_getlength(to_replace_bs)) { + error_setg(errp, "cannot replace image with a mirror image of " + "different size"); + return; + } + } + if ((sync == MIRROR_SYNC_MODE_FULL || !source) && mode != NEW_IMAGE_MODE_EXISTING) { @@ -2238,7 +2261,12 @@ void qmp_drive_mirror(const char *device, const char *target, return; } - mirror_start(bs, target_bs, speed, granularity, buf_size, sync, + /* pass the node name to replace to mirror start since it's loose coupling + * and will allow to check whether the node still exist at mirror completion + */ + mirror_start(bs, target_bs, + has_replaces ? replaces : NULL, + speed, granularity, buf_size, sync, on_source_error, on_target_error, block_job_cb, bs, &local_err); if (local_err != NULL) { diff --git a/hmp.c b/hmp.c index ef0d583..9a4b8da 100644 --- a/hmp.c +++ b/hmp.c @@ -930,7 +930,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict) } qmp_drive_mirror(device, filename, !!format, format, - false, NULL, + false, NULL, false, NULL, full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP, true, mode, false, 0, false, 0, false, 0, false, 0, false, 0, &err); diff --git a/include/block/block.h b/include/block/block.h index 7d86e29..b0ed701 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -179,6 +179,7 @@ typedef enum BlockOpType { BLOCK_OP_TYPE_MIRROR, BLOCK_OP_TYPE_RESIZE, BLOCK_OP_TYPE_STREAM, + BLOCK_OP_TYPE_REPLACE, BLOCK_OP_TYPE_MAX, } BlockOpType; @@ -319,6 +320,9 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs, BlockDriverState *candidate); bool bdrv_is_first_non_filter(BlockDriverState *candidate); +/* check if a named node can be replaced when doing drive-mirror */ +BlockDriverState *check_to_replace_node(const char *node_name, Error **errp); + /* async block I/O */ typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t sector, int sector_num); diff --git a/include/block/block_int.h b/include/block/block_int.h index 8d58334..81ce2ee 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -492,6 +492,8 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, * mirror_start: * @bs: Block device to operate on. * @target: Block device to write to. + * @replaces: Block graph node name to replace once the mirror is done. Can + * only be used when full mirroring is selected. * @speed: The maximum speed, in bytes per second, or 0 for unlimited. * @granularity: The chosen granularity for the dirty bitmap. * @buf_size: The amount of data that can be in flight at one time. @@ -508,6 +510,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, * @bs will be switched to read from @target. */ void mirror_start(BlockDriverState *bs, BlockDriverState *target, + const char *replaces, int64_t speed, int64_t granularity, int64_t buf_size, MirrorSyncMode mode, BlockdevOnError on_source_error, BlockdevOnError on_target_error, diff --git a/qapi/block-core.json b/qapi/block-core.json index 4c28d7b..0f8e703 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -772,6 +772,10 @@ # @node-name: #optional the new block driver state node name in the graph # (Since 2.1) # +# @replaces: #optional with sync=full graph node name to be replaced by the new +# image when a whole image copy is done. This can be used to repair +# broken Quorum files. (Since 2.1) +# # @mode: #optional whether and how QEMU should create a new image, default is # 'absolute-paths'. # @@ -804,7 +808,7 @@ ## { 'command': 'drive-mirror', 'data': { 'device': 'str', 'target': 'str', '*format': 'str', - '*node-name': 'str', + '*node-name': 'str', '*replaces': 'str', 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', '*speed': 'int', '*granularity': 'uint32', '*buf-size': 'int', '*on-source-error': 'BlockdevOnError', diff --git a/qmp-commands.hx b/qmp-commands.hx index 98c28f5..4a76054 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1293,7 +1293,7 @@ EQMP { .name = "drive-mirror", .args_type = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?," - "node-name:s?," + "node-name:s?,replaces:s?," "on-source-error:s?,on-target-error:s?," "granularity:i?,buf-size:i?", .mhandler.cmd_new = qmp_marshal_input_drive_mirror, @@ -1317,6 +1317,8 @@ Arguments: - "format": format of new image (json-string, optional) - "node-name": the name of the new block driver state in the node graph (json-string, optional) +- "replaces": the block driver node name to replace when finished + (json-string, optional) - "mode": how an image file should be created into the target file/device (NewImageMode, optional, default 'absolute-paths') - "speed": maximum speed of the streaming job, in bytes per second