[PATCH 0/7] Apply COR-filter to the block-stream permanently

[PATCH 0/7] Apply COR-filter to the block-stream permanently

[Andrey Shinkevich]

Note: this series is based on the one "block: Deal with filters"
      by Max Reitz that can be found in the branches:

      https://git.xanclic.moe/XanClic/qemu child-access-functions-v6
      https://github.com/XanClic/qemu child-access-functions-v6

      When running iotests, apply "char-socket: Fix race condition"
      to avoid sporadic segmentation faults.

With this series, all the block-stream COR operations pass through
the COR-filter.

Andrey Shinkevich (7):
  block: prepare block-stream for using COR-filter
  stream: exclude a link to filter from freezing
  block: protect parallel jobs from overlapping
  copy-on-read: Support refreshing filename
  qapi: add filter-node-name to block-stream
  iotests: prepare 245 for using filter in block-stream
  block: apply COR-filter to block-stream jobs

 block/copy-on-read.c       |   7 ++
 block/stream.c             | 170 +++++++++++++++++++++++++++++++++++++++------
 blockdev.c                 |  15 +++-
 blockjob.c                 |  15 +++-
 include/block/block_int.h  |   7 +-
 monitor/hmp-cmds.c         |   4 +-
 qapi/block-core.json       |   6 ++
 tests/qemu-iotests/030     |   6 +-
 tests/qemu-iotests/141.out |   2 +-
 tests/qemu-iotests/245     |  15 ++--
 10 files changed, 210 insertions(+), 37 deletions(-)

-- 
1.8.3.1

[PATCH 1/7] block: prepare block-stream for using COR-filter

[Andrey Shinkevich]

This patch is the first one in the series where the COR-filter node
will be hard-coded for using in the block-stream job. The job may
be run with a block-commit job in parallel. Set the condition to
avoid the job conflicts.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 blockdev.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 758e0b5..72d28ce 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3297,7 +3297,9 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
     }
 
     /* Check for op blockers in the whole chain between bs and base */
