[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH for-5.0 v2 15/23] mirror: Prevent loops
From: |
Max Reitz |
Subject: |
Re: [PATCH for-5.0 v2 15/23] mirror: Prevent loops |
Date: |
Fri, 20 Dec 2019 12:39:14 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 |
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?
Max
signature.asc
Description: OpenPGP digital signature