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

[Vladimir Sementsov-Ogievskiy]

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)

> 
> The rest looks good to me.
> 
> Max
> 
>>       }
>>   
>> -    s->base = base;
>> +    s->bottom = bottom;
>>       s->backing_file_str = g_strdup(backing_file_str);
>>       s->bs_read_only = bs_read_only;
>>       s->chain_frozen = true;
> 


-- 
Best regards,
Vladimir