-    for (iter = bs; iter && iter != base_bs; iter = bdrv_filtered_bs(iter)) {
+    for (iter = bdrv_skip_rw_filters(bs);
+        iter && iter != bdrv_skip_rw_filters(base_bs);
+        iter = bdrv_backing_chain_next(iter)) {
         if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
             goto out;
         }
@@ -3455,7 +3457,8 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
 
     assert(bdrv_get_aio_context(base_bs) == aio_context);
 
-    for (iter = top_bs; iter != bdrv_filtered_bs(base_bs);
+    for (iter = bdrv_skip_rw_filters(top_bs);
+         iter != bdrv_filtered_bs(base_bs);
          iter = bdrv_filtered_bs(iter))
     {
         if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_COMMIT_TARGET, errp)) {
-- 
1.8.3.1

Re: [PATCH 1/7] block: prepare block-stream for using COR-filter

[Vladimir Sementsov-Ogievskiy]

20.04.2020 21:36, Andrey Shinkevich wrote:
> This patch is the first one in the series where the COR-filter node
> will be hard-coded for using in the block-stream job. The job may
> be run with a block-commit job in parallel. Set the condition to
> avoid the job conflicts.

I think, just skipping all filters from checking is wrong.

What is the problem, exactly?

As I understand, we just need the following logic:

stream job, being started with top and base parameters should:

1. calculate bottom-node = non-filter-overlay(base), which assumes finding
last non-filter in a chain from top to base, excluding base

2. I think, we should leave top as is, even if it is filter, it's up to user.

3. add stream-filter above top

4. Take any locks (freeze, op-blockers, etc) on the chain from stream-filter to bottom-node (including both ends), so nobody should touch these nodes. Do not lock any other nodes.

Similarly, commit job, being started with top and base parameters should:

1. I think, if base is a filter, we should set base = non-filter-overlay(base).

2. I think, we should leave top as is, even if it is filter, it's up to user. (hmm, so, commit may be used to remove filters ?)

3. Add commit-filter above top

4. Take any locks (freeze, op-blockers, etc) on the chain from commit-filter to base (including both ends), so nobody should touch these nodes. Do not lock any other nodes.

====

If we make it behave as such, is there still a problem?

> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   blockdev.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 758e0b5..72d28ce 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3297,7 +3297,9 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>       }
>   
>       /* Check for op blockers in the whole chain between bs and base */
> -    for (iter = bs; iter && iter != base_bs; iter = bdrv_filtered_bs(iter)) {
> +    for (iter = bdrv_skip_rw_filters(bs);
> +        iter && iter != bdrv_skip_rw_filters(base_bs);
> +        iter = bdrv_backing_chain_next(iter)) {
>           if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
>               goto out;
>           }
> @@ -3455,7 +3457,8 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
>   
>       assert(bdrv_get_aio_context(base_bs) == aio_context);
>   
> -    for (iter = top_bs; iter != bdrv_filtered_bs(base_bs);
> +    for (iter = bdrv_skip_rw_filters(top_bs);
> +         iter != bdrv_filtered_bs(base_bs);
>            iter = bdrv_filtered_bs(iter))
>       {
>           if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_COMMIT_TARGET, errp)) {
> 


-- 
Best regards,
Vladimir

[PATCH 2/7] stream: exclude a link to filter from freezing

[Andrey Shinkevich]

A node above the base can be the filter of the concurrent job. In that
case, the filter cannot be removed being a part of the frozen chain.
Exclude the link to filter node from freezing and provide the safety
check for a concurrent job.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 block/stream.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index bd4a351..d8b4bbe 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -244,7 +244,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
          above_base = bdrv_filtered_bs(above_base))
     {}
 
-    if (bdrv_freeze_chain(bs, above_base, errp) < 0) {
+    if (bdrv_freeze_chain(bs, bottom_cow_node, errp) < 0) {
         return;
     }
 
@@ -257,6 +257,15 @@ void stream_start(const char *job_id, BlockDriverState *bs,
         }
     }
 
+    /*
+     * Check for an overlapping block-commit job that is not allowed.
+     */
+    if (bdrv_freeze_chain(bottom_cow_node, above_base, errp) < 0) {
+        goto fail;
+    } else {
+        bdrv_unfreeze_chain(bottom_cow_node, above_base);
+    }
+
     /* Prevent concurrent jobs trying to modify the graph structure here, we
      * already have our own plans. Also don't allow resize as the image size is
      * queried only at the job start and then cached. */
@@ -276,7 +285,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
      * bdrv_reopen_set_read_only() due to parallel block jobs running.
      */
     base = bdrv_filtered_bs(above_base);
-    for (iter = bdrv_filtered_bs(bs); iter && iter != base;
+    for (iter = bdrv_filtered_bs(bs);
+         iter && iter != base && iter->drv && !iter->drv->is_filter;
          iter = bdrv_filtered_bs(iter))
     {
         block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
@@ -298,5 +308,5 @@ fail:
     if (bs_read_only) {
         bdrv_reopen_set_read_only(bs, true, NULL);
     }
-    bdrv_unfreeze_chain(bs, above_base);
+    bdrv_unfreeze_chain(bs, bottom_cow_node);
 }
-- 
1.8.3.1

Re: [PATCH 2/7] stream: exclude a link to filter from freezing

[Vladimir Sementsov-Ogievskiy]

20.04.2020 21:36, Andrey Shinkevich wrote:
> A node above the base can be the filter of the concurrent job. In that
> case, the filter cannot be removed being a part of the frozen chain.
> Exclude the link to filter node from freezing and provide the safety
> check for a concurrent job.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block/stream.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index bd4a351..d8b4bbe 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -244,7 +244,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>            above_base = bdrv_filtered_bs(above_base))
>       {}
>   
> -    if (bdrv_freeze_chain(bs, above_base, errp) < 0) {
> +    if (bdrv_freeze_chain(bs, bottom_cow_node, errp) < 0) {
>           return;
>       }
>   
> @@ -257,6 +257,15 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>           }
>       }
>   
> +    /*
> +     * Check for an overlapping block-commit job that is not allowed.
> +     */
> +    if (bdrv_freeze_chain(bottom_cow_node, above_base, errp) < 0) {
> +        goto fail;
> +    } else {
> +        bdrv_unfreeze_chain(bottom_cow_node, above_base);
> +    }
> +
>       /* Prevent concurrent jobs trying to modify the graph structure here, we
>        * already have our own plans. Also don't allow resize as the image size is
>        * queried only at the job start and then cached. */
> @@ -276,7 +285,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>        * bdrv_reopen_set_read_only() due to parallel block jobs running.
>        */
>       base = bdrv_filtered_bs(above_base);
> -    for (iter = bdrv_filtered_bs(bs); iter && iter != base;
> +    for (iter = bdrv_filtered_bs(bs);
> +         iter && iter != base && iter->drv && !iter->drv->is_filter;
>            iter = bdrv_filtered_bs(iter))
>       {
>           block_job_add_bdrv(&s->common, "intermediate node", iter, 0,

So, we still "own" these nodes above base, but just exclude them from freezing?
It's wrong I think. We should exclude them at all (look my previous answer).

> @@ -298,5 +308,5 @@ fail:
>       if (bs_read_only) {
>           bdrv_reopen_set_read_only(bs, true, NULL);
>       }
> -    bdrv_unfreeze_chain(bs, above_base);
> +    bdrv_unfreeze_chain(bs, bottom_cow_node);
>   }
> 


-- 
Best regards,
Vladimir

[PATCH 3/7] block: protect parallel jobs from overlapping

[Andrey Shinkevich]

When it comes to the check for the blocked operations, the node may be
a filter linked to blk. In that case, do not miss to set blocked
operations for the underlying node.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 blockjob.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/blockjob.c b/blockjob.c
index 73d9f1b..2898929 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -189,7 +189,14 @@ void block_job_remove_all_bdrv(BlockJob *job)
     GSList *l;
     for (l = job->nodes; l; l = l->next) {
         BdrvChild *c = l->data;
-        bdrv_op_unblock_all(c->bs, job->blocker);
+        BlockDriverState *bs = c->bs;
+        bdrv_op_unblock_all(bs, job->blocker);
+        if (bs->drv && bs->drv->is_filter) {
+            bs = bdrv_filtered_bs(bs);
+            if (bs) {
+                bdrv_op_unblock_all(bs, job->blocker);
+            }
+        }
         bdrv_root_unref_child(c);
     }
     g_slist_free(job->nodes);
@@ -230,6 +237,12 @@ int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
 
     job->nodes = g_slist_prepend(job->nodes, c);
     bdrv_op_block_all(bs, job->blocker);
+    if (bs->drv && bs->drv->is_filter) {
+        bs = bdrv_filtered_bs(bs);
+        if (bs) {
+            bdrv_op_block_all(bs, job->blocker);
+        }
+    }
 
     return 0;
 }
-- 
1.8.3.1

Re: [PATCH 3/7] block: protect parallel jobs from overlapping

[Vladimir Sementsov-Ogievskiy]

20.04.2020 21:36, Andrey Shinkevich wrote:
> When it comes to the check for the blocked operations, the node may be
> a filter linked to blk.

"blk" commonly refers to BlockBackend, which is unrelated here. You mean just a filter.

> In that case, do not miss to set blocked
> operations for the underlying node.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   blockjob.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 73d9f1b..2898929 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -189,7 +189,14 @@ void block_job_remove_all_bdrv(BlockJob *job)
>       GSList *l;
>       for (l = job->nodes; l; l = l->next) {
>           BdrvChild *c = l->data;
> -        bdrv_op_unblock_all(c->bs, job->blocker);
> +        BlockDriverState *bs = c->bs;
> +        bdrv_op_unblock_all(bs, job->blocker);
> +        if (bs->drv && bs->drv->is_filter) {
> +            bs = bdrv_filtered_bs(bs);
> +            if (bs) {
> +                bdrv_op_unblock_all(bs, job->blocker);
> +            }
> +        }
>           bdrv_root_unref_child(c);
>       }
>       g_slist_free(job->nodes);
> @@ -230,6 +237,12 @@ int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
>   
>       job->nodes = g_slist_prepend(job->nodes, c);
>       bdrv_op_block_all(bs, job->blocker);
> +    if (bs->drv && bs->drv->is_filter) {
> +        bs = bdrv_filtered_bs(bs);
> +        if (bs) {
> +            bdrv_op_block_all(bs, job->blocker);

This will lead to setting op blocker twice, if there are filters inside backing chain. Is it safe?

Still, I don't think it's correct thing. Job should add all it's nodes by hand. If it add some
filter node, but don't add it's filtered node, it is definitely doing something wrong (see my
answer to 1/7).


> +        }
> +    }
>   
>       return 0;
>   }
> 


-- 
Best regards,
Vladimir

[PATCH 4/7] copy-on-read: Support refreshing filename

[Andrey Shinkevich]

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 block/copy-on-read.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index ad6577d..e45eab9 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -21,6 +21,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "block/block_int.h"
 #include "qemu/module.h"
 
@@ -141,6 +142,11 @@ static bool cor_recurse_is_first_non_filter(BlockDriverState *bs,
     return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
 }
 
+static void cor_refresh_filename(BlockDriverState *bs)
+{
+    pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
+            bs->file->bs->filename);
+}
 
 static BlockDriver bdrv_copy_on_read = {
     .format_name                        = "copy-on-read",
@@ -161,6 +167,7 @@ static BlockDriver bdrv_copy_on_read = {
     .bdrv_lock_medium                   = cor_lock_medium,
 
     .bdrv_recurse_is_first_non_filter   = cor_recurse_is_first_non_filter,
+    .bdrv_refresh_filename              = cor_refresh_filename,
 
     .has_variable_length                = true,
     .is_filter                          = true,
-- 
1.8.3.1

Re: [PATCH 4/7] copy-on-read: Support refreshing filename

[Vladimir Sementsov-Ogievskiy]

Some information about why we need it would be nice.

Still I see same function in commit.c, so most probably it's OK.


20.04.2020 21:36, Andrey Shinkevich wrote:
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block/copy-on-read.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index ad6577d..e45eab9 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -21,6 +21,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>   #include "block/block_int.h"
>   #include "qemu/module.h"
>   
> @@ -141,6 +142,11 @@ static bool cor_recurse_is_first_non_filter(BlockDriverState *bs,
>       return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
>   }
>   
> +static void cor_refresh_filename(BlockDriverState *bs)
> +{
> +    pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
> +            bs->file->bs->filename);
> +}
>   
>   static BlockDriver bdrv_copy_on_read = {
>       .format_name                        = "copy-on-read",
> @@ -161,6 +167,7 @@ static BlockDriver bdrv_copy_on_read = {
>       .bdrv_lock_medium                   = cor_lock_medium,
>   
>       .bdrv_recurse_is_first_non_filter   = cor_recurse_is_first_non_filter,
> +    .bdrv_refresh_filename              = cor_refresh_filename,
>   
>       .has_variable_length                = true,
>       .is_filter                          = true,
> 


-- 
Best regards,
Vladimir

[PATCH 5/7] qapi: add filter-node-name to block-stream

[Andrey Shinkevich]

Provide the possibility to pass the 'filter-node-name' parameter to the
block-stream job as it is done for the commit block job.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 block/stream.c            | 5 +++--
 blockdev.c                | 8 +++++++-
 include/block/block_int.h | 7 ++++++-
 monitor/hmp-cmds.c        | 4 ++--
 qapi/block-core.json      | 6 ++++++
 5 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index d8b4bbe..fab7923 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -229,7 +229,9 @@ static const BlockJobDriver stream_job_driver = {
 void stream_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *base, const char *backing_file_str,
                   int creation_flags, int64_t speed,
-                  BlockdevOnError on_error, Error **errp)
+                  BlockdevOnError on_error,
+                  const char *filter_node_name,
+                  Error **errp)
 {
     StreamBlockJob *s;
     BlockDriverState *iter;
@@ -265,7 +267,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
     } else {
         bdrv_unfreeze_chain(bottom_cow_node, above_base);
     }
-
     /* Prevent concurrent jobs trying to modify the graph structure here, we
      * already have our own plans. Also don't allow resize as the image size is
      * queried only at the job start and then cached. */
diff --git a/blockdev.c b/blockdev.c
index 72d28ce..f275a64 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3242,6 +3242,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
                       bool has_backing_file, const char *backing_file,
                       bool has_speed, int64_t speed,
                       bool has_on_error, BlockdevOnError on_error,
+                      bool has_filter_node_name, const char *filter_node_name,
                       bool has_auto_finalize, bool auto_finalize,
                       bool has_auto_dismiss, bool auto_dismiss,
                       Error **errp)
@@ -3257,6 +3258,10 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         on_error = BLOCKDEV_ON_ERROR_REPORT;
     }
 
