From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37052) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0VfS-0005BP-Io for qemu-devel@nongnu.org; Fri, 27 Jun 2014 08:54:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X0VfL-0004Jk-RJ for qemu-devel@nongnu.org; Fri, 27 Jun 2014 08:54:14 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:45158 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0VfL-0004Jc-Dl for qemu-devel@nongnu.org; Fri, 27 Jun 2014 08:54:07 -0400 Date: Fri, 27 Jun 2014 14:53:58 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140627125357.GB21497@irqsave.net> References: <1402912857-10509-1-git-send-email-benoit.canet@irqsave.net> <1402912857-10509-4-git-send-email-benoit.canet@irqsave.net> <20140627115702.GD5223@noname.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140627115702.GD5223@noname.redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v10 3/4] block: Add replaces argument to drive-mirror List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , qemu-devel@nongnu.org, stefanha@redhat.com, mreitz@redhat.com The Friday 27 Jun 2014 =E0 13:57:02 (+0200), Kevin Wolf wrote : > Am 16.06.2014 um 12:00 hat Beno=EEt Canet geschrieben: > > drive-mirror will bdrv_swap the new BDS named node-name with the one > > pointed by replaces when the mirroring is finished. > >=20 > > Signed-off-by: Benoit Canet > > Reviewed-by: Max Reitz > > --- > > 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(-) > >=20 > > 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) > > =20 > > return false; > > } > > + > > +BlockDriverState *check_to_replace_node(const char *node_name, Error= **errp) > > +{ > > + BlockDriverState *to_replace_bs =3D bdrv_find_node(node_name); > > + if (!to_replace_bs) { > > + error_setg(errp, "Node name '%s' not found", > > + node_name); >=20 > Unnecessary line break. >=20 > > + return NULL; > > + } > > + > > + if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, err= p)) { > > + return NULL; > > + } > > + > > + return to_replace_bs; > > +} > > + >=20 > Empty line before EOF. >=20 > > 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 =3D=3D 0) { > > - if (bdrv_get_flags(s->target) !=3D bdrv_get_flags(s->common.= bs)) { > > - bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NUL= L); > > + BlockDriverState *to_replace =3D s->common.bs; > > + if (s->to_replace) { > > + to_replace =3D s->to_replace; > > } > > - bdrv_swap(s->target, s->common.bs); > > + if (bdrv_get_flags(s->target) !=3D 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 =3D=3D 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; > > } > > =20 > > + /* check the target bs is not blocked and block all operations o= n it */ > > + if (s->replaces) { > > + s->to_replace =3D check_to_replace_node(s->replaces, errp); > > + >=20 > This empty line looks unusual. >=20 > > + if (!s->to_replace) { > > + return; > > + } >=20 > 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...) >=20 > 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. >=20 > Now I think the (accidental?) restriction to only replacing quorum node= s > actually makes this patch pretty safe, so maybe it would be nice to kee= p > this behaviour; but we need to fix it to not fail silently but return a= n > explicit error. >=20 > > + 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 =3D true; > > block_job_resume(job); > > } > > @@ -562,14 +592,15 @@ static const BlockJobDriver commit_active_job_d= river =3D { > > }; > > =20 > > 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 *bas= e) > > + 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 *ba= se) > > { > > MirrorBlockJob *s; > > =20 > > @@ -600,6 +631,7 @@ static void mirror_start_job(BlockDriverState *bs= , BlockDriverState *target, > > return; > > } > > =20 > > + s->replaces =3D g_strdup(replaces); > > s->on_source_error =3D on_source_error; > > s->on_target_error =3D on_target_error; > > s->target =3D target; >=20 > 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, an= d > then it would refer to the new BDS on completion time. >=20 > 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. >=20 > > @@ -621,6 +653,7 @@ static void mirror_start_job(BlockDriverState *bs= , BlockDriverState *target, > > } > > =20 > > void mirror_start(BlockDriverState *bs, BlockDriverState *target, > > + const char *replaces, > > int64_t speed, int64_t granularity, int64_t buf_si= ze, > > MirrorSyncMode mode, BlockdevOnError on_source_err= or, > > BlockdevOnError on_target_error, > > @@ -632,7 +665,8 @@ void mirror_start(BlockDriverState *bs, BlockDriv= erState *target, > > =20 > > is_none_mode =3D mode =3D=3D MIRROR_SYNC_MODE_NONE; > > base =3D mode =3D=3D MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NUL= L; > > - 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, e= rrp, > > &mirror_job_driver, is_none_mode, base); > > } > > @@ -680,7 +714,7 @@ void commit_active_start(BlockDriverState *bs, Bl= ockDriverState *base, > > } > > =20 > > 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_node= s(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, cons= t char *target, > > return; > > } > > =20 > > + if (has_replaces) { > > + BlockDriverState *to_replace_bs; > > + > > + if (!has_node_name) { > > + error_setg(errp, "a node-name must be provided when repl= acing a" > > + " named node of the graph"); > > + return; > > + } > > + > > + to_replace_bs =3D check_to_replace_node(replaces, errp); > > + > > + if (!to_replace_bs) { > > + return; > > + } > > + > > + if (size !=3D bdrv_getlength(to_replace_bs)) { > > + error_setg(errp, "cannot replace image with a mirror ima= ge of " > > + "different size"); > > + return; > > + } >=20 > We may want to loosen some of these restrictions later, but it's good t= o > start with more restrictions if in doubt. >=20 > > + } > > + > > if ((sync =3D=3D MIRROR_SYNC_MODE_FULL || !source) > > && mode !=3D NEW_IMAGE_MODE_EXISTING) > > { >=20 > Kevin