[PATCH v12 00/14] Apply COR-filter to the block-stream permanently

[PATCH v12 00/14] Apply COR-filter to the block-stream permanently

[Andrey Shinkevich via]

The node insert/remove functions were added at the block generic layer.
COR-filter options structure was added to the QAPI.
The test case #310 was added to check the 'bottom' node limit for COR.
The 'supported_read_flags' member was added to the BDS structure
(with the flags check at the block generic layer for drivers).

v12:
  02: New.
  03: Only the temporary drop filter function left.
  05: New (suggested by Max)
  06: 'base' -> 'bottom' option.
  07: Fixes based on the review of the v11.
  08: New.
  09: The comment ext was modified.
  10: The read flags check at the block generic layer.
  11: COR flag was added.
  12: The condition was fixed.
  13: The 'backing-file' parameter returned. No deprecation.
  14: The COR-filter 'add' function replaced with the 'insert node' generic
      function. Fixes based on the review of the v11.

Andrey Shinkevich (14):
  copy-on-read: support preadv/pwritev_part functions
  block: add insert/remove node functions
  copy-on-read: add filter drop function
  qapi: add filter-node-name to block-stream
  qapi: create BlockdevOptionsCor structure for COR driver
  copy-on-read: pass bottom node name to COR driver
  copy-on-read: limit COR operations to bottom node
  iotests: add #310 to test bottom node in COR driver
  block: modify the comment for BDRV_REQ_PREFETCH flag
  block: include supported_read_flags into BDS structure
  copy-on-read: add support for read flags to COR-filter
  copy-on-read: skip non-guest reads if no copy needed
  stream: skip filters when writing backing file name to QCOW2 header
  block: apply COR-filter to block-stream jobs

 block.c                        |  49 ++++++++++++++
 block/copy-on-read.c           | 144 +++++++++++++++++++++++++++++++++++++----
 block/copy-on-read.h           |  32 +++++++++
 block/io.c                     |  12 +++-
 block/monitor/block-hmp-cmds.c |   4 +-
 block/stream.c                 | 117 ++++++++++++++++++++++-----------
 blockdev.c                     |  13 ++--
 include/block/block.h          |  11 +++-
 include/block/block_int.h      |  11 +++-
 qapi/block-core.json           |  27 +++++++-
 tests/qemu-iotests/030         |  51 ++-------------
 tests/qemu-iotests/030.out     |   4 +-
 tests/qemu-iotests/141.out     |   2 +-
 tests/qemu-iotests/245         |  22 +++++--
 tests/qemu-iotests/310         | 109 +++++++++++++++++++++++++++++++
 tests/qemu-iotests/310.out     |  15 +++++
 tests/qemu-iotests/group       |   3 +-
 17 files changed, 503 insertions(+), 123 deletions(-)
 create mode 100644 block/copy-on-read.h
 create mode 100755 tests/qemu-iotests/310
 create mode 100644 tests/qemu-iotests/310.out

-- 
1.8.3.1

[PATCH v12 01/14] copy-on-read: support preadv/pwritev_part functions

[Andrey Shinkevich via]

Add support for the recently introduced functions
bdrv_co_preadv_part()
and
bdrv_co_pwritev_part()
to the COR-filter driver.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/copy-on-read.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 2816e61..cb03e0f 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -74,21 +74,25 @@ static int64_t cor_getlength(BlockDriverState *bs)
 }
 
 
-static int coroutine_fn cor_co_preadv(BlockDriverState *bs,
-                                      uint64_t offset, uint64_t bytes,
-                                      QEMUIOVector *qiov, int flags)
+static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
+                                           uint64_t offset, uint64_t bytes,
+                                           QEMUIOVector *qiov,
+                                           size_t qiov_offset,
+                                           int flags)
 {
-    return bdrv_co_preadv(bs->file, offset, bytes, qiov,
-                          flags | BDRV_REQ_COPY_ON_READ);
+    return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
+                               flags | BDRV_REQ_COPY_ON_READ);
 }
 
 
-static int coroutine_fn cor_co_pwritev(BlockDriverState *bs,
-                                       uint64_t offset, uint64_t bytes,
-                                       QEMUIOVector *qiov, int flags)
+static int coroutine_fn cor_co_pwritev_part(BlockDriverState *bs,
+                                            uint64_t offset,
+                                            uint64_t bytes,
+                                            QEMUIOVector *qiov,
+                                            size_t qiov_offset, int flags)
 {
-
-    return bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
+    return bdrv_co_pwritev_part(bs->file, offset, bytes, qiov, qiov_offset,
+                                flags);
 }
 
 
@@ -137,8 +141,8 @@ static BlockDriver bdrv_copy_on_read = {
 
     .bdrv_getlength                     = cor_getlength,
 
-    .bdrv_co_preadv                     = cor_co_preadv,
-    .bdrv_co_pwritev                    = cor_co_pwritev,
+    .bdrv_co_preadv_part                = cor_co_preadv_part,
+    .bdrv_co_pwritev_part               = cor_co_pwritev_part,
     .bdrv_co_pwrite_zeroes              = cor_co_pwrite_zeroes,
     .bdrv_co_pdiscard                   = cor_co_pdiscard,
     .bdrv_co_pwritev_compressed         = cor_co_pwritev_compressed,
-- 
1.8.3.1

[PATCH v12 02/14] block: add insert/remove node functions

[Andrey Shinkevich via]

Provide API for a node insertion to and removal from a backing chain.

Suggested-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 block.c               | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  3 +++
 2 files changed, 52 insertions(+)

diff --git a/block.c b/block.c
index 430edf7..502b483 100644
--- a/block.c
+++ b/block.c
@@ -4670,6 +4670,55 @@ static void bdrv_delete(BlockDriverState *bs)
     g_free(bs);
 }
 
+BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
+                                   int flags, Error **errp)
+{
+    BlockDriverState *new_node_bs;
+    Error *local_err = NULL;
+
+    new_node_bs =  bdrv_open(NULL, NULL, node_options, flags, errp);
+    if (new_node_bs == NULL) {
+        error_prepend(errp, "Could not create node: ");
+        return NULL;
+    }
+
+    bdrv_drained_begin(bs);
+    bdrv_replace_node(bs, new_node_bs, &local_err);
+    bdrv_drained_end(bs);
+
+    if (local_err) {
+        bdrv_unref(new_node_bs);
+        error_propagate(errp, local_err);
+        return NULL;
+    }
+
+    return new_node_bs;
+}
+
+void bdrv_remove_node(BlockDriverState *bs)
+{
+    BdrvChild *child;
+    BlockDriverState *inferior_bs;
+
+    child = bdrv_filter_or_cow_child(bs);
+    if (!child) {
+        return;
+    }
+    inferior_bs = child->bs;
+
+    /* Retain the BDS until we complete the graph change. */
+    bdrv_ref(inferior_bs);
+    /* Hold a guest back from writing while permissions are being reset. */
+    bdrv_drained_begin(inferior_bs);
+    /* Refresh permissions before the graph change. */
+    bdrv_child_refresh_perms(bs, child, &error_abort);
+    bdrv_replace_node(bs, inferior_bs, &error_abort);
+
+    bdrv_drained_end(inferior_bs);
+    bdrv_unref(inferior_bs);
+    bdrv_unref(bs);
+}
+
 /*
  * Run consistency checks on an image
  *
diff --git a/include/block/block.h b/include/block/block.h
index d16c401..ae7612f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -350,6 +350,9 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
                  Error **errp);
 void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
                        Error **errp);
+BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
+                                   int flags, Error **errp);
+void bdrv_remove_node(BlockDriverState *bs);
 
 int bdrv_parse_aio(const char *mode, int *flags);
 int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);
-- 
1.8.3.1

Re: [PATCH v12 02/14] block: add insert/remove node functions

[Vladimir Sementsov-Ogievskiy]

22.10.2020 21:13, Andrey Shinkevich wrote:
> Provide API for a node insertion to and removal from a backing chain.
> 
> Suggested-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block.c               | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>   include/block/block.h |  3 +++
>   2 files changed, 52 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 430edf7..502b483 100644
> --- a/block.c
> +++ b/block.c
> @@ -4670,6 +4670,55 @@ static void bdrv_delete(BlockDriverState *bs)
>       g_free(bs);
>   }
>   
> +BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
> +                                   int flags, Error **errp)
> +{
> +    BlockDriverState *new_node_bs;
> +    Error *local_err = NULL;
> +
> +    new_node_bs =  bdrv_open(NULL, NULL, node_options, flags, errp);
> +    if (new_node_bs == NULL) {
> +        error_prepend(errp, "Could not create node: ");
> +        return NULL;
> +    }
> +
> +    bdrv_drained_begin(bs);
> +    bdrv_replace_node(bs, new_node_bs, &local_err);
> +    bdrv_drained_end(bs);
> +
> +    if (local_err) {
> +        bdrv_unref(new_node_bs);
> +        error_propagate(errp, local_err);
> +        return NULL;
> +    }
> +
> +    return new_node_bs;
> +}
> +
> +void bdrv_remove_node(BlockDriverState *bs)
> +{
> +    BdrvChild *child;
> +    BlockDriverState *inferior_bs;
> +
> +    child = bdrv_filter_or_cow_child(bs);
> +    if (!child) {
> +        return;
> +    }
> +    inferior_bs = child->bs;
> +
> +    /* Retain the BDS until we complete the graph change. */
> +    bdrv_ref(inferior_bs);
> +    /* Hold a guest back from writing while permissions are being reset. */
> +    bdrv_drained_begin(inferior_bs);
> +    /* Refresh permissions before the graph change. */
> +    bdrv_child_refresh_perms(bs, child, &error_abort);
> +    bdrv_replace_node(bs, inferior_bs, &error_abort);
> +
> +    bdrv_drained_end(inferior_bs);
> +    bdrv_unref(inferior_bs);
> +    bdrv_unref(bs);
> +}

The function is unused, and as I understand can't work as intended without s->active handling. I think it should be dropped

> +
>   /*
>    * Run consistency checks on an image
>    *
> diff --git a/include/block/block.h b/include/block/block.h
> index d16c401..ae7612f 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -350,6 +350,9 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
>                    Error **errp);
>   void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
>                          Error **errp);
> +BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
> +                                   int flags, Error **errp);
> +void bdrv_remove_node(BlockDriverState *bs);
>   
>   int bdrv_parse_aio(const char *mode, int *flags);
>   int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);
> 


-- 
Best regards,
Vladimir

Re: [PATCH v12 02/14] block: add insert/remove node functions

[Vladimir Sementsov-Ogievskiy]

23.10.2020 17:24, Vladimir Sementsov-Ogievskiy wrote:
> 22.10.2020 21:13, Andrey Shinkevich wrote:
>> Provide API for a node insertion to and removal from a backing chain.
>>
>> Suggested-by: Max Reitz <mreitz@redhat.com>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>   block.c               | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/block/block.h |  3 +++
>>   2 files changed, 52 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index 430edf7..502b483 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4670,6 +4670,55 @@ static void bdrv_delete(BlockDriverState *bs)
>>       g_free(bs);
>>   }
>> +BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
>> +                                   int flags, Error **errp)
>> +{
>> +    BlockDriverState *new_node_bs;
>> +    Error *local_err = NULL;
>> +
>> +    new_node_bs =  bdrv_open(NULL, NULL, node_options, flags, errp);
>> +    if (new_node_bs == NULL) {
>> +        error_prepend(errp, "Could not create node: ");
>> +        return NULL;
>> +    }
>> +
>> +    bdrv_drained_begin(bs);
>> +    bdrv_replace_node(bs, new_node_bs, &local_err);
>> +    bdrv_drained_end(bs);
>> +
>> +    if (local_err) {
>> +        bdrv_unref(new_node_bs);
>> +        error_propagate(errp, local_err);
>> +        return NULL;
>> +    }
>> +
>> +    return new_node_bs;
>> +}
>> +
>> +void bdrv_remove_node(BlockDriverState *bs)
>> +{
>> +    BdrvChild *child;
>> +    BlockDriverState *inferior_bs;
>> +
>> +    child = bdrv_filter_or_cow_child(bs);
>> +    if (!child) {
>> +        return;
>> +    }
>> +    inferior_bs = child->bs;
>> +
>> +    /* Retain the BDS until we complete the graph change. */
>> +    bdrv_ref(inferior_bs);
>> +    /* Hold a guest back from writing while permissions are being reset. */
>> +    bdrv_drained_begin(inferior_bs);
>> +    /* Refresh permissions before the graph change. */
>> +    bdrv_child_refresh_perms(bs, child, &error_abort);
>> +    bdrv_replace_node(bs, inferior_bs, &error_abort);
>> +
>> +    bdrv_drained_end(inferior_bs);
>> +    bdrv_unref(inferior_bs);
>> +    bdrv_unref(bs);
>> +}
> 
> The function is unused, and as I understand can't work as intended without s->active handling. I think it should be dropped

with bdrv_remove_node dropped:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> 
>> +
>>   /*
>>    * Run consistency checks on an image
>>    *
>> diff --git a/include/block/block.h b/include/block/block.h
>> index d16c401..ae7612f 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -350,6 +350,9 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
>>                    Error **errp);
>>   void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
>>                          Error **errp);
>> +BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
>> +                                   int flags, Error **errp);
>> +void bdrv_remove_node(BlockDriverState *bs);
>>   int bdrv_parse_aio(const char *mode, int *flags);
>>   int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);
>>
> 
> 


-- 
Best regards,
Vladimir

[PATCH v12 03/14] copy-on-read: add filter drop function

[Andrey Shinkevich via]

Provide API for the COR-filter removal. Also, drop the filter child
permissions for an inactive state when the filter node is being
removed. This function may be considered as an intermediate solution
before we are able to use bdrv_remove_node(). It will be possible once
the QEMU permission update system has overhauled.
To insert the filter, the block generic layer function
bdrv_insert_node() can be used.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 block/copy-on-read.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 block/copy-on-read.h | 32 ++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+)
 create mode 100644 block/copy-on-read.h

diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index cb03e0f..618c4c4 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -23,11 +23,20 @@
 #include "qemu/osdep.h"
 #include "block/block_int.h"
 #include "qemu/module.h"
+#include "qapi/error.h"
+#include "block/copy-on-read.h"
+
+
+typedef struct BDRVStateCOR {
+    bool active;
+} BDRVStateCOR;
 
 
 static int cor_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
+    BDRVStateCOR *state = bs->opaque;
+
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
                                BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
                                false, errp);
@@ -42,6 +51,13 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags,
         ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
             bs->file->bs->supported_zero_flags);
 
+    state->active = true;
+
+    /*
+     * We don't need to call bdrv_child_refresh_perms() now as the permissions
+     * will be updated later when the filter node gets its parent.
+     */
+
     return 0;
 }
 
