Re: [PATCH for-5.0 v2 15/23] mirror: Prevent loops

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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 <mreitz@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