From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33657) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0XBF-0000fL-LJ for qemu-devel@nongnu.org; Fri, 27 Jun 2014 10:31:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X0XB9-0005FL-LX for qemu-devel@nongnu.org; Fri, 27 Jun 2014 10:31:09 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:45474 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0XB9-0005F8-Cj for qemu-devel@nongnu.org; Fri, 27 Jun 2014 10:31:03 -0400 Date: Fri, 27 Jun 2014 16:30:55 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140627143054.GA6608@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> <20140627125357.GB21497@irqsave.net> <20140627133700.GG5223@noname.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140627133700.GG5223@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 15:37:00 (+0200), Kevin Wolf wrote : > Am 27.06.2014 um 14:53 hat Beno=EEt Canet geschrieben: > > 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(BlockDriverS= tate *candidate) > > > > =20 > > > > return false; > > > > } > > > > + > > > > +BlockDriverState *check_to_replace_node(const char *node_name, E= rror **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,= errp)) { > > > > + 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 targ= et */ > > > > + 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->com= mon.bs)) { > > > > - bdrv_reopen(s->target, bdrv_get_flags(s->common.bs),= NULL); > > > > + 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_rep= lace)) { > > > > + bdrv_reopen(s->target, bdrv_get_flags(to_replace), N= ULL); > > > > + } > > > > + bdrv_swap(s->target, to_replace); > > > > if (s->common.driver->job_type =3D=3D BLOCK_JOB_TYPE_COM= MIT) { > > > > /* 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, E= rror **errp) > > > > return; > > > > } > > > > =20 > > > > + /* check the target bs is not blocked and block all operatio= ns on it */ > > > > + if (s->replaces) { > > > > + s->to_replace =3D check_to_replace_node(s->replaces, err= p); > > > > + > > >=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 cas= e of > > > a REPLACE blocker being set, this is a silent failure. > >=20 > > Why would it be silent ? > > errp is directly passed to check_to_replace_node. >=20 > 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=EEt >=20 > Not a problem of your patch, though. >=20 > Kevin >=20 > > > 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 somew= hat > > > 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 a= ny > > > backing_blocker (we need to check whether they should get something > > > similar from the quorum BDS), so this is probably why it escaped yo= ur > > > 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 = 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 retu= rn an > > > explicit error. >=20