@@ -57,6 +73,17 @@ static void cor_child_perm(BlockDriverState *bs, BdrvChild *c,
                            uint64_t perm, uint64_t shared,
                            uint64_t *nperm, uint64_t *nshared)
 {
+    BDRVStateCOR *s = bs->opaque;
+
+    if (!s->active) {
+        /*
+         * While the filter is being removed
+         */
+        *nperm = 0;
+        *nshared = BLK_PERM_ALL;
+        return;
+    }
+
     *nperm = perm & PERM_PASSTHROUGH;
     *nshared = (shared & PERM_PASSTHROUGH) | PERM_UNCHANGED;
 
@@ -135,6 +162,7 @@ static void cor_lock_medium(BlockDriverState *bs, bool locked)
 
 static BlockDriver bdrv_copy_on_read = {
     .format_name                        = "copy-on-read",
+    .instance_size                      = sizeof(BDRVStateCOR),
 
     .bdrv_open                          = cor_open,
     .bdrv_child_perm                    = cor_child_perm,
@@ -154,6 +182,34 @@ static BlockDriver bdrv_copy_on_read = {
     .is_filter                          = true,
 };
 
+
+void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs)
+{
+    BdrvChild *child;
+    BlockDriverState *bs;
+    BDRVStateCOR *s = cor_filter_bs->opaque;
+
+    child = bdrv_filter_child(cor_filter_bs);
+    if (!child) {
+        return;
+    }
+    bs = child->bs;
+
+    /* Retain the BDS until we complete the graph change. */
+    bdrv_ref(bs);
+    /* Hold a guest back from writing while permissions are being reset. */
+    bdrv_drained_begin(bs);
+    /* Drop permissions before the graph change. */
+    s->active = false;
+    bdrv_child_refresh_perms(cor_filter_bs, child, &error_abort);
+    bdrv_replace_node(cor_filter_bs, bs, &error_abort);
+
+    bdrv_drained_end(bs);
+    bdrv_unref(bs);
+    bdrv_unref(cor_filter_bs);
+}
+
+
 static void bdrv_copy_on_read_init(void)
 {
     bdrv_register(&bdrv_copy_on_read);
diff --git a/block/copy-on-read.h b/block/copy-on-read.h
new file mode 100644
index 0000000..7bf405d
--- /dev/null
+++ b/block/copy-on-read.h
@@ -0,0 +1,32 @@
+/*
+ * Copy-on-read filter block driver
+ *
+ * The filter driver performs Copy-On-Read (COR) operations
+ *
+ * Copyright (c) 2018-2020 Virtuozzo International GmbH.
+ *
+ * Author:
+ *   Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef BLOCK_COPY_ON_READ
+#define BLOCK_COPY_ON_READ
+
+#include "block/block_int.h"
+
+void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs);
+
+#endif /* BLOCK_COPY_ON_READ */
-- 
1.8.3.1

Re: [PATCH v12 03/14] copy-on-read: add filter drop function

[Vladimir Sementsov-Ogievskiy]

22.10.2020 21:13, Andrey Shinkevich wrote:
> Provide API for the COR-filter removal. Also, drop the filter child
> permissions for an inactive state when the filter node is being
> removed. This function may be considered as an intermediate solution
> before we are able to use bdrv_remove_node(). It will be possible once
> the QEMU permission update system has overhauled.
> To insert the filter, the block generic layer function
> bdrv_insert_node() can be used.
> 
> Signed-off-by: Andrey Shinkevich<andrey.shinkevich@virtuozzo.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH v12 04/14] qapi: add filter-node-name to block-stream

[Andrey Shinkevich via]

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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/monitor/block-hmp-cmds.c | 4 ++--
 block/stream.c                 | 4 +++-
 blockdev.c                     | 4 +++-
 include/block/block_int.h      | 7 ++++++-
 qapi/block-core.json           | 6 ++++++
 5 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index d15a2be..e8a58f3 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -508,8 +508,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/block/stream.c b/block/stream.c
index 8ce6729..e0540ee 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -221,7 +221,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;
diff --git a/blockdev.c b/blockdev.c
index fe6fb5d..c917625 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2499,6 +2499,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)
@@ -2581,7 +2582,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 38cad9d..f782737 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1134,6 +1134,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
@@ -1146,7 +1149,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/qapi/block-core.json b/qapi/block-core.json
index ee5ebef..0a64306 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2542,6 +2542,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.2)
+#
 # @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.
@@ -2572,6 +2577,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

[PATCH v12 05/14] qapi: create BlockdevOptionsCor structure for COR driver

[Andrey Shinkevich via]

Create the BlockdevOptionsCor structure for COR driver specific options
splitting it off form the BlockdevOptionsGenericFormat. The only option
'bottom' node in the structure denotes an image file that limits the
COR operations in the backing chain.

Suggested-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 qapi/block-core.json | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0a64306..bf465f6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3938,6 +3938,25 @@
   'data': { 'throttle-group': 'str',
             'file' : 'BlockdevRef'
              } }
+
+##
+# @BlockdevOptionsCor:
+#
+# Driver specific block device options for the copy-on-read driver.
+#
+# @bottom: the name of a non-filter node (allocation-bearing layer) that limits
+#          the COR operations in the backing chain (inclusive).
+#          For the block-stream job, it will be the first non-filter overlay of
+#          the base node. We do not involve the base node into the COR
+#          operations because the base may change due to a concurrent
+#          block-commit job on the same backing chain.
+#
+# Since: 5.2
+##
+{ 'struct': 'BlockdevOptionsCor',
+  'base': 'BlockdevOptionsGenericFormat',
+  'data': { '*bottom': 'str' } }
+
 ##
 # @BlockdevOptions:
 #
@@ -3990,7 +4009,7 @@
       'bochs':      'BlockdevOptionsGenericFormat',
       'cloop':      'BlockdevOptionsGenericFormat',
       'compress':   'BlockdevOptionsGenericFormat',
-      'copy-on-read':'BlockdevOptionsGenericFormat',
+      'copy-on-read':'BlockdevOptionsCor',
       'dmg':        'BlockdevOptionsGenericFormat',
       'file':       'BlockdevOptionsFile',
       'ftp':        'BlockdevOptionsCurlFtp',
-- 
1.8.3.1

Re: [PATCH v12 05/14] qapi: create BlockdevOptionsCor structure for COR driver

[Vladimir Sementsov-Ogievskiy]

22.10.2020 21:13, Andrey Shinkevich wrote:
> Create the BlockdevOptionsCor structure for COR driver specific options
> splitting it off form the BlockdevOptionsGenericFormat. The only option
> 'bottom' node in the structure denotes an image file that limits the
> COR operations in the backing chain.
> 
> Suggested-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   qapi/block-core.json | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0a64306..bf465f6 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3938,6 +3938,25 @@
>     'data': { 'throttle-group': 'str',
>               'file' : 'BlockdevRef'
>                } }
> +
> +##
> +# @BlockdevOptionsCor:
> +#
> +# Driver specific block device options for the copy-on-read driver.
> +#
> +# @bottom: the name of a non-filter node (allocation-bearing layer) that limits
> +#          the COR operations in the backing chain (inclusive).
> +#          For the block-stream job, it will be the first non-filter overlay of
> +#          the base node. We do not involve the base node into the COR
> +#          operations because the base may change due to a concurrent
> +#          block-commit job on the same backing chain. 
> +#
> +# Since: 5.2
> +##
> +{ 'struct': 'BlockdevOptionsCor',
> +  'base': 'BlockdevOptionsGenericFormat',
> +  'data': { '*bottom': 'str' } }
> +
>   ##
>   # @BlockdevOptions:
>   #
> @@ -3990,7 +4009,7 @@
>         'bochs':      'BlockdevOptionsGenericFormat',
>         'cloop':      'BlockdevOptionsGenericFormat',
>         'compress':   'BlockdevOptionsGenericFormat',
> -      'copy-on-read':'BlockdevOptionsGenericFormat',
> +      'copy-on-read':'BlockdevOptionsCor',
>         'dmg':        'BlockdevOptionsGenericFormat',
>         'file':       'BlockdevOptionsFile',
>         'ftp':        'BlockdevOptionsCurlFtp',
> 

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Still, I'd prefer this to be merged with further two patches, to not add non-working interfaces even if this will be fixed two commits further. We do similar things sometimes to simplify big commits, but in this case merged 03/04/05 doesn't seem too big.


-- 
Best regards,
Vladimir

[PATCH v12 06/14] copy-on-read: pass bottom node name to COR driver

[Andrey Shinkevich via]

We are going to use the COR-filter for a block-stream job.
To limit COR operations by the base node in the backing chain during
stream job, pass the bottom node name, that is the first non-filter
overlay of the base, to the copy-on-read driver as the base node itself
may change due to possible concurrent jobs.
The rest of the functionality will be implemented in the patch that
follows.

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

diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 618c4c4..3d8e4db 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -24,18 +24,24 @@
 #include "block/block_int.h"
 #include "qemu/module.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qdict.h"
 #include "block/copy-on-read.h"
 
 
 typedef struct BDRVStateCOR {
     bool active;
+    BlockDriverState *bottom_bs;
 } BDRVStateCOR;
 
 
 static int cor_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
+    BlockDriverState *bottom_bs = NULL;
     BDRVStateCOR *state = bs->opaque;
+    /* Find a bottom node name, if any */
+    const char *bottom_node = qdict_get_try_str(options, "bottom");
 
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
                                BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -51,7 +57,17 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags,
         ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
             bs->file->bs->supported_zero_flags);
 
+    if (bottom_node) {
+        bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp);
+        if (!bottom_bs) {
+            error_setg(errp, QERR_BASE_NOT_FOUND, bottom_node);
+            qdict_del(options, "bottom");
+            return -EINVAL;
+        }
+        qdict_del(options, "bottom");
+    }
     state->active = true;
+    state->bottom_bs = bottom_bs;
 
     /*
      * We don't need to call bdrv_child_refresh_perms() now as the permissions
-- 
1.8.3.1

Re: [PATCH v12 06/14] copy-on-read: pass bottom node name to COR driver

[Vladimir Sementsov-Ogievskiy]

22.10.2020 21:13, Andrey Shinkevich wrote:
> We are going to use the COR-filter for a block-stream job.
> To limit COR operations by the base node in the backing chain during
> stream job, pass the bottom node name, that is the first non-filter
> overlay of the base, to the copy-on-read driver as the base node itself
> may change due to possible concurrent jobs.
> The rest of the functionality will be implemented in the patch that
> follows.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block/copy-on-read.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index 618c4c4..3d8e4db 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -24,18 +24,24 @@
>   #include "block/block_int.h"
>   #include "qemu/module.h"
>   #include "qapi/error.h"
> +#include "qapi/qmp/qerror.h"
> +#include "qapi/qmp/qdict.h"
>   #include "block/copy-on-read.h"
>   
>   
>   typedef struct BDRVStateCOR {
>       bool active;
> +    BlockDriverState *bottom_bs;
>   } BDRVStateCOR;
>   
>   
>   static int cor_open(BlockDriverState *bs, QDict *options, int flags,
>                       Error **errp)
>   {
> +    BlockDriverState *bottom_bs = NULL;
>       BDRVStateCOR *state = bs->opaque;
> +    /* Find a bottom node name, if any */
> +    const char *bottom_node = qdict_get_try_str(options, "bottom");
>   
>       bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
>                                  BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
> @@ -51,7 +57,17 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags,
>           ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
>               bs->file->bs->supported_zero_flags);
>   
> +    if (bottom_node) {
> +        bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp);
> +        if (!bottom_bs) {
> +            error_setg(errp, QERR_BASE_NOT_FOUND, bottom_node);

QERR_BASE_NOT_FOUND is unrelated here. Also, I see a comment in qerror.h that such macros should not be used in new code. And don't forget to drop qerror.h include line.

> +            qdict_del(options, "bottom");

this may be moved above "bottom_bs = ..", to not call it after "if" in separate.

> +            return -EINVAL;
> +        }
> +        qdict_del(options, "bottom");
> +    }
>       state->active = true;
> +    state->bottom_bs = bottom_bs;
>   
>       /*
>        * We don't need to call bdrv_child_refresh_perms() now as the permissions
> 


-- 
Best regards,
Vladimir

Re: [PATCH v12 06/14] copy-on-read: pass bottom node name to COR driver

[Andrey Shinkevich]

On 23.10.2020 17:45, Vladimir Sementsov-Ogievskiy wrote:
> 22.10.2020 21:13, Andrey Shinkevich wrote:
>> We are going to use the COR-filter for a block-stream job.
>> To limit COR operations by the base node in the backing chain during
>> stream job, pass the bottom node name, that is the first non-filter
>> overlay of the base, to the copy-on-read driver as the base node itself
>> may change due to possible concurrent jobs.
>> The rest of the functionality will be implemented in the patch that
>> follows.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>   block/copy-on-read.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>> index 618c4c4..3d8e4db 100644
>> --- a/block/copy-on-read.c
>> +++ b/block/copy-on-read.c
>> @@ -24,18 +24,24 @@
>>   #include "block/block_int.h"
>>   #include "qemu/module.h"
>>   #include "qapi/error.h"
>> +#include "qapi/qmp/qerror.h"
>> +#include "qapi/qmp/qdict.h"
>>   #include "block/copy-on-read.h"
>>   typedef struct BDRVStateCOR {
>>       bool active;
>> +    BlockDriverState *bottom_bs;
>>   } BDRVStateCOR;
>>   static int cor_open(BlockDriverState *bs, QDict *options, int flags,
>>                       Error **errp)
>>   {
>> +    BlockDriverState *bottom_bs = NULL;
>>       BDRVStateCOR *state = bs->opaque;
>> +    /* Find a bottom node name, if any */
>> +    const char *bottom_node = qdict_get_try_str(options, "bottom");
>>       bs->file = bdrv_open_child(NULL, options, "file", bs, 
>> &child_of_bds,
>>                                  BDRV_CHILD_FILTERED | 
>> BDRV_CHILD_PRIMARY,
>> @@ -51,7 +57,17 @@ static int cor_open(BlockDriverState *bs, QDict 
>> *options, int flags,
>>           ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
>>               bs->file->bs->supported_zero_flags);
>> +    if (bottom_node) {
>> +        bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp);
>> +        if (!bottom_bs) {
>> +            error_setg(errp, QERR_BASE_NOT_FOUND, bottom_node);
> 
> QERR_BASE_NOT_FOUND is unrelated here. Also, I see a comment in qerror.h 
> that such macros should not be used in new code. And don't forget to 
> drop qerror.h include line.
> 

I have been surprized because I don't have it in my branch and instead I do:
error_setg(errp, "Bottom node '%s' not found", bottom_node);

>> +            qdict_del(options, "bottom");
> 
> this may be moved above "bottom_bs = ..", to not call it after "if" in 
> separate.
> 

Please, see the "Re: [PATCH v11 04/13] copy-on-read: pass overlay base 
node name to COR driver".

>> +            return -EINVAL;
>> +        }
>> +        qdict_del(options, "bottom");
>> +    }
>>       state->active = true;
>> +    state->bottom_bs = bottom_bs;
>>       /*
>>        * We don't need to call bdrv_child_refresh_perms() now as the 
>> permissions
>>
> 
> 

Re: [PATCH v12 06/14] copy-on-read: pass bottom node name to COR driver

[Vladimir Sementsov-Ogievskiy]

23.10.2020 18:31, Andrey Shinkevich wrote:
> 
> On 23.10.2020 17:45, Vladimir Sementsov-Ogievskiy wrote:
>> 22.10.2020 21:13, Andrey Shinkevich wrote:
>>> We are going to use the COR-filter for a block-stream job.
>>> To limit COR operations by the base node in the backing chain during
>>> stream job, pass the bottom node name, that is the first non-filter
>>> overlay of the base, to the copy-on-read driver as the base node itself
>>> may change due to possible concurrent jobs.
>>> The rest of the functionality will be implemented in the patch that
>>> follows.
>>>
>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>> ---
>>>   block/copy-on-read.c | 16 ++++++++++++++++
>>>   1 file changed, 16 insertions(+)
>>>
>>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>>> index 618c4c4..3d8e4db 100644
>>> --- a/block/copy-on-read.c
>>> +++ b/block/copy-on-read.c
>>> @@ -24,18 +24,24 @@
>>>   #include "block/block_int.h"
>>>   #include "qemu/module.h"
>>>   #include "qapi/error.h"
>>> +#include "qapi/qmp/qerror.h"
>>> +#include "qapi/qmp/qdict.h"
>>>   #include "block/copy-on-read.h"
>>>   typedef struct BDRVStateCOR {
>>>       bool active;
>>> +    BlockDriverState *bottom_bs;
>>>   } BDRVStateCOR;
>>>   static int cor_open(BlockDriverState *bs, QDict *options, int flags,
>>>                       Error **errp)
>>>   {
>>> +    BlockDriverState *bottom_bs = NULL;
>>>       BDRVStateCOR *state = bs->opaque;
>>> +    /* Find a bottom node name, if any */
>>> +    const char *bottom_node = qdict_get_try_str(options, "bottom");
>>>       bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
>>>                                  BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
>>> @@ -51,7 +57,17 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags,
>>>           ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
>>>               bs->file->bs->supported_zero_flags);
>>> +    if (bottom_node) {
>>> +        bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp);
>>> +        if (!bottom_bs) {
>>> +            error_setg(errp, QERR_BASE_NOT_FOUND, bottom_node);
>>
>> QERR_BASE_NOT_FOUND is unrelated here. Also, I see a comment in qerror.h that such macros should not be used in new code. And don't forget to drop qerror.h include line.
>>
> 
> I have been surprized because I don't have it in my branch and instead I do:
> error_setg(errp, "Bottom node '%s' not found", bottom_node);

OK, with it:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> 
>>> +            qdict_del(options, "bottom");
>>
>> this may be moved above "bottom_bs = ..", to not call it after "if" in separate.
>>
> 
> Please, see the "Re: [PATCH v11 04/13] copy-on-read: pass overlay base node name to COR driver".

Hm, assume that it may free the string itself, OK then.

> 
>>> +            return -EINVAL;
>>> +        }
>>> +        qdict_del(options, "bottom");
>>> +    }
>>>       state->active = true;
>>> +    state->bottom_bs = bottom_bs;
>>>       /*
>>>        * We don't need to call bdrv_child_refresh_perms() now as the permissions
>>>
>>
>>


-- 
Best regards,
Vladimir

[PATCH v12 07/14] copy-on-read: limit COR operations to bottom node

[Andrey Shinkevich via]

Limit COR operations to the bottom node (inclusively) in the backing
chain when the bottom node name is given. It will be useful for a block
stream job when the COR-filter is applied. The bottom node is passed as
the base itself may change due to concurrent commit jobs on the same
backing chain.

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

diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 3d8e4db..8178a91 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -123,8 +123,46 @@ static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
                                            size_t qiov_offset,
                                            int flags)
 {
-    return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
-                               flags | BDRV_REQ_COPY_ON_READ);
+    int64_t n = 0;
+    int local_flags;
+    int ret;
+    BDRVStateCOR *state = bs->opaque;
+
+    if (!state->bottom_bs) {
+        return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
+                                   flags | BDRV_REQ_COPY_ON_READ);
+    }
+
+    while (bytes) {
+        local_flags = flags;
+
+        /* In case of failure, try to copy-on-read anyway */
+        ret = bdrv_is_allocated(bs->file->bs, offset, bytes, &n);
+        if (!ret || ret < 0) {
+            ret = bdrv_is_allocated_above(bdrv_backing_chain_next(bs->file->bs),
+                                          state->bottom_bs, true, offset,
+                                          n, &n);
+            if (ret == 1 || ret < 0) {
+                local_flags |= BDRV_REQ_COPY_ON_READ;
+            }
+            /* Finish earlier if the end of a backing file has been reached */
+            if (ret == 0 && n == 0) {
+                break;
+            }
+        }
+
+        ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset,
+                                  local_flags);
+        if (ret < 0) {
+            return ret;
+        }
+
+        offset += n;
+        qiov_offset += n;
+        bytes -= n;
+    }
+
+    return 0;
 }
 
 
