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

[Max Reitz]

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.)

Max

signature.asc
Description: OpenPGP digital signature