From: Max Reitz <mreitz@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
"qemu-block@nongnu.org" <qemu-block@nongnu.org>
Cc: Kevin Wolf <kwolf@redhat.com>, Alberto Garcia <berto@igalia.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Subject: Re: [PATCH for-5.0 v2 15/23] mirror: Prevent loops
Date: Fri, 20 Dec 2019 13:10:37 +0100 [thread overview]
Message-ID: <2ff9a984-7f7c-82fd-57e6-f0c93d44ded0@redhat.com> (raw)
In-Reply-To: <e2e89921-9121-a67a-474c-1f7f11a305ef@virtuozzo.com>
[-- Attachment #1.1: Type: text/plain, Size: 11457 bytes --]
On 20.12.19 12:55, Vladimir Sementsov-Ogievskiy wrote:
> 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 <mreitz@redhat.com>
>>>>>> ---
>>>>>> 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.
OK. I hope next-year-me has a good and consistent idea on what to do.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2019-12-20 12:33 UTC|newest]
Thread overview: 75+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-11 16:01 [PATCH for-5.0 v2 00/23] block: Fix check_to_replace_node() Max Reitz
2019-11-11 16:01 ` [PATCH for-5.0 v2 01/23] blockdev: Allow external snapshots everywhere Max Reitz
2019-11-11 16:01 ` [PATCH for-5.0 v2 02/23] blockdev: Allow resizing everywhere Max Reitz
2019-12-06 14:04 ` Alberto Garcia
2019-12-09 13:56 ` Max Reitz
2019-11-11 16:01 ` [PATCH for-5.0 v2 03/23] block: Drop bdrv_is_first_non_filter() Max Reitz
2019-11-11 16:01 ` [PATCH for-5.0 v2 04/23] iotests: Let 041 use -blockdev for quorum children Max Reitz
2019-11-11 16:01 ` [PATCH for-5.0 v2 05/23] quorum: Fix child permissions Max Reitz
2019-11-29 9:14 ` Vladimir Sementsov-Ogievskiy
2019-11-11 16:01 ` [PATCH for-5.0 v2 06/23] block: Add bdrv_recurse_can_replace() Max Reitz
2019-11-29 9:34 ` Vladimir Sementsov-Ogievskiy
2019-11-29 10:23 ` Max Reitz
2019-11-29 11:04 ` Vladimir Sementsov-Ogievskiy
2019-11-11 16:02 ` [PATCH for-5.0 v2 07/23] blkverify: Implement .bdrv_recurse_can_replace() Max Reitz
2019-11-29 9:41 ` Vladimir Sementsov-Ogievskiy
2019-11-11 16:02 ` [PATCH for-5.0 v2 08/23] quorum: Store children in own structure Max Reitz
2019-11-29 9:46 ` Vladimir Sementsov-Ogievskiy
2019-11-11 16:02 ` [PATCH for-5.0 v2 09/23] quorum: Add QuorumChild.to_be_replaced Max Reitz
2019-11-29 9:59 ` Vladimir Sementsov-Ogievskiy
2019-11-11 16:02 ` [PATCH for-5.0 v2 10/23] quorum: Implement .bdrv_recurse_can_replace() Max Reitz
2019-11-29 10:18 ` Vladimir Sementsov-Ogievskiy
2019-11-29 12:50 ` Max Reitz
2020-02-05 15:55 ` Kevin Wolf
2020-02-05 16:03 ` Kevin Wolf
2020-02-06 10:21 ` Max Reitz
2020-02-06 14:42 ` Kevin Wolf
2020-02-06 15:19 ` Max Reitz
2020-02-06 15:42 ` Kevin Wolf
2020-02-06 16:44 ` Max Reitz
2019-11-11 16:02 ` [PATCH for-5.0 v2 11/23] block: Use bdrv_recurse_can_replace() Max Reitz
2019-11-29 11:07 ` Vladimir Sementsov-Ogievskiy
2020-02-05 15:57 ` Kevin Wolf
2019-11-11 16:02 ` [PATCH for-5.0 v2 12/23] block: Remove bdrv_recurse_is_first_non_filter() Max Reitz
2019-11-11 16:02 ` [PATCH for-5.0 v2 13/23] mirror: Double-check immediately before replacing Max Reitz
2019-11-29 11:18 ` Vladimir Sementsov-Ogievskiy
2019-11-11 16:02 ` [PATCH for-5.0 v2 14/23] quorum: Stop marking it as a filter Max Reitz
2019-11-11 16:02 ` [PATCH for-5.0 v2 15/23] mirror: Prevent loops Max Reitz
2019-11-29 12:01 ` Vladimir Sementsov-Ogievskiy
2019-11-29 13:46 ` Max Reitz
2019-11-29 13:55 ` Vladimir Sementsov-Ogievskiy
2019-11-29 14:17 ` Max Reitz
2019-11-29 14:26 ` Vladimir Sementsov-Ogievskiy
2019-11-29 14:38 ` Max Reitz
2019-12-02 12:12 ` Vladimir Sementsov-Ogievskiy
2019-12-09 14:43 ` Max Reitz
2019-12-13 11:18 ` Vladimir Sementsov-Ogievskiy
2019-12-20 11:39 ` Max Reitz
2019-12-20 11:55 ` Vladimir Sementsov-Ogievskiy
2019-12-20 12:10 ` Max Reitz [this message]
2019-11-11 16:02 ` [PATCH for-5.0 v2 16/23] iotests: Use complete_and_wait() in 155 Max Reitz
2019-11-11 16:02 ` [PATCH for-5.0 v2 17/23] iotests: Use skip_if_unsupported decorator in 041 Max Reitz
2019-12-03 12:03 ` Vladimir Sementsov-Ogievskiy
2019-11-11 16:02 ` [PATCH for-5.0 v2 18/23] iotests: Add VM.assert_block_path() Max Reitz
2019-12-03 12:59 ` Vladimir Sementsov-Ogievskiy
2019-12-09 15:10 ` Max Reitz
2019-12-13 11:26 ` Vladimir Sementsov-Ogievskiy
2019-12-13 11:27 ` Vladimir Sementsov-Ogievskiy
2019-12-20 11:42 ` Max Reitz
2019-11-11 16:02 ` [PATCH for-5.0 v2 19/23] iotests: Resolve TODOs in 041 Max Reitz
2019-12-03 13:32 ` Vladimir Sementsov-Ogievskiy
2019-12-03 13:33 ` Vladimir Sementsov-Ogievskiy
2019-12-09 15:15 ` Max Reitz
2019-12-13 11:31 ` Vladimir Sementsov-Ogievskiy
2019-11-11 16:02 ` [PATCH for-5.0 v2 20/23] iotests: Use self.image_len in TestRepairQuorum Max Reitz
2019-11-11 16:02 ` [PATCH for-5.0 v2 21/23] iotests: Add tests for invalid Quorum @replaces Max Reitz
2019-12-03 14:40 ` Vladimir Sementsov-Ogievskiy
2019-11-11 16:02 ` [PATCH for-5.0 v2 22/23] iotests: Check that @replaces can replace filters Max Reitz
2019-12-03 15:58 ` Vladimir Sementsov-Ogievskiy
2019-12-09 15:17 ` Max Reitz
2019-11-11 16:02 ` [PATCH for-5.0 v2 23/23] iotests: Mirror must not attempt to create loops Max Reitz
2019-12-03 17:03 ` Vladimir Sementsov-Ogievskiy
2019-11-29 12:24 ` [PATCH for-5.0 v2 00/23] block: Fix check_to_replace_node() Vladimir Sementsov-Ogievskiy
2019-11-29 12:49 ` Max Reitz
2019-11-29 12:55 ` Vladimir Sementsov-Ogievskiy
2019-11-29 13:08 ` Max Reitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2ff9a984-7f7c-82fd-57e6-f0c93d44ded0@redhat.com \
--to=mreitz@redhat.com \
--cc=berto@igalia.com \
--cc=kwolf@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=vsementsov@virtuozzo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).