From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44266) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRK2e-0002rj-2u for qemu-devel@nongnu.org; Tue, 09 Sep 2014 07:57:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XRK2Y-0004T2-5F for qemu-devel@nongnu.org; Tue, 09 Sep 2014 07:57:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:12705) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRK2X-0004Sq-Qk for qemu-devel@nongnu.org; Tue, 09 Sep 2014 07:56:54 -0400 Date: Tue, 9 Sep 2014 13:56:46 +0200 From: Kevin Wolf Message-ID: <20140909115646.GI4847@noname.str.redhat.com> References: <1408723870-7826-1-git-send-email-benoit.canet@nodalink.com> <1408723870-7826-2-git-send-email-benoit.canet@nodalink.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1408723870-7826-2-git-send-email-benoit.canet@nodalink.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] block: Make op blockers recursive List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: jcody@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com Am 22.08.2014 um 18:11 hat Beno=EEt Canet geschrieben: > Since the block layer code is starting to modify the BDS graph right in= the > middle of BDS chains (block-mirror's replace parameter for example) QEM= U needs > to properly block and unblock whole BDS subtrees; recursion is a neat w= ay to > achieve this task. >=20 > This patch also takes care of modifying the op blockers users. >=20 > Signed-off-by: Benoit Canet > --- > block.c | 69 +++++++++++++++++++++++++++++++++++++++= +++++--- > block/blkverify.c | 21 +++++++++++++++ > block/commit.c | 3 +++ > block/mirror.c | 17 ++++++++---- > block/quorum.c | 25 +++++++++++++++++ > block/stream.c | 3 +++ > block/vmdk.c | 34 +++++++++++++++++++++++ > include/block/block.h | 2 +- > include/block/block_int.h | 6 +++++ > 9 files changed, 171 insertions(+), 9 deletions(-) >=20 > diff --git a/block.c b/block.c > index 6fa0201..d964b6c 100644 > --- a/block.c > +++ b/block.c > @@ -5446,7 +5446,9 @@ bool bdrv_op_is_blocked(BlockDriverState *bs, Blo= ckOpType op, Error **errp) > return false; > } > =20 > -void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason= ) > +/* do the real work of blocking a BDS */ > +static void bdrv_do_op_block(BlockDriverState *bs, BlockOpType op, > + Error *reason) > { > BdrvOpBlocker *blocker; > assert((int) op >=3D 0 && op < BLOCK_OP_TYPE_MAX); > @@ -5456,7 +5458,9 @@ void bdrv_op_block(BlockDriverState *bs, BlockOpT= ype op, Error *reason) > QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list); > } > =20 > -void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reas= on) > +/* do the real work of unblocking a BDS */ > +static void bdrv_do_op_unblock(BlockDriverState *bs, BlockOpType op, > + Error *reason) > { > BdrvOpBlocker *blocker, *next; > assert((int) op >=3D 0 && op < BLOCK_OP_TYPE_MAX); > @@ -5468,6 +5472,65 @@ void bdrv_op_unblock(BlockDriverState *bs, Block= OpType op, Error *reason) > } > } > =20 > +static bool bdrv_op_is_blocked_by(BlockDriverState *bs, BlockOpType op= , > + Error *reason) > +{ > + BdrvOpBlocker *blocker, *next; > + assert((int) op >=3D 0 && op < BLOCK_OP_TYPE_MAX); > + QLIST_FOREACH_SAFE(blocker, &bs->op_blockers[op], list, next) { This doesn't actually need the SAFE version. > + if (blocker->reason =3D=3D reason) { > + return true; > + } > + } > + return false; > +} > + > +/* block recursively a BDS */ > +void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason= ) > +{ > + if (!bs) { > + return; > + } > + > + /* prevent recursion loop */ > + if (bdrv_op_is_blocked_by(bs, op, reason)) { > + return; > + } > + > + /* block first for recursion loop protection to work */ > + bdrv_do_op_block(bs, op, reason); > + > + bdrv_op_block(bs->file, op, reason); > + bdrv_op_block(bs->backing_hd, op, reason); > + > + if (bs->drv && bs->drv->bdrv_op_recursive_block) { > + bs->drv->bdrv_op_recursive_block(bs, op, reason); > + } Here you block bs->file/bs->backing_hd automatically, no matter whether the block driver implements the callback or not. I'm not sure if we can do it like this for all times, but for now that should be okay. > +} > + > +/* unblock recursively a BDS */ > +void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reas= on) > +{ > + if (!bs) { > + return; > + } > + > + /* prevent recursion loop */ > + if (!bdrv_op_is_blocked_by(bs, op, reason)) { > + return; > + } > + > + /* unblock first for recursion loop protection to work */ > + bdrv_do_op_unblock(bs, op, reason); > + > + bdrv_op_unblock(bs->file, op, reason); > + bdrv_op_unblock(bs->backing_hd, op, reason); > + > + if (bs->drv && bs->drv->bdrv_op_recursive_unblock) { > + bs->drv->bdrv_op_recursive_unblock(bs, op, reason); > + } > +} > + > void bdrv_op_block_all(BlockDriverState *bs, Error *reason) > { > int i; > @@ -5848,7 +5911,7 @@ BlockDriverState *check_to_replace_node(const cha= r *node_name, Error **errp) > return NULL; > } > =20 > - if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, errp)= ) { > + if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_MIRROR_REPLACE= , errp)) { That rename could have been a separate patch. > return NULL; > } > =20 > diff --git a/block/blkverify.c b/block/blkverify.c > index 621b785..75ec3df 100644 > --- a/block/blkverify.c > +++ b/block/blkverify.c > @@ -320,6 +320,24 @@ static void blkverify_attach_aio_context(BlockDriv= erState *bs, > bdrv_attach_aio_context(s->test_file, new_context); > } > =20 > +static void blkverify_op_recursive_block(BlockDriverState *bs, BlockOp= Type op, > + Error *reason) > +{ > + BDRVBlkverifyState *s =3D bs->opaque; > + > + bdrv_op_block(bs->file, op, reason); > + bdrv_op_block(s->test_file, op, reason); > +} > + > +static void blkverify_op_recursive_unblock(BlockDriverState *bs, Block= OpType op, > + Error *reason) > +{ > + BDRVBlkverifyState *s =3D bs->opaque; > + > + bdrv_op_unblock(bs->file, op, reason); > + bdrv_op_unblock(s->test_file, op, reason); > +} Here you block bs->file again, even though the block.c function has already done it. Nothing bad happens, because bdrv_op_block() simply stops when the blocker is already set, but the code would be nicer if we did block bs->file only in one place. (I'm leaning towards doing it in the drivers, but that's your decision.) > static BlockDriver bdrv_blkverify =3D { > .format_name =3D "blkverify", > .protocol_name =3D "blkverify", > @@ -337,6 +355,9 @@ static BlockDriver bdrv_blkverify =3D { > .bdrv_attach_aio_context =3D blkverify_attach_aio_context= , > .bdrv_detach_aio_context =3D blkverify_detach_aio_context= , > =20 > + .bdrv_op_recursive_block =3D blkverify_op_recursive_block= , > + .bdrv_op_recursive_unblock =3D blkverify_op_recursive_unblo= ck, > + > .is_filter =3D true, > .bdrv_recurse_is_first_non_filter =3D blkverify_recurse_is_first_n= on_filter, > }; > diff --git a/block/commit.c b/block/commit.c > index 91517d3..8a122b7 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -142,6 +142,9 @@ wait: > =20 > if (!block_job_is_cancelled(&s->common) && sector_num =3D=3D end) = { > /* success */ > + /* unblock only BDS to be dropped */ > + bdrv_op_unblock_all(top, s->common.blocker); > + bdrv_op_block_all(base, s->common.blocker); This suggests that bdrv_op_(un)block_all() doesn't provide the right interface, but we rather want an optional base argument in it, so that you can (un)block a specific part of the backing file chain. > ret =3D bdrv_drop_intermediate(active, top, base, s->backing_f= ile_str); > } > =20 > diff --git a/block/mirror.c b/block/mirror.c > index 5e7a166..28ed47d 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -324,6 +324,7 @@ static void coroutine_fn mirror_run(void *opaque) > uint64_t last_pause_ns; > BlockDriverInfo bdi; > char backing_filename[1024]; > + BlockDriverState *to_replace; > int ret =3D 0; > int n; > =20 > @@ -512,14 +513,16 @@ immediate_exit: > g_free(s->in_flight_bitmap); > bdrv_release_dirty_bitmap(bs, s->dirty_bitmap); > bdrv_iostatus_disable(s->target); > + to_replace =3D s->common.bs; > + if (s->to_replace) { > + bdrv_op_unblock_all(s->to_replace, s->replace_blocker); > + to_replace =3D s->to_replace; > + } > if (s->should_complete && ret =3D=3D 0) { > - BlockDriverState *to_replace =3D s->common.bs; > - if (s->to_replace) { > - to_replace =3D s->to_replace; > - } > if (bdrv_get_flags(s->target) !=3D bdrv_get_flags(to_replace))= { > bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL); > } > + bdrv_op_unblock_all(to_replace, bs->job->blocker); You need to unblock all of the BDSes that are going to be removed. Aren't you unblocking more than that here? It's not a real problem because block_job_completed() would unblock the rest anyway and its bdrv_op_unblock_all() call is simply ignored now, but it would be nicer to just unblock here what's actually needed. > 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 lo= op then > @@ -530,7 +533,6 @@ immediate_exit: > } > } > if (s->to_replace) { > - bdrv_op_unblock_all(s->to_replace, s->replace_blocker); > error_free(s->replace_blocker); > bdrv_unref(s->to_replace); > } > @@ -648,6 +650,11 @@ static void mirror_start_job(BlockDriverState *bs,= BlockDriverState *target, > return; > } > =20 > + /* remove BLOCK_OP_TYPE_MIRROR_REPLACE from the list of > + * blocked operations so the replaces parameter can work > + */ > + bdrv_op_unblock(bs, BLOCK_OP_TYPE_MIRROR_REPLACE, bs->job->blocker= ); What purpose does a blocker serve when it's disabled before it is checked? I would only ever expect a bdrv_op_unblock() after some operation on the BDS has finished, but not when starting an operation that needs it. > s->replaces =3D g_strdup(replaces); > s->on_source_error =3D on_source_error; > s->on_target_error =3D on_target_error; > diff --git a/block/stream.c b/block/stream.c > index cdea3e8..2c917b7 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -192,6 +192,9 @@ wait: > } > } > ret =3D bdrv_change_backing_file(bs, base_id, base_fmt); > + /* unblock only BDS to be dropped */ > + bdrv_op_unblock_all(bs->backing_hd, s->common.blocker); > + bdrv_op_block_all(base, s->common.blocker); Same thing as for commit. > close_unused_images(bs, base, base_id); > } > =20 > diff --git a/block/vmdk.c b/block/vmdk.c > index 01412a8..bc5b17f 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -2220,6 +2220,38 @@ static QemuOptsList vmdk_create_opts =3D { > } > }; > =20 > +static void vmdk_op_recursive_block(BlockDriverState *bs, BlockOpType = op, > + Error *reason) > +{ > + BDRVVmdkState *s =3D bs->opaque; > + int i; > + > + bdrv_op_block(bs->file, op, reason); > + bdrv_op_block(bs->backing_hd, op, reason); Already blocked in block.c, remove one of the two > + for (i =3D 0; i < s->num_extents; i++) { > + if (s->extents[i].file) { > + bdrv_op_block(s->extents[i].file, op, reason); > + } > + } > +} Kevin