From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38473) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XLqEF-0006vp-1W for qemu-devel@nongnu.org; Mon, 25 Aug 2014 05:06:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XLqE9-0007uI-Qc for qemu-devel@nongnu.org; Mon, 25 Aug 2014 05:06:18 -0400 Received: from dew.nodalink.com ([95.130.14.197]:40517) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XLqE9-0007u3-DS for qemu-devel@nongnu.org; Mon, 25 Aug 2014 05:06:13 -0400 Date: Mon, 25 Aug 2014 09:06:11 +0000 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140825090611.GA18202@nodalink.com> References: <1408723870-7826-1-git-send-email-benoit.canet@nodalink.com> <1408723870-7826-2-git-send-email-benoit.canet@nodalink.com> <20140825060424.GA17482@T430.nay.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140825060424.GA17482@T430.nay.redhat.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: Fam Zheng Cc: kwolf@redhat.com, jcody@redhat.com, stefanha@redhat.com, qemu-devel@nongnu.org, =?iso-8859-1?Q?Beno=EEt?= Canet On Mon, Aug 25, 2014 at 02:04:24PM +0800, Fam Zheng wrote: > On Fri, 08/22 18:11, Beno=EEt Canet wrote: > > 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) Q= EMU needs > > to properly block and unblock whole BDS subtrees; recursion is a neat= way to > > achieve this task. > >=20 > > This patch also takes care of modifying the op blockers users. >=20 > Is this going to replace backing_blocker? >=20 > I think it is too general an approach to control the operation properly= , > because the op blocker may not work in the same way for all types of BD= S > connections. In other words, the choosing of op blockers are likely > conditional on graph edge types, that's why backing_blocker was added h= ere. For > example, A VMDK extent connection will probably need a different set of > blockers than bs->file connection. >=20 > So could you explain in which cases is the recursive blocking/unblockin= g > useful? It's designed for the new crop of block operations operating on BDS locat= ed in the middle of the backing chain: Jeff's patches, intermediate live stream= ing or intermediate mirroring. Recursively blocking BDS allows to do these operations safely. Best regards Beno=EEt >=20 > Fam >=20 > >=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, B= lockOpType op, Error **errp) > > return false; > > } > > =20 > > -void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reas= on) > > +/* 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, BlockO= pType op, Error *reason) > > QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list); > > } > > =20 > > -void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *re= ason) > > +/* 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, Blo= ckOpType 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) { > > + if (blocker->reason =3D=3D reason) { > > + return true; > > + } > > + } > > + return false; > > +} > > + > > +/* block recursively a BDS */ > > +void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reas= on) > > +{ > > + 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); > > + } > > +} > > + > > +/* unblock recursively a BDS */ > > +void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *re= ason) > > +{ > > + 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 c= har *node_name, Error **errp) > > return NULL; > > } > > =20 > > - if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, err= p)) { > > + if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_MIRROR_REPLA= CE, errp)) { > > 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(BlockDr= iverState *bs, > > bdrv_attach_aio_context(s->test_file, new_context); > > } > > =20 > > +static void blkverify_op_recursive_block(BlockDriverState *bs, Block= OpType 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, Blo= ckOpType op, > > + Error *reason) > > +{ > > + BDRVBlkverifyState *s =3D bs->opaque; > > + > > + bdrv_op_unblock(bs->file, op, reason); > > + bdrv_op_unblock(s->test_file, op, reason); > > +} > > + > > 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_conte= xt, > > .bdrv_detach_aio_context =3D blkverify_detach_aio_conte= xt, > > =20 > > + .bdrv_op_recursive_block =3D blkverify_op_recursive_blo= ck, > > + .bdrv_op_recursive_unblock =3D blkverify_op_recursive_unb= lock, > > + > > .is_filter =3D true, > > .bdrv_recurse_is_first_non_filter =3D blkverify_recurse_is_first= _non_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); > > ret =3D bdrv_drop_intermediate(active, top, base, s->backing= _file_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); > > 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 > > @@ -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 *b= s, 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->block= er); > > + > > 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/quorum.c b/block/quorum.c > > index d5ee9c0..c260171 100644 > > --- a/block/quorum.c > > +++ b/block/quorum.c > > @@ -945,6 +945,28 @@ static void quorum_attach_aio_context(BlockDrive= rState *bs, > > } > > } > > =20 > > +static void quorum_op_recursive_block(BlockDriverState *bs, BlockOpT= ype op, > > + Error *reason) > > +{ > > + BDRVQuorumState *s =3D bs->opaque; > > + int i; > > + > > + for (i =3D 0; i < s->num_children; i++) { > > + bdrv_op_block(s->bs[i], op, reason); > > + } > > +} > > + > > +static void quorum_op_recursive_unblock(BlockDriverState *bs, BlockO= pType op, > > + Error *reason) > > +{ > > + BDRVQuorumState *s =3D bs->opaque; > > + int i; > > + > > + for (i =3D 0; i < s->num_children; i++) { > > + bdrv_op_unblock(s->bs[i], op, reason); > > + } > > +} > > + > > static BlockDriver bdrv_quorum =3D { > > .format_name =3D "quorum", > > .protocol_name =3D "quorum", > > @@ -965,6 +987,9 @@ static BlockDriver bdrv_quorum =3D { > > .bdrv_detach_aio_context =3D quorum_detach_aio_contex= t, > > .bdrv_attach_aio_context =3D quorum_attach_aio_contex= t, > > =20 > > + .bdrv_op_recursive_block =3D quorum_op_recursive_bloc= k, > > + .bdrv_op_recursive_unblock =3D quorum_op_recursive_unbl= ock, > > + > > .is_filter =3D true, > > .bdrv_recurse_is_first_non_filter =3D quorum_recurse_is_first_= non_filter, > > }; > > 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); > > 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, BlockOpTyp= e 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); > > + > > + for (i =3D 0; i < s->num_extents; i++) { > > + if (s->extents[i].file) { > > + bdrv_op_block(s->extents[i].file, op, reason); > > + } > > + } > > +} > > + > > +static void vmdk_op_recursive_unblock(BlockDriverState *bs, BlockOpT= ype op, > > + Error *reason) > > +{ > > + BDRVVmdkState *s =3D bs->opaque; > > + int i; > > + > > + bdrv_op_unblock(bs->file, op, reason); > > + bdrv_op_unblock(bs->backing_hd, op, reason); > > + > > + for (i =3D 0; i < s->num_extents; i++) { > > + if (s->extents[i].file) { > > + bdrv_op_unblock(s->extents[i].file, op, reason); > > + } > > + } > > +} > > + > > static BlockDriver bdrv_vmdk =3D { > > .format_name =3D "vmdk", > > .instance_size =3D sizeof(BDRVVmdkState), > > @@ -2242,6 +2274,8 @@ static BlockDriver bdrv_vmdk =3D { > > .bdrv_get_info =3D vmdk_get_info, > > .bdrv_detach_aio_context =3D vmdk_detach_aio_context, > > .bdrv_attach_aio_context =3D vmdk_attach_aio_context, > > + .bdrv_op_recursive_block =3D vmdk_op_recursive_block, > > + .bdrv_op_recursive_unblock =3D vmdk_op_recursive_unblock, > > =20 > > .supports_backing =3D true, > > .create_opts =3D &vmdk_create_opts, > > diff --git a/include/block/block.h b/include/block/block.h > > index e94b701..4729a0a 100644 > > --- a/include/block/block.h > > +++ b/include/block/block.h > > @@ -175,7 +175,7 @@ typedef enum BlockOpType { > > BLOCK_OP_TYPE_MIRROR, > > BLOCK_OP_TYPE_RESIZE, > > BLOCK_OP_TYPE_STREAM, > > - BLOCK_OP_TYPE_REPLACE, > > + BLOCK_OP_TYPE_MIRROR_REPLACE, > > BLOCK_OP_TYPE_MAX, > > } BlockOpType; > > =20 > > diff --git a/include/block/block_int.h b/include/block/block_int.h > > index 7b541a0..a802abc 100644 > > --- a/include/block/block_int.h > > +++ b/include/block/block_int.h > > @@ -266,6 +266,12 @@ struct BlockDriver { > > void (*bdrv_io_unplug)(BlockDriverState *bs); > > void (*bdrv_flush_io_queue)(BlockDriverState *bs); > > =20 > > + /* helps blockers to propagate recursively */ > > + void (*bdrv_op_recursive_block)(BlockDriverState *bs, BlockOpTyp= e op, > > + Error *reason); > > + void (*bdrv_op_recursive_unblock)(BlockDriverState *bs, BlockOpT= ype op, > > + Error *reason); > > + > > QLIST_ENTRY(BlockDriver) list; > > }; > > =20 > > --=20 > > 2.1.0 > >=20