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

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
>>>>> ---
>>>>>     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