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

[Max Reitz]

On 29.11.19 15:26, Vladimir Sementsov-Ogievskiy wrote:
> 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?

I don’t know whether that’s a good point.  We have strong restrictions
on @replaces anyway (that’s the point of this series, to fix them).  So
if we want to loosen those restrictions and allow the user to do
anything they want because it’s their job to be careful, that would be a
whole different series.

Also, one of the examples in the commit message is using @replaces with
drive-mirror sync=none mode=absolute-paths.  @replaces must be a child
of the source.  So what will happen is that it’s replaced and then we
can’t attach the source as the backing file of the target.  So the
target will probably just read garbage, given that it now lacks the
source as its backing file.

So I’m not sold on “If the user knows what’ll happen, it’s all good”.
Because I don’t think they’ll really know.  I’d rather keep it tight
until someone complains.

Max

signature.asc
Description: OpenPGP digital signature