On 20.12.19 12:55, Vladimir Sementsov-Ogievskiy wrote: > 20.12.2019 14:39, Max Reitz wrote: >> On 13.12.19 12:18, Vladimir Sementsov-Ogievskiy wrote: >>> 09.12.2019 17:43, Max Reitz wrote: >>>> On 02.12.19 13:12, 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 >>>>>> --- >>>>>> 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. >>>>>> + */ >>>>>> + 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); >>>>>> + } >>>>> >>>>> you may swap if and else branch, dropping "!" mark.. >>>> >>>> Yes, but I just personally prefer to have the error case in the else branch. >>>> >>>>>> } 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; >>>>>> + } >>>>> >>>>> here min_level=1, so we don't handle the case, described in mirror_exit_common.. >>>>> I don't see why.. blockdev_mirror_common is called from qmp_drive_mirror, >>>>> including the case with MIRROR_SYNC_MODE_NONE and NEW_IMAGE_MODE_ABSOLUTE_PATHS.. >>>>> >>>>> What I'm missing? >>>> >>>> Hmm. Well. >>>> >>>> If it broke drive-mirror sync=none, I suppose I would have noticed by >>>> running the iotests. But I didn’t, and that’s because this code here is >>>> reached only if the user actually specified @replaces. (As opposed to >>>> the mirror_exit_common code, where @to_replace may simply be @src if not >>>> overridden by the user.) >>>> >>>> The only reason why I allow it in mirror_exit_common is because we have >>>> to. But if the user manually specifies this configuration, we can’t >>>> guarantee it’s safe. >>>> >>>> OTOH, well, if we allow it for drive-mirror sync=none, why not allow it >>>> when manually specified with blockdev-mirror? >>>> >>>> What’s your opinion? >>> >>> Hmm, I think, that allowing to_replaces to be direct backing child of target >>> (like in mirror_exit_common) is safe enough. User doesn't know that >>> such replacing includes also replacing own child of the target, >>> which leads to the loop.. It's not obvious. And behavior of >>> bdrv_replace_node() which just doesn't create this loop, doesn't >>> seem something too tricky. Hmm.. >>> >>> We could mention in qapi spec, that replacing doesn't break backing >>> link of the target, for it to be absolutely defined. >>> >>> But should we allow replaces to be some other (not backing and not filtered) >>> child of target?.. >> >> Well, my opinion is that this is a bit of weird thing to do and that it >> basically does ask for a loop. >> >> I’m OK with excluding the sync=none case, because (1) that’s so >> obviously a loop that it can’t be what the user honestly wants; (2) how >> it’s resolved is rather obvious, too: There is exactly one edge that >> causes the loop, so you simply don’t change that one; (3) drive-mirror >> sync=none does this case automatically, so we should probably allow >> users to do it manually with blockdev-mirror, too. >> >>>>>> + >>>>>> + 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; >>>>>> + } >>>>> >>>>> hmm.. so for MODE_NONE we disallow to_replace == src? >>>> >>>> I suppose that’s basically the same as above. Should we allow this case >>>> when specified explicitly by the user? >>>> >>> >>> I'm a bit more closer to allowing it, for consistency with automatic path, with >>> unspecified replaces. Are we sure that nobody uses it? >> >> Well, there are multiple cases, as shown in the commit message. I think >> that for drive-mirror sync=none, nobody uses @replaces, because it just >> doesn’t work. >> >> But, well, that’s just because drive-mirror does graph manipulation that >> blockdev-mirror doesn’t (i.e., changing the target’s backing file on >> completion). So maybe we should just prevent loops for drive-mirror, >> but let the user do what they want when they use blockdev-mirror? >> > > Well, the question finally is, how much to restrict from things for which we > don't know are they useful or not. I don't know) I think, finally, I'm OK with > either way we discussed, or with this patch as is. If it breaks some existing > scenario it will be easy to fix. OK. I hope next-year-me has a good and consistent idea on what to do. Max