+    if (!has_filter_node_name) {
+        filter_node_name = NULL;
+    }
+
     bs = bdrv_lookup_bs(device, device, errp);
     if (!bs) {
         return;
@@ -3324,7 +3329,8 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
     }
 
     stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
-                 job_flags, has_speed ? speed : 0, on_error, &local_err);
+                 job_flags, has_speed ? speed : 0, on_error,
+                 filter_node_name, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto out;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 993bafc..5ac4891 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1052,6 +1052,9 @@ int is_windows_drive(const char *filename);
  *                  See @BlockJobCreateFlags
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @on_error: The action to take upon error.
+ * @filter_node_name: The node name that should be assigned to the filter
+ * driver that the commit job inserts into the graph above @bs. NULL means
+ * that a node name should be autogenerated.
  * @errp: Error object.
  *
  * Start a streaming operation on @bs.  Clusters that are unallocated
@@ -1064,7 +1067,9 @@ int is_windows_drive(const char *filename);
 void stream_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *base, const char *backing_file_str,
                   int creation_flags, int64_t speed,
-                  BlockdevOnError on_error, Error **errp);
+                  BlockdevOnError on_error,
+                  const char *filter_node_name,
+                  Error **errp);
 
 /**
  * commit_start:
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 5ca3ebe..0432ac9 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2044,8 +2044,8 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
 
     qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
                      false, NULL, qdict_haskey(qdict, "speed"), speed, true,
-                     BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
-                     &error);
+                     BLOCKDEV_ON_ERROR_REPORT, false, NULL, false, false,
+                     false, false, &error);
 
     hmp_handle_error(mon, &error);
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3c54717..169f8ea 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2552,6 +2552,11 @@
 #            'stop' and 'enospc' can only be used if the block device
 #            supports io-status (see BlockInfo).  Since 1.3.
 #
+# @filter-node-name: the node name that should be assigned to the
+#                    filter driver that the stream job inserts into the graph
+#                    above @device. If this option is not given, a node name is
+#                    autogenerated. (Since: 5.0)
+#
 # @auto-finalize: When false, this job will wait in a PENDING state after it has
 #                 finished its work, waiting for @block-job-finalize before
 #                 making any block graph changes.
@@ -2581,6 +2586,7 @@
   'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
             '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
             '*on-error': 'BlockdevOnError',
+            '*filter-node-name': 'str',
             '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
 
 ##
-- 
1.8.3.1

Re: [PATCH 5/7] qapi: add filter-node-name to block-stream

[Eric Blake]

On 4/20/20 1:36 PM, Andrey Shinkevich wrote:
> Provide the possibility to pass the 'filter-node-name' parameter to the
> block-stream job as it is done for the commit block job.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block/stream.c            | 5 +++--
>   blockdev.c                | 8 +++++++-
>   include/block/block_int.h | 7 ++++++-
>   monitor/hmp-cmds.c        | 4 ++--
>   qapi/block-core.json      | 6 ++++++
>   5 files changed, 24 insertions(+), 6 deletions(-)
> 

I haven't reviewed the mechanics of this series yet, but from the 
high-level UI perspective:


> +++ b/qapi/block-core.json
> @@ -2552,6 +2552,11 @@
>   #            'stop' and 'enospc' can only be used if the block device
>   #            supports io-status (see BlockInfo).  Since 1.3.
>   #
> +# @filter-node-name: the node name that should be assigned to the
> +#                    filter driver that the stream job inserts into the graph
> +#                    above @device. If this option is not given, a node name is
> +#                    autogenerated. (Since: 5.0)

This should be 5.1, not 5.0.

> +#
>   # @auto-finalize: When false, this job will wait in a PENDING state after it has
>   #                 finished its work, waiting for @block-job-finalize before
>   #                 making any block graph changes.
> @@ -2581,6 +2586,7 @@
>     'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
>               '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
>               '*on-error': 'BlockdevOnError',
> +            '*filter-node-name': 'str',
>               '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>   
>   ##
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 5/7] qapi: add filter-node-name to block-stream

[Dr. David Alan Gilbert]

* Andrey Shinkevich (andrey.shinkevich@virtuozzo.com) wrote:
> Provide the possibility to pass the 'filter-node-name' parameter to the
> block-stream job as it is done for the commit block job.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>  block/stream.c            | 5 +++--
>  blockdev.c                | 8 +++++++-
>  include/block/block_int.h | 7 ++++++-
>  monitor/hmp-cmds.c        | 4 ++--
>  qapi/block-core.json      | 6 ++++++
>  5 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index d8b4bbe..fab7923 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -229,7 +229,9 @@ static const BlockJobDriver stream_job_driver = {
>  void stream_start(const char *job_id, BlockDriverState *bs,
>                    BlockDriverState *base, const char *backing_file_str,
>                    int creation_flags, int64_t speed,
> -                  BlockdevOnError on_error, Error **errp)
> +                  BlockdevOnError on_error,
> +                  const char *filter_node_name,
> +                  Error **errp)
>  {
>      StreamBlockJob *s;
>      BlockDriverState *iter;
> @@ -265,7 +267,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>      } else {
>          bdrv_unfreeze_chain(bottom_cow_node, above_base);
>      }
> -
>      /* Prevent concurrent jobs trying to modify the graph structure here, we
>       * already have our own plans. Also don't allow resize as the image size is
>       * queried only at the job start and then cached. */
> diff --git a/blockdev.c b/blockdev.c
> index 72d28ce..f275a64 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3242,6 +3242,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>                        bool has_backing_file, const char *backing_file,
>                        bool has_speed, int64_t speed,
>                        bool has_on_error, BlockdevOnError on_error,
> +                      bool has_filter_node_name, const char *filter_node_name,
>                        bool has_auto_finalize, bool auto_finalize,
>                        bool has_auto_dismiss, bool auto_dismiss,
>                        Error **errp)
> @@ -3257,6 +3258,10 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>          on_error = BLOCKDEV_ON_ERROR_REPORT;
>      }
>  
> +    if (!has_filter_node_name) {
> +        filter_node_name = NULL;
> +    }
> +
>      bs = bdrv_lookup_bs(device, device, errp);
>      if (!bs) {
>          return;
> @@ -3324,7 +3329,8 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>      }
>  
>      stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
> -                 job_flags, has_speed ? speed : 0, on_error, &local_err);
> +                 job_flags, has_speed ? speed : 0, on_error,
> +                 filter_node_name, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          goto out;
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 993bafc..5ac4891 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1052,6 +1052,9 @@ int is_windows_drive(const char *filename);
>   *                  See @BlockJobCreateFlags
>   * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
>   * @on_error: The action to take upon error.
> + * @filter_node_name: The node name that should be assigned to the filter
> + * driver that the commit job inserts into the graph above @bs. NULL means
> + * that a node name should be autogenerated.
>   * @errp: Error object.
>   *
>   * Start a streaming operation on @bs.  Clusters that are unallocated
> @@ -1064,7 +1067,9 @@ int is_windows_drive(const char *filename);
>  void stream_start(const char *job_id, BlockDriverState *bs,
>                    BlockDriverState *base, const char *backing_file_str,
>                    int creation_flags, int64_t speed,
> -                  BlockdevOnError on_error, Error **errp);
> +                  BlockdevOnError on_error,
> +                  const char *filter_node_name,
> +                  Error **errp);
>  
>  /**
>   * commit_start:
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 5ca3ebe..0432ac9 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -2044,8 +2044,8 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
>  
>      qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
>                       false, NULL, qdict_haskey(qdict, "speed"), speed, true,
> -                     BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
> -                     &error);
> +                     BLOCKDEV_ON_ERROR_REPORT, false, NULL, false, false,
> +                     false, false, &error);
>  
>      hmp_handle_error(mon, &error);

That's moved now into block/monitor/block-hmp-cmds.c
Feel free to add the extra parameter to the HMP side as well!

Dave

>  }
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 3c54717..169f8ea 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2552,6 +2552,11 @@
>  #            'stop' and 'enospc' can only be used if the block device
>  #            supports io-status (see BlockInfo).  Since 1.3.
>  #
> +# @filter-node-name: the node name that should be assigned to the
> +#                    filter driver that the stream job inserts into the graph
> +#                    above @device. If this option is not given, a node name is
> +#                    autogenerated. (Since: 5.0)
> +#
>  # @auto-finalize: When false, this job will wait in a PENDING state after it has
>  #                 finished its work, waiting for @block-job-finalize before
>  #                 making any block graph changes.
> @@ -2581,6 +2586,7 @@
>    'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
>              '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
>              '*on-error': 'BlockdevOnError',
> +            '*filter-node-name': 'str',
>              '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>  
>  ##
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 5/7] qapi: add filter-node-name to block-stream

[Vladimir Sementsov-Ogievskiy]

20.04.2020 21:36, Andrey Shinkevich wrote:
> Provide the possibility to pass the 'filter-node-name' parameter to the
> block-stream job as it is done for the commit block job.

So, you add it, but it actually do nothing for now. I'd prefer to make this patch the last one, so it actually make the change.

> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block/stream.c            | 5 +++--
>   blockdev.c                | 8 +++++++-
>   include/block/block_int.h | 7 ++++++-
>   monitor/hmp-cmds.c        | 4 ++--
>   qapi/block-core.json      | 6 ++++++
>   5 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index d8b4bbe..fab7923 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -229,7 +229,9 @@ static const BlockJobDriver stream_job_driver = {
>   void stream_start(const char *job_id, BlockDriverState *bs,
>                     BlockDriverState *base, const char *backing_file_str,
>                     int creation_flags, int64_t speed,
> -                  BlockdevOnError on_error, Error **errp)
> +                  BlockdevOnError on_error,
> +                  const char *filter_node_name,
> +                  Error **errp)
>   {
>       StreamBlockJob *s;
>       BlockDriverState *iter;
> @@ -265,7 +267,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>       } else {
>           bdrv_unfreeze_chain(bottom_cow_node, above_base);
>       }
> -

extra hunk

>       /* Prevent concurrent jobs trying to modify the graph structure here, we
>        * already have our own plans. Also don't allow resize as the image size is
>        * queried only at the job start and then cached. */
> diff --git a/blockdev.c b/blockdev.c
> index 72d28ce..f275a64 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3242,6 +3242,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>                         bool has_backing_file, const char *backing_file,
>                         bool has_speed, int64_t speed,
>                         bool has_on_error, BlockdevOnError on_error,
> +                      bool has_filter_node_name, const char *filter_node_name,
>                         bool has_auto_finalize, bool auto_finalize,
>                         bool has_auto_dismiss, bool auto_dismiss,
>                         Error **errp)
> @@ -3257,6 +3258,10 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>           on_error = BLOCKDEV_ON_ERROR_REPORT;
>       }
>   
> +    if (!has_filter_node_name) {
> +        filter_node_name = NULL;
> +    }

