On Tue, 09/09 14:28, Benoît Canet wrote: > On Tue, Sep 09, 2014 at 01:56:46PM +0200, Kevin Wolf wrote: > > Am 22.08.2014 um 18:11 hat Benoît 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) 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. > > > > > > Signed-off-by: Benoit Canet <benoit.ca...@nodalink.com> > > > --- > > > 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) { > > > > This doesn't actually need the SAFE version. > > > > > + 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); > > > + } > > > > 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 *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)) { > > > > That rename could have been a separate patch. > > > > > 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); > > > +} > > > > 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 = { > > > .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); > > > > 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. > > Good idea. > > > > > > 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); > > > > 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 == 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); > > > > 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.
I agree. It makes no sense if the blocker is also the checker. > > BLOCK_OP_TYPE_MIRROR_REPLACE is checked and blocked by block-job-complete > during the time the mirror finish when an arbitrary node of the graph must be > replaced. It seems to me mirror unblocks this operation from the job->blocker when job starts, and never block it (with the job->blocker) again. It's leaked. Fam > > The start of the mirroring block everything including > BLOCK_OP_TYPE_MIRROR_REPLACE so this hunk relax the blocking so > block-job-complete > can have a chance of being able to block it. > > The comment of this hunk seems to be deficient and the code not self > explanatory. > > On the other way maybe block-job-complete is done wrong from the start but > relaxing BLOCK_OP_TYPE_MIRROR_REPLACE when the mirror start is the only way > to make block-job-complete work as intended. > > Thanks > > Benoît