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.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.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
signature.asc
Description: OpenPGP digital signature