it is guaranteed to be zeroed in this case, so you don't need to set it

> +
>       bs = bdrv_lookup_bs(device, device, errp);
>       if (!bs) {
>           return;
> @@ -3324,7 +3329,8 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>       }
>   
>       stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
> -                 job_flags, has_speed ? speed : 0, on_error, &local_err);
> +                 job_flags, has_speed ? speed : 0, on_error,
> +                 filter_node_name, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);
>           goto out;
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 993bafc..5ac4891 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1052,6 +1052,9 @@ int is_windows_drive(const char *filename);
>    *                  See @BlockJobCreateFlags
>    * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
>    * @on_error: The action to take upon error.
> + * @filter_node_name: The node name that should be assigned to the filter
> + * driver that the commit job inserts into the graph above @bs. NULL means
> + * that a node name should be autogenerated.
>    * @errp: Error object.
>    *
>    * Start a streaming operation on @bs.  Clusters that are unallocated
> @@ -1064,7 +1067,9 @@ int is_windows_drive(const char *filename);
>   void stream_start(const char *job_id, BlockDriverState *bs,
>                     BlockDriverState *base, const char *backing_file_str,
>                     int creation_flags, int64_t speed,
> -                  BlockdevOnError on_error, Error **errp);
> +                  BlockdevOnError on_error,
> +                  const char *filter_node_name,
> +                  Error **errp);
>   
>   /**
>    * commit_start:
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 5ca3ebe..0432ac9 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -2044,8 +2044,8 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
>   
>       qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
>                        false, NULL, qdict_haskey(qdict, "speed"), speed, true,
> -                     BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
> -                     &error);
> +                     BLOCKDEV_ON_ERROR_REPORT, false, NULL, false, false,
> +                     false, false, &error);
>   
>       hmp_handle_error(mon, &error);
>   }
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 3c54717..169f8ea 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2552,6 +2552,11 @@
>   #            'stop' and 'enospc' can only be used if the block device
>   #            supports io-status (see BlockInfo).  Since 1.3.
>   #
> +# @filter-node-name: the node name that should be assigned to the
> +#                    filter driver that the stream job inserts into the graph
> +#                    above @device. If this option is not given, a node name is
> +#                    autogenerated. (Since: 5.0)
> +#
>   # @auto-finalize: When false, this job will wait in a PENDING state after it has
>   #                 finished its work, waiting for @block-job-finalize before
>   #                 making any block graph changes.
> @@ -2581,6 +2586,7 @@
>     'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
>               '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
>               '*on-error': 'BlockdevOnError',
> +            '*filter-node-name': 'str',
>               '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>   
>   ##
> 


-- 
Best regards,
Vladimir

Re: [PATCH 5/7] qapi: add filter-node-name to block-stream

[Vladimir Sementsov-Ogievskiy]

21.04.2020 15:40, Vladimir Sementsov-Ogievskiy wrote:
> 20.04.2020 21:36, Andrey Shinkevich wrote:
>> Provide the possibility to pass the 'filter-node-name' parameter to the
>> block-stream job as it is done for the commit block job.
> 
> So, you add it, but it actually do nothing for now. I'd prefer to make this patch the last one, so it actually make the change.
> 

Ah, I see, you need it, to fix iotests.. And otherwise we'll have to merge it into 7/7. Hmm. OK than, but note in a commit message
that actual change is in further patch.



-- 
Best regards,
Vladimir

[PATCH 6/7] iotests: prepare 245 for using filter in block-stream

[Andrey Shinkevich]

The preliminary patch modifies the test 245 to prepare the block-stream
job for using COR-filter. The filter breaks the backing chain being
connected to the underlying node by file child link.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/245 | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 049ef6a..9baeb2b 100644
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -896,15 +896,19 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         # make hd1 read-only and block-stream requires it to be read-write
         # (Which error message appears depends on whether the stream job is
         # already done with copying at this point.)
-        self.reopen(opts, {},
+        opts = hd_opts(1)
+        opts['backing'] = hd_opts(2)
+        opts['backing']['backing'] = None
+        self.reopen(opts, {'read-only': True},
             ["Can't set node 'hd1' to r/o with copy-on-read enabled",
              "Cannot make block node read-only, there is a writer on it"])
 
         # We can't remove hd2 while the stream job is ongoing
-        opts['backing']['backing'] = None
-        self.reopen(opts, {'backing.read-only': False}, "Cannot change 'backing' link from 'hd1' to 'hd2'")
+        opts['backing'] = None
+        self.reopen(opts, {'read-only': False}, "Cannot change 'backing' link from 'hd1' to 'hd2'")
 
         # We can detach hd1 from hd0 because it doesn't affect the stream job
+        opts = hd_opts(0)
         opts['backing'] = None
         self.reopen(opts)
 
-- 
1.8.3.1

[PATCH 7/7] block: apply COR-filter to block-stream jobs

[Andrey Shinkevich]

The patch completes the series with the COR-filter insertion to any
block-stream operation. It also makes changes to the iotests 030, 141
and 245.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 block/stream.c             | 151 +++++++++++++++++++++++++++++++++++++++------
 tests/qemu-iotests/030     |   6 +-
 tests/qemu-iotests/141.out |   2 +-
 tests/qemu-iotests/245     |   5 +-
 4 files changed, 141 insertions(+), 23 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index fab7923..af14ba8 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -16,6 +16,7 @@
 #include "block/block_int.h"
 #include "block/blockjob_int.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/ratelimit.h"
 #include "sysemu/block-backend.h"
@@ -33,6 +34,8 @@ typedef struct StreamBlockJob {
     BlockJob common;
     BlockDriverState *bottom_cow_node;
     BlockDriverState *above_base;
+    BlockDriverState *cor_filter_bs;
+    BlockDriverState *target_bs;
     BlockdevOnError on_error;
     char *backing_file_str;
     bool bs_read_only;
@@ -46,22 +49,11 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
     assert(bytes < SIZE_MAX);
 
     /* Copy-on-read the unallocated clusters */
