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

[Vladimir Sementsov-Ogievskiy]

20.12.2019 14:39, Max Reitz wrote:
> On 13.12.19 12:18, Vladimir Sementsov-Ogievskiy wrote:
>> 09.12.2019 17:43, Max Reitz wrote:
>>> 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 <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.
>>>>> +             */
>>>>> +            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?
>>
>> Hmm, I think, that allowing to_replaces to be direct backing child of target
>> (like in mirror_exit_common) is safe enough. User doesn't know that
>> such replacing includes also replacing own child of the target,
>> which leads to the loop.. It's not obvious. And behavior of
>> bdrv_replace_node() which just doesn't create this loop, doesn't
>> seem something too tricky. Hmm..
>>
>> We could mention in qapi spec, that replacing doesn't break backing
>> link of the target, for it to be absolutely defined.
>>
>> But should we allow replaces to be some other (not backing and not filtered)
>> child of target?..
> 
> Well, my opinion is that this is a bit of weird thing to do and that it
> basically does ask for a loop.
> 
> I’m OK with excluding the sync=none case, because (1) that’s so
> obviously a loop that it can’t be what the user honestly wants; (2) how
> it’s resolved is rather obvious, too: There is exactly one edge that
> causes the loop, so you simply don’t change that one; (3) drive-mirror
> sync=none does this case automatically, so we should probably allow
> users to do it manually with blockdev-mirror, too.
> 
>>>>> +
>>>>> +        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?
>>>
>>
>> I'm a bit more closer to allowing it, for consistency with automatic path, 
>> with
>> unspecified replaces. Are we sure that nobody uses it?
> 
> Well, there are multiple cases, as shown in the commit message.  I think
> that for drive-mirror sync=none, nobody uses @replaces, because it just
> doesn’t work.
> 
> But, well, that’s just because drive-mirror does graph manipulation that
> blockdev-mirror doesn’t (i.e., changing the target’s backing file on
> completion).  So maybe we should just prevent loops for drive-mirror,
> but let the user do what they want when they use blockdev-mirror?
> 

Well, the question finally is, how much to restrict from things for which we
don't know are they useful or not. I don't know) I think, finally, I'm OK with
either way we discussed, or with this patch as is. If it breaks some existing
scenario it will be easy to fix.

-- 
Best regards,
Vladimir