29.11.2019 17:17, Max Reitz wrote: > On 29.11.19 14:55, Vladimir Sementsov-Ogievskiy wrote: >> 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 <[email protected]> >>>>> --- >>>>> 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. > > The question to me is whether it’s really useful. The thing is that > maybe bdrv_replace_node() can make sense of it. But still, from the > user’s perspective, they kind of are asking for a loop whenever > to_replace is a child of target. It just so happens that we must allow > one of these cases because it’s the default case for sync=none. > > So I’d rather forbid all such cases, because it should be understandable > to users why... >
Okay, I don't have more arguments:) Honestly, I just feel that relying on existing of chains between nodes of some hardcoded length is not good generic criteria... bdrv_replace_node never creates loops.. Maybe, just document this behavior in qapi? And (maybe) return error, if we see that bdrv_replace_node will be noop? And if it is not noop, may be user don't tries to create a loop, but instead, user is powerful, knows how bdrv_replace_node works and wants exactly this behavior? -- Best regards, Vladimir
