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 <[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. >> + */ >> + 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?
>> +
>> + 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?
Max
>> +
>> 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.
>>
>
>
signature.asc
Description: OpenPGP digital signature