-- 
1.8.3.1

Re: [PATCH v12 07/14] copy-on-read: limit COR operations to bottom node

[Vladimir Sementsov-Ogievskiy]

22.10.2020 21:13, Andrey Shinkevich wrote:
> Limit COR operations to the bottom node (inclusively) in the backing
> chain when the bottom node name is given. It will be useful for a block
> stream job when the COR-filter is applied. The bottom node is passed as
> the base itself may change due to concurrent commit jobs on the same
> backing chain.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block/copy-on-read.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index 3d8e4db..8178a91 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -123,8 +123,46 @@ static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
>                                              size_t qiov_offset,
>                                              int flags)
>   {
> -    return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
> -                               flags | BDRV_REQ_COPY_ON_READ);
> +    int64_t n = 0;

no need to initialize n actually.

> +    int local_flags;
> +    int ret;
> +    BDRVStateCOR *state = bs->opaque;
> +
> +    if (!state->bottom_bs) {
> +        return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
> +                                   flags | BDRV_REQ_COPY_ON_READ);
> +    }
> +
> +    while (bytes) {
> +        local_flags = flags;
> +
> +        /* In case of failure, try to copy-on-read anyway */
> +        ret = bdrv_is_allocated(bs->file->bs, offset, bytes, &n);
> +        if (!ret || ret < 0) {

simper: if (ret <= 0) {

> +            ret = bdrv_is_allocated_above(bdrv_backing_chain_next(bs->file->bs),
> +                                          state->bottom_bs, true, offset,
> +                                          n, &n);
> +            if (ret == 1 || ret < 0) {
> +                local_flags |= BDRV_REQ_COPY_ON_READ;
> +            }
> +            /* Finish earlier if the end of a backing file has been reached */
> +            if (ret == 0 && n == 0) {

"n == 0" should be enough

> +                break;
> +            }
> +        }
> +
> +        ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset,
> +                                  local_flags);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +
> +        offset += n;
> +        qiov_offset += n;
> +        bytes -= n;
> +    }
> +
> +    return 0;
>   }
>   

My remarks are minor, so:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

And yes, I still think that this is good to be merged with two previous patches.


-- 
Best regards,
Vladimir

[PATCH v12 08/14] iotests: add #310 to test bottom node in COR driver

[Andrey Shinkevich via]

The test case #310 is similar to #216 by Max Reitz. The difference is
that the test #310 involves a bottom node to the COR filter driver.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/310     | 109 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/310.out |  15 +++++++
 tests/qemu-iotests/group   |   3 +-
 3 files changed, 126 insertions(+), 1 deletion(-)
 create mode 100755 tests/qemu-iotests/310
 create mode 100644 tests/qemu-iotests/310.out

diff --git a/tests/qemu-iotests/310 b/tests/qemu-iotests/310
new file mode 100755
index 0000000..5ad7ad2
--- /dev/null
+++ b/tests/qemu-iotests/310
@@ -0,0 +1,109 @@
+#!/usr/bin/env python3
+#
+# Copy-on-read tests using a COR filter with a bottom node
+#
+# Copyright (c) 2020 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import iotests
+from iotests import log, qemu_img, qemu_io_silent
+
+# Need backing file support
+iotests.script_initialize(supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk'],
+                          supported_platforms=['linux'])
+
+log('')
+log('=== Copy-on-read across nodes ===')
+log('')
+
+# This test is similar to the 216 one by Max Reitz <mreitz@redhat.com>
+# The difference is that this test case involves a bottom node to the
+# COR filter driver.
+
+with iotests.FilePath('base.img') as base_img_path, \
+     iotests.FilePath('mid.img') as mid_img_path, \
+     iotests.FilePath('top.img') as top_img_path, \
+     iotests.VM() as vm:
+
+    log('--- Setting up images ---')
+    log('')
+
+    assert qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M') == 0
+    assert qemu_io_silent(base_img_path, '-c', 'write -P 1 0M 1M') == 0
+    assert qemu_io_silent(base_img_path, '-c', 'write -P 1 3M 1M') == 0
+    assert qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
+                    '-F', iotests.imgfmt, mid_img_path) == 0
+    assert qemu_io_silent(mid_img_path,  '-c', 'write -P 3 2M 1M') == 0
+    assert qemu_io_silent(mid_img_path,  '-c', 'write -P 3 4M 1M') == 0
+    assert qemu_img('create', '-f', iotests.imgfmt, '-b', mid_img_path,
+                    '-F', iotests.imgfmt, top_img_path) == 0
+    assert qemu_io_silent(top_img_path,  '-c', 'write -P 2 1M 1M') == 0
+
+    log('Done')
+
+    log('')
+    log('--- Doing COR ---')
+    log('')
+
+    vm.launch()
+
+    log(vm.qmp('blockdev-add',
+                    node_name='node0',
+                    driver='copy-on-read',
+                    bottom='node2',
+                    file={
+                        'driver': iotests.imgfmt,
+                            'file': {
+                                'driver': 'file',
+                                'filename': top_img_path
+                            },
+                            'backing': {
+                                'node-name': 'node2',
+                                'driver': iotests.imgfmt,
+                                'file': {
+                                    'driver': 'file',
+                                    'filename': mid_img_path
+                                },
+                                'backing': {
+                                    #'node-name': 'node2',
+                                    'driver': iotests.imgfmt,
+                                    'file': {
+                                        'driver': 'file',
+                                        'filename': base_img_path
+                                    }
+                                },
+                            }
+                    }))
+
+    # Trigger COR
+    log(vm.qmp('human-monitor-command',
+               command_line='qemu-io node0 "read 0 5M"'))
+
+    vm.shutdown()
+
+    log('')
+    log('--- Checking COR result ---')
+    log('')
+
+    assert qemu_io_silent(base_img_path, '-c', 'discard 0 4M') == 0
+    assert qemu_io_silent(mid_img_path, '-c', 'discard 0M 5M') == 0
+    assert qemu_io_silent(top_img_path,  '-c', 'read -P 1 0M 1M') != 0
+    assert qemu_io_silent(top_img_path,  '-c', 'read -P 2 1M 1M') == 0
+    assert qemu_io_silent(top_img_path,  '-c', 'read -P 3 2M 1M') == 0
+    assert qemu_io_silent(top_img_path,  '-c', 'read -P 1 3M 1M') != 0
+    assert qemu_io_silent(top_img_path,  '-c', 'read -P 3 4M 1M') == 0
+
+    log('Done')
diff --git a/tests/qemu-iotests/310.out b/tests/qemu-iotests/310.out
new file mode 100644
index 0000000..a70aa5c
--- /dev/null
+++ b/tests/qemu-iotests/310.out
@@ -0,0 +1,15 @@
+
+=== Copy-on-read across nodes ===
+
+--- Setting up images ---
+
+Done
+
+--- Doing COR ---
+
+{"return": {}}
+{"return": ""}
+
+--- Checking COR result ---
+
+Done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 3432989..769029b 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -314,4 +314,5 @@
 303 rw quick
 304 rw quick
 305 rw quick
-307 rw quick export
+307 rw quick
+310 rw quick export
-- 
1.8.3.1

Re: [PATCH v12 08/14] iotests: add #310 to test bottom node in COR driver

[Vladimir Sementsov-Ogievskiy]

22.10.2020 21:13, Andrey Shinkevich wrote:
> The test case #310 is similar to #216 by Max Reitz. The difference is
> that the test #310 involves a bottom node to the COR filter driver.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/310     | 109 +++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/310.out |  15 +++++++
>   tests/qemu-iotests/group   |   3 +-
>   3 files changed, 126 insertions(+), 1 deletion(-)
>   create mode 100755 tests/qemu-iotests/310
>   create mode 100644 tests/qemu-iotests/310.out
> 
> diff --git a/tests/qemu-iotests/310 b/tests/qemu-iotests/310
> new file mode 100755
> index 0000000..5ad7ad2
> --- /dev/null
> +++ b/tests/qemu-iotests/310
> @@ -0,0 +1,109 @@
> +#!/usr/bin/env python3
> +#
> +# Copy-on-read tests using a COR filter with a bottom node
> +#
> +# Copyright (c) 2020 Virtuozzo International GmbH

Probably you should keep original copyright too.

> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import iotests
> +from iotests import log, qemu_img, qemu_io_silent
> +
> +# Need backing file support
> +iotests.script_initialize(supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk'],
> +                          supported_platforms=['linux'])
> +
> +log('')
> +log('=== Copy-on-read across nodes ===')
> +log('')
> +
> +# This test is similar to the 216 one by Max Reitz <mreitz@redhat.com>
> +# The difference is that this test case involves a bottom node to the
> +# COR filter driver.
> +
> +with iotests.FilePath('base.img') as base_img_path, \
> +     iotests.FilePath('mid.img') as mid_img_path, \
> +     iotests.FilePath('top.img') as top_img_path, \
> +     iotests.VM() as vm:
> +
> +    log('--- Setting up images ---')
> +    log('')
> +
> +    assert qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M') == 0
> +    assert qemu_io_silent(base_img_path, '-c', 'write -P 1 0M 1M') == 0
> +    assert qemu_io_silent(base_img_path, '-c', 'write -P 1 3M 1M') == 0
> +    assert qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
> +                    '-F', iotests.imgfmt, mid_img_path) == 0
> +    assert qemu_io_silent(mid_img_path,  '-c', 'write -P 3 2M 1M') == 0
> +    assert qemu_io_silent(mid_img_path,  '-c', 'write -P 3 4M 1M') == 0
> +    assert qemu_img('create', '-f', iotests.imgfmt, '-b', mid_img_path,
> +                    '-F', iotests.imgfmt, top_img_path) == 0
> +    assert qemu_io_silent(top_img_path,  '-c', 'write -P 2 1M 1M') == 0

An ascii-art of what we have done won't hart, like:

#      0 1 2 3 4
# top    2
# mid      3   3
# base 1     1


> +
> +    log('Done')
> +
> +    log('')
> +    log('--- Doing COR ---')
> +    log('')
> +
> +    vm.launch()
> +
> +    log(vm.qmp('blockdev-add',
> +                    node_name='node0',
> +                    driver='copy-on-read',
> +                    bottom='node2',
> +                    file={
> +                        'driver': iotests.imgfmt,
> +                            'file': {
> +                                'driver': 'file',
> +                                'filename': top_img_path
> +                            },
> +                            'backing': {
> +                                'node-name': 'node2',
> +                                'driver': iotests.imgfmt,
> +                                'file': {
> +                                    'driver': 'file',
> +                                    'filename': mid_img_path
> +                                },
> +                                'backing': {
> +                                    #'node-name': 'node2',
> +                                    'driver': iotests.imgfmt,
> +                                    'file': {
> +                                        'driver': 'file',
> +                                        'filename': base_img_path
> +                                    }
> +                                },
> +                            }
> +                    }))
> +
> +    # Trigger COR
> +    log(vm.qmp('human-monitor-command',
> +               command_line='qemu-io node0 "read 0 5M"'))
> +
> +    vm.shutdown()
> +
> +    log('')
> +    log('--- Checking COR result ---')
> +    log('')
> +
> +    assert qemu_io_silent(base_img_path, '-c', 'discard 0 4M') == 0
> +    assert qemu_io_silent(mid_img_path, '-c', 'discard 0M 5M') == 0
> +    assert qemu_io_silent(top_img_path,  '-c', 'read -P 1 0M 1M') != 0

Better assert what it should be instead of what it should not:

  assert qemu_io_silent(top_img_path,  '-c', 'read -P 0 0 1M') == 0

> +    assert qemu_io_silent(top_img_path,  '-c', 'read -P 2 1M 1M') == 0
> +    assert qemu_io_silent(top_img_path,  '-c', 'read -P 3 2M 1M') == 0
> +    assert qemu_io_silent(top_img_path,  '-c', 'read -P 1 3M 1M') != 0

  and here

  assert qemu_io_silent(top_img_path,  '-c', 'read -P 0 3M 1M') == 0


> +    assert qemu_io_silent(top_img_path,  '-c', 'read -P 3 4M 1M') == 0
> +
> +    log('Done')
> diff --git a/tests/qemu-iotests/310.out b/tests/qemu-iotests/310.out
> new file mode 100644
> index 0000000..a70aa5c
> --- /dev/null
> +++ b/tests/qemu-iotests/310.out
> @@ -0,0 +1,15 @@
> +
> +=== Copy-on-read across nodes ===
> +
> +--- Setting up images ---
> +
> +Done
> +
> +--- Doing COR ---
> +
> +{"return": {}}
> +{"return": ""}
> +
> +--- Checking COR result ---
> +
> +Done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 3432989..769029b 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -314,4 +314,5 @@
>   303 rw quick
>   304 rw quick
>   305 rw quick
> -307 rw quick export
> +307 rw quick
> +310 rw quick export
> 

you shouldn't modify 307... line here, and I think "export" is unrelated.

so, with 307 as is, and add 310 without "export" group, and also two my previous suggestions:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH v12 09/14] block: modify the comment for BDRV_REQ_PREFETCH flag

[Andrey Shinkevich via]

