29.11.2019 16:46, Max Reitz wrote: > On 29.11.19 13:01, Vladimir Sementsov-Ogievskiy wrote: >> 11.11.2019 19:02, Max Reitz wrote: >>> While bdrv_replace_node() will not follow through with it, a specific >>> @replaces asks the mirror job to create a loop. >>> >>> For example, say both the source and the target share a child where the >>> source is a filter; by letting @replaces point to the common child, you >>> ask for a loop. >>> >>> Or if you use @replaces in drive-mirror with sync=none and >>> mode=absolute-paths, you generally ask for a loop (@replaces must point >>> to a child of the source, and sync=none makes the source the backing >>> file of the target after the job). >>> >>> bdrv_replace_node() will not create those loops, but by doing so, it >>> ignores the user-requested configuration, which is not ideally either. >>> (In the first example above, the target's child will remain what it was, >>> which may still be reasonable. But in the second example, the target >>> will just not become a child of the source, which is precisely what was >>> requested with @replaces.) >>> >>> So prevent such configurations, both before the job, and before it >>> actually completes. >>> >>> Signed-off-by: Max Reitz <mre...@redhat.com> >>> --- >>> block.c | 30 ++++++++++++++++++++++++ >>> block/mirror.c | 19 +++++++++++++++- >>> blockdev.c | 48 ++++++++++++++++++++++++++++++++++++++- >>> include/block/block_int.h | 3 +++ >>> 4 files changed, 98 insertions(+), 2 deletions(-) >>> >>> diff --git a/block.c b/block.c >>> index 0159f8e510..e3922a0474 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -6259,6 +6259,36 @@ out: >>> return to_replace_bs; >>> } >>> >>> +/* >>> + * Return true iff @child is a (recursive) child of @parent, with at >>> + * least @min_level edges between them. >>> + * >>> + * (If @min_level == 0, return true if @child == @parent. For >>> + * @min_level == 1, @child needs to be at least a real child; for >>> + * @min_level == 2, it needs to be at least a grand-child; and so on.) >>> + */ >>> +bool bdrv_is_child_of(BlockDriverState *child, BlockDriverState *parent, >>> + int min_level) >>> +{ >>> + BdrvChild *c; >>> + >>> + if (child == parent && min_level <= 0) { >>> + return true; >>> + } >>> + >>> + if (!parent) { >>> + return false; >>> + } >>> + >>> + QLIST_FOREACH(c, &parent->children, next) { >>> + if (bdrv_is_child_of(child, c->bs, min_level - 1)) { >>> + return true; >>> + } >>> + } >>> + >>> + return false; >>> +} >>> + >>> /** >>> * Iterates through the list of runtime option keys that are said to >>> * be "strong" for a BDS. An option is called "strong" if it changes >>> diff --git a/block/mirror.c b/block/mirror.c >>> index 68a4404666..b258c7e98b 100644 >>> --- a/block/mirror.c >>> +++ b/block/mirror.c >>> @@ -701,7 +701,24 @@ static int mirror_exit_common(Job *job) >>> * there. >>> */ >>> if (bdrv_recurse_can_replace(src, to_replace)) { >>> - bdrv_replace_node(to_replace, target_bs, &local_err); >>> + /* >>> + * It is OK for @to_replace to be an immediate child of >>> + * @target_bs, because that is what happens with >>> + * drive-mirror sync=none mode=absolute-paths: target_bs's >>> + * backing file will be the source node, which is also >>> + * to_replace (by default). >>> + * bdrv_replace_node() handles this case by not letting >>> + * target_bs->backing point to itself, but to the source >>> + * still. >>> + */ >> >> Hmm.. So, we want the following valid case: >> >> (other parents of source) ----> source = to_replace <--- backing --- target >> >> becomes >> >> (other parents of source) ----> target --- backing ---> source >> >> But it seems for me, that the following is not less valid: >> >> (other parents of source) ----> source = to_replace <--- backing --- X <--- >> backing --- target >> >> becomes >> >> (other parents of source) ----> target --- backing ---> X --- backing ---> >> source > > I think it is less valid. The first case works with sync=none, because > target is initially empty and then you just copy all new data, so the > target keeps looking like the source. > > But in the second case, there are intermediate nodes that mean that > target may well not look like the source.
Maybe, it's valid if target node is a filter? Or, otherwise, it's backing is a filter, but this seems less useful. > > (Yes, you have the same problem if you use sync=none or sync=full to a > completely independent node. But that still means that while the first > case is always valid, the second may be problematic.) > >> And what we actually want to prevent, is when to_replace is not source, but >> child (may be not direct) >> of source.. >> >> Also, with your check you still allow silent no-change in the following case: >> >> source --- backing --> to_replace <-- backing -- target > > You mean if source is a filter on to_replace? (Because otherwise you > can’t replace that node.) > > Is that really a no-change? Shouldn’t we get > > source --> target --> to_replace Ah, yes, it's OK. > > ? (And what else would you expect?) > > So maybe we don’t want to prevent that, because I think it can make sense. > > Max > >> ==== >> >> In other words, replacing make sense, only if to_replace has some other >> parents, which are not >> children (may be not direct) of target.. And the only known such case is >> when in the same time >> to_replace == source. >> >> so, shouldn't the following be >> >> if (to_replace == src || !bdrv_is_child_of(to_replace, target_bs, 1) { >> >> or, may be, to allow also replace filters above src, keeping backing link : >> >> if (bdrv_is_child_of(src, to_replace, 0) || !bdrv_is_child_of(to_replace, >> target_bs, 1) { >> >>> + if (!bdrv_is_child_of(to_replace, target_bs, 2)) { >>> + bdrv_replace_node(to_replace, target_bs, &local_err); >>> + } else { >>> + error_setg(&local_err, "Can no longer replace '%s' by >>> '%s', " >>> + "because the former is now a child of the >>> latter, " >>> + "and doing so would thus create a loop", >>> + to_replace->node_name, target_bs->node_name); >>> + } >>> } else { >>> error_setg(&local_err, "Can no longer replace '%s' by '%s', " >>> "because it can no longer be guaranteed that >>> doing so " >>> diff --git a/blockdev.c b/blockdev.c >>> index 9dc2238bf3..d29f147f72 100644 >>> --- a/blockdev.c >>> +++ b/blockdev.c >>> @@ -3824,7 +3824,7 @@ static void blockdev_mirror_common(const char >>> *job_id, BlockDriverState *bs, >>> } >>> >>> if (has_replaces) { >>> - BlockDriverState *to_replace_bs; >>> + BlockDriverState *to_replace_bs, *target_backing_bs; >>> AioContext *replace_aio_context; >>> int64_t bs_size, replace_size; >>> >>> @@ -3839,6 +3839,52 @@ static void blockdev_mirror_common(const char >>> *job_id, BlockDriverState *bs, >>> return; >>> } >>> >>> + if (bdrv_is_child_of(to_replace_bs, target, 1)) { >>> + error_setg(errp, "Replacing %s by %s would result in a loop, " >>> + "because the former is a child of the latter", >>> + to_replace_bs->node_name, target->node_name); >>> + return; >>> + } >>> + >>> + if (backing_mode == MIRROR_SOURCE_BACKING_CHAIN || >>> + backing_mode == MIRROR_OPEN_BACKING_CHAIN) >>> + { >>> + /* >>> + * While we do not quite know what OPEN_BACKING_CHAIN >>> + * (used for mode=existing) will yield, it is probably >>> + * best to restrict it exactly like SOURCE_BACKING_CHAIN, >>> + * because that is our best guess. >>> + */ >>> + switch (sync) { >>> + case MIRROR_SYNC_MODE_FULL: >>> + target_backing_bs = NULL; >>> + break; >>> + >>> + case MIRROR_SYNC_MODE_TOP: >>> + target_backing_bs = backing_bs(bs); >>> + break; >>> + >>> + case MIRROR_SYNC_MODE_NONE: >>> + target_backing_bs = bs; >>> + break; >>> + >>> + default: >>> + abort(); >>> + } >>> + } else { >>> + assert(backing_mode == MIRROR_LEAVE_BACKING_CHAIN); >>> + target_backing_bs = backing_bs(target); >>> + } >>> + >>> + if (bdrv_is_child_of(to_replace_bs, target_backing_bs, 0)) { >>> + error_setg(errp, "Replacing '%s' by '%s' with this sync mode >>> would " >>> + "result in a loop, because the former would be a >>> child " >>> + "of the latter's backing file ('%s') after the >>> mirror " >>> + "job", to_replace_bs->node_name, target->node_name, >>> + target_backing_bs->node_name); >>> + return; >>> + } >>> + >>> replace_aio_context = bdrv_get_aio_context(to_replace_bs); >>> aio_context_acquire(replace_aio_context); >>> replace_size = bdrv_getlength(to_replace_bs); >>> diff --git a/include/block/block_int.h b/include/block/block_int.h >>> index 589a797fab..7064a1a4fa 100644 >>> --- a/include/block/block_int.h >>> +++ b/include/block/block_int.h >>> @@ -1266,6 +1266,9 @@ void bdrv_format_default_perms(BlockDriverState *bs, >>> BdrvChild *c, >>> bool bdrv_recurse_can_replace(BlockDriverState *bs, >>> BlockDriverState *to_replace); >>> >>> +bool bdrv_is_child_of(BlockDriverState *child, BlockDriverState *parent, >>> + int min_level); >>> + >>> /* >>> * Default implementation for drivers to pass bdrv_co_block_status() to >>> * their file. >>> >> >> > > -- Best regards, Vladimir