-    return blk_co_pread(blk, offset, bytes, buf, BDRV_REQ_COPY_ON_READ);
+    return blk_co_pread(blk, offset, bytes, buf, 0);
 }
 
-static void stream_abort(Job *job)
-{
-    StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
-
-    if (s->chain_frozen) {
-        BlockJob *bjob = &s->common;
-        bdrv_unfreeze_chain(blk_bs(bjob->blk), s->above_base);
-    }
-}
-
-static int stream_prepare(Job *job)
+static int stream_change_backing_file(StreamBlockJob *s)
 {
-    StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
     BlockJob *bjob = &s->common;
     BlockDriverState *bs = blk_bs(bjob->blk);
     BlockDriverState *unfiltered_bs = bdrv_skip_rw_filters(bs);
@@ -69,9 +61,6 @@ static int stream_prepare(Job *job)
     Error *local_err = NULL;
     int ret = 0;
 
-    bdrv_unfreeze_chain(bs, s->above_base);
-    s->chain_frozen = false;
-
     if (bdrv_filtered_cow_child(unfiltered_bs)) {
         const char *base_id = NULL, *base_fmt = NULL;
         if (base) {
@@ -91,11 +80,58 @@ static int stream_prepare(Job *job)
     return ret;
 }
 
+static int stream_exit(Job *job, bool abort)
+{
+    StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
+    BlockJob *bjob = &s->common;
+    BlockDriverState *target_bs = s->target_bs;
+    int ret = 0;
+
+    if (s->chain_frozen) {
+        bdrv_unfreeze_chain(s->target_bs, s->bottom_cow_node);
+        s->chain_frozen = false;
+    }
+
+    /* Retain the BDS until we complete the graph change. */
+    bdrv_ref(target_bs);
+    /* Hold a guest back from writing while permissions are being reset. */
+    bdrv_drained_begin(target_bs);
+    /* Drop permissions before the graph change. */
+    bdrv_child_try_set_perm(bdrv_filtered_rw_child(s->cor_filter_bs),
+                            0, BLK_PERM_ALL, &error_abort);
+    if (!abort) {
+        ret = stream_change_backing_file(s);
+    }
+
+    bdrv_replace_node(s->cor_filter_bs, target_bs, &error_abort);
+    /* Switch the BB back to the filter so that job terminated properly. */
+    blk_remove_bs(bjob->blk);
+    blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
+    blk_insert_bs(bjob->blk, s->cor_filter_bs, &error_abort);
+
+    bdrv_drained_end(target_bs);
+    bdrv_unref(target_bs);
+    /* Submit control over filter to the job instance. */
+    bdrv_unref(s->cor_filter_bs);
+
+    return ret;
+}
+
+static int stream_prepare(Job *job)
+{
+    return stream_exit(job, false);
+}
+
+static void stream_abort(Job *job)
+{
+    stream_exit(job, job->ret < 0);
+}
+
 static void stream_clean(Job *job)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
     BlockJob *bjob = &s->common;
-    BlockDriverState *bs = blk_bs(bjob->blk);
+    BlockDriverState *bs = s->target_bs;
 
     /* Reopen the image back in read-only mode if necessary */
     if (s->bs_read_only) {
@@ -212,6 +248,72 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
     return error;
 }
 
+static BlockDriverState *create_filter_node(BlockDriverState *bs,
+                                            const char *filter_node_name,
+                                            Error **errp)
+{
+    QDict *opts = qdict_new();
+
+    qdict_put_str(opts, "driver", "copy-on-read");
+    qdict_put_str(opts, "file", bdrv_get_node_name(bs));
+    if (filter_node_name) {
+        qdict_put_str(opts, "node-name", filter_node_name);
+    }
+
+    return bdrv_open(NULL, NULL, opts, BDRV_O_RDWR, errp);
+}
+
+static void remove_filter(BlockDriverState *cor_filter_bs)
+{
+    BdrvChild *child;
+    BlockDriverState *bs;
+
+    child = bdrv_filtered_rw_child(cor_filter_bs);
+    if (!child) {
+        return;
+    }
+    bs = child->bs;
+
+    /* Hold a guest back from writing until we remove the filter */
+    bdrv_drained_begin(bs);
+    bdrv_child_try_set_perm(child, 0, BLK_PERM_ALL,
+                            &error_abort);
+    bdrv_replace_node(cor_filter_bs, bs, &error_abort);
+    bdrv_drained_end(bs);
+
+    bdrv_unref(cor_filter_bs);
+}
+
+static BlockDriverState *insert_filter(BlockDriverState *bs,
+                                       const char *filter_node_name,
+                                       Error **errp)
+{
+    BlockDriverState *cor_filter_bs;
+    Error *local_err = NULL;
+
+    cor_filter_bs = create_filter_node(bs, filter_node_name, errp);
+    if (cor_filter_bs == NULL) {
+        error_prepend(errp, "Could not create filter node: ");
+        return NULL;
+    }
+
+    if (!filter_node_name) {
+        cor_filter_bs->implicit = true;
+    }
+
+    bdrv_drained_begin(bs);
+    bdrv_replace_node(bs, cor_filter_bs, &local_err);
+    bdrv_drained_end(bs);
+
+    if (local_err) {
+        bdrv_unref(cor_filter_bs);
+        error_propagate(errp, local_err);
+        return NULL;
+    }
+
+    return cor_filter_bs;
+}
+
 static const BlockJobDriver stream_job_driver = {
     .job_driver = {
         .instance_size = sizeof(StreamBlockJob),
@@ -237,6 +339,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
     BlockDriverState *iter;
     bool bs_read_only;
     int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
+    BlockDriverState *cor_filter_bs = NULL;
     BlockDriverState *bottom_cow_node = bdrv_find_overlay(bs, base);
     BlockDriverState *above_base;
 
@@ -267,10 +370,16 @@ void stream_start(const char *job_id, BlockDriverState *bs,
     } else {
         bdrv_unfreeze_chain(bottom_cow_node, above_base);
     }
+
+    cor_filter_bs = insert_filter(bs, filter_node_name, errp);
+    if (cor_filter_bs == NULL) {
+        goto fail;
+    }
+
     /* Prevent concurrent jobs trying to modify the graph structure here, we
      * 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,
+    s = block_job_create(job_id, &stream_job_driver, NULL, cor_filter_bs,
                          basic_flags | BLK_PERM_GRAPH_MOD,
                          basic_flags | BLK_PERM_WRITE,
                          speed, creation_flags, NULL, NULL, errp);
@@ -294,6 +403,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
                            basic_flags, &error_abort);
     }
 
+    s->cor_filter_bs = cor_filter_bs;
+    s->target_bs = bs;
     s->bottom_cow_node = bottom_cow_node;
     s->above_base = above_base;
     s->backing_file_str = g_strdup(backing_file_str);
@@ -310,4 +421,8 @@ fail:
         bdrv_reopen_set_read_only(bs, true, NULL);
     }
     bdrv_unfreeze_chain(bs, bottom_cow_node);
+
+    if (cor_filter_bs) {
+        remove_filter(cor_filter_bs);
+    }
 }
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 1b69f31..31a5164 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -269,7 +269,9 @@ class TestParallelOps(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
 
         # Set a speed limit to make sure that this job blocks the rest
-        result = self.vm.qmp('block-stream', device='node4', job_id='stream-node4', base=self.imgs[1], speed=1024*1024)
+        result = self.vm.qmp('block-stream', device='node4',
+                             job_id='stream-node4', base=self.imgs[1],
+                             filter_node_name='stream-filter', speed=1024*1024)
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-stream', device='node5', job_id='stream-node5', base=self.imgs[2])
@@ -287,7 +289,7 @@ class TestParallelOps(iotests.QMPTestCase):
         # block-commit should also fail if it touches nodes used by the stream job
         result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[4], job_id='commit-node4')
         self.assert_qmp(result, 'error/desc',
-            "Node 'node4' is busy: block device is in use by block job: stream")
+            "Node 'stream-filter' is busy: block device is in use by block job: stream")
 
         result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[1], top=self.imgs[3], job_id='commit-node1')
         self.assert_qmp(result, 'error/desc',
diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out
index 4d71d9d..8cc020f 100644
--- a/tests/qemu-iotests/141.out
+++ b/tests/qemu-iotests/141.out
@@ -78,7 +78,7 @@ wrote 1048576/1048576 bytes at offset 0
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}
 {"return": {}}
-{"error": {"class": "GenericError", "desc": "Node drv0 is in use"}}
+{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: stream"}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "job0", "len": 1048576, "offset": 524288, "speed": 1, "type": "stream"}}
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 9baeb2b..18839dc 100644
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -907,10 +907,11 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         opts['backing'] = None
         self.reopen(opts, {'read-only': False}, "Cannot change 'backing' link from 'hd1' to 'hd2'")
 
-        # We can detach hd1 from hd0 because it doesn't affect the stream job
+        # We can't detach hd1 from hd0 while the stream job is ongoing
         opts = hd_opts(0)
         opts['backing'] = None
-        self.reopen(opts)
+        self.reopen(opts, {},
+            "Cannot change backing link if 'hd0' has an implicit backing file")
 
         self.vm.run_job('stream0', auto_finalize = False, auto_dismiss = True)
 
-- 
1.8.3.1

Re: [PATCH 7/7] block: apply COR-filter to block-stream jobs

[Vladimir Sementsov-Ogievskiy]

20.04.2020 21:36, Andrey Shinkevich wrote:
> The patch completes the series with the COR-filter insertion to any
> block-stream operation. It also makes changes to the iotests 030, 141
> and 245.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block/stream.c             | 151 +++++++++++++++++++++++++++++++++++++++------
>   tests/qemu-iotests/030     |   6 +-
>   tests/qemu-iotests/141.out |   2 +-
>   tests/qemu-iotests/245     |   5 +-
>   4 files changed, 141 insertions(+), 23 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index fab7923..af14ba8 100644
> --- a/block/stream.c
> +++ b/block/stream.c


[..]

> +static int stream_exit(Job *job, bool abort)
> +{
> +    StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
> +    BlockJob *bjob = &s->common;
> +    BlockDriverState *target_bs = s->target_bs;
> +    int ret = 0;
> +
> +    if (s->chain_frozen) {
> +        bdrv_unfreeze_chain(s->target_bs, s->bottom_cow_node);
> +        s->chain_frozen = false;
> +    }
> +
> +    /* Retain the BDS until we complete the graph change. */
> +    bdrv_ref(target_bs);
> +    /* Hold a guest back from writing while permissions are being reset. */
> +    bdrv_drained_begin(target_bs);
> +    /* Drop permissions before the graph change. */
> +    bdrv_child_try_set_perm(bdrv_filtered_rw_child(s->cor_filter_bs),
> +                            0, BLK_PERM_ALL, &error_abort);

Hmm. I don't remember what's wrong with it, but something is. Neither mirror nor backup do this now,
instead they use some flag, that permissions are not needed anymore and call bdrv_child_refresh_perms()


Also, strange that you have insert_filter function, but don't use it here.

Also, could we keep add/remove filter api in block/copy-on-read.c, like it is done for backup-top ?

-- 
Best regards,
Vladimir

Re: [PATCH 7/7] block: apply COR-filter to block-stream jobs

[Andrey Shinkevich]

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

1. The mirror technology instantiates its own BLK, sets 0/BLK_PERM_ALL for it on exit and then, via subsequent call to bdrv_child_refresh_perms(), calls bdrv_child_try_set_perm(). We don't have the extra BLK for our filter in stream and do call the same function directly.

2. The function remove_filter() can't be used in the stream job on exit because we should remove the filter and change a backing file in the same critical 'drain' section.

3. Due to the specifics above, I suggest that we make insert/remove filter as an interface when there gets another user of it.

Andrey

________________________________
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sent: Tuesday, April 21, 2020 3:58 PM
To: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>; qemu-block@nongnu.org <qemu-block@nongnu.org>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; kwolf@redhat.com <kwolf@redhat.com>; mreitz@redhat.com <mreitz@redhat.com>; jsnow@redhat.com <jsnow@redhat.com>; armbru@redhat.com <armbru@redhat.com>; dgilbert@redhat.com <dgilbert@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>
Subject: Re: [PATCH 7/7] block: apply COR-filter to block-stream jobs

20.04.2020 21:36, Andrey Shinkevich wrote:
> The patch completes the series with the COR-filter insertion to any
> block-stream operation. It also makes changes to the iotests 030, 141
> and 245.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block/stream.c             | 151 +++++++++++++++++++++++++++++++++++++++------
>   tests/qemu-iotests/030     |   6 +-
>   tests/qemu-iotests/141.out |   2 +-
>   tests/qemu-iotests/245     |   5 +-
>   4 files changed, 141 insertions(+), 23 deletions(-)
>
> diff --git a/block/stream.c b/block/stream.c
> index fab7923..af14ba8 100644
> --- a/block/stream.c
> +++ b/block/stream.c


[..]

> +static int stream_exit(Job *job, bool abort)
> +{
> +    StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
> +    BlockJob *bjob = &s->common;
> +    BlockDriverState *target_bs = s->target_bs;
> +    int ret = 0;
> +
> +    if (s->chain_frozen) {
> +        bdrv_unfreeze_chain(s->target_bs, s->bottom_cow_node);
> +        s->chain_frozen = false;
> +    }
> +
> +    /* Retain the BDS until we complete the graph change. */
> +    bdrv_ref(target_bs);
> +    /* Hold a guest back from writing while permissions are being reset. */
> +    bdrv_drained_begin(target_bs);
> +    /* Drop permissions before the graph change. */
> +    bdrv_child_try_set_perm(bdrv_filtered_rw_child(s->cor_filter_bs),
> +                            0, BLK_PERM_ALL, &error_abort);

Hmm. I don't remember what's wrong with it, but something is. Neither mirror nor backup do this now,
instead they use some flag, that permissions are not needed anymore and call bdrv_child_refresh_perms()

Also, strange that you have insert_filter function, but don't use it here.

Also, could we keep add/remove filter api in block/copy-on-read.c, like it is done for backup-top ?

--
Best regards,
Vladimir

[-- Attachment #2: Type: text/html, Size: 7120 bytes --]

Re: [PATCH 7/7] block: apply COR-filter to block-stream jobs

[Vladimir Sementsov-Ogievskiy]

27.04.2020 7:08, Andrey Shinkevich wrote:
> 1. The mirror technology instantiates its own BLK, sets 0/BLK_PERM_ALL for it on exit and then, via subsequent call to bdrv_child_refresh_perms(), calls bdrv_child_try_set_perm(). We don't have the extra BLK for our filter in stream and do call the same function directly.

deal with blk is unrelated, as I understand. Changing permissions shoud go through bdrv_child_refresh_perms and modification of the state of the node, like it is done in upstream mirror-top and backup-top. Just call bdrv_child_try_set_perm() is very risky: on any subsequent call of bdrv_child_refresh_perms (may be implicit, by other functions) you'll lose the permissions you set by hand. So, again, setting permissions by hand is wrong thing.

> 
> 2. The function remove_filter() can't be used in the stream job on exit because we should remove the filter and change a backing file in the same critical 'drain' section.

Hmm. It's a question of refactoring. Why not create function remove_filter_drained?

> 
> 3. Due to the specifics above, I suggest that we make insert/remove filter as an interface when there gets another user of it.
> 

Please look at backup-top filter. Could we be as close to it as possible? I'm sure, that finally we should have one API for filter insertion/removing, not per filter.

> 
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> *From:* Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> *Sent:* Tuesday, April 21, 2020 3:58 PM
> *To:* Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>; qemu-block@nongnu.org <qemu-block@nongnu.org>
> *Cc:* qemu-devel@nongnu.org <qemu-devel@nongnu.org>; kwolf@redhat.com <kwolf@redhat.com>; mreitz@redhat.com <mreitz@redhat.com>; jsnow@redhat.com <jsnow@redhat.com>; armbru@redhat.com <armbru@redhat.com>; dgilbert@redhat.com <dgilbert@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>
> *Subject:* Re: [PATCH 7/7] block: apply COR-filter to block-stream jobs
> 20.04.2020 21:36, Andrey Shinkevich wrote:
>> The patch completes the series with the COR-filter insertion to any
>> block-stream operation. It also makes changes to the iotests 030, 141
>> and 245.
>> 
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>   block/stream.c             | 151 +++++++++++++++++++++++++++++++++++++++------
>>   tests/qemu-iotests/030     |   6 +-
>>   tests/qemu-iotests/141.out |   2 +-
>>   tests/qemu-iotests/245     |   5 +-
>>   4 files changed, 141 insertions(+), 23 deletions(-)
>> 
>> diff --git a/block/stream.c b/block/stream.c
>> index fab7923..af14ba8 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
> 
> 
> [..]
> 
>> +static int stream_exit(Job *job, bool abort)
>> +{
>> +    StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>> +    BlockJob *bjob = &s->common;
>> +    BlockDriverState *target_bs = s->target_bs;
>> +    int ret = 0;
>> +
>> +    if (s->chain_frozen) {
>> +        bdrv_unfreeze_chain(s->target_bs, s->bottom_cow_node);
>> +        s->chain_frozen = false;
>> +    }
>> +
>> +    /* Retain the BDS until we complete the graph change. */
>> +    bdrv_ref(target_bs);
>> +    /* Hold a guest back from writing while permissions are being reset. */
>> +    bdrv_drained_begin(target_bs);
>> +    /* Drop permissions before the graph change. */
>> +    bdrv_child_try_set_perm(bdrv_filtered_rw_child(s->cor_filter_bs),
>> +                            0, BLK_PERM_ALL, &error_abort);
> 
> Hmm. I don't remember what's wrong with it, but something is. Neither mirror nor backup do this now,
> instead they use some flag, that permissions are not needed anymore and call bdrv_child_refresh_perms()
> 
> Also, strange that you have insert_filter function, but don't use it here.
> 
> Also, could we keep add/remove filter api in block/copy-on-read.c, like it is done for backup-top ?
> 
> -- 
> Best regards,
> Vladimir


-- 
Best regards,
Vladimir

Re: [PATCH 0/7] Apply COR-filter to the block-stream permanently

[Vladimir Sementsov-Ogievskiy]

20.04.2020 21:36, Andrey Shinkevich wrote:
> Note: this series is based on the one "block: Deal with filters"
>        by Max Reitz that can be found in the branches:
> 
>        https://git.xanclic.moe/XanClic/qemu child-access-functions-v6
>        https://github.com/XanClic/qemu child-access-functions-v6

Would be very good to not make a dependency and keep the series parallel. I believe, that we can proceed with this series taking a small subset of Max's series.

> 
>        When running iotests, apply "char-socket: Fix race condition"
>        to avoid sporadic segmentation faults.
> 
> With this series, all the block-stream COR operations pass through
> the COR-filter.
> 
> Andrey Shinkevich (7):
>    block: prepare block-stream for using COR-filter
>    stream: exclude a link to filter from freezing
>    block: protect parallel jobs from overlapping
>    copy-on-read: Support refreshing filename
>    qapi: add filter-node-name to block-stream
>    iotests: prepare 245 for using filter in block-stream
>    block: apply COR-filter to block-stream jobs
> 
>   block/copy-on-read.c       |   7 ++
>   block/stream.c             | 170 +++++++++++++++++++++++++++++++++++++++------
>   blockdev.c                 |  15 +++-
>   blockjob.c                 |  15 +++-
>   include/block/block_int.h  |   7 +-
>   monitor/hmp-cmds.c         |   4 +-
>   qapi/block-core.json       |   6 ++
>   tests/qemu-iotests/030     |   6 +-
>   tests/qemu-iotests/141.out |   2 +-
>   tests/qemu-iotests/245     |  15 ++--
>   10 files changed, 210 insertions(+), 37 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH 0/7] Apply COR-filter to the block-stream permanently

[Andrey Shinkevich]

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

I am going to do that in version 3. Please review the changes in the version 2 first.
Thanks,
Andrey

________________________________
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sent: Tuesday, April 21, 2020 4:12 PM
To: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>; qemu-block@nongnu.org <qemu-block@nongnu.org>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; kwolf@redhat.com <kwolf@redhat.com>; mreitz@redhat.com <mreitz@redhat.com>; jsnow@redhat.com <jsnow@redhat.com>; armbru@redhat.com <armbru@redhat.com>; dgilbert@redhat.com <dgilbert@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>
Subject: Re: [PATCH 0/7] Apply COR-filter to the block-stream permanently

20.04.2020 21:36, Andrey Shinkevich wrote:
> Note: this series is based on the one "block: Deal with filters"
>        by Max Reitz that can be found in the branches:
>
>        https://git.xanclic.moe/XanClic/qemu child-access-functions-v6
>        https://github.com/XanClic/qemu child-access-functions-v6

Would be very good to not make a dependency and keep the series parallel. I believe, that we can proceed with this series taking a small subset of Max's series.

>
>        When running iotests, apply "char-socket: Fix race condition"
>        to avoid sporadic segmentation faults.
>
> With this series, all the block-stream COR operations pass through
> the COR-filter.
>
> Andrey Shinkevich (7):
>    block: prepare block-stream for using COR-filter
>    stream: exclude a link to filter from freezing
>    block: protect parallel jobs from overlapping
>    copy-on-read: Support refreshing filename
>    qapi: add filter-node-name to block-stream
>    iotests: prepare 245 for using filter in block-stream
>    block: apply COR-filter to block-stream jobs
>
>   block/copy-on-read.c       |   7 ++
>   block/stream.c             | 170 +++++++++++++++++++++++++++++++++++++++------
>   blockdev.c                 |  15 +++-
>   blockjob.c                 |  15 +++-
>   include/block/block_int.h  |   7 +-
>   monitor/hmp-cmds.c         |   4 +-
>   qapi/block-core.json       |   6 ++
>   tests/qemu-iotests/030     |   6 +-
>   tests/qemu-iotests/141.out |   2 +-
>   tests/qemu-iotests/245     |  15 ++--
>   10 files changed, 210 insertions(+), 37 deletions(-)
>


--
Best regards,
Vladimir

[-- Attachment #2: Type: text/html, Size: 4418 bytes --]