[Qemu-devel] [PATCH v3 0/3] block/stream: get rid of the base

[Qemu-devel] [PATCH v3 0/3] block/stream: get rid of the base

[Andrey Shinkevich]

This series introduces a bottom intermediate node that eliminates the
dependency on the base that may change while stream job is running.
It happens when stream/commit parallel jobs are running on the same
backing chain. The base node of the stream job may be a top node of
the parallel commit job and can change before the stream job is
completed. We avoid that dependency by introducing the bottom node.

v3:
bottom = bdrv_find_overlay() moved into the stream_start()

v2:
The function bdrv_find_overlay() with the same functionality has been
used in the qmp_block_stream() to find a bottom node.
The algorithm in the bdrv_do_is_allocated_above() was simplified.
The commit messages were modified.

Discussed in the e-mail threads with the message IDs
<1553866154-257311-1-git-send-email-andrey.shinkevich@virtuozzo.com>
<1550762799-830661-1-git-send-email-andrey.shinkevich@virtuozzo.com>
<1554120365-39119-1-git-send-email-andrey.shinkevich@virtuozzo.com>

Andrey Shinkevich (3):
  block: include base when checking image chain for block allocation
  block/stream: refactor stream_run: drop goto
  block/stream: introduce a bottom node

 block/io.c             | 33 ++++++++++++++++++++----
 block/stream.c         | 69 +++++++++++++++++++++++++-------------------------
 block/trace-events     |  2 +-
 include/block/block.h  |  4 +++
 tests/qemu-iotests/245 |  4 +--
 5 files changed, 69 insertions(+), 43 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 0/3] block/stream: get rid of the base

[Andrey Shinkevich]

This series introduces a bottom intermediate node that eliminates the
dependency on the base that may change while stream job is running.
It happens when stream/commit parallel jobs are running on the same
backing chain. The base node of the stream job may be a top node of
the parallel commit job and can change before the stream job is
completed. We avoid that dependency by introducing the bottom node.

v3:
bottom = bdrv_find_overlay() moved into the stream_start()

v2:
The function bdrv_find_overlay() with the same functionality has been
used in the qmp_block_stream() to find a bottom node.
The algorithm in the bdrv_do_is_allocated_above() was simplified.
The commit messages were modified.

Discussed in the e-mail threads with the message IDs
<1553866154-257311-1-git-send-email-andrey.shinkevich@virtuozzo.com>
<1550762799-830661-1-git-send-email-andrey.shinkevich@virtuozzo.com>
<1554120365-39119-1-git-send-email-andrey.shinkevich@virtuozzo.com>

Andrey Shinkevich (3):
  block: include base when checking image chain for block allocation
  block/stream: refactor stream_run: drop goto
  block/stream: introduce a bottom node

 block/io.c             | 33 ++++++++++++++++++++----
 block/stream.c         | 69 +++++++++++++++++++++++++-------------------------
 block/trace-events     |  2 +-
 include/block/block.h  |  4 +++
 tests/qemu-iotests/245 |  4 +--
 5 files changed, 69 insertions(+), 43 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/3] block: include base when checking image chain for block allocation

[Andrey Shinkevich]

This patch is used in the 'block/stream: introduce a bottom node'
that is following. Instead of the base node, the caller may pass
the node that has the base as its backing image to the new function
bdrv_is_allocated_above_inclusive() and get rid of the dependency
on the base that may change during commit/stream parallel jobs.
Now, if the specified base is not found in the backing image chain,
the QEMU will crash.

Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c            | 33 ++++++++++++++++++++++++++++-----
 include/block/block.h |  4 ++++
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/block/io.c b/block/io.c
index dfc153b..7e6019f 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2317,7 +2317,8 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset,
  * Given an image chain: ... -> [BASE] -> [INTER1] -> [INTER2] -> [TOP]
  *
  * Return true if (a prefix of) the given range is allocated in any image
- * between BASE and TOP (inclusive).  BASE can be NULL to check if the given
+ * between BASE and TOP (TOP included). To check the BASE image, set the
+ * 'base_included' to 'true'. The BASE can be NULL to check if the given
  * offset is allocated in any image of the chain.  Return false otherwise,
  * or negative errno on failure.
  *
@@ -2329,16 +2330,19 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset,
  * but 'pnum' will only be 0 when end of file is reached.
  *
  */
-int bdrv_is_allocated_above(BlockDriverState *top,
-                            BlockDriverState *base,
-                            int64_t offset, int64_t bytes, int64_t *pnum)
+static int bdrv_do_is_allocated_above(BlockDriverState *top,
+                                      BlockDriverState *base,
+                                      bool base_included, int64_t offset,
+                                      int64_t bytes, int64_t *pnum)
 {
     BlockDriverState *intermediate;
     int ret;
     int64_t n = bytes;
 
+    assert(base || !base_included);
+
     intermediate = top;
-    while (intermediate && intermediate != base) {
+    while (base_included || intermediate != base) {
         int64_t pnum_inter;
         int64_t size_inter;
 
@@ -2360,6 +2364,10 @@ int bdrv_is_allocated_above(BlockDriverState *top,
             n = pnum_inter;
         }
 
+        if (intermediate == base) {
+            break;
+        }
+
         intermediate = backing_bs(intermediate);
     }
 
@@ -2367,6 +2375,21 @@ int bdrv_is_allocated_above(BlockDriverState *top,
     return 0;
 }
 
+int bdrv_is_allocated_above(BlockDriverState *top,
+                            BlockDriverState *base,
+                            int64_t offset, int64_t bytes, int64_t *pnum)
+{
+    return bdrv_do_is_allocated_above(top, base, false, offset, bytes, pnum);
+}
+
+int bdrv_is_allocated_above_inclusive(BlockDriverState *top,
+                                      BlockDriverState *base,
+                                      int64_t offset, int64_t bytes,
+                                      int64_t *pnum)
+{
+    return bdrv_do_is_allocated_above(top, base, true, offset, bytes, pnum);
+}
+
 typedef struct BdrvVmstateCo {
     BlockDriverState    *bs;
     QEMUIOVector        *qiov;
diff --git a/include/block/block.h b/include/block/block.h
index c7a2619..be1e9a0 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -449,6 +449,10 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
 int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
                             int64_t offset, int64_t bytes, int64_t *pnum);
 
+int bdrv_is_allocated_above_inclusive(BlockDriverState *top,
+                                      BlockDriverState *base,
+                                      int64_t offset, int64_t bytes,
+                                      int64_t *pnum);
 bool bdrv_is_read_only(BlockDriverState *bs);
 int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
                            bool ignore_allow_rdw, Error **errp);
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/3] block: include base when checking image chain for block allocation

[Andrey Shinkevich]

This patch is used in the 'block/stream: introduce a bottom node'
that is following. Instead of the base node, the caller may pass
the node that has the base as its backing image to the new function
bdrv_is_allocated_above_inclusive() and get rid of the dependency
on the base that may change during commit/stream parallel jobs.
Now, if the specified base is not found in the backing image chain,
the QEMU will crash.

Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c            | 33 ++++++++++++++++++++++++++++-----
 include/block/block.h |  4 ++++
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/block/io.c b/block/io.c
index dfc153b..7e6019f 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2317,7 +2317,8 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset,
  * Given an image chain: ... -> [BASE] -> [INTER1] -> [INTER2] -> [TOP]
  *
  * Return true if (a prefix of) the given range is allocated in any image
- * between BASE and TOP (inclusive).  BASE can be NULL to check if the given
+ * between BASE and TOP (TOP included). To check the BASE image, set the
+ * 'base_included' to 'true'. The BASE can be NULL to check if the given
  * offset is allocated in any image of the chain.  Return false otherwise,
  * or negative errno on failure.
  *
@@ -2329,16 +2330,19 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset,
  * but 'pnum' will only be 0 when end of file is reached.
  *
  */
-int bdrv_is_allocated_above(BlockDriverState *top,
-                            BlockDriverState *base,
-                            int64_t offset, int64_t bytes, int64_t *pnum)
+static int bdrv_do_is_allocated_above(BlockDriverState *top,
+                                      BlockDriverState *base,
+                                      bool base_included, int64_t offset,
+                                      int64_t bytes, int64_t *pnum)
 {
     BlockDriverState *intermediate;
     int ret;
     int64_t n = bytes;
 
+    assert(base || !base_included);
+
     intermediate = top;
-    while (intermediate && intermediate != base) {
+    while (base_included || intermediate != base) {
         int64_t pnum_inter;
         int64_t size_inter;
 
@@ -2360,6 +2364,10 @@ int bdrv_is_allocated_above(BlockDriverState *top,
             n = pnum_inter;
         }
 
+        if (intermediate == base) {
+            break;
+        }
+
         intermediate = backing_bs(intermediate);
     }
 
@@ -2367,6 +2375,21 @@ int bdrv_is_allocated_above(BlockDriverState *top,
     return 0;
 }
 
+int bdrv_is_allocated_above(BlockDriverState *top,
+                            BlockDriverState *base,
+                            int64_t offset, int64_t bytes, int64_t *pnum)
+{
+    return bdrv_do_is_allocated_above(top, base, false, offset, bytes, pnum);
+}
+
+int bdrv_is_allocated_above_inclusive(BlockDriverState *top,
+                                      BlockDriverState *base,
+                                      int64_t offset, int64_t bytes,
+                                      int64_t *pnum)
+{
+    return bdrv_do_is_allocated_above(top, base, true, offset, bytes, pnum);
+}
+
 typedef struct BdrvVmstateCo {
     BlockDriverState    *bs;
     QEMUIOVector        *qiov;
diff --git a/include/block/block.h b/include/block/block.h
index c7a2619..be1e9a0 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -449,6 +449,10 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
 int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
                             int64_t offset, int64_t bytes, int64_t *pnum);
 
+int bdrv_is_allocated_above_inclusive(BlockDriverState *top,
+                                      BlockDriverState *base,
+                                      int64_t offset, int64_t bytes,
+                                      int64_t *pnum);
 bool bdrv_is_read_only(BlockDriverState *bs);
 int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
                            bool ignore_allow_rdw, Error **errp);
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/3] block/stream: refactor stream_run: drop goto

[Andrey Shinkevich]

The goto is unnecessary in the stream_run() since the common exit
code was removed in the commit eb23654dbe43b549ea2a9ebff9d8e:
"jobs: utilize job_exit shim".

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/stream.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index bfaebb8..7a4e0dc 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -122,13 +122,12 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
     void *buf;
 
     if (!bs->backing) {
-        goto out;
+        return 0;
     }
 
     len = bdrv_getlength(bs);
     if (len < 0) {
-        ret = len;
-        goto out;
+        return len;
     }
     job_progress_set_remaining(&s->common.job, len);
 
@@ -205,14 +204,10 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
         bdrv_disable_copy_on_read(bs);
     }
 
-    /* Do not remove the backing file if an error was there but ignored.  */
-    ret = error;
-
     qemu_vfree(buf);
 
-out:
-    /* Modify backing chain and close BDSes in main loop */
-    return ret;
+    /* Do not remove the backing file if an error was there but ignored. */
+    return error;
 }
 
 static const BlockJobDriver stream_job_driver = {
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/3] block/stream: refactor stream_run: drop goto

[Andrey Shinkevich]

The goto is unnecessary in the stream_run() since the common exit
code was removed in the commit eb23654dbe43b549ea2a9ebff9d8e:
"jobs: utilize job_exit shim".

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/stream.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index bfaebb8..7a4e0dc 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -122,13 +122,12 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
     void *buf;
 
     if (!bs->backing) {
-        goto out;
+        return 0;
     }
 
     len = bdrv_getlength(bs);
     if (len < 0) {
-        ret = len;
-        goto out;
+        return len;
     }
     job_progress_set_remaining(&s->common.job, len);
 
@@ -205,14 +204,10 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
         bdrv_disable_copy_on_read(bs);
     }
 
-    /* Do not remove the backing file if an error was there but ignored.  */
-    ret = error;
-
     qemu_vfree(buf);
 
-out:
-    /* Modify backing chain and close BDSes in main loop */
-    return ret;
+    /* Do not remove the backing file if an error was there but ignored. */
+    return error;
 }
 
 static const BlockJobDriver stream_job_driver = {
-- 
1.8.3.1

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

[Andrey Shinkevich]

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 <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/stream.c         | 56 +++++++++++++++++++++++++++-----------------------
 block/trace-events     |  2 +-
 tests/qemu-iotests/245 |  4 ++--
 3 files changed, 33 insertions(+), 29 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 7a4e0dc..7a25cd7 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -31,7 +31,7 @@ enum {
 
 typedef struct StreamBlockJob {
     BlockJob common;
-    BlockDriverState *base;
+    BlockDriverState *bottom;
     BlockdevOnError on_error;
     char *backing_file_str;
     bool bs_read_only;
@@ -56,7 +56,7 @@ static void stream_abort(Job *job)
 
     if (s->chain_frozen) {
         BlockJob *bjob = &s->common;
-        bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->base);
+        bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->bottom);
     }
 }
 
@@ -65,11 +65,11 @@ static int stream_prepare(Job *job)
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
     BlockJob *bjob = &s->common;
     BlockDriverState *bs = blk_bs(bjob->blk);
-    BlockDriverState *base = s->base;
+    BlockDriverState *base = backing_bs(s->bottom);
     Error *local_err = NULL;
     int ret = 0;
 
-    bdrv_unfreeze_backing_chain(bs, base);
+    bdrv_unfreeze_backing_chain(bs, s->bottom);
     s->chain_frozen = false;
 
     if (bs->backing) {
@@ -112,7 +112,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
     BlockBackend *blk = s->common.blk;
     BlockDriverState *bs = blk_bs(blk);
-    BlockDriverState *base = s->base;
+    bool enable_cor = !backing_bs(s->bottom);
     int64_t len;
     int64_t offset = 0;
     uint64_t delay_ns = 0;
@@ -121,7 +121,8 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
     int64_t n = 0; /* bytes */
     void *buf;
 
-    if (!bs->backing) {
+    if (bs == s->bottom) {
+        /* Nothing to stream */
         return 0;
     }
 
@@ -138,7 +139,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
      * backing chain since the copy-on-read operation does not take base into
      * account.
      */
-    if (!base) {
+    if (enable_cor) {
         bdrv_enable_copy_on_read(bs);
     }
 
@@ -161,9 +162,8 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
         } else if (ret >= 0) {
             /* Copy if allocated in the intermediate images.  Limit to the
              * known-unallocated area [offset, offset+n*BDRV_SECTOR_SIZE).  */
-            ret = bdrv_is_allocated_above(backing_bs(bs), base,
-                                          offset, n, &n);
-
+            ret = bdrv_is_allocated_above_inclusive(backing_bs(bs), s->bottom,
+                                                    offset, n, &n);
             /* Finish early if end of backing file has been reached */
             if (ret == 0 && n == 0) {
                 n = len - offset;
@@ -200,7 +200,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
         }
     }
 
-    if (!base) {
+    if (enable_cor) {
         bdrv_disable_copy_on_read(bs);
     }
 
@@ -232,8 +232,13 @@ void stream_start(const char *job_id, BlockDriverState *bs,
     StreamBlockJob *s;
     BlockDriverState *iter;
     bool bs_read_only;
+    BlockDriverState *bottom = NULL;
+    int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
+
+    /* Find the bottom node that has the base as its backing image */
+    bottom = bdrv_find_overlay(bs, base);
 
-    if (bdrv_freeze_backing_chain(bs, base, errp) < 0) {
+    if (bdrv_freeze_backing_chain(bs, bottom, errp) < 0) {
         return;
     }
 
@@ -250,32 +255,31 @@ 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);
     }
 
-    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;
 
     s->on_error = on_error;
-    trace_stream_start(bs, base, s);
+    trace_stream_start(bs, bottom, s);
     job_start(&s->common.job);
     return;
 
diff --git a/block/trace-events b/block/trace-events
index 7335a42..5366d45 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -20,7 +20,7 @@ bdrv_co_copy_range_to(void *src, uint64_t src_offset, void *dst, uint64_t dst_of
 
 # stream.c
 stream_one_iteration(void *s, int64_t offset, uint64_t bytes, int is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d"
-stream_start(void *bs, void *base, void *s) "bs %p base %p s %p"
+stream_start(void *bs, void *bottom, void *s) "bs %p bottom %p s %p"
 
 # commit.c
 commit_one_iteration(void *s, int64_t offset, uint64_t bytes, int is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d"
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 7891a21..d11e73c 100644
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -859,9 +859,9 @@ class TestBlockdevReopen(iotests.QMPTestCase):
                              device = 'hd0', base_node = 'hd2', speed = 512 * 1024)
         self.assert_qmp(result, 'return', {})
 
-        # We can't remove hd2 while the stream job is ongoing
+        # We can remove hd2 while the stream job is ongoing
         opts['backing']['backing'] = None
-        self.reopen(opts, {}, "Cannot change 'backing' link from 'hd1' to 'hd2'")
+        self.reopen(opts, {})
 
         # We can't remove hd1 while the stream job is ongoing
         opts['backing'] = None
-- 
1.8.3.1

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

[Andrey Shinkevich]

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 <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/stream.c         | 56 +++++++++++++++++++++++++++-----------------------
 block/trace-events     |  2 +-
 tests/qemu-iotests/245 |  4 ++--
 3 files changed, 33 insertions(+), 29 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 7a4e0dc..7a25cd7 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -31,7 +31,7 @@ enum {
 
 typedef struct StreamBlockJob {
     BlockJob common;
-    BlockDriverState *base;
+    BlockDriverState *bottom;
     BlockdevOnError on_error;
     char *backing_file_str;
     bool bs_read_only;
@@ -56,7 +56,7 @@ static void stream_abort(Job *job)
 
     if (s->chain_frozen) {
         BlockJob *bjob = &s->common;
-        bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->base);
+        bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->bottom);
     }
 }
 
@@ -65,11 +65,11 @@ static int stream_prepare(Job *job)
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
     BlockJob *bjob = &s->common;
     BlockDriverState *bs = blk_bs(bjob->blk);
-    BlockDriverState *base = s->base;
+    BlockDriverState *base = backing_bs(s->bottom);
     Error *local_err = NULL;
     int ret = 0;
 
-    bdrv_unfreeze_backing_chain(bs, base);
+    bdrv_unfreeze_backing_chain(bs, s->bottom);
     s->chain_frozen = false;
 
     if (bs->backing) {
@@ -112,7 +112,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
     BlockBackend *blk = s->common.blk;
     BlockDriverState *bs = blk_bs(blk);
-    BlockDriverState *base = s->base;
+    bool enable_cor = !backing_bs(s->bottom);
     int64_t len;
     int64_t offset = 0;
     uint64_t delay_ns = 0;
@@ -121,7 +121,8 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
     int64_t n = 0; /* bytes */
     void *buf;
 
-    if (!bs->backing) {
+    if (bs == s->bottom) {
+        /* Nothing to stream */
         return 0;
     }
 
@@ -138,7 +139,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
      * backing chain since the copy-on-read operation does not take base into
      * account.
      */
-    if (!base) {
+    if (enable_cor) {
         bdrv_enable_copy_on_read(bs);
     }
 
@@ -161,9 +162,8 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
         } else if (ret >= 0) {
             /* Copy if allocated in the intermediate images.  Limit to the
              * known-unallocated area [offset, offset+n*BDRV_SECTOR_SIZE).  */
-            ret = bdrv_is_allocated_above(backing_bs(bs), base,
-                                          offset, n, &n);
-
+            ret = bdrv_is_allocated_above_inclusive(backing_bs(bs), s->bottom,
+                                                    offset, n, &n);
             /* Finish early if end of backing file has been reached */
             if (ret == 0 && n == 0) {
                 n = len - offset;
@@ -200,7 +200,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
         }
     }
 
-    if (!base) {
+    if (enable_cor) {
         bdrv_disable_copy_on_read(bs);
     }
 
@@ -232,8 +232,13 @@ void stream_start(const char *job_id, BlockDriverState *bs,
     StreamBlockJob *s;
     BlockDriverState *iter;
     bool bs_read_only;
+    BlockDriverState *bottom = NULL;
+    int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
+
+    /* Find the bottom node that has the base as its backing image */
+    bottom = bdrv_find_overlay(bs, base);
 
-    if (bdrv_freeze_backing_chain(bs, base, errp) < 0) {
+    if (bdrv_freeze_backing_chain(bs, bottom, errp) < 0) {
         return;
     }
 
@@ -250,32 +255,31 @@ 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);
     }
 
-    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;
 
     s->on_error = on_error;
-    trace_stream_start(bs, base, s);
+    trace_stream_start(bs, bottom, s);
     job_start(&s->common.job);
     return;
 
diff --git a/block/trace-events b/block/trace-events
index 7335a42..5366d45 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -20,7 +20,7 @@ bdrv_co_copy_range_to(void *src, uint64_t src_offset, void *dst, uint64_t dst_of
 
 # stream.c
 stream_one_iteration(void *s, int64_t offset, uint64_t bytes, int is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d"
-stream_start(void *bs, void *base, void *s) "bs %p base %p s %p"
+stream_start(void *bs, void *bottom, void *s) "bs %p bottom %p s %p"
 
 # commit.c
 commit_one_iteration(void *s, int64_t offset, uint64_t bytes, int is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d"
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 7891a21..d11e73c 100644
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -859,9 +859,9 @@ class TestBlockdevReopen(iotests.QMPTestCase):
                              device = 'hd0', base_node = 'hd2', speed = 512 * 1024)
         self.assert_qmp(result, 'return', {})
 
-        # We can't remove hd2 while the stream job is ongoing
+        # We can remove hd2 while the stream job is ongoing
         opts['backing']['backing'] = None
-        self.reopen(opts, {}, "Cannot change 'backing' link from 'hd1' to 'hd2'")
+        self.reopen(opts, {})
 
         # We can't remove hd1 while the stream job is ongoing
         opts['backing'] = None
-- 
1.8.3.1

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

[Vladimir Sementsov-Ogievskiy]

05.04.2019 19:56, Andrey Shinkevich wrote:
> 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 <vsementsov@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/stream.c         | 56 +++++++++++++++++++++++++++-----------------------
>   block/trace-events     |  2 +-
>   tests/qemu-iotests/245 |  4 ++--
>   3 files changed, 33 insertions(+), 29 deletions(-)
> 

[..]

> @@ -232,8 +232,13 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>       StreamBlockJob *s;
>       BlockDriverState *iter;
>       bool bs_read_only;
> +    BlockDriverState *bottom = NULL;

Why to set NULL? you can set to bdrv_find_overlay() here.

> +    int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
> +
> +    /* Find the bottom node that has the base as its backing image */

I don't think we need comment, as it's exactly what bdrv_find_overly does.

> +    bottom = bdrv_find_overlay(bs, base);
>   
> -    if (bdrv_freeze_backing_chain(bs, base, errp) < 0) {
> +    if (bdrv_freeze_backing_chain(bs, bottom, errp) < 0) {
>           return;
>       }
>   


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

05.04.2019 19:56, Andrey Shinkevich wrote:
> 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 <vsementsov@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/stream.c         | 56 +++++++++++++++++++++++++++-----------------------
>   block/trace-events     |  2 +-
>   tests/qemu-iotests/245 |  4 ++--
>   3 files changed, 33 insertions(+), 29 deletions(-)
> 

[..]

> @@ -232,8 +232,13 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>       StreamBlockJob *s;
>       BlockDriverState *iter;
>       bool bs_read_only;
> +    BlockDriverState *bottom = NULL;

Why to set NULL? you can set to bdrv_find_overlay() here.

> +    int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
> +
> +    /* Find the bottom node that has the base as its backing image */

I don't think we need comment, as it's exactly what bdrv_find_overly does.

> +    bottom = bdrv_find_overlay(bs, base);
>   
> -    if (bdrv_freeze_backing_chain(bs, base, errp) < 0) {
> +    if (bdrv_freeze_backing_chain(bs, bottom, errp) < 0) {
>           return;
>       }
>   


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 1/3] block: include base when checking image chain for block allocation

[Alberto Garcia]

On Fri 05 Apr 2019 06:56:17 PM CEST, Andrey Shinkevich wrote:
> +int bdrv_is_allocated_above(BlockDriverState *top,
> +                            BlockDriverState *base,
> +                            int64_t offset, int64_t bytes, int64_t *pnum)
> +{
> +    return bdrv_do_is_allocated_above(top, base, false, offset, bytes, pnum);
> +}
> +
> +int bdrv_is_allocated_above_inclusive(BlockDriverState *top,
> +                                      BlockDriverState *base,
> +                                      int64_t offset, int64_t bytes,
> +                                      int64_t *pnum)
> +{
> +    return bdrv_do_is_allocated_above(top, base, true, offset, bytes, pnum);
> +}

Instead of having these two, isn't it simpler to add an 'include_base'
parameter to the original function?

Another alternative (I haven't checked this one so it could be more
cumbersome): change the semantics of the function to always include the
base and modify the callers.

Berto

Re: [Qemu-devel] [PATCH v3 1/3] block: include base when checking image chain for block allocation

[Alberto Garcia]

On Fri 05 Apr 2019 06:56:17 PM CEST, Andrey Shinkevich wrote:
> +int bdrv_is_allocated_above(BlockDriverState *top,
> +                            BlockDriverState *base,
> +                            int64_t offset, int64_t bytes, int64_t *pnum)
> +{
> +    return bdrv_do_is_allocated_above(top, base, false, offset, bytes, pnum);
> +}
> +
> +int bdrv_is_allocated_above_inclusive(BlockDriverState *top,
> +                                      BlockDriverState *base,
> +                                      int64_t offset, int64_t bytes,
> +                                      int64_t *pnum)
> +{
> +    return bdrv_do_is_allocated_above(top, base, true, offset, bytes, pnum);
> +}

Instead of having these two, isn't it simpler to add an 'include_base'
parameter to the original function?

Another alternative (I haven't checked this one so it could be more
cumbersome): change the semantics of the function to always include the
base and modify the callers.

Berto

Re: [Qemu-devel] [PATCH v3 1/3] block: include base when checking image chain for block allocation

[Vladimir Sementsov-Ogievskiy]

08.04.2019 18:03, Alberto Garcia wrote:
> On Fri 05 Apr 2019 06:56:17 PM CEST, Andrey Shinkevich wrote:
>> +int bdrv_is_allocated_above(BlockDriverState *top,
>> +                            BlockDriverState *base,
>> +                            int64_t offset, int64_t bytes, int64_t *pnum)
>> +{
>> +    return bdrv_do_is_allocated_above(top, base, false, offset, bytes, pnum);
>> +}
>> +
>> +int bdrv_is_allocated_above_inclusive(BlockDriverState *top,
>> +                                      BlockDriverState *base,
>> +                                      int64_t offset, int64_t bytes,
>> +                                      int64_t *pnum)
>> +{
>> +    return bdrv_do_is_allocated_above(top, base, true, offset, bytes, pnum);
>> +}
> 
> Instead of having these two, isn't it simpler to add an 'include_base'
> parameter to the original function?
> 
> Another alternative (I haven't checked this one so it could be more
> cumbersome): change the semantics of the function to always include the
> base and modify the callers.
> 

The idea was exactly to avoid modifying all callers.. And it seems good, at least as first step.
We may want to modify other callers but it is out of stream series.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 1/3] block: include base when checking image chain for block allocation

[Vladimir Sementsov-Ogievskiy]

08.04.2019 18:03, Alberto Garcia wrote:
> On Fri 05 Apr 2019 06:56:17 PM CEST, Andrey Shinkevich wrote:
>> +int bdrv_is_allocated_above(BlockDriverState *top,
>> +                            BlockDriverState *base,
>> +                            int64_t offset, int64_t bytes, int64_t *pnum)
>> +{
>> +    return bdrv_do_is_allocated_above(top, base, false, offset, bytes, pnum);
>> +}
>> +
>> +int bdrv_is_allocated_above_inclusive(BlockDriverState *top,
>> +                                      BlockDriverState *base,
>> +                                      int64_t offset, int64_t bytes,
>> +                                      int64_t *pnum)
>> +{
>> +    return bdrv_do_is_allocated_above(top, base, true, offset, bytes, pnum);
>> +}
> 
> Instead of having these two, isn't it simpler to add an 'include_base'
> parameter to the original function?
> 
> Another alternative (I haven't checked this one so it could be more
> cumbersome): change the semantics of the function to always include the
> base and modify the callers.
> 

The idea was exactly to avoid modifying all callers.. And it seems good, at least as first step.
We may want to modify other callers but it is out of stream series.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 1/3] block: include base when checking image chain for block allocation

[Andrey Shinkevich]

On 08/04/2019 18:03, Alberto Garcia wrote:
> On Fri 05 Apr 2019 06:56:17 PM CEST, Andrey Shinkevich wrote:
>> +int bdrv_is_allocated_above(BlockDriverState *top,
>> +                            BlockDriverState *base,
>> +                            int64_t offset, int64_t bytes, int64_t *pnum)
>> +{
>> +    return bdrv_do_is_allocated_above(top, base, false, offset, bytes, pnum);
>> +}
>> +
>> +int bdrv_is_allocated_above_inclusive(BlockDriverState *top,
>> +                                      BlockDriverState *base,
>> +                                      int64_t offset, int64_t bytes,
>> +                                      int64_t *pnum)
>> +{
>> +    return bdrv_do_is_allocated_above(top, base, true, offset, bytes, pnum);
>> +}
> 
> Instead of having these two, isn't it simpler to add an 'include_base'
> parameter to the original function?
> 
> Another alternative (I haven't checked this one so it could be more
> cumbersome): change the semantics of the function to always include the
> base and modify the callers.
> 
> Berto
> 

The idea behind those two functions was to keep the rest of the code
unmodified. Currently, we have the issue with the block-stream
parallel jobs. What if we manage this case first and then, when
proved to be robust, take care of the rest?
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH v3 1/3] block: include base when checking image chain for block allocation

[Andrey Shinkevich]

On 08/04/2019 18:03, Alberto Garcia wrote:
> On Fri 05 Apr 2019 06:56:17 PM CEST, Andrey Shinkevich wrote:
>> +int bdrv_is_allocated_above(BlockDriverState *top,
>> +                            BlockDriverState *base,
>> +                            int64_t offset, int64_t bytes, int64_t *pnum)
>> +{
>> +    return bdrv_do_is_allocated_above(top, base, false, offset, bytes, pnum);
>> +}
>> +
>> +int bdrv_is_allocated_above_inclusive(BlockDriverState *top,
>> +                                      BlockDriverState *base,
>> +                                      int64_t offset, int64_t bytes,
>> +                                      int64_t *pnum)
>> +{
>> +    return bdrv_do_is_allocated_above(top, base, true, offset, bytes, pnum);
>> +}
> 
> Instead of having these two, isn't it simpler to add an 'include_base'
> parameter to the original function?
> 
> Another alternative (I haven't checked this one so it could be more
> cumbersome): change the semantics of the function to always include the
> base and modify the callers.
> 
> Berto
> 

The idea behind those two functions was to keep the rest of the code
unmodified. Currently, we have the issue with the block-stream
parallel jobs. What if we manage this case first and then, when
proved to be robust, take care of the rest?
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH v3 1/3] block: include base when checking image chain for block allocation

[Alberto Garcia]

On Mon 08 Apr 2019 05:16:22 PM CEST, Andrey Shinkevich wrote:
> On 08/04/2019 18:03, Alberto Garcia wrote:
>> On Fri 05 Apr 2019 06:56:17 PM CEST, Andrey Shinkevich wrote:
>>> +int bdrv_is_allocated_above(BlockDriverState *top,
>>> +                            BlockDriverState *base,
>>> +                            int64_t offset, int64_t bytes, int64_t *pnum)
>>> +{
>>> +    return bdrv_do_is_allocated_above(top, base, false, offset, bytes, pnum);
>>> +}
>>> +
>>> +int bdrv_is_allocated_above_inclusive(BlockDriverState *top,
>>> +                                      BlockDriverState *base,
>>> +                                      int64_t offset, int64_t bytes,
>>> +                                      int64_t *pnum)
>>> +{
>>> +    return bdrv_do_is_allocated_above(top, base, true, offset, bytes, pnum);
>>> +}
>> 
>> Instead of having these two, isn't it simpler to add an 'include_base'
>> parameter to the original function?
>> 
>> Another alternative (I haven't checked this one so it could be more
>> cumbersome): change the semantics of the function to always include the
>> base and modify the callers.
>
> The idea behind those two functions was to keep the rest of the code
> unmodified. Currently, we have the issue with the block-stream
> parallel jobs. What if we manage this case first and then, when proved
> to be robust, take care of the rest?

Sure, that makes sense if you need to do significant changes to the
other callers, but if you just need to pass an extra 'false'
parameter...

It's not a big deal, I just think you'd have a simpler patch.

Berto

Re: [Qemu-devel] [PATCH v3 1/3] block: include base when checking image chain for block allocation

[Alberto Garcia]

On Mon 08 Apr 2019 05:16:22 PM CEST, Andrey Shinkevich wrote:
> On 08/04/2019 18:03, Alberto Garcia wrote:
>> On Fri 05 Apr 2019 06:56:17 PM CEST, Andrey Shinkevich wrote:
>>> +int bdrv_is_allocated_above(BlockDriverState *top,
>>> +                            BlockDriverState *base,
>>> +                            int64_t offset, int64_t bytes, int64_t *pnum)
>>> +{
>>> +    return bdrv_do_is_allocated_above(top, base, false, offset, bytes, pnum);
>>> +}
>>> +
>>> +int bdrv_is_allocated_above_inclusive(BlockDriverState *top,
>>> +                                      BlockDriverState *base,
>>> +                                      int64_t offset, int64_t bytes,
>>> +                                      int64_t *pnum)
>>> +{
>>> +    return bdrv_do_is_allocated_above(top, base, true, offset, bytes, pnum);
>>> +}
>> 
>> Instead of having these two, isn't it simpler to add an 'include_base'
>> parameter to the original function?
>> 
>> Another alternative (I haven't checked this one so it could be more
>> cumbersome): change the semantics of the function to always include the
>> base and modify the callers.
>
> The idea behind those two functions was to keep the rest of the code
> unmodified. Currently, we have the issue with the block-stream
> parallel jobs. What if we manage this case first and then, when proved
> to be robust, take care of the rest?

Sure, that makes sense if you need to do significant changes to the
other callers, but if you just need to pass an extra 'false'
parameter...

It's not a big deal, I just think you'd have a simpler patch.

Berto

Re: [Qemu-devel] [PATCH v3 1/3] block: include base when checking image chain for block allocation

[Andrey Shinkevich]

On 08/04/2019 18:22, Alberto Garcia wrote:
> On Mon 08 Apr 2019 05:16:22 PM CEST, Andrey Shinkevich wrote:
>> On 08/04/2019 18:03, Alberto Garcia wrote:
>>> On Fri 05 Apr 2019 06:56:17 PM CEST, Andrey Shinkevich wrote:
>>>> +int bdrv_is_allocated_above(BlockDriverState *top,
>>>> +                            BlockDriverState *base,
>>>> +                            int64_t offset, int64_t bytes, int64_t *pnum)
>>>> +{
>>>> +    return bdrv_do_is_allocated_above(top, base, false, offset, bytes, pnum);
>>>> +}
>>>> +
>>>> +int bdrv_is_allocated_above_inclusive(BlockDriverState *top,
>>>> +                                      BlockDriverState *base,
>>>> +                                      int64_t offset, int64_t bytes,
>>>> +                                      int64_t *pnum)
>>>> +{
>>>> +    return bdrv_do_is_allocated_above(top, base, true, offset, bytes, pnum);
>>>> +}
>>>
>>> Instead of having these two, isn't it simpler to add an 'include_base'
>>> parameter to the original function?
>>>
>>> Another alternative (I haven't checked this one so it could be more
>>> cumbersome): change the semantics of the function to always include the
>>> base and modify the callers.
>>
>> The idea behind those two functions was to keep the rest of the code
>> unmodified. Currently, we have the issue with the block-stream
>> parallel jobs. What if we manage this case first and then, when proved
>> to be robust, take care of the rest?
> 
> Sure, that makes sense if you need to do significant changes to the
> other callers, but if you just need to pass an extra 'false'
> parameter...
> 
> It's not a big deal, I just think you'd have a simpler patch.
> 
> Berto
> 

OK, I'll do that with the next v4.
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH v3 1/3] block: include base when checking image chain for block allocation

[Andrey Shinkevich]

On 08/04/2019 18:22, Alberto Garcia wrote:
> On Mon 08 Apr 2019 05:16:22 PM CEST, Andrey Shinkevich wrote:
>> On 08/04/2019 18:03, Alberto Garcia wrote:
>>> On Fri 05 Apr 2019 06:56:17 PM CEST, Andrey Shinkevich wrote:
>>>> +int bdrv_is_allocated_above(BlockDriverState *top,
>>>> +                            BlockDriverState *base,
>>>> +                            int64_t offset, int64_t bytes, int64_t *pnum)
>>>> +{
>>>> +    return bdrv_do_is_allocated_above(top, base, false, offset, bytes, pnum);
>>>> +}
>>>> +
>>>> +int bdrv_is_allocated_above_inclusive(BlockDriverState *top,
>>>> +                                      BlockDriverState *base,
>>>> +                                      int64_t offset, int64_t bytes,
>>>> +                                      int64_t *pnum)
>>>> +{
>>>> +    return bdrv_do_is_allocated_above(top, base, true, offset, bytes, pnum);
>>>> +}
>>>
>>> Instead of having these two, isn't it simpler to add an 'include_base'
>>> parameter to the original function?
>>>
>>> Another alternative (I haven't checked this one so it could be more
>>> cumbersome): change the semantics of the function to always include the
>>> base and modify the callers.
>>
>> The idea behind those two functions was to keep the rest of the code
>> unmodified. Currently, we have the issue with the block-stream
>> parallel jobs. What if we manage this case first and then, when proved
>> to be robust, take care of the rest?
> 
> Sure, that makes sense if you need to do significant changes to the
> other callers, but if you just need to pass an extra 'false'
> parameter...
> 
> It's not a big deal, I just think you'd have a simpler patch.
> 
> Berto
> 

OK, I'll do that with the next v4.
-- 
With the best regards,
Andrey Shinkevich

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

[Alberto Garcia]

On Fri 05 Apr 2019 06:56:19 PM CEST, Andrey Shinkevich wrote:
> @@ -232,8 +232,13 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>      StreamBlockJob *s;
>      BlockDriverState *iter;
>      bool bs_read_only;
> +    BlockDriverState *bottom = NULL;
> +    int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
> +
> +    /* Find the bottom node that has the base as its backing image */
> +    bottom = bdrv_find_overlay(bs, base);

What happens if bs == base here ??

> +    /*
> +     * 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);
>      }

This stops when iter == bottom, so bottom is not actually blocked.

> diff --git a/block/trace-events b/block/trace-events
> index 7335a42..5366d45 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -20,7 +20,7 @@ bdrv_co_copy_range_to(void *src, uint64_t src_offset, void *dst, uint64_t dst_of
>  
>  # stream.c
>  stream_one_iteration(void *s, int64_t offset, uint64_t bytes, int is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d"
> -stream_start(void *bs, void *base, void *s) "bs %p base %p s %p"
> +stream_start(void *bs, void *bottom, void *s) "bs %p bottom %p s %p"

Is this change still correct? We don't pass bottom anymore.

Berto

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

[Alberto Garcia]

On Fri 05 Apr 2019 06:56:19 PM CEST, Andrey Shinkevich wrote:
> @@ -232,8 +232,13 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>      StreamBlockJob *s;
>      BlockDriverState *iter;
>      bool bs_read_only;
> +    BlockDriverState *bottom = NULL;
> +    int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
> +
> +    /* Find the bottom node that has the base as its backing image */
> +    bottom = bdrv_find_overlay(bs, base);

What happens if bs == base here ??

> +    /*
> +     * 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);
>      }

This stops when iter == bottom, so bottom is not actually blocked.

> diff --git a/block/trace-events b/block/trace-events
> index 7335a42..5366d45 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -20,7 +20,7 @@ bdrv_co_copy_range_to(void *src, uint64_t src_offset, void *dst, uint64_t dst_of
>  
>  # stream.c
>  stream_one_iteration(void *s, int64_t offset, uint64_t bytes, int is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d"
> -stream_start(void *bs, void *base, void *s) "bs %p base %p s %p"
> +stream_start(void *bs, void *bottom, void *s) "bs %p bottom %p s %p"

Is this change still correct? We don't pass bottom anymore.

Berto

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

[Andrey Shinkevich]

On 08/04/2019 18:39, Alberto Garcia wrote:
> On Fri 05 Apr 2019 06:56:19 PM CEST, Andrey Shinkevich wrote:
>> @@ -232,8 +232,13 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>>       StreamBlockJob *s;
>>       BlockDriverState *iter;
>>       bool bs_read_only;
>> +    BlockDriverState *bottom = NULL;
>> +    int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
>> +
>> +    /* Find the bottom node that has the base as its backing image */
>> +    bottom = bdrv_find_overlay(bs, base);
> 
> What happens if bs == base here ??
> 
>> +    /*
>> +     * 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);
>>       }
> 
> This stops when iter == bottom, so bottom is not actually blocked.
> 
>> diff --git a/block/trace-events b/block/trace-events
>> index 7335a42..5366d45 100644
>> --- a/block/trace-events
>> +++ b/block/trace-events
>> @@ -20,7 +20,7 @@ bdrv_co_copy_range_to(void *src, uint64_t src_offset, void *dst, uint64_t dst_of
>>   
>>   # stream.c
>>   stream_one_iteration(void *s, int64_t offset, uint64_t bytes, int is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d"
>> -stream_start(void *bs, void *base, void *s) "bs %p base %p s %p"
>> +stream_start(void *bs, void *bottom, void *s) "bs %p bottom %p s %p"
> 
> Is this change still correct? We don't pass bottom anymore.
> 
> Berto
> 

Yes, I will roll It back.
-- 
With the best regards,
Andrey Shinkevich

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

[Andrey Shinkevich]

On 08/04/2019 18:39, Alberto Garcia wrote:
> On Fri 05 Apr 2019 06:56:19 PM CEST, Andrey Shinkevich wrote:
>> @@ -232,8 +232,13 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>>       StreamBlockJob *s;
>>       BlockDriverState *iter;
>>       bool bs_read_only;
>> +    BlockDriverState *bottom = NULL;
>> +    int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
>> +
>> +    /* Find the bottom node that has the base as its backing image */
>> +    bottom = bdrv_find_overlay(bs, base);
> 
> What happens if bs == base here ??
> 
>> +    /*
>> +     * 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);
>>       }
> 
> This stops when iter == bottom, so bottom is not actually blocked.
> 
>> diff --git a/block/trace-events b/block/trace-events
>> index 7335a42..5366d45 100644
>> --- a/block/trace-events
>> +++ b/block/trace-events
>> @@ -20,7 +20,7 @@ bdrv_co_copy_range_to(void *src, uint64_t src_offset, void *dst, uint64_t dst_of
>>   
>>   # stream.c
>>   stream_one_iteration(void *s, int64_t offset, uint64_t bytes, int is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d"
>> -stream_start(void *bs, void *base, void *s) "bs %p base %p s %p"
>> +stream_start(void *bs, void *bottom, void *s) "bs %p bottom %p s %p"
> 
> Is this change still correct? We don't pass bottom anymore.
> 
> Berto
> 

Yes, I will roll It back.
-- 
With the best regards,
Andrey Shinkevich

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

[Andrey Shinkevich]

On 08/04/2019 18:39, Alberto Garcia wrote:
> On Fri 05 Apr 2019 06:56:19 PM CEST, Andrey Shinkevich wrote:
>> @@ -232,8 +232,13 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>>       StreamBlockJob *s;
>>       BlockDriverState *iter;
>>       bool bs_read_only;
>> +    BlockDriverState *bottom = NULL;
>> +    int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
>> +
>> +    /* Find the bottom node that has the base as its backing image */
>> +    bottom = bdrv_find_overlay(bs, base);
> 
> What happens if bs == base here ??
> 

Actually, that condition is checked in the qmp_block_stream().
I used to have the 'assert(bottom)' after the call to the
bdrv_find_overlay(bs, base) in the qmp_block_stream() of the v2.

>> +    /*
>> +     * 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);
>>       }
> 
> This stops when iter == bottom, so bottom is not actually blocked.

The bottom is actually blocked because
backing_bs(iter) == bottom is passed to the block_job_add_bdrv()
with the last iteration of the loop.

> 
>> diff --git a/block/trace-events b/block/trace-events
>> index 7335a42..5366d45 100644
>> --- a/block/trace-events
>> +++ b/block/trace-events
>> @@ -20,7 +20,7 @@ bdrv_co_copy_range_to(void *src, uint64_t src_offset, void *dst, uint64_t dst_of
>>   
>>   # stream.c
>>   stream_one_iteration(void *s, int64_t offset, uint64_t bytes, int is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d"
>> -stream_start(void *bs, void *base, void *s) "bs %p base %p s %p"
>> +stream_start(void *bs, void *bottom, void *s) "bs %p bottom %p s %p"
> 
> Is this change still correct? We don't pass bottom anymore.
> 
> Berto
> 

-- 
With the best regards,
Andrey Shinkevich

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

[Andrey Shinkevich]

On 08/04/2019 18:39, Alberto Garcia wrote:
> On Fri 05 Apr 2019 06:56:19 PM CEST, Andrey Shinkevich wrote:
>> @@ -232,8 +232,13 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>>       StreamBlockJob *s;
>>       BlockDriverState *iter;
>>       bool bs_read_only;
>> +    BlockDriverState *bottom = NULL;
>> +    int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
>> +
>> +    /* Find the bottom node that has the base as its backing image */
>> +    bottom = bdrv_find_overlay(bs, base);
> 
> What happens if bs == base here ??
> 

Actually, that condition is checked in the qmp_block_stream().
I used to have the 'assert(bottom)' after the call to the
bdrv_find_overlay(bs, base) in the qmp_block_stream() of the v2.

>> +    /*
>> +     * 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);
>>       }
> 
> This stops when iter == bottom, so bottom is not actually blocked.

The bottom is actually blocked because
backing_bs(iter) == bottom is passed to the block_job_add_bdrv()
with the last iteration of the loop.

> 
>> diff --git a/block/trace-events b/block/trace-events
>> index 7335a42..5366d45 100644
>> --- a/block/trace-events
>> +++ b/block/trace-events
>> @@ -20,7 +20,7 @@ bdrv_co_copy_range_to(void *src, uint64_t src_offset, void *dst, uint64_t dst_of
>>   
>>   # stream.c
>>   stream_one_iteration(void *s, int64_t offset, uint64_t bytes, int is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d"
>> -stream_start(void *bs, void *base, void *s) "bs %p base %p s %p"
>> +stream_start(void *bs, void *bottom, void *s) "bs %p bottom %p s %p"
> 
> Is this change still correct? We don't pass bottom anymore.
> 
> Berto
> 

-- 
With the best regards,
Andrey Shinkevich

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

[Alberto Garcia]

On Mon 08 Apr 2019 08:17:37 PM CEST, Andrey Shinkevich wrote:
>>> +    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);
>>>       }
>> 
>> This stops when iter == bottom, so bottom is not actually blocked.
>
> The bottom is actually blocked because backing_bs(iter) == bottom is
> passed to the block_job_add_bdrv() with the last iteration of the
> loop.

Right, I hadn't noticed that you are passing backing_bs(iter) to
block_job_add_bdrv() now.

Berto

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

[Alberto Garcia]

On Mon 08 Apr 2019 08:17:37 PM CEST, Andrey Shinkevich wrote:
>>> +    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);
>>>       }
>> 
>> This stops when iter == bottom, so bottom is not actually blocked.
>
> The bottom is actually blocked because backing_bs(iter) == bottom is
> passed to the block_job_add_bdrv() with the last iteration of the
> loop.

Right, I hadn't noticed that you are passing backing_bs(iter) to
block_job_add_bdrv() now.

Berto