Modify the comment for the flag BDRV_REQ_PREFETCH as we are going to
use it alone and pass it to the COR-filter driver for further
processing.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 include/block/block.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index ae7612f..1b6742f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -81,9 +81,11 @@ typedef enum {
     BDRV_REQ_NO_FALLBACK        = 0x100,
 
     /*
-     * BDRV_REQ_PREFETCH may be used only together with BDRV_REQ_COPY_ON_READ
-     * on read request and means that caller doesn't really need data to be
-     * written to qiov parameter which may be NULL.
+     * BDRV_REQ_PREFETCH makes sense only in the context of copy-on-read
+     * (i.e., together with the BDRV_REQ_COPY_ON_READ flag or when a COR
+     * filter is involved), in which case it signals that the COR operation
+     * need not read the data into memory (qiov) but only ensure they are
+     * copied to the top layer (i.e., that COR operation is done).
      */
     BDRV_REQ_PREFETCH  = 0x200,
     /* Mask of valid flags */
-- 
1.8.3.1

Re: [PATCH v12 09/14] block: modify the comment for BDRV_REQ_PREFETCH flag

[Vladimir Sementsov-Ogievskiy]

22.10.2020 21:13, Andrey Shinkevich wrote:
> Modify the comment for the flag BDRV_REQ_PREFETCH as we are going to
> use it alone and pass it to the COR-filter driver for further
> processing.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   include/block/block.h | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index ae7612f..1b6742f 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -81,9 +81,11 @@ typedef enum {
>       BDRV_REQ_NO_FALLBACK        = 0x100,
>   
>       /*
> -     * BDRV_REQ_PREFETCH may be used only together with BDRV_REQ_COPY_ON_READ
> -     * on read request and means that caller doesn't really need data to be
> -     * written to qiov parameter which may be NULL.
> +     * BDRV_REQ_PREFETCH makes sense only in the context of copy-on-read
> +     * (i.e., together with the BDRV_REQ_COPY_ON_READ flag or when a COR
> +     * filter is involved), in which case it signals that the COR operation
> +     * need not read the data into memory (qiov) but only ensure they are
> +     * copied to the top layer (i.e., that COR operation is done).
>        */
>       BDRV_REQ_PREFETCH  = 0x200,
>       /* Mask of valid flags */
> 

I like the wording.. I think this patch should be merged together with 11 and 12 to make sense.

-- 
Best regards,
Vladimir

[PATCH v12 10/14] block: include supported_read_flags into BDS structure

[Andrey Shinkevich via]

Add the new member supported_read_flags to the BlockDriverState
structure. It will control the flags set for copy-on-read operations.
Make the block generic layer evaluate supported read flags before they
go to a block driver.

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

diff --git a/block/io.c b/block/io.c
index 54f0968..78ddf13 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1392,6 +1392,9 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
     if (flags & BDRV_REQ_COPY_ON_READ) {
         int64_t pnum;
 
+        /* The flag BDRV_REQ_COPY_ON_READ has reached its addressee */
+        flags &= ~BDRV_REQ_COPY_ON_READ;
+
         ret = bdrv_is_allocated(bs, offset, bytes, &pnum);
         if (ret < 0) {
             goto out;
@@ -1413,9 +1416,13 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
         goto out;
     }
 
+    if (flags & ~bs->supported_read_flags) {
+        abort();
+    }
+
     max_bytes = ROUND_UP(MAX(0, total_bytes - offset), align);
     if (bytes <= max_bytes && bytes <= max_transfer) {
-        ret = bdrv_driver_preadv(bs, offset, bytes, qiov, qiov_offset, 0);
+        ret = bdrv_driver_preadv(bs, offset, bytes, qiov, qiov_offset, flags);
         goto out;
     }
 
@@ -1428,7 +1435,8 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
 
             ret = bdrv_driver_preadv(bs, offset + bytes - bytes_remaining,
                                      num, qiov,
-                                     qiov_offset + bytes - bytes_remaining, 0);
+                                     qiov_offset + bytes - bytes_remaining,
+                                     flags);
             max_bytes -= num;
         } else {
             num = bytes_remaining;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f782737..474174c 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -873,6 +873,10 @@ struct BlockDriverState {
     /* I/O Limits */
     BlockLimits bl;
 
+    /*
+     * Flags honored during pread
+     */
+    unsigned int supported_read_flags;
     /* Flags honored during pwrite (so far: BDRV_REQ_FUA,
      * BDRV_REQ_WRITE_UNCHANGED).
      * If a driver does not support BDRV_REQ_WRITE_UNCHANGED, those
-- 
1.8.3.1

Re: [PATCH v12 10/14] block: include supported_read_flags into BDS structure

[Vladimir Sementsov-Ogievskiy]

22.10.2020 21:13, Andrey Shinkevich wrote:
> Add the new member supported_read_flags to the BlockDriverState
> structure. It will control the flags set for copy-on-read operations.
> Make the block generic layer evaluate supported read flags before they
> go to a block driver.
> 
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block/io.c                | 12 ++++++++++--
>   include/block/block_int.h |  4 ++++
>   2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 54f0968..78ddf13 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1392,6 +1392,9 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
>       if (flags & BDRV_REQ_COPY_ON_READ) {
>           int64_t pnum;
>   
> +        /* The flag BDRV_REQ_COPY_ON_READ has reached its addressee */
> +        flags &= ~BDRV_REQ_COPY_ON_READ;
> +
>           ret = bdrv_is_allocated(bs, offset, bytes, &pnum);
>           if (ret < 0) {
>               goto out;
> @@ -1413,9 +1416,13 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
>           goto out;
>       }
>   
> +    if (flags & ~bs->supported_read_flags) {
> +        abort();
> +    }
> +

So, you decided to be strict with all read-flags passed to driver, not only PREFETCH.. It makes sense.


>       max_bytes = ROUND_UP(MAX(0, total_bytes - offset), align);
>       if (bytes <= max_bytes && bytes <= max_transfer) {
> -        ret = bdrv_driver_preadv(bs, offset, bytes, qiov, qiov_offset, 0);
> +        ret = bdrv_driver_preadv(bs, offset, bytes, qiov, qiov_offset, flags);
>           goto out;
>       }
>   
> @@ -1428,7 +1435,8 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
>   
>               ret = bdrv_driver_preadv(bs, offset + bytes - bytes_remaining,
>                                        num, qiov,
> -                                     qiov_offset + bytes - bytes_remaining, 0);
> +                                     qiov_offset + bytes - bytes_remaining,
> +                                     flags);
>               max_bytes -= num;
>           } else {
>               num = bytes_remaining;
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f782737..474174c 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -873,6 +873,10 @@ struct BlockDriverState {
>       /* I/O Limits */
>       BlockLimits bl;
>   
> +    /*
> +     * Flags honored during pread
> +     */
> +    unsigned int supported_read_flags;
>       /* Flags honored during pwrite (so far: BDRV_REQ_FUA,
>        * BDRV_REQ_WRITE_UNCHANGED).
>        * If a driver does not support BDRV_REQ_WRITE_UNCHANGED, those
> 


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH v12 11/14] copy-on-read: add support for read flags to COR-filter

[Andrey Shinkevich via]

Add the BDRV_REQ_COPY_ON_READ and BDRV_REQ_PREFETCH flags to the
supported_read_flags of the COR-filter.

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

diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 8178a91..a2b180a 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -50,6 +50,8 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
+    bs->supported_read_flags = BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH;
+
     bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
         (BDRV_REQ_FUA & bs->file->bs->supported_write_flags);
 
-- 
1.8.3.1

Re: [PATCH v12 11/14] copy-on-read: add support for read flags to COR-filter

[Vladimir Sementsov-Ogievskiy]

22.10.2020 21:13, Andrey Shinkevich wrote:
> Add the BDRV_REQ_COPY_ON_READ and BDRV_REQ_PREFETCH flags to the
> supported_read_flags of the COR-filter.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block/copy-on-read.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index 8178a91..a2b180a 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -50,6 +50,8 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags,
>           return -EINVAL;
>       }
>   
> +    bs->supported_read_flags = BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH;
> +
>       bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
>           (BDRV_REQ_FUA & bs->file->bs->supported_write_flags);
>   
> 

This should be merged with the following patch, otherwise it doesn't make sense. You mark filter as supporting PREFETCH, but actually it just ignores it (and may crash on trying to read into qiov=NULL).

-- 
Best regards,
Vladimir

Re: [PATCH v12 11/14] copy-on-read: add support for read flags to COR-filter

[Vladimir Sementsov-Ogievskiy]

27.10.2020 17:46, Vladimir Sementsov-Ogievskiy wrote:
> 22.10.2020 21:13, Andrey Shinkevich wrote:
>> Add the BDRV_REQ_COPY_ON_READ and BDRV_REQ_PREFETCH flags to the
>> supported_read_flags of the COR-filter.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>   block/copy-on-read.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>> index 8178a91..a2b180a 100644
>> --- a/block/copy-on-read.c
>> +++ b/block/copy-on-read.c
>> @@ -50,6 +50,8 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags,
>>           return -EINVAL;
>>       }
>> +    bs->supported_read_flags = BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH;
>> +
>>       bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
>>           (BDRV_REQ_FUA & bs->file->bs->supported_write_flags);
>>
> 
> This should be merged with the following patch, otherwise it doesn't make sense. You mark filter as supporting PREFETCH, but actually it just ignores it (and may crash on trying to read into qiov=NULL).
> 

Ah, no, problem is not in qiov=NULL, but in that we will just pass PREFETCH to bs->file, which may not support it and crash in block.io in the new abort() from patch 10.


Also, any reason to add support for BDRV_REQ_COPY_ON_READ ? What it means for cor filter? I don't know. It make sense only for generic layer and handled in generic layer. It never passed to driver, so let's not declare support for it.

-- 
Best regards,
Vladimir

[PATCH v12 12/14] copy-on-read: skip non-guest reads if no copy needed

[Andrey Shinkevich via]

If the flag BDRV_REQ_PREFETCH was set, skip idling read/write
operations in COR-driver. It can be taken into account for the
COR-algorithms optimization. That check is being made during the
block stream job by the moment.

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

diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index a2b180a..081e661 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -153,10 +153,14 @@ static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
             }
         }
 
-        ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset,
-                                  local_flags);
-        if (ret < 0) {
-            return ret;
+        /* Skip if neither read nor write are needed */
+        if ((local_flags & (BDRV_REQ_PREFETCH | BDRV_REQ_COPY_ON_READ)) !=
+            BDRV_REQ_PREFETCH) {
+            ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset,
+                                      local_flags);
+            if (ret < 0) {
+                return ret;
+            }
         }
 
         offset += n;
-- 
1.8.3.1

[PATCH v12 13/14] stream: skip filters when writing backing file name to QCOW2 header

[Andrey Shinkevich via]

Avoid writing a filter JSON file name and a filter format name to QCOW2
image when the backing file is changed after the block stream job.
A user is still able to assign the 'backing-file' parameter for a
block-stream job keeping in mind the possible issue mentioned above.
If the user does not specify the 'backing-file' parameter, QEMU will
assign it automatically.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 block/stream.c | 15 +++++++++++++--
 blockdev.c     |  9 ++-------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index e0540ee..1ba74ab 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
     BlockDriverState *bs = blk_bs(bjob->blk);
     BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
     BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
+    BlockDriverState *base_unfiltered = NULL;
     Error *local_err = NULL;
     int ret = 0;
 
@@ -75,8 +76,18 @@ static int stream_prepare(Job *job)
         const char *base_id = NULL, *base_fmt = NULL;
         if (base) {
             base_id = s->backing_file_str;
-            if (base->drv) {
-                base_fmt = base->drv->format_name;
+            if (base_id) {
+                if (base->drv) {
+                    base_fmt = base->drv->format_name;
+                }
+            } else {
+                base_unfiltered = bdrv_skip_filters(base);
+                if (base_unfiltered) {
+                    base_id = base_unfiltered->filename;
+                    if (base_unfiltered->drv) {
+                        base_fmt = base_unfiltered->drv->format_name;
+                    }
+                }
             }
         }
         bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
diff --git a/blockdev.c b/blockdev.c
index c917625..0e9c783 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2508,7 +2508,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
     BlockDriverState *base_bs = NULL;
     AioContext *aio_context;
     Error *local_err = NULL;
-    const char *base_name = NULL;
     int job_flags = JOB_DEFAULT;
 
     if (!has_on_error) {
@@ -2536,7 +2535,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
             goto out;
         }
         assert(bdrv_get_aio_context(base_bs) == aio_context);
-        base_name = base;
     }
 
     if (has_base_node) {
@@ -2551,7 +2549,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         }
         assert(bdrv_get_aio_context(base_bs) == aio_context);
         bdrv_refresh_filename(base_bs);
-        base_name = base_bs->filename;
     }
 
     /* Check for op blockers in the whole chain between bs and base */
@@ -2571,9 +2568,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         goto out;
     }
 
-    /* backing_file string overrides base bs filename */
-    base_name = has_backing_file ? backing_file : base_name;
-
     if (has_auto_finalize && !auto_finalize) {
         job_flags |= JOB_MANUAL_FINALIZE;
     }
@@ -2581,7 +2575,8 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         job_flags |= JOB_MANUAL_DISMISS;
     }
 
-    stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
+    stream_start(has_job_id ? job_id : NULL, bs, base_bs,
+                 has_backing_file ? backing_file : NULL,
                  job_flags, has_speed ? speed : 0, on_error,
                  filter_node_name, &local_err);
     if (local_err) {
-- 
1.8.3.1

Re: [PATCH v12 13/14] stream: skip filters when writing backing file name to QCOW2 header

[Vladimir Sementsov-Ogievskiy]

22.10.2020 21:13, Andrey Shinkevich wrote:
> Avoid writing a filter JSON file name and a filter format name to QCOW2
> image when the backing file is changed after the block stream job.
> A user is still able to assign the 'backing-file' parameter for a
> block-stream job keeping in mind the possible issue mentioned above.
> If the user does not specify the 'backing-file' parameter, QEMU will
> assign it automatically.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block/stream.c | 15 +++++++++++++--
>   blockdev.c     |  9 ++-------
>   2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index e0540ee..1ba74ab 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
>       BlockDriverState *bs = blk_bs(bjob->blk);
>       BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
>       BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
> +    BlockDriverState *base_unfiltered = NULL;
>       Error *local_err = NULL;
>       int ret = 0;
>   
> @@ -75,8 +76,18 @@ static int stream_prepare(Job *job)
>           const char *base_id = NULL, *base_fmt = NULL;
>           if (base) {
>               base_id = s->backing_file_str;
> -            if (base->drv) {
> -                base_fmt = base->drv->format_name;
> +            if (base_id) {
> +                if (base->drv) {
> +                    base_fmt = base->drv->format_name;

hmm. this doesn't make real sense: so, we assume that user specified backing_file_str, which may not relate to base, but we use base->drv->format_name? But it may be name of the filter driver, which would be wrong..

Any ideas?

1. we can use base_fmt=NULL, to provoke probing on next open of the qcow2 file..
2. we can do probing now
3. we can at least check, if backing_file_str == base_unfiltered->filename, in this case we can use base_unfiltered->drv->format_name


> +                }
> +            } else {
> +                base_unfiltered = bdrv_skip_filters(base);
> +                if (base_unfiltered) {
> +                    base_id = base_unfiltered->filename;
> +                    if (base_unfiltered->drv) {
> +                        base_fmt = base_unfiltered->drv->format_name;
> +                    }
> +                }
>               }
>           }
>           bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
> diff --git a/blockdev.c b/blockdev.c
> index c917625..0e9c783 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2508,7 +2508,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>       BlockDriverState *base_bs = NULL;
>       AioContext *aio_context;
>       Error *local_err = NULL;
> -    const char *base_name = NULL;
>       int job_flags = JOB_DEFAULT;
>   
>       if (!has_on_error) {
> @@ -2536,7 +2535,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>               goto out;
>           }
>           assert(bdrv_get_aio_context(base_bs) == aio_context);
> -        base_name = base;
>       }
>   
>       if (has_base_node) {
> @@ -2551,7 +2549,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>           }
>           assert(bdrv_get_aio_context(base_bs) == aio_context);
>           bdrv_refresh_filename(base_bs);
> -        base_name = base_bs->filename;
>       }
>   
>       /* Check for op blockers in the whole chain between bs and base */
> @@ -2571,9 +2568,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>           goto out;
>       }
>   
> -    /* backing_file string overrides base bs filename */
> -    base_name = has_backing_file ? backing_file : base_name;
> -
>       if (has_auto_finalize && !auto_finalize) {
>           job_flags |= JOB_MANUAL_FINALIZE;
>       }
> @@ -2581,7 +2575,8 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>           job_flags |= JOB_MANUAL_DISMISS;
>       }
>   
> -    stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
> +    stream_start(has_job_id ? job_id : NULL, bs, base_bs,
> +                 has_backing_file ? backing_file : NULL,

backing_file should be NULL if has_backing_file is false, so you can use just backing_file instead of ternary operator.

>                    job_flags, has_speed ? speed : 0, on_error,
>                    filter_node_name, &local_err);
>       if (local_err) {
> 


-- 
Best regards,
Vladimir

Re: [PATCH v12 13/14] stream: skip filters when writing backing file name to QCOW2 header

[Andrey Shinkevich]

On 27.10.2020 18:09, Vladimir Sementsov-Ogievskiy wrote:
> 22.10.2020 21:13, Andrey Shinkevich wrote:
>> Avoid writing a filter JSON file name and a filter format name to QCOW2
>> image when the backing file is changed after the block stream job.
>> A user is still able to assign the 'backing-file' parameter for a
>> block-stream job keeping in mind the possible issue mentioned above.
>> If the user does not specify the 'backing-file' parameter, QEMU will
>> assign it automatically.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>   block/stream.c | 15 +++++++++++++--
>>   blockdev.c     |  9 ++-------
>>   2 files changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/stream.c b/block/stream.c
>> index e0540ee..1ba74ab 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
>>       BlockDriverState *bs = blk_bs(bjob->blk);
>>       BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
>>       BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
>> +    BlockDriverState *base_unfiltered = NULL;
>>       Error *local_err = NULL;
>>       int ret = 0;
>> @@ -75,8 +76,18 @@ static int stream_prepare(Job *job)
>>           const char *base_id = NULL, *base_fmt = NULL;
>>           if (base) {
>>               base_id = s->backing_file_str;
>> -            if (base->drv) {
>> -                base_fmt = base->drv->format_name;
>> +            if (base_id) {
>> +                if (base->drv) {
>> +                    base_fmt = base->drv->format_name;
> 
> hmm. this doesn't make real sense: so, we assume that user specified 
> backing_file_str, which may not relate to base, but we use 
> base->drv->format_name? But it may be name of the filter driver, which 
> would be wrong..
> 
> Any ideas?
> 
> 1. we can use base_fmt=NULL, to provoke probing on next open of the 
> qcow2 file..

I would choose this item #1 but have to check the probing code logic... 
Particularly, I do not remember now if the probing is able to recognize 
a protocol.
The logic for the format_name in the QEMU existent code (I has kept it 
here in the patch) is a slippery way for an imprudent user. That's why I 
staked on the backing_file_str deprication in the previous version.

> 2. we can do probing now
> 3. we can at least check, if backing_file_str == 

Not bad for the sanity check but we will search a node by the file name 
again - not good ((

> base_unfiltered->filename, in this case we can use 
> base_unfiltered->drv->format_name
> 
> 
>> +                }
>> +            } else {
>> +                base_unfiltered = bdrv_skip_filters(base);
>> +                if (base_unfiltered) {
>> +                    base_id = base_unfiltered->filename;
>> +                    if (base_unfiltered->drv) {
>> +                        base_fmt = base_unfiltered->drv->format_name;
>> +                    }
>> +                }
>>               }
>>           }
>>           bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
>> diff --git a/blockdev.c b/blockdev.c
>> index c917625..0e9c783 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c

[...]

>> -    stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
>> +    stream_start(has_job_id ? job_id : NULL, bs, base_bs,
>> +                 has_backing_file ? backing_file : NULL,
> 
> backing_file should be NULL if has_backing_file is false, so you can use 
> just backing_file instead of ternary operator.
> 

Yes, if reliable. I has kept the conformation with the ternary operator 
at the first parameter above.

Andrey

>>                    job_flags, has_speed ? speed : 0, on_error,
>>                    filter_node_name, &local_err);
>>       if (local_err) {
>>
> 
> 

Re: [PATCH v12 13/14] stream: skip filters when writing backing file name to QCOW2 header

[Vladimir Sementsov-Ogievskiy]

27.10.2020 19:01, Andrey Shinkevich wrote:
> On 27.10.2020 18:09, Vladimir Sementsov-Ogievskiy wrote:
>> 22.10.2020 21:13, Andrey Shinkevich wrote:
>>> Avoid writing a filter JSON file name and a filter format name to QCOW2
>>> image when the backing file is changed after the block stream job.
>>> A user is still able to assign the 'backing-file' parameter for a
>>> block-stream job keeping in mind the possible issue mentioned above.
>>> If the user does not specify the 'backing-file' parameter, QEMU will
>>> assign it automatically.
>>>
>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>> ---
>>>   block/stream.c | 15 +++++++++++++--
>>>   blockdev.c     |  9 ++-------
>>>   2 files changed, 15 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/block/stream.c b/block/stream.c
>>> index e0540ee..1ba74ab 100644
>>> --- a/block/stream.c
>>> +++ b/block/stream.c
>>> @@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
>>>       BlockDriverState *bs = blk_bs(bjob->blk);
>>>       BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
>>>       BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
>>> +    BlockDriverState *base_unfiltered = NULL;
>>>       Error *local_err = NULL;
>>>       int ret = 0;
>>> @@ -75,8 +76,18 @@ static int stream_prepare(Job *job)
>>>           const char *base_id = NULL, *base_fmt = NULL;
>>>           if (base) {
>>>               base_id = s->backing_file_str;
>>> -            if (base->drv) {
>>> -                base_fmt = base->drv->format_name;
>>> +            if (base_id) {
>>> +                if (base->drv) {
>>> +                    base_fmt = base->drv->format_name;
>>
>> hmm. this doesn't make real sense: so, we assume that user specified backing_file_str, which may not relate to base, but we use base->drv->format_name? But it may be name of the filter driver, which would be wrong..
>>
>> Any ideas?
>>
>> 1. we can use base_fmt=NULL, to provoke probing on next open of the qcow2 file..
> 
> I would choose this item #1 but have to check the probing code logic... Particularly, I do not remember now if the probing is able to recognize a protocol.
> The logic for the format_name in the QEMU existent code (I has kept it here in the patch) is a slippery way for an imprudent user. That's why I staked on the backing_file_str deprication in the previous version.
> 
>> 2. we can do probing now
>> 3. we can at least check, if backing_file_str == 
> 
> Not bad for the sanity check but we will search a node by the file name again - not good ((

Not search, but only check one very likely option.

Additionally to 1. or 3. (or combined), or even keeping things as is (i.e. wrong, but it is preexisting), we can:

  - add backing-format argument to qapi as pair for backing-file
  - deprecate using backing-file without backing-format.

Then, after deprecation period we'll have correct code. This may be done in separate.

> 
>> base_unfiltered->filename, in this case we can use base_unfiltered->drv->format_name
>>
>>
>>> +                }
>>> +            } else {
>>> +                base_unfiltered = bdrv_skip_filters(base);
>>> +                if (base_unfiltered) {
>>> +                    base_id = base_unfiltered->filename;
>>> +                    if (base_unfiltered->drv) {
>>> +                        base_fmt = base_unfiltered->drv->format_name;
>>> +                    }
>>> +                }
>>>               }
>>>           }
>>>           bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
>>> diff --git a/blockdev.c b/blockdev.c
>>> index c917625..0e9c783 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
> 
> [...]
> 
>>> -    stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
>>> +    stream_start(has_job_id ? job_id : NULL, bs, base_bs,
>>> +                 has_backing_file ? backing_file : NULL,
>>
>> backing_file should be NULL if has_backing_file is false, so you can use just backing_file instead of ternary operator.
>>
> 
> Yes, if reliable. I has kept the conformation with the ternary operator at the first parameter above.
> 
> Andrey
> 
>>>                    job_flags, has_speed ? speed : 0, on_error,
>>>                    filter_node_name, &local_err);
>>>       if (local_err) {
>>>
>>
>>


-- 
Best regards,
Vladimir

Re: [PATCH v12 13/14] stream: skip filters when writing backing file name to QCOW2 header

[Andrey Shinkevich]

On 27.10.2020 19:21, Vladimir Sementsov-Ogievskiy wrote:
> 27.10.2020 19:01, Andrey Shinkevich wrote:
>> On 27.10.2020 18:09, Vladimir Sementsov-Ogievskiy wrote:
>>> 22.10.2020 21:13, Andrey Shinkevich wrote:
>>>> Avoid writing a filter JSON file name and a filter format name to QCOW2
>>>> image when the backing file is changed after the block stream job.
>>>> A user is still able to assign the 'backing-file' parameter for a
>>>> block-stream job keeping in mind the possible issue mentioned above.
>>>> If the user does not specify the 'backing-file' parameter, QEMU will
>>>> assign it automatically.
>>>>
>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>> ---
>>>>   block/stream.c | 15 +++++++++++++--
>>>>   blockdev.c     |  9 ++-------
>>>>   2 files changed, 15 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/block/stream.c b/block/stream.c
>>>> index e0540ee..1ba74ab 100644
>>>> --- a/block/stream.c
>>>> +++ b/block/stream.c
>>>> @@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
>>>>       BlockDriverState *bs = blk_bs(bjob->blk);
>>>>       BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
>>>>       BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
>>>> +    BlockDriverState *base_unfiltered = NULL;
>>>>       Error *local_err = NULL;
>>>>       int ret = 0;
>>>> @@ -75,8 +76,18 @@ static int stream_prepare(Job *job)
>>>>           const char *base_id = NULL, *base_fmt = NULL;
>>>>           if (base) {
>>>>               base_id = s->backing_file_str;
>>>> -            if (base->drv) {
>>>> -                base_fmt = base->drv->format_name;
>>>> +            if (base_id) {
>>>> +                if (base->drv) {
>>>> +                    base_fmt = base->drv->format_name;
>>>
>>> hmm. this doesn't make real sense: so, we assume that user specified 
>>> backing_file_str, which may not relate to base, but we use 
>>> base->drv->format_name? But it may be name of the filter driver, 
>>> which would be wrong..
>>>
>>> Any ideas?
>>>
>>> 1. we can use base_fmt=NULL, to provoke probing on next open of the 
>>> qcow2 file..
>>
>> I would choose this item #1 but have to check the probing code 
>> logic... Particularly, I do not remember now if the probing is able to 
>> recognize a protocol.
>> The logic for the format_name in the QEMU existent code (I has kept it 
>> here in the patch) is a slippery way for an imprudent user. That's why 
>> I staked on the backing_file_str deprication in the previous version.
>>
>>> 2. we can do probing now
>>> 3. we can at least check, if backing_file_str == 
>>
>> Not bad for the sanity check but we will search a node by the file 
>> name again - not good ((
> 
> Not search, but only check one very likely option.

Yes, just strcmp(). And why a user may not merely specify a desired 
backing file as the base?

> 
> Additionally to 1. or 3. (or combined), or even keeping things as is 
> (i.e. wrong, but it is preexisting), we can:
> 
>   - add backing-format argument to qapi as pair for backing-file
>   - deprecate using backing-file without backing-format.
> 
> Then, after deprecation period we'll have correct code. This may be done 
> in separate.
> 
>>
>>> base_unfiltered->filename, in this case we can use 
>>> base_unfiltered->drv->format_name
>>>
>>>
>>>> +                }
>>>> +            } else {
>>>> +                base_unfiltered = bdrv_skip_filters(base);
>>>> +                if (base_unfiltered) {
>>>> +                    base_id = base_unfiltered->filename;
>>>> +                    if (base_unfiltered->drv) {
>>>> +                        base_fmt = base_unfiltered->drv->format_name;
>>>> +                    }
>>>> +                }
>>>>               }
>>>>           }
>>>>           bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index c917625..0e9c783 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>
>> [...]
>>
>>>> -    stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
>>>> +    stream_start(has_job_id ? job_id : NULL, bs, base_bs,
>>>> +                 has_backing_file ? backing_file : NULL,
>>>
>>> backing_file should be NULL if has_backing_file is false, so you can 
>>> use just backing_file instead of ternary operator.
>>>
>>
>> Yes, if reliable. I has kept the conformation with the ternary 
>> operator at the first parameter above.
>>
>> Andrey
>>
>>>>                    job_flags, has_speed ? speed : 0, on_error,
>>>>                    filter_node_name, &local_err);
>>>>       if (local_err) {
>>>>
>>>
>>>
> 
> 

Re: [PATCH v12 13/14] stream: skip filters when writing backing file name to QCOW2 header

[Vladimir Sementsov-Ogievskiy]

27.10.2020 19:42, Andrey Shinkevich wrote:
> On 27.10.2020 19:21, Vladimir Sementsov-Ogievskiy wrote:
>> 27.10.2020 19:01, Andrey Shinkevich wrote:
>>> On 27.10.2020 18:09, Vladimir Sementsov-Ogievskiy wrote:
>>>> 22.10.2020 21:13, Andrey Shinkevich wrote:
>>>>> Avoid writing a filter JSON file name and a filter format name to QCOW2
>>>>> image when the backing file is changed after the block stream job.
>>>>> A user is still able to assign the 'backing-file' parameter for a
>>>>> block-stream job keeping in mind the possible issue mentioned above.
>>>>> If the user does not specify the 'backing-file' parameter, QEMU will
>>>>> assign it automatically.
>>>>>
>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>>> ---
>>>>>   block/stream.c | 15 +++++++++++++--
>>>>>   blockdev.c     |  9 ++-------
>>>>>   2 files changed, 15 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/block/stream.c b/block/stream.c
>>>>> index e0540ee..1ba74ab 100644
>>>>> --- a/block/stream.c
>>>>> +++ b/block/stream.c
>>>>> @@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
>>>>>       BlockDriverState *bs = blk_bs(bjob->blk);
>>>>>       BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
>>>>>       BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
>>>>> +    BlockDriverState *base_unfiltered = NULL;
>>>>>       Error *local_err = NULL;
>>>>>       int ret = 0;
>>>>> @@ -75,8 +76,18 @@ static int stream_prepare(Job *job)
>>>>>           const char *base_id = NULL, *base_fmt = NULL;
>>>>>           if (base) {
>>>>>               base_id = s->backing_file_str;
>>>>> -            if (base->drv) {
>>>>> -                base_fmt = base->drv->format_name;
>>>>> +            if (base_id) {
>>>>> +                if (base->drv) {
>>>>> +                    base_fmt = base->drv->format_name;
>>>>
>>>> hmm. this doesn't make real sense: so, we assume that user specified backing_file_str, which may not relate to base, but we use base->drv->format_name? But it may be name of the filter driver, which would be wrong..
>>>>
>>>> Any ideas?
>>>>
>>>> 1. we can use base_fmt=NULL, to provoke probing on next open of the qcow2 file..
>>>
>>> I would choose this item #1 but have to check the probing code logic... Particularly, I do not remember now if the probing is able to recognize a protocol.
>>> The logic for the format_name in the QEMU existent code (I has kept it here in the patch) is a slippery way for an imprudent user. That's why I staked on the backing_file_str deprication in the previous version.
>>>
>>>> 2. we can do probing now
>>>> 3. we can at least check, if backing_file_str == 
>>>
>>> Not bad for the sanity check but we will search a node by the file name again - not good ((
>>
>> Not search, but only check one very likely option.
> 
> Yes, just strcmp(). And why a user may not merely specify a desired backing file as the base?

*shrung*

> 
>>
>> Additionally to 1. or 3. (or combined), or even keeping things as is (i.e. wrong, but it is preexisting), we can:
>>
>>   - add backing-format argument to qapi as pair for backing-file
>>   - deprecate using backing-file without backing-format.
>>
>> Then, after deprecation period we'll have correct code. This may be done in separate.
>>
>>>
>>>> base_unfiltered->filename, in this case we can use base_unfiltered->drv->format_name
>>>>
>>>>
>>>>> +                }
>>>>> +            } else {
>>>>> +                base_unfiltered = bdrv_skip_filters(base);
>>>>> +                if (base_unfiltered) {
>>>>> +                    base_id = base_unfiltered->filename;
>>>>> +                    if (base_unfiltered->drv) {
>>>>> +                        base_fmt = base_unfiltered->drv->format_name;
>>>>> +                    }
>>>>> +                }
>>>>>               }
>>>>>           }
>>>>>           bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>> index c917625..0e9c783 100644
>>>>> --- a/blockdev.c
>>>>> +++ b/blockdev.c
>>>
>>> [...]
>>>
>>>>> -    stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
>>>>> +    stream_start(has_job_id ? job_id : NULL, bs, base_bs,
>>>>> +                 has_backing_file ? backing_file : NULL,
>>>>
>>>> backing_file should be NULL if has_backing_file is false, so you can use just backing_file instead of ternary operator.
>>>>
>>>
>>> Yes, if reliable. I has kept the conformation with the ternary operator at the first parameter above.
>>>
>>> Andrey
>>>
>>>>>                    job_flags, has_speed ? speed : 0, on_error,
>>>>>                    filter_node_name, &local_err);
>>>>>       if (local_err) {
>>>>>
>>>>
>>>>
>>
>>


-- 
Best regards,
Vladimir

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

[Andrey Shinkevich via]

This patch completes the series with the COR-filter insertion for
block-stream operations. Adding the filter makes it possible for copied
regions to be discarded in backing files during the block-stream job,
what will reduce the disk overuse.
The COR-filter insertion incurs changes in the iotests case
245:test_block_stream_4 that reopens the backing chain during a
block-stream job. There are changes in the iotests #030 as well.
The iotests case 030:test_stream_parallel was deleted due to multiple
conflicts between the concurrent job operations over the same backing
chain. The base backing node for one job is the top node for another
job. It may change due to the filter node inserted into the backing
chain while both jobs are running. Another issue is that the parts of
the backing chain are being frozen by the running job and may not be
changed by the concurrent job when needed. The concept of the parallel
jobs with common nodes is considered vital no more.

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

diff --git a/block/stream.c b/block/stream.c
index 1ba74ab..f6ed315 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -17,8 +17,10 @@
 #include "block/blockjob_int.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qdict.h"
 #include "qemu/ratelimit.h"
 #include "sysemu/block-backend.h"
+#include "block/copy-on-read.h"
 
 enum {
     /*
@@ -33,6 +35,8 @@ typedef struct StreamBlockJob {
     BlockJob common;
     BlockDriverState *base_overlay; /* COW overlay (stream from this) */
     BlockDriverState *above_base;   /* Node directly above the base */
+    BlockDriverState *cor_filter_bs;
+    BlockDriverState *target_bs;
     BlockdevOnError on_error;
     char *backing_file_str;
     bool bs_read_only;
@@ -44,8 +48,7 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
 {
     assert(bytes < SIZE_MAX);
 
-    return blk_co_preadv(blk, offset, bytes, NULL,
-                         BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH);
+    return blk_co_preadv(blk, offset, bytes, NULL, BDRV_REQ_PREFETCH);
 }
 
 static void stream_abort(Job *job)
@@ -53,23 +56,20 @@ static void stream_abort(Job *job)
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
 
     if (s->chain_frozen) {
-        BlockJob *bjob = &s->common;
-        bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->above_base);
+        bdrv_unfreeze_backing_chain(s->cor_filter_bs, s->above_base);
     }
 }
 
 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 *unfiltered_bs = bdrv_skip_filters(bs);
+    BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
     BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
     BlockDriverState *base_unfiltered = NULL;
     Error *local_err = NULL;
     int ret = 0;
 
-    bdrv_unfreeze_backing_chain(bs, s->above_base);
+    bdrv_unfreeze_backing_chain(s->cor_filter_bs, s->above_base);
     s->chain_frozen = false;
 
     if (bdrv_cow_child(unfiltered_bs)) {
@@ -105,15 +105,16 @@ static void stream_clean(Job *job)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
     BlockJob *bjob = &s->common;
-    BlockDriverState *bs = blk_bs(bjob->blk);
 
     /* Reopen the image back in read-only mode if necessary */
     if (s->bs_read_only) {
         /* Give up write permissions before making it read-only */
         blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
-        bdrv_reopen_set_read_only(bs, true, NULL);
+        bdrv_reopen_set_read_only(s->target_bs, true, NULL);
     }
 
+    bdrv_cor_filter_drop(s->cor_filter_bs);
+
     g_free(s->backing_file_str);
 }
 
@@ -121,9 +122,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 *unfiltered_bs = bdrv_skip_filters(bs);
-    bool enable_cor = !bdrv_cow_child(s->base_overlay);
+    BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
     int64_t len;
     int64_t offset = 0;
     uint64_t delay_ns = 0;
@@ -135,21 +134,12 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
         return 0;
     }
 
-    len = bdrv_getlength(bs);
+    len = bdrv_getlength(s->target_bs);
     if (len < 0) {
         return len;
     }
     job_progress_set_remaining(&s->common.job, len);
 
-    /* Turn on copy-on-read for the whole block device so that guest read
-     * requests help us make progress.  Only do this when copying the entire
-     * backing chain since the copy-on-read operation does not take base into
-     * account.
-     */
-    if (enable_cor) {
-        bdrv_enable_copy_on_read(bs);
-    }
-
     for ( ; offset < len; offset += n) {
         bool copy;
         int ret;
@@ -208,10 +198,6 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
         }
     }
 
-    if (enable_cor) {
-        bdrv_disable_copy_on_read(bs);
-    }
-
     /* Do not remove the backing file if an error was there but ignored. */
     return error;
 }
@@ -241,7 +227,9 @@ void stream_start(const char *job_id, BlockDriverState *bs,
     bool bs_read_only;
     int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
     BlockDriverState *base_overlay = bdrv_find_overlay(bs, base);
+    BlockDriverState *cor_filter_bs = NULL;
     BlockDriverState *above_base;
+    QDict *opts;
 
     if (!base_overlay) {
         error_setg(errp, "'%s' is not in the backing chain of '%s'",
@@ -275,17 +263,52 @@ void stream_start(const char *job_id, BlockDriverState *bs,
         }
     }
 
-    /* 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,
-                         basic_flags | BLK_PERM_GRAPH_MOD,
-                         basic_flags | BLK_PERM_WRITE,
+    opts = qdict_new();
+
+    qdict_put_str(opts, "driver", "copy-on-read");
+    qdict_put_str(opts, "file", bdrv_get_node_name(bs));
+    if (base) {
+        /* Pass the base_overlay node name as 'bottom' to COR driver */
+        qdict_put_str(opts, "bottom", base_overlay->node_name);
+    }
+    if (filter_node_name) {
+        qdict_put_str(opts, "node-name", filter_node_name);
+    }
+
+    cor_filter_bs = bdrv_insert_node(bs, opts, BDRV_O_RDWR, errp);
+    if (cor_filter_bs == NULL) {
+        goto fail;
+    }
+
+    if (!filter_node_name) {
+        cor_filter_bs->implicit = true;
+    }
+
+    if (bdrv_freeze_backing_chain(cor_filter_bs, bs, errp) < 0) {
+        bdrv_cor_filter_drop(cor_filter_bs);
+        cor_filter_bs = NULL;
+        goto fail;
+    }
+
+    s = block_job_create(job_id, &stream_job_driver, NULL, cor_filter_bs,
+                         BLK_PERM_CONSISTENT_READ,
+                         basic_flags | BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD,
                          speed, creation_flags, NULL, NULL, errp);
     if (!s) {
         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.
+     */
+    if (block_job_add_bdrv(&s->common, "active node", bs,
+                           basic_flags | BLK_PERM_GRAPH_MOD,
+                           basic_flags | BLK_PERM_WRITE, &error_abort)) {
+        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 forbid writes
@@ -306,6 +329,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
     s->base_overlay = base_overlay;
     s->above_base = above_base;
     s->backing_file_str = g_strdup(backing_file_str);
+    s->cor_filter_bs = cor_filter_bs;
+    s->target_bs = bs;
     s->bs_read_only = bs_read_only;
     s->chain_frozen = true;
 
@@ -318,5 +343,10 @@ fail:
     if (bs_read_only) {
         bdrv_reopen_set_read_only(bs, true, NULL);
     }
-    bdrv_unfreeze_backing_chain(bs, above_base);
+    if (cor_filter_bs) {
+        bdrv_unfreeze_backing_chain(cor_filter_bs, above_base);
+        bdrv_cor_filter_drop(cor_filter_bs);
+    } else {
+        bdrv_unfreeze_backing_chain(bs, above_base);
+    }
 }
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index dcb4b5d..0064590 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -227,61 +227,20 @@ class TestParallelOps(iotests.QMPTestCase):
         for img in self.imgs:
             os.remove(img)
 
-    # Test that it's possible to run several block-stream operations
-    # in parallel in the same snapshot chain
-    @unittest.skipIf(os.environ.get('QEMU_CHECK_BLOCK_AUTO'), 'disabled in CI')
-    def test_stream_parallel(self):
-        self.assert_no_active_block_jobs()
-
-        # Check that the maps don't match before the streaming operations
-        for i in range(2, self.num_imgs, 2):
-            self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[i]),
-                                qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[i-1]),
-                                'image file map matches backing file before streaming')
-
-        # Create all streaming jobs
-        pending_jobs = []
-        for i in range(2, self.num_imgs, 2):
-            node_name = 'node%d' % i
-            job_id = 'stream-%s' % node_name
-            pending_jobs.append(job_id)
-            result = self.vm.qmp('block-stream', device=node_name, job_id=job_id, base=self.imgs[i-2], speed=1024)
-            self.assert_qmp(result, 'return', {})
-
-        for job in pending_jobs:
-            result = self.vm.qmp('block-job-set-speed', device=job, speed=0)
-            self.assert_qmp(result, 'return', {})
-
-        # Wait for all jobs to be finished.
-        while len(pending_jobs) > 0:
-            for event in self.vm.get_qmp_events(wait=True):
-                if event['event'] == 'BLOCK_JOB_COMPLETED':
-                    job_id = self.dictpath(event, 'data/device')
-                    self.assertTrue(job_id in pending_jobs)
-                    self.assert_qmp_absent(event, 'data/error')
-                    pending_jobs.remove(job_id)
-
-        self.assert_no_active_block_jobs()
-        self.vm.shutdown()
-
-        # Check that all maps match now
-        for i in range(2, self.num_imgs, 2):
-            self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i]),
-                             qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]),
-                             'image file map does not match backing file after streaming')
-
     # Test that it's not possible to perform two block-stream
     # operations if there are nodes involved in both.
     def test_overlapping_1(self):
         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])
         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-stream', device='node3', job_id='stream-node3', base=self.imgs[2])
         self.assert_qmp(result, 'error/desc',
@@ -294,7 +253,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/030.out b/tests/qemu-iotests/030.out
index 6d9bee1..5eb508d 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-...........................
+..........................
 ----------------------------------------------------------------------
-Ran 27 tests
+Ran 26 tests
 
 OK
diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out
index 08e0aec..028a16f 100644
--- a/tests/qemu-iotests/141.out
+++ b/tests/qemu-iotests/141.out
@@ -99,7 +99,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"}}
 {'execute': 'blockdev-del', 'arguments': {'node-name': 'drv0'}}
-{"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"}}
 {'execute': 'block-job-cancel', 'arguments': {'device': 'job0'}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "job0"}}
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index e60c832..af3273a 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -899,17 +899,25 @@ 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, {},
-            ["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"])
+        # As the COR-filter node is inserted into the backing chain with the
+        # 'block-stream' operation, we move the options to their proper nodes.
+        opts = hd_opts(1)
+        opts['backing'] = hd_opts(2)
+        opts['backing']['backing'] = None
+        self.reopen(opts, {'read-only': True},
+             ["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
+        # We can't detach hd1 from hd0 because there is the COR-filter implicit
+        # node in between.
+        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 v12 14/14] block: apply COR-filter to block-stream jobs

[Vladimir Sementsov-Ogievskiy]

22.10.2020 21:13, Andrey Shinkevich wrote:
> This patch completes the series with the COR-filter insertion for
> block-stream operations. Adding the filter makes it possible for copied
> regions to be discarded in backing files during the block-stream job,
> what will reduce the disk overuse.
> The COR-filter insertion incurs changes in the iotests case
> 245:test_block_stream_4 that reopens the backing chain during a
> block-stream job. There are changes in the iotests #030 as well.
> The iotests case 030:test_stream_parallel was deleted due to multiple
> conflicts between the concurrent job operations over the same backing
> chain. The base backing node for one job is the top node for another
> job. It may change due to the filter node inserted into the backing
> chain while both jobs are running. Another issue is that the parts of
> the backing chain are being frozen by the running job and may not be
> changed by the concurrent job when needed. The concept of the parallel
> jobs with common nodes is considered vital no more.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block/stream.c             | 98 ++++++++++++++++++++++++++++++----------------
>   tests/qemu-iotests/030     | 51 +++---------------------
>   tests/qemu-iotests/030.out |  4 +-
>   tests/qemu-iotests/141.out |  2 +-
>   tests/qemu-iotests/245     | 22 +++++++----
>   5 files changed, 87 insertions(+), 90 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index 1ba74ab..f6ed315 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -17,8 +17,10 @@
>   #include "block/blockjob_int.h"
>   #include "qapi/error.h"
>   #include "qapi/qmp/qerror.h"
> +#include "qapi/qmp/qdict.h"
>   #include "qemu/ratelimit.h"
>   #include "sysemu/block-backend.h"
> +#include "block/copy-on-read.h"
>   
>   enum {
>       /*
> @@ -33,6 +35,8 @@ typedef struct StreamBlockJob {
>       BlockJob common;
>       BlockDriverState *base_overlay; /* COW overlay (stream from this) */
>       BlockDriverState *above_base;   /* Node directly above the base */
> +    BlockDriverState *cor_filter_bs;
> +    BlockDriverState *target_bs;
>       BlockdevOnError on_error;
>       char *backing_file_str;
>       bool bs_read_only;
> @@ -44,8 +48,7 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
>   {
>       assert(bytes < SIZE_MAX);
>   
> -    return blk_co_preadv(blk, offset, bytes, NULL,
> -                         BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH);
> +    return blk_co_preadv(blk, offset, bytes, NULL, BDRV_REQ_PREFETCH);
>   }
>   
>   static void stream_abort(Job *job)
> @@ -53,23 +56,20 @@ static void stream_abort(Job *job)
>       StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>   
>       if (s->chain_frozen) {
> -        BlockJob *bjob = &s->common;
> -        bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->above_base);
> +        bdrv_unfreeze_backing_chain(s->cor_filter_bs, s->above_base);
>       }
>   }
>   
>   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 *unfiltered_bs = bdrv_skip_filters(bs);
> +    BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
>       BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
>       BlockDriverState *base_unfiltered = NULL;
>       Error *local_err = NULL;
>       int ret = 0;
>   
> -    bdrv_unfreeze_backing_chain(bs, s->above_base);
> +    bdrv_unfreeze_backing_chain(s->cor_filter_bs, s->above_base);
>       s->chain_frozen = false;
>   
>       if (bdrv_cow_child(unfiltered_bs)) {
> @@ -105,15 +105,16 @@ static void stream_clean(Job *job)
>   {
>       StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>       BlockJob *bjob = &s->common;
> -    BlockDriverState *bs = blk_bs(bjob->blk);
>   
>       /* Reopen the image back in read-only mode if necessary */
>       if (s->bs_read_only) {
>           /* Give up write permissions before making it read-only */
>           blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
> -        bdrv_reopen_set_read_only(bs, true, NULL);
> +        bdrv_reopen_set_read_only(s->target_bs, true, NULL);
>       }
>   
> +    bdrv_cor_filter_drop(s->cor_filter_bs);
> +
>       g_free(s->backing_file_str);
>   }
>   
> @@ -121,9 +122,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 *unfiltered_bs = bdrv_skip_filters(bs);
> -    bool enable_cor = !bdrv_cow_child(s->base_overlay);
> +    BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
>       int64_t len;
>       int64_t offset = 0;
>       uint64_t delay_ns = 0;
> @@ -135,21 +134,12 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
>           return 0;
>       }
>   
> -    len = bdrv_getlength(bs);
> +    len = bdrv_getlength(s->target_bs);
>       if (len < 0) {
>           return len;
>       }
>       job_progress_set_remaining(&s->common.job, len);
>   
> -    /* Turn on copy-on-read for the whole block device so that guest read
> -     * requests help us make progress.  Only do this when copying the entire
> -     * backing chain since the copy-on-read operation does not take base into
> -     * account.
> -     */
> -    if (enable_cor) {
> -        bdrv_enable_copy_on_read(bs);
> -    }
> -
>       for ( ; offset < len; offset += n) {
>           bool copy;
>           int ret;
> @@ -208,10 +198,6 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
>           }
>       }
>   
> -    if (enable_cor) {
> -        bdrv_disable_copy_on_read(bs);
> -    }
> -
>       /* Do not remove the backing file if an error was there but ignored. */
>       return error;
>   }
> @@ -241,7 +227,9 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>       bool bs_read_only;
>       int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
>       BlockDriverState *base_overlay = bdrv_find_overlay(bs, base);
> +    BlockDriverState *cor_filter_bs = NULL;
>       BlockDriverState *above_base;
> +    QDict *opts;
>   
>       if (!base_overlay) {
>           error_setg(errp, "'%s' is not in the backing chain of '%s'",
> @@ -275,17 +263,52 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>           }
>       }
>   
> -    /* 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,
> -                         basic_flags | BLK_PERM_GRAPH_MOD,
> -                         basic_flags | BLK_PERM_WRITE,
> +    opts = qdict_new();
> +
> +    qdict_put_str(opts, "driver", "copy-on-read");
> +    qdict_put_str(opts, "file", bdrv_get_node_name(bs));
> +    if (base) {
> +        /* Pass the base_overlay node name as 'bottom' to COR driver */
> +        qdict_put_str(opts, "bottom", base_overlay->node_name);
> +    }
> +    if (filter_node_name) {
> +        qdict_put_str(opts, "node-name", filter_node_name);
> +    }
> +
> +    cor_filter_bs = bdrv_insert_node(bs, opts, BDRV_O_RDWR, errp);
> +    if (cor_filter_bs == NULL) {
> +        goto fail;
> +    }
> +
> +    if (!filter_node_name) {
> +        cor_filter_bs->implicit = true;
> +    }
> +
> +    if (bdrv_freeze_backing_chain(cor_filter_bs, bs, errp) < 0) {
> +        bdrv_cor_filter_drop(cor_filter_bs);
> +        cor_filter_bs = NULL;
> +        goto fail;
> +    }
> +
> +    s = block_job_create(job_id, &stream_job_driver, NULL, cor_filter_bs,
> +                         BLK_PERM_CONSISTENT_READ,
> +                         basic_flags | BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD,

I think that BLK_PERM_GRAPH_MOD is something outdated. We have chain-feeze, what BLK_PERM_GRAPH_MOD adds to it? I don't know, and doubt that somebody knows.

>                            speed, creation_flags, NULL, NULL, errp);
>       if (!s) {
>           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.
> +     */
> +    if (block_job_add_bdrv(&s->common, "active node", bs,
> +                           basic_flags | BLK_PERM_GRAPH_MOD,

why not 0, like for other nodes? We don't use this BdrvChild at all, why to requre permissions?

> +                           basic_flags | BLK_PERM_WRITE, &error_abort)) {
> +        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 forbid writes
> @@ -306,6 +329,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>       s->base_overlay = base_overlay;
>       s->above_base = above_base;
>       s->backing_file_str = g_strdup(backing_file_str);
> +    s->cor_filter_bs = cor_filter_bs;
> +    s->target_bs = bs;
>       s->bs_read_only = bs_read_only;
>       s->chain_frozen = true;
>   
> @@ -318,5 +343,10 @@ fail:
>       if (bs_read_only) {
>           bdrv_reopen_set_read_only(bs, true, NULL);
>       }
> -    bdrv_unfreeze_backing_chain(bs, above_base);
> +    if (cor_filter_bs) {
> +        bdrv_unfreeze_backing_chain(cor_filter_bs, above_base);
> +        bdrv_cor_filter_drop(cor_filter_bs);
> +    } else {
> +        bdrv_unfreeze_backing_chain(bs, above_base);
> +    }
>   }
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index dcb4b5d..0064590 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -227,61 +227,20 @@ class TestParallelOps(iotests.QMPTestCase):
>           for img in self.imgs:
>               os.remove(img)
>   
> -    # Test that it's possible to run several block-stream operations
> -    # in parallel in the same snapshot chain
> -    @unittest.skipIf(os.environ.get('QEMU_CHECK_BLOCK_AUTO'), 'disabled in CI')
> -    def test_stream_parallel(self):

Didn't we agree to add "bottom" paramter to qmp? Than this test-case can be rewritten using
node-names and new "bottom" stream argument.

> -        self.assert_no_active_block_jobs()
> -
> -        # Check that the maps don't match before the streaming operations
> -        for i in range(2, self.num_imgs, 2):
> -            self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[i]),
> -                                qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[i-1]),
> -                                'image file map matches backing file before streaming')
> -
> -        # Create all streaming jobs
> -        pending_jobs = []
> -        for i in range(2, self.num_imgs, 2):
> -            node_name = 'node%d' % i
> -            job_id = 'stream-%s' % node_name
> -            pending_jobs.append(job_id)
> -            result = self.vm.qmp('block-stream', device=node_name, job_id=job_id, base=self.imgs[i-2], speed=1024)
> -            self.assert_qmp(result, 'return', {})
> -
> -        for job in pending_jobs:
> -            result = self.vm.qmp('block-job-set-speed', device=job, speed=0)
> -            self.assert_qmp(result, 'return', {})
> -
> -        # Wait for all jobs to be finished.
> -        while len(pending_jobs) > 0:
> -            for event in self.vm.get_qmp_events(wait=True):
> -                if event['event'] == 'BLOCK_JOB_COMPLETED':
> -                    job_id = self.dictpath(event, 'data/device')
> -                    self.assertTrue(job_id in pending_jobs)
> -                    self.assert_qmp_absent(event, 'data/error')
> -                    pending_jobs.remove(job_id)
> -
> -        self.assert_no_active_block_jobs()
> -        self.vm.shutdown()
> -
> -        # Check that all maps match now
> -        for i in range(2, self.num_imgs, 2):
> -            self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i]),
> -                             qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]),
> -                             'image file map does not match backing file after streaming')
> -
>       # Test that it's not possible to perform two block-stream
>       # operations if there are nodes involved in both.
>       def test_overlapping_1(self):
>           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])
>           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-stream', device='node3', job_id='stream-node3', base=self.imgs[2])
>           self.assert_qmp(result, 'error/desc',
> @@ -294,7 +253,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/030.out b/tests/qemu-iotests/030.out
> index 6d9bee1..5eb508d 100644
> --- a/tests/qemu-iotests/030.out
> +++ b/tests/qemu-iotests/030.out
> @@ -1,5 +1,5 @@
> -...........................
> +..........................
>   ----------------------------------------------------------------------
> -Ran 27 tests
> +Ran 26 tests
>   
>   OK
> diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out
> index 08e0aec..028a16f 100644
> --- a/tests/qemu-iotests/141.out
> +++ b/tests/qemu-iotests/141.out
> @@ -99,7 +99,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"}}
>   {'execute': 'blockdev-del', 'arguments': {'node-name': 'drv0'}}
> -{"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"}}
>   {'execute': 'block-job-cancel', 'arguments': {'device': 'job0'}}
>   {"return": {}}
>   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "job0"}}
> diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
> index e60c832..af3273a 100755
> --- a/tests/qemu-iotests/245
> +++ b/tests/qemu-iotests/245
> @@ -899,17 +899,25 @@ 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, {},
> -            ["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"])
> +        # As the COR-filter node is inserted into the backing chain with the
> +        # 'block-stream' operation, we move the options to their proper nodes.
> +        opts = hd_opts(1)
> +        opts['backing'] = hd_opts(2)
> +        opts['backing']['backing'] = None
> +        self.reopen(opts, {'read-only': True},
> +             ["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
> +        # We can't detach hd1 from hd0 because there is the COR-filter implicit
> +        # node in between.
> +        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)
>   
> 


-- 
Best regards,
Vladimir

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

[Andrey Shinkevich]

On 27.10.2020 19:13, Vladimir Sementsov-Ogievskiy wrote:
> 22.10.2020 21:13, Andrey Shinkevich wrote:
>> This patch completes the series with the COR-filter insertion for
>> block-stream operations. Adding the filter makes it possible for copied
>> regions to be discarded in backing files during the block-stream job,
>> what will reduce the disk overuse.
>> The COR-filter insertion incurs changes in the iotests case
>> 245:test_block_stream_4 that reopens the backing chain during a
>> block-stream job. There are changes in the iotests #030 as well.
>> The iotests case 030:test_stream_parallel was deleted due to multiple
>> conflicts between the concurrent job operations over the same backing
>> chain. The base backing node for one job is the top node for another
>> job. It may change due to the filter node inserted into the backing
>> chain while both jobs are running. Another issue is that the parts of
>> the backing chain are being frozen by the running job and may not be
>> changed by the concurrent job when needed. The concept of the parallel
>> jobs with common nodes is considered vital no more.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>   block/stream.c             | 98 
>> ++++++++++++++++++++++++++++++----------------
>>   tests/qemu-iotests/030     | 51 +++---------------------
>>   tests/qemu-iotests/030.out |  4 +-
>>   tests/qemu-iotests/141.out |  2 +-
>>   tests/qemu-iotests/245     | 22 +++++++----
>>   5 files changed, 87 insertions(+), 90 deletions(-)
>>
>> diff --git a/block/stream.c b/block/stream.c


[...]

>> +    s = block_job_create(job_id, &stream_job_driver, NULL, 
>> cor_filter_bs,
>> +                         BLK_PERM_CONSISTENT_READ,
>> +                         basic_flags | BLK_PERM_WRITE | 
>> BLK_PERM_GRAPH_MOD,
> 
> I think that BLK_PERM_GRAPH_MOD is something outdated. We have 
> chain-feeze, what BLK_PERM_GRAPH_MOD adds to it? I don't know, and doubt 
> that somebody knows.
> 

That is true for the commit/mirror jobs also. If we agree to remove the 
flag BLK_PERM_GRAPH_MOD from all these jobs, it will be made in a 
separate series, won't it?

>>                            speed, creation_flags, NULL, NULL, errp);
>>       if (!s) {
>>           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.
>> +     */
>> +    if (block_job_add_bdrv(&s->common, "active node", bs,
>> +                           basic_flags | BLK_PERM_GRAPH_MOD,
> 
> why not 0, like for other nodes? We don't use this BdrvChild at all, why 
> to requre permissions?
> 

Yes, '0' s right.

>> +                           basic_flags | BLK_PERM_WRITE, 
>> &error_abort)) {
>> +        goto fail;
>> +    }
>> +
>>       /* Block all intermediate nodes between bs and base, because 


[...]

>> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
>> index dcb4b5d..0064590 100755
>> --- a/tests/qemu-iotests/030
>> +++ b/tests/qemu-iotests/030
>> @@ -227,61 +227,20 @@ class TestParallelOps(iotests.QMPTestCase):
>>           for img in self.imgs:
>>               os.remove(img)
>> -    # Test that it's possible to run several block-stream operations
>> -    # in parallel in the same snapshot chain
>> -    @unittest.skipIf(os.environ.get('QEMU_CHECK_BLOCK_AUTO'), 
>> 'disabled in CI')
>> -    def test_stream_parallel(self):
> 
> Didn't we agree to add "bottom" paramter to qmp? Than this test-case can 
> be rewritten using
> node-names and new "bottom" stream argument.
> 

I guess it will not help for the whole test. Particularly, there is an 
issue with freezing the child link to COR-filter of the cuncurrent job, 
then it fails to finish first.

Andrey

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

[Vladimir Sementsov-Ogievskiy]

27.10.2020 20:48, Andrey Shinkevich wrote:
> 
> On 27.10.2020 19:13, Vladimir Sementsov-Ogievskiy wrote:
>> 22.10.2020 21:13, Andrey Shinkevich wrote:
>>> This patch completes the series with the COR-filter insertion for
>>> block-stream operations. Adding the filter makes it possible for copied
>>> regions to be discarded in backing files during the block-stream job,
>>> what will reduce the disk overuse.
>>> The COR-filter insertion incurs changes in the iotests case
>>> 245:test_block_stream_4 that reopens the backing chain during a
>>> block-stream job. There are changes in the iotests #030 as well.
>>> The iotests case 030:test_stream_parallel was deleted due to multiple
>>> conflicts between the concurrent job operations over the same backing
>>> chain. The base backing node for one job is the top node for another
>>> job. It may change due to the filter node inserted into the backing
>>> chain while both jobs are running. Another issue is that the parts of
>>> the backing chain are being frozen by the running job and may not be
>>> changed by the concurrent job when needed. The concept of the parallel
>>> jobs with common nodes is considered vital no more.
>>>
>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>> ---
>>>   block/stream.c             | 98 ++++++++++++++++++++++++++++++----------------
>>>   tests/qemu-iotests/030     | 51 +++---------------------
>>>   tests/qemu-iotests/030.out |  4 +-
>>>   tests/qemu-iotests/141.out |  2 +-
>>>   tests/qemu-iotests/245     | 22 +++++++----
>>>   5 files changed, 87 insertions(+), 90 deletions(-)
>>>
>>> diff --git a/block/stream.c b/block/stream.c
> 
> 
> [...]
> 
>>> +    s = block_job_create(job_id, &stream_job_driver, NULL, cor_filter_bs,
>>> +                         BLK_PERM_CONSISTENT_READ,
>>> +                         basic_flags | BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD,
>>
>> I think that BLK_PERM_GRAPH_MOD is something outdated. We have chain-feeze, what BLK_PERM_GRAPH_MOD adds to it? I don't know, and doubt that somebody knows.
>>
> 
> That is true for the commit/mirror jobs also. If we agree to remove the flag BLK_PERM_GRAPH_MOD from all these jobs, it will be made in a separate series, won't it?

Hmm. At least, let's not implement new logic based on BLK_PERM_GRAPH_MOD. In original code it's only block_job_create's perm, not in shared_perm, not somewhere else.. So, if we keep it, let's keep it as is: only in perm in block_job_create, not implementing additional perm/shared_perm logic.

> 
>>>                            speed, creation_flags, NULL, NULL, errp);
>>>       if (!s) {
>>>           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.
>>> +     */
>>> +    if (block_job_add_bdrv(&s->common, "active node", bs,
>>> +                           basic_flags | BLK_PERM_GRAPH_MOD,
>>
>> why not 0, like for other nodes? We don't use this BdrvChild at all, why to requre permissions?
>>
> 
> Yes, '0' s right.
> 
>>> +                           basic_flags | BLK_PERM_WRITE, &error_abort)) {
>>> +        goto fail;
>>> +    }
>>> +
>>>       /* Block all intermediate nodes between bs and base, because 
> 
> 
> [...]
> 
>>> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
>>> index dcb4b5d..0064590 100755
>>> --- a/tests/qemu-iotests/030
>>> +++ b/tests/qemu-iotests/030
>>> @@ -227,61 +227,20 @@ class TestParallelOps(iotests.QMPTestCase):
>>>           for img in self.imgs:
>>>               os.remove(img)
>>> -    # Test that it's possible to run several block-stream operations
>>> -    # in parallel in the same snapshot chain
>>> -    @unittest.skipIf(os.environ.get('QEMU_CHECK_BLOCK_AUTO'), 'disabled in CI')
>>> -    def test_stream_parallel(self):
>>
>> Didn't we agree to add "bottom" paramter to qmp? Than this test-case can be rewritten using
>> node-names and new "bottom" stream argument.
>>
> 
> I guess it will not help for the whole test. Particularly, there is an issue with freezing the child link to COR-filter of the cuncurrent job, then it fails to finish first.

We should not have such frozen link, as our bottom node should be above COR-filter of concurrent job.


-- 
Best regards,
Vladimir

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

[Andrey Shinkevich]

On 27.10.2020 20:57, Vladimir Sementsov-Ogievskiy wrote:
> 27.10.2020 20:48, Andrey Shinkevich wrote:
>>
>> On 27.10.2020 19:13, Vladimir Sementsov-Ogievskiy wrote:
>>> 22.10.2020 21:13, Andrey Shinkevich wrote:
>>>> This patch completes the series with the COR-filter insertion for
>>>> block-stream operations. Adding the filter makes it possible for copied
>>>> regions to be discarded in backing files during the block-stream job,
>>>> what will reduce the disk overuse.
>>>> The COR-filter insertion incurs changes in the iotests case
>>>> 245:test_block_stream_4 that reopens the backing chain during a
>>>> block-stream job. There are changes in the iotests #030 as well.
>>>> The iotests case 030:test_stream_parallel was deleted due to multiple
>>>> conflicts between the concurrent job operations over the same backing
>>>> chain. The base backing node for one job is the top node for another
>>>> job. It may change due to the filter node inserted into the backing
>>>> chain while both jobs are running. Another issue is that the parts of
>>>> the backing chain are being frozen by the running job and may not be
>>>> changed by the concurrent job when needed. The concept of the parallel
>>>> jobs with common nodes is considered vital no more.
>>>>
>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>> ---
>>>>   block/stream.c             | 98 
>>>> ++++++++++++++++++++++++++++++----------------
>>>>   tests/qemu-iotests/030     | 51 +++---------------------
>>>>   tests/qemu-iotests/030.out |  4 +-
>>>>   tests/qemu-iotests/141.out |  2 +-
>>>>   tests/qemu-iotests/245     | 22 +++++++----
>>>>   5 files changed, 87 insertions(+), 90 deletions(-)
>>>>
>>>> diff --git a/block/stream.c b/block/stream.c
>>
>>
>> [...]
>>
>>>> +    s = block_job_create(job_id, &stream_job_driver, NULL, 
>>>> cor_filter_bs,
>>>> +                         BLK_PERM_CONSISTENT_READ,
>>>> +                         basic_flags | BLK_PERM_WRITE | 
>>>> BLK_PERM_GRAPH_MOD,
>>>
>>> I think that BLK_PERM_GRAPH_MOD is something outdated. We have 
>>> chain-feeze, what BLK_PERM_GRAPH_MOD adds to it? I don't know, and 
>>> doubt that somebody knows.
>>>
>>
>> That is true for the commit/mirror jobs also. If we agree to remove 
>> the flag BLK_PERM_GRAPH_MOD from all these jobs, it will be made in a 
>> separate series, won't it?
> 
> Hmm. At least, let's not implement new logic based on 
> BLK_PERM_GRAPH_MOD. In original code it's only block_job_create's perm, 
> not in shared_perm, not somewhere else.. So, if we keep it, let's keep 
> it as is: only in perm in block_job_create, not implementing additional 
> perm/shared_perm logic.
> 

With @perm=0 in the block_job_add_bdrv(&s->common, "active node"...), it 
won't.

>>
>>>>                            speed, creation_flags, NULL, NULL, errp);
>>>>       if (!s) {
>>>>           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.
>>>> +     */
>>>> +    if (block_job_add_bdrv(&s->common, "active node", bs,
>>>> +                           basic_flags | BLK_PERM_GRAPH_MOD,
>>>
>>> why not 0, like for other nodes? We don't use this BdrvChild at all, 
>>> why to requre permissions?
>>>
>>
>> Yes, '0' s right.
>>
>>>> +                           basic_flags | BLK_PERM_WRITE, 
>>>> &error_abort)) {
>>>> +        goto fail;
>>>> +    }
>>>> +
>>>>       /* Block all intermediate nodes between bs and base, because 
>>
>>
>> [...]
>>
>>>> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
>>>> index dcb4b5d..0064590 100755
>>>> --- a/tests/qemu-iotests/030
>>>> +++ b/tests/qemu-iotests/030
>>>> @@ -227,61 +227,20 @@ class TestParallelOps(iotests.QMPTestCase):
>>>>           for img in self.imgs:
>>>>               os.remove(img)
>>>> -    # Test that it's possible to run several block-stream operations
>>>> -    # in parallel in the same snapshot chain
>>>> -    @unittest.skipIf(os.environ.get('QEMU_CHECK_BLOCK_AUTO'), 
>>>> 'disabled in CI')
>>>> -    def test_stream_parallel(self):
>>>
>>> Didn't we agree to add "bottom" paramter to qmp? Than this test-case 
>>> can be rewritten using
>>> node-names and new "bottom" stream argument.
>>>
>>
>> I guess it will not help for the whole test. Particularly, there is an 
>> issue with freezing the child link to COR-filter of the cuncurrent 
>> job, then it fails to finish first.
> 
> We should not have such frozen link, as our bottom node should be above 
> COR-filter of concurrent job.
> 
> 

The bdrv_freeze_backing_chain(bs, above_base, errp) does that job. Max 
insisted on keeping it.

Andrey

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

[Andrey Shinkevich]

On 27.10.2020 21:24, Andrey Shinkevich wrote:
> 
> On 27.10.2020 20:57, Vladimir Sementsov-Ogievskiy wrote:
>> 27.10.2020 20:48, Andrey Shinkevich wrote:
>>>
>>> On 27.10.2020 19:13, Vladimir Sementsov-Ogievskiy wrote:
>>>> 22.10.2020 21:13, Andrey Shinkevich wrote:
>>>>> This patch completes the series with the COR-filter insertion for
>>>>> block-stream operations. Adding the filter makes it possible for 
>>>>> copied
>>>>> regions to be discarded in backing files during the block-stream job,
>>>>> what will reduce the disk overuse.
>>>>> The COR-filter insertion incurs changes in the iotests case
>>>>> 245:test_block_stream_4 that reopens the backing chain during a
>>>>> block-stream job. There are changes in the iotests #030 as well.
>>>>> The iotests case 030:test_stream_parallel was deleted due to multiple
>>>>> conflicts between the concurrent job operations over the same backing
>>>>> chain. The base backing node for one job is the top node for another
>>>>> job. It may change due to the filter node inserted into the backing
>>>>> chain while both jobs are running. Another issue is that the parts of
>>>>> the backing chain are being frozen by the running job and may not be
>>>>> changed by the concurrent job when needed. The concept of the parallel
>>>>> jobs with common nodes is considered vital no more.
>>>>>
>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>>> ---
>>>>>   block/stream.c             | 98 
>>>>> ++++++++++++++++++++++++++++++----------------
>>>>>   tests/qemu-iotests/030     | 51 +++---------------------
>>>>>   tests/qemu-iotests/030.out |  4 +-
>>>>>   tests/qemu-iotests/141.out |  2 +-
>>>>>   tests/qemu-iotests/245     | 22 +++++++----
>>>>>   5 files changed, 87 insertions(+), 90 deletions(-)
>>>>>
>>>>> diff --git a/block/stream.c b/block/stream.c
>>>
>>>
>>> [...]
>>>
>>>>> +    s = block_job_create(job_id, &stream_job_driver, NULL, 
>>>>> cor_filter_bs,
>>>>> +                         BLK_PERM_CONSISTENT_READ,
>>>>> +                         basic_flags | BLK_PERM_WRITE | 
>>>>> BLK_PERM_GRAPH_MOD,
>>>>
>>>> I think that BLK_PERM_GRAPH_MOD is something outdated. We have 
>>>> chain-feeze, what BLK_PERM_GRAPH_MOD adds to it? I don't know, and 
>>>> doubt that somebody knows.
>>>>
>>>
>>> That is true for the commit/mirror jobs also. If we agree to remove 
>>> the flag BLK_PERM_GRAPH_MOD from all these jobs, it will be made in a 
>>> separate series, won't it?
>>
>> Hmm. At least, let's not implement new logic based on 
>> BLK_PERM_GRAPH_MOD. In original code it's only block_job_create's 
>> perm, not in shared_perm, not somewhere else.. So, if we keep it, 
>> let's keep it as is: only in perm in block_job_create, not 
>> implementing additional perm/shared_perm logic.
>>
> 
> With @perm=0 in the block_job_add_bdrv(&s->common, "active node"...), it 
> won't.
> 
>>>
>>>>>                            speed, creation_flags, NULL, NULL, errp);
>>>>>       if (!s) {
>>>>>           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.
>>>>> +     */
>>>>> +    if (block_job_add_bdrv(&s->common, "active node", bs,
>>>>> +                           basic_flags | BLK_PERM_GRAPH_MOD,
>>>>
>>>> why not 0, like for other nodes? We don't use this BdrvChild at all, 
>>>> why to requre permissions?
>>>>
>>>
>>> Yes, '0' s right.
>>>
>>>>> +                           basic_flags | BLK_PERM_WRITE, 
>>>>> &error_abort)) {
>>>>> +        goto fail;
>>>>> +    }
>>>>> +
>>>>>       /* Block all intermediate nodes between bs and base, because 
>>>
>>>
>>> [...]
>>>
>>>>> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
>>>>> index dcb4b5d..0064590 100755
>>>>> --- a/tests/qemu-iotests/030
>>>>> +++ b/tests/qemu-iotests/030
>>>>> @@ -227,61 +227,20 @@ class TestParallelOps(iotests.QMPTestCase):
>>>>>           for img in self.imgs:
>>>>>               os.remove(img)
>>>>> -    # Test that it's possible to run several block-stream operations
>>>>> -    # in parallel in the same snapshot chain
>>>>> -    @unittest.skipIf(os.environ.get('QEMU_CHECK_BLOCK_AUTO'), 
>>>>> 'disabled in CI')
>>>>> -    def test_stream_parallel(self):
>>>>
>>>> Didn't we agree to add "bottom" paramter to qmp? Than this test-case 
>>>> can be rewritten using
>>>> node-names and new "bottom" stream argument.
>>>>

The QMP new "bottom" option is passed to the COR-driver. It is done 
withing the stream-job code. So, it works.

>>>
>>> I guess it will not help for the whole test. Particularly, there is 
>>> an issue with freezing the child link to COR-filter of the cuncurrent 
>>> job, then it fails to finish first.
>>
>> We should not have such frozen link, as our bottom node should be 
>> above COR-filter of concurrent job.
>>
>>
> 
> The bdrv_freeze_backing_chain(bs, above_base, errp) does that job. Max 
> insisted on keeping it.
> 
> Andrey

I have kept the test_stream_parallel() deleted in the coming v13 because 
it was agreed to make the above_base node frozen. With this, the test 
case can not pass. It is also true because the operations over the 
COR-filter node are blocked for the parallel jobs.

Andrey

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

[Vladimir Sementsov-Ogievskiy]

02.12.2020 21:18, Andrey Shinkevich wrote:
> 
> On 27.10.2020 21:24, Andrey Shinkevich wrote:
>>
>> On 27.10.2020 20:57, Vladimir Sementsov-Ogievskiy wrote:
>>> 27.10.2020 20:48, Andrey Shinkevich wrote:
>>>>
>>>> On 27.10.2020 19:13, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 22.10.2020 21:13, Andrey Shinkevich wrote:
>>>>>> This patch completes the series with the COR-filter insertion for
>>>>>> block-stream operations. Adding the filter makes it possible for copied
>>>>>> regions to be discarded in backing files during the block-stream job,
>>>>>> what will reduce the disk overuse.
>>>>>> The COR-filter insertion incurs changes in the iotests case
>>>>>> 245:test_block_stream_4 that reopens the backing chain during a
>>>>>> block-stream job. There are changes in the iotests #030 as well.
>>>>>> The iotests case 030:test_stream_parallel was deleted due to multiple
>>>>>> conflicts between the concurrent job operations over the same backing
>>>>>> chain. The base backing node for one job is the top node for another
>>>>>> job. It may change due to the filter node inserted into the backing
>>>>>> chain while both jobs are running. Another issue is that the parts of
>>>>>> the backing chain are being frozen by the running job and may not be
>>>>>> changed by the concurrent job when needed. The concept of the parallel
>>>>>> jobs with common nodes is considered vital no more.
>>>>>>
>>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>>>> ---
>>>>>>   block/stream.c             | 98 ++++++++++++++++++++++++++++++----------------
>>>>>>   tests/qemu-iotests/030     | 51 +++---------------------
>>>>>>   tests/qemu-iotests/030.out |  4 +-
>>>>>>   tests/qemu-iotests/141.out |  2 +-
>>>>>>   tests/qemu-iotests/245     | 22 +++++++----
>>>>>>   5 files changed, 87 insertions(+), 90 deletions(-)
>>>>>>
>>>>>> diff --git a/block/stream.c b/block/stream.c
>>>>
>>>>
>>>> [...]
>>>>
>>>>>> +    s = block_job_create(job_id, &stream_job_driver, NULL, cor_filter_bs,
>>>>>> +                         BLK_PERM_CONSISTENT_READ,
>>>>>> +                         basic_flags | BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD,
>>>>>
>>>>> I think that BLK_PERM_GRAPH_MOD is something outdated. We have chain-feeze, what BLK_PERM_GRAPH_MOD adds to it? I don't know, and doubt that somebody knows.
>>>>>
>>>>
>>>> That is true for the commit/mirror jobs also. If we agree to remove the flag BLK_PERM_GRAPH_MOD from all these jobs, it will be made in a separate series, won't it?
>>>
>>> Hmm. At least, let's not implement new logic based on BLK_PERM_GRAPH_MOD. In original code it's only block_job_create's perm, not in shared_perm, not somewhere else.. So, if we keep it, let's keep it as is: only in perm in block_job_create, not implementing additional perm/shared_perm logic.
>>>
>>
>> With @perm=0 in the block_job_add_bdrv(&s->common, "active node"...), it won't.
>>
>>>>
>>>>>>                            speed, creation_flags, NULL, NULL, errp);
>>>>>>       if (!s) {
>>>>>>           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.
>>>>>> +     */
>>>>>> +    if (block_job_add_bdrv(&s->common, "active node", bs,
>>>>>> +                           basic_flags | BLK_PERM_GRAPH_MOD,
>>>>>
>>>>> why not 0, like for other nodes? We don't use this BdrvChild at all, why to requre permissions?
>>>>>
>>>>
>>>> Yes, '0' s right.
>>>>
>>>>>> +                           basic_flags | BLK_PERM_WRITE, &error_abort)) {
>>>>>> +        goto fail;
>>>>>> +    }
>>>>>> +
>>>>>>       /* Block all intermediate nodes between bs and base, because 
>>>>
>>>>
>>>> [...]
>>>>
>>>>>> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
>>>>>> index dcb4b5d..0064590 100755
>>>>>> --- a/tests/qemu-iotests/030
>>>>>> +++ b/tests/qemu-iotests/030
>>>>>> @@ -227,61 +227,20 @@ class TestParallelOps(iotests.QMPTestCase):
>>>>>>           for img in self.imgs:
>>>>>>               os.remove(img)
>>>>>> -    # Test that it's possible to run several block-stream operations
>>>>>> -    # in parallel in the same snapshot chain
>>>>>> -    @unittest.skipIf(os.environ.get('QEMU_CHECK_BLOCK_AUTO'), 'disabled in CI')
>>>>>> -    def test_stream_parallel(self):
>>>>>
>>>>> Didn't we agree to add "bottom" paramter to qmp? Than this test-case can be rewritten using
>>>>> node-names and new "bottom" stream argument.
>>>>>
> 
> The QMP new "bottom" option is passed to the COR-driver. It is done withing the stream-job code. So, it works.

Yes. But we also want "bottom" option for stream-job, and deprecate "base" option. Then we can rewrite the test using "bottom" option, all should work

> 
>>>>
>>>> I guess it will not help for the whole test. Particularly, there is an issue with freezing the child link to COR-filter of the cuncurrent job, then it fails to finish first.
>>>
>>> We should not have such frozen link, as our bottom node should be above COR-filter of concurrent job.
>>>
>>>
>>
>> The bdrv_freeze_backing_chain(bs, above_base, errp) does that job. Max insisted on keeping it.
>>
>> Andrey
> 
> I have kept the test_stream_parallel() deleted in the coming v13 because it was agreed to make the above_base node frozen. With this, the test case can not pass. It is also true because the operations over the COR-filter node are blocked for the parallel jobs.
> 
> Andrey


-- 
Best regards,
Vladimir