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