[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] block: Make op blockers recursive
From: |
Benoît Canet |
Subject: |
Re: [Qemu-devel] [PATCH] block: Make op blockers recursive |
Date: |
Mon, 25 Aug 2014 09:06:11 +0000 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Mon, Aug 25, 2014 at 02:04:24PM +0800, Fam Zheng wrote:
> On Fri, 08/22 18:11, Benoît 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) QEMU
> > needs
> > to properly block and unblock whole BDS subtrees; recursion is a neat way to
> > achieve this task.
> >
> > This patch also takes care of modifying the op blockers users.
>
> Is this going to replace backing_blocker?
>
> 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 BDS
> connections. In other words, the choosing of op blockers are likely
> conditional on graph edge types, that's why backing_blocker was added here.
> For
> example, A VMDK extent connection will probably need a different set of
> blockers than bs->file connection.
>
> So could you explain in which cases is the recursive blocking/unblocking
> useful?
It's designed for the new crop of block operations operating on BDS located in
the middle of the backing chain: Jeff's patches, intermediate live streaming or
intermediate mirroring.
Recursively blocking BDS allows to do these operations safely.
Best regards
Benoît
>
> Fam
>
> >
> > Signed-off-by: Benoit Canet <address@hidden>
> > ---
> > 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(-)
> >
> > 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,
> > BlockOpType op, Error **errp)
> > return false;
> > }
> >
> > -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 >= 0 && op < BLOCK_OP_TYPE_MAX);
> > @@ -5456,7 +5458,9 @@ void bdrv_op_block(BlockDriverState *bs, BlockOpType
> > op, Error *reason)
> > QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list);
> > }
> >
> > -void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason)
> > +/* 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 >= 0 && op < BLOCK_OP_TYPE_MAX);
> > @@ -5468,6 +5472,65 @@ void bdrv_op_unblock(BlockDriverState *bs,
> > BlockOpType op, Error *reason)
> > }
> > }
> >
> > +static bool bdrv_op_is_blocked_by(BlockDriverState *bs, BlockOpType op,
> > + Error *reason)
> > +{
> > + BdrvOpBlocker *blocker, *next;
> > + assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
> > + QLIST_FOREACH_SAFE(blocker, &bs->op_blockers[op], list, next) {
> > + if (blocker->reason == 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);
> > + }
> > +}
> > +
> > +/* unblock recursively a BDS */
> > +void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason)
> > +{
> > + 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 char
> > *node_name, Error **errp)
> > return NULL;
> > }
> >
> > - 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)) {
> > return NULL;
> > }
> >
> > 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(BlockDriverState *bs,
> > bdrv_attach_aio_context(s->test_file, new_context);
> > }
> >
> > +static void blkverify_op_recursive_block(BlockDriverState *bs, BlockOpType
> > op,
> > + Error *reason)
> > +{
> > + BDRVBlkverifyState *s = 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,
> > BlockOpType op,
> > + Error *reason)
> > +{
> > + BDRVBlkverifyState *s = bs->opaque;
> > +
> > + bdrv_op_unblock(bs->file, op, reason);
> > + bdrv_op_unblock(s->test_file, op, reason);
> > +}
> > +
> > static BlockDriver bdrv_blkverify = {
> > .format_name = "blkverify",
> > .protocol_name = "blkverify",
> > @@ -337,6 +355,9 @@ static BlockDriver bdrv_blkverify = {
> > .bdrv_attach_aio_context = blkverify_attach_aio_context,
> > .bdrv_detach_aio_context = blkverify_detach_aio_context,
> >
> > + .bdrv_op_recursive_block = blkverify_op_recursive_block,
> > + .bdrv_op_recursive_unblock = blkverify_op_recursive_unblock,
> > +
> > .is_filter = true,
> > .bdrv_recurse_is_first_non_filter =
> > 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:
> >
> > if (!block_job_is_cancelled(&s->common) && sector_num == 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 = bdrv_drop_intermediate(active, top, base,
> > s->backing_file_str);
> > }
> >
> > 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 = 0;
> > int n;
> >
> > @@ -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 = s->common.bs;
> > + if (s->to_replace) {
> > + bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
> > + to_replace = s->to_replace;
> > + }
> > if (s->should_complete && ret == 0) {
> > - BlockDriverState *to_replace = s->common.bs;
> > - if (s->to_replace) {
> > - to_replace = s->to_replace;
> > - }
> > if (bdrv_get_flags(s->target) != 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 == 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 *bs,
> > BlockDriverState *target,
> > return;
> > }
> >
> > + /* 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);
> > +
> > s->replaces = g_strdup(replaces);
> > s->on_source_error = on_source_error;
> > s->on_target_error = 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(BlockDriverState
> > *bs,
> > }
> > }
> >
> > +static void quorum_op_recursive_block(BlockDriverState *bs, BlockOpType op,
> > + Error *reason)
> > +{
> > + BDRVQuorumState *s = bs->opaque;
> > + int i;
> > +
> > + for (i = 0; i < s->num_children; i++) {
> > + bdrv_op_block(s->bs[i], op, reason);
> > + }
> > +}
> > +
> > +static void quorum_op_recursive_unblock(BlockDriverState *bs, BlockOpType
> > op,
> > + Error *reason)
> > +{
> > + BDRVQuorumState *s = bs->opaque;
> > + int i;
> > +
> > + for (i = 0; i < s->num_children; i++) {
> > + bdrv_op_unblock(s->bs[i], op, reason);
> > + }
> > +}
> > +
> > static BlockDriver bdrv_quorum = {
> > .format_name = "quorum",
> > .protocol_name = "quorum",
> > @@ -965,6 +987,9 @@ static BlockDriver bdrv_quorum = {
> > .bdrv_detach_aio_context = quorum_detach_aio_context,
> > .bdrv_attach_aio_context = quorum_attach_aio_context,
> >
> > + .bdrv_op_recursive_block = quorum_op_recursive_block,
> > + .bdrv_op_recursive_unblock = quorum_op_recursive_unblock,
> > +
> > .is_filter = true,
> > .bdrv_recurse_is_first_non_filter =
> > 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 = 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);
> > }
> >
> > 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 = {
> > }
> > };
> >
> > +static void vmdk_op_recursive_block(BlockDriverState *bs, BlockOpType op,
> > + Error *reason)
> > +{
> > + BDRVVmdkState *s = bs->opaque;
> > + int i;
> > +
> > + bdrv_op_block(bs->file, op, reason);
> > + bdrv_op_block(bs->backing_hd, op, reason);
> > +
> > + for (i = 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, BlockOpType op,
> > + Error *reason)
> > +{
> > + BDRVVmdkState *s = bs->opaque;
> > + int i;
> > +
> > + bdrv_op_unblock(bs->file, op, reason);
> > + bdrv_op_unblock(bs->backing_hd, op, reason);
> > +
> > + for (i = 0; i < s->num_extents; i++) {
> > + if (s->extents[i].file) {
> > + bdrv_op_unblock(s->extents[i].file, op, reason);
> > + }
> > + }
> > +}
> > +
> > static BlockDriver bdrv_vmdk = {
> > .format_name = "vmdk",
> > .instance_size = sizeof(BDRVVmdkState),
> > @@ -2242,6 +2274,8 @@ static BlockDriver bdrv_vmdk = {
> > .bdrv_get_info = vmdk_get_info,
> > .bdrv_detach_aio_context = vmdk_detach_aio_context,
> > .bdrv_attach_aio_context = vmdk_attach_aio_context,
> > + .bdrv_op_recursive_block = vmdk_op_recursive_block,
> > + .bdrv_op_recursive_unblock = vmdk_op_recursive_unblock,
> >
> > .supports_backing = true,
> > .create_opts = &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;
> >
> > 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);
> >
> > + /* helps blockers to propagate recursively */
> > + void (*bdrv_op_recursive_block)(BlockDriverState *bs, BlockOpType op,
> > + Error *reason);
> > + void (*bdrv_op_recursive_unblock)(BlockDriverState *bs, BlockOpType op,
> > + Error *reason);
> > +
> > QLIST_ENTRY(BlockDriver) list;
> > };
> >
> > --
> > 2.1.0
> >