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 <ben...@irqsave.net>
> > Reviewed-by: Max Reitz <mre...@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
