Re: [Qemu-devel] [PATCH v6 3/3] block/stream: introduce a bottom node

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4793 bytes --]

On 29.05.19 13:44, Vladimir Sementsov-Ogievskiy wrote:
> 29.05.2019 14:23, Max Reitz wrote:
>> On 29.05.19 09:34, Vladimir Sementsov-Ogievskiy wrote:
>>> 28.05.2019 20:33, Max Reitz wrote:
>>>> On 06.05.19 17:34, Vladimir Sementsov-Ogievskiy wrote:
>>>>> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>>>
>>>>> The bottom node is the intermediate block device that has the base as its
>>>>> backing image. It is used instead of the base node while a block stream
>>>>> job is running to avoid dependency on the base that may change due to the
>>>>> parallel jobs. The change may take place due to a filter node as well that
>>>>> is inserted between the base and the intermediate bottom node. It occurs
>>>>> when the base node is the top one for another commit or stream job.
>>>>> After the introduction of the bottom node, don't freeze its backing child,
>>>>> that's the base, anymore.
>>>>>
>>>>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>>>>> ---
>>>>>    block/stream.c         | 49 +++++++++++++++++++++---------------------
>>>>>    tests/qemu-iotests/245 |  4 ++--
>>>>>    2 files changed, 27 insertions(+), 26 deletions(-)
>>>>>
>>>>> diff --git a/block/stream.c b/block/stream.c
>>>>> index 65b13b27e0..fc97c89f81 100644
>>>>> --- a/block/stream.c
>>>>> +++ b/block/stream.c
>>>>
>>>> [...]
>>>>
>>>>> @@ -248,26 +250,25 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>>>>>         * already have our own plans. Also don't allow resize as the image size is
>>>>>         * queried only at the job start and then cached. */
>>>>>        s = block_job_create(job_id, &stream_job_driver, NULL, bs,
>>>>> -                         BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
>>>>> -                         BLK_PERM_GRAPH_MOD,
>>>>> -                         BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
>>>>> -                         BLK_PERM_WRITE,
>>>>> +                         basic_flags | BLK_PERM_GRAPH_MOD,
>>>>> +                         basic_flags | BLK_PERM_WRITE,
>>>>>                             speed, creation_flags, NULL, NULL, errp);
>>>>>        if (!s) {
>>>>>            goto fail;
>>>>>        }
>>>>>    
>>>>> -    /* Block all intermediate nodes between bs and base, because they will
>>>>> -     * disappear from the chain after this operation. The streaming job reads
>>>>> -     * every block only once, assuming that it doesn't change, so block writes
>>>>> -     * and resizes. */
>>>>> -    for (iter = backing_bs(bs); iter && iter != base; iter = backing_bs(iter)) {
>>>>> -        block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
>>>>> -                           BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED,
>>>>> -                           &error_abort);
>>>>> +    /*
>>>>> +     * Block all intermediate nodes between bs and bottom (inclusive), because
>>>>> +     * they will disappear from the chain after this operation. The streaming
>>>>> +     * job reads every block only once, assuming that it doesn't change, so
>>>>> +     * forbid writes and resizes.
>>>>> +     */
>>>>> +    for (iter = bs; iter != bottom; iter = backing_bs(iter)) {
>>>>> +        block_job_add_bdrv(&s->common, "intermediate node", backing_bs(iter),
>>>>> +                           0, basic_flags, &error_abort);
>>>>
>>>> I don’t understand this change.  Isn’t it doing exactly the same as before?
>>>>
>>>> (If so, I just find it harder to read because every iteration isn’t
>>>> about @iter but backing_bs(iter).)
>>>
>>> Hm, it's the same, but not using base. We may save old loop if calculate base exactly before
>>> the loop (or at least not separated from it by any yield-point)
>>
>> But we are still in stream_start() here.  base cannot have changed yet,
>> can it?
>>
>> (I don’t even think this is about yield points.  As long as
>> stream_start() doesn’t return, the QMP monitor won’t receive any new
>> commands.)
>>
> 
> But block graph may be modified not only from qmp. From block jobs too. If base is another filter, it may
> be dropped in any time.

Ah, yes, that’s true.  And I suppose that can happen in
bdrv_reopen_set_read_only().  Hm.

OK, I still don’t like the loop how it is currently written.  Maybe I’d
like it better with s/iter/parent_bs/?

Well, or you proposed would work, too, i.e. base = backing_bs(bottom)
just before the loop – with a comment that explains why we need that.
That’s probably better.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]