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

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

[Vladimir Sementsov-Ogievskiy]

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.

v6: [resend by Vladimir]
  01: improve comment in block/io.c, suggested by Alberto

v5: [resend by Vladimir]
  01: use comment wording in block/io.c suggested by Alberto
  Now the whole series are reviewed-by Alberto and me.

v4:
trace_stream_start reverted to the base.
bdrv_is_allocated_above_inclusive() deleted and the new parameter
'bool include_base' was added to the bdrv_is_allocated_above().

Andrey Shinkevich (2):
  block: include base when checking image chain for block allocation
  block/stream: introduce a bottom node

Vladimir Sementsov-Ogievskiy (1):
  block/stream: refactor stream_run: drop goto

 include/block/block.h  |  3 +-
 block/commit.c         |  2 +-
 block/io.c             | 20 ++++++++++----
 block/mirror.c         |  2 +-
 block/replication.c    |  2 +-
 block/stream.c         | 62 ++++++++++++++++++++----------------------
 tests/qemu-iotests/245 |  4 +--
 7 files changed, 50 insertions(+), 45 deletions(-)

-- 
2.18.0

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

[Vladimir Sementsov-Ogievskiy]

From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

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 function
bdrv_is_allocated_above() with a new parameter include_base = true
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>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
      [only improve comment in block/io.c as Alberto suggested]
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 include/block/block.h |  3 ++-
 block/commit.c        |  2 +-
 block/io.c            | 20 ++++++++++++++------
 block/mirror.c        |  2 +-
 block/replication.c   |  2 +-
 block/stream.c        |  2 +-
 6 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index c7a26199aa..f98e8582a1 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -447,7 +447,8 @@ int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base,
 int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
                       int64_t *pnum);
 int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
-                            int64_t offset, int64_t bytes, int64_t *pnum);
+                            bool include_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,
diff --git a/block/commit.c b/block/commit.c
index 27537d995b..1750de1029 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -176,7 +176,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
             break;
         }
         /* Copy if allocated above the base */
-        ret = bdrv_is_allocated_above(blk_bs(s->top), blk_bs(s->base),
+        ret = bdrv_is_allocated_above(blk_bs(s->top), blk_bs(s->base), false,
                                       offset, COMMIT_BUFFER_SIZE, &n);
         copy = (ret == 1);
         trace_commit_one_iteration(s, offset, n, ret);
diff --git a/block/io.c b/block/io.c
index dfc153b8d8..3cfa100234 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2316,10 +2316,11 @@ 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
- * offset is allocated in any image of the chain.  Return false otherwise,
- * or negative errno on failure.
+ * Return 1 if (a prefix of) the given range is allocated in any image
+ * between BASE and TOP (BASE is only included if include_base is set).
+ * BASE can be NULL to check if the given offset is allocated in any
+ * image of the chain.  Return 0 otherwise, or negative errno on
+ * failure.
  *
  * 'pnum' is set to the number of bytes (including and immediately
  * following the specified offset) that are known to be in the same
@@ -2331,14 +2332,17 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset,
  */
 int bdrv_is_allocated_above(BlockDriverState *top,
                             BlockDriverState *base,
-                            int64_t offset, int64_t bytes, int64_t *pnum)
+                            bool include_base, int64_t offset,
+                            int64_t bytes, int64_t *pnum)
 {
     BlockDriverState *intermediate;
     int ret;
     int64_t n = bytes;
 
+    assert(base || !include_base);
+
     intermediate = top;
-    while (intermediate && intermediate != base) {
+    while (include_base || 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);
     }
 
diff --git a/block/mirror.c b/block/mirror.c
index ff15cfb197..923548f92f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -807,7 +807,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
             return 0;
         }
 
-        ret = bdrv_is_allocated_above(bs, base, offset, bytes, &count);
+        ret = bdrv_is_allocated_above(bs, base, false, offset, bytes, &count);
         if (ret < 0) {
             return ret;
         }
diff --git a/block/replication.c b/block/replication.c
index 3d4dedddfc..fc8d2adc68 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -272,7 +272,7 @@ static coroutine_fn int replication_co_writev(BlockDriverState *bs,
     while (remaining_sectors > 0) {
         int64_t count;
 
-        ret = bdrv_is_allocated_above(top->bs, base->bs,
+        ret = bdrv_is_allocated_above(top->bs, base->bs, false,
                                       sector_num * BDRV_SECTOR_SIZE,
                                       remaining_sectors * BDRV_SECTOR_SIZE,
                                       &count);
diff --git a/block/stream.c b/block/stream.c
index 1a906fd860..97fddb2608 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -160,7 +160,7 @@ 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,
+            ret = bdrv_is_allocated_above(backing_bs(bs), base, false,
                                           offset, n, &n);
 
             /* Finish early if end of backing file has been reached */
-- 
2.18.0

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1423 bytes --]

On 06.05.19 17:34, Vladimir Sementsov-Ogievskiy wrote:
> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> 
> 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 function
> bdrv_is_allocated_above() with a new parameter include_base = true
> 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.

I don’t quite like crashing somewhere in bdrv_is_allocated().  If you
want it to crash, it should do so at some deliberate point, e.g. by
adding an assert(intermediate) at the beginning of the loop.

Max

> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>       [only improve comment in block/io.c as Alberto suggested]
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  include/block/block.h |  3 ++-
>  block/commit.c        |  2 +-
>  block/io.c            | 20 ++++++++++++++------
>  block/mirror.c        |  2 +-
>  block/replication.c   |  2 +-
>  block/stream.c        |  2 +-
>  6 files changed, 20 insertions(+), 11 deletions(-)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Vladimir Sementsov-Ogievskiy]

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 97fddb2608..65b13b27e0 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -120,13 +120,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);
 
@@ -203,14 +202,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 = {
-- 
2.18.0

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

On 06.05.19 17:34, Vladimir Sementsov-Ogievskiy wrote:
> 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(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Vladimir Sementsov-Ogievskiy]

From: Andrey Shinkevich <andrey.shinkevich@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 <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@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
@@ -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;
@@ -54,7 +54,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);
     }
 }
 
@@ -63,11 +63,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) {
@@ -110,7 +110,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;
@@ -119,7 +119,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;
     }
 
@@ -136,7 +137,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);
     }
 
@@ -159,9 +160,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, false,
+            ret = bdrv_is_allocated_above(backing_bs(bs), s->bottom, true,
                                           offset, n, &n);
-
             /* Finish early if end of backing file has been reached */
             if (ret == 0 && n == 0) {
                 n = len - offset;
@@ -198,7 +198,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
         }
     }
 
-    if (!base) {
+    if (enable_cor) {
         bdrv_disable_copy_on_read(bs);
     }
 
@@ -230,8 +230,10 @@ void stream_start(const char *job_id, BlockDriverState *bs,
     StreamBlockJob *s;
     BlockDriverState *iter;
     bool bs_read_only;
+    int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
+    BlockDriverState *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;
     }
 
@@ -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);
     }
 
-    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;
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index a04c6235c1..006f9f72f0 100644
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -865,9 +865,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
-- 
2.18.0

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3587 bytes --]

On 06.05.19 17:34, Vladimir Sementsov-Ogievskiy wrote:
> From: Andrey Shinkevich <andrey.shinkevich@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 <vsementsov@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Alberto Garcia <berto@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).)

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;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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 <andrey.shinkevich@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 <vsementsov@virtuozzo.com>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Alberto Garcia <berto@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

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3997 bytes --]

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.shinkevich@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 <vsementsov@virtuozzo.com>
>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Reviewed-by: Alberto Garcia <berto@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.)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Vladimir Sementsov-Ogievskiy]

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.shinkevich@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 <vsementsov@virtuozzo.com>
>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> Reviewed-by: Alberto Garcia <berto@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

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4793 bytes --]

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.shinkevich@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 <vsementsov@virtuozzo.com>
>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> Reviewed-by: Alberto Garcia <berto@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Vladimir Sementsov-Ogievskiy]

ping

06.05.2019 18:34, Vladimir Sementsov-Ogievskiy wrote:
> 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.
> 
> v6: [resend by Vladimir]
>    01: improve comment in block/io.c, suggested by Alberto
> 
> v5: [resend by Vladimir]
>    01: use comment wording in block/io.c suggested by Alberto
>    Now the whole series are reviewed-by Alberto and me.
> 
> v4:
> trace_stream_start reverted to the base.
> bdrv_is_allocated_above_inclusive() deleted and the new parameter
> 'bool include_base' was added to the bdrv_is_allocated_above().
> 
> Andrey Shinkevich (2):
>    block: include base when checking image chain for block allocation
>    block/stream: introduce a bottom node
> 
> Vladimir Sementsov-Ogievskiy (1):
>    block/stream: refactor stream_run: drop goto
> 
>   include/block/block.h  |  3 +-
>   block/commit.c         |  2 +-
>   block/io.c             | 20 ++++++++++----
>   block/mirror.c         |  2 +-
>   block/replication.c    |  2 +-
>   block/stream.c         | 62 ++++++++++++++++++++----------------------
>   tests/qemu-iotests/245 |  4 +--
>   7 files changed, 50 insertions(+), 45 deletions(-)
> 


-- 
Best regards,
Vladimir