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
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
====
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
