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

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
>>>>
>>>> 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 <address@hidden>
>>>> Signed-off-by: Andrey Shinkevich <address@hidden>
>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>> Reviewed-by: Alberto Garcia <address@hidden>
>>>> ---
>>>>    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.



-- 
Best regards,
Vladimir