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.shinkev...@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 <vsement...@virtuozzo.com>
>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkev...@virtuozzo.com>
>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
>>>> Reviewed-by: Alberto Garcia <be...@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.
--
Best regards,
Vladimir
