[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, 29 Nov 2019 15:17:47 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 |
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...
Max
signature.asc
Description: OpenPGP digital signature
- [PATCH for-5.0 v2 11/23] block: Use bdrv_recurse_can_replace(), (continued)
- [PATCH for-5.0 v2 11/23] block: Use bdrv_recurse_can_replace(), Max Reitz, 2019/11/11
- [PATCH for-5.0 v2 12/23] block: Remove bdrv_recurse_is_first_non_filter(), Max Reitz, 2019/11/11
- [PATCH for-5.0 v2 13/23] mirror: Double-check immediately before replacing, Max Reitz, 2019/11/11
- [PATCH for-5.0 v2 14/23] quorum: Stop marking it as a filter, Max Reitz, 2019/11/11
- [PATCH for-5.0 v2 15/23] mirror: Prevent loops, Max Reitz, 2019/11/11
[PATCH for-5.0 v2 16/23] iotests: Use complete_and_wait() in 155, Max Reitz, 2019/11/11
[PATCH for-5.0 v2 17/23] iotests: Use skip_if_unsupported decorator in 041, Max Reitz, 2019/11/11
[PATCH for-5.0 v2 18/23] iotests: Add VM.assert_block_path(), Max Reitz, 2019/11/11
[PATCH for-5.0 v2 19/23] iotests: Resolve TODOs in 041, Max Reitz, 2019/11/11
[PATCH for-5.0 v2 21/23] iotests: Add tests for invalid Quorum @replaces, Max Reitz, 2019/11/11
[PATCH for-5.0 v2 22/23] iotests: Check that @replaces can replace filters, Max Reitz, 2019/11/11
[PATCH for-5.0 v2 20/23] iotests: Use self.image_len in TestRepairQuorum, Max Reitz, 2019/11/11
[PATCH for-5.0 v2 23/23] iotests: Mirror must not attempt to create loops, Max Reitz, 2019/11/11