[PATCH v2 0/7] copy-before-write: on-cbw-error and cbw-timeout

[PATCH v2 0/7] copy-before-write: on-cbw-error and cbw-timeout

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here are two new options for copy-before-write filter:

on-cbw-error allows to alter the behavior on copy-before-write operation
failure: not break guest write but break the snapshot (and therefore
backup process)

cbw-timeout allows to limit cbw operation by some timeout.

So, for example, using cbw-timeout=60 and on-cbw-error=break-snapshot
you can be sure that guest write will not stuck for more than 60
seconds and will never fail due to backup problems.

This series unites and fixes my
"[PATCH 0/3] block: copy-before-write: on-cbw-error behavior" and
"[PATCH 0/4] block: copy-before-write: cbw-timeout"

Supersedes: <20220301205929.2006041-1-vsementsov@openvz.org>
Supersedes: <20220302162442.2052461-1-vsementsov@openvz.org>

Vladimir Sementsov-Ogievskiy (7):
  block/copy-before-write: refactor option parsing
  block/copy-before-write: add on-cbw-error open parameter
  iotests: add copy-before-write: on-cbw-error tests
  util: add qemu-co-timeout
  block/block-copy: block_copy(): add timeout_ns parameter
  block/copy-before-write: implement cbw-timeout option
  iotests: copy-before-write: add cases for cbw-timeout option

 block/block-copy.c                            |  26 ++-
 block/copy-before-write.c                     | 136 +++++++++---
 include/block/block-copy.h                    |   2 +-
 include/qemu/coroutine.h                      |  13 ++
 qapi/block-core.json                          |  30 ++-
 tests/qemu-iotests/tests/copy-before-write    | 206 ++++++++++++++++++
 .../qemu-iotests/tests/copy-before-write.out  |   5 +
 util/meson.build                              |   1 +
 util/qemu-co-timeout.c                        |  89 ++++++++
 9 files changed, 464 insertions(+), 44 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/copy-before-write
 create mode 100644 tests/qemu-iotests/tests/copy-before-write.out
 create mode 100644 util/qemu-co-timeout.c

-- 
2.35.1

[PATCH v2 1/7] block/copy-before-write: refactor option parsing

[Vladimir Sementsov-Ogievskiy]

We are going to add one more option of enum type. Let's refactor option
parsing so that we can simply work with BlockdevOptionsCbw object.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
---
 block/copy-before-write.c | 68 +++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 27 deletions(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index a8a06fdc09..394e73b094 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -24,6 +24,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/qmp/qjson.h"
 
 #include "sysemu/block-backend.h"
 #include "qemu/cutils.h"
@@ -328,46 +329,49 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c,
     }
 }
 
-static bool cbw_parse_bitmap_option(QDict *options, BdrvDirtyBitmap **bitmap,
-                                    Error **errp)
+static BlockdevOptionsCbw *cbw_parse_options(QDict *options, Error **errp)
 {
-    QDict *bitmap_qdict = NULL;
-    BlockDirtyBitmap *bmp_param = NULL;
+    QDict *cbw_qdict = NULL;
+    BlockdevOptionsCbw *opts = NULL;
     Visitor *v = NULL;
-    bool ret = false;
 
-    *bitmap = NULL;
+    cbw_qdict = qdict_clone_shallow(options);
 
-    qdict_extract_subqdict(options, &bitmap_qdict, "bitmap.");
-    if (!qdict_size(bitmap_qdict)) {
-        ret = true;
-        goto out;
-    }
-
-    v = qobject_input_visitor_new_flat_confused(bitmap_qdict, errp);
+    /*
+     * Delete BlockdevOptions base fields, that are not part of
+     * BlockdevOptionsCbw.
+     */
+    qdict_del(cbw_qdict, "driver");
+    qdict_del(cbw_qdict, "node-name");
+    qdict_del(cbw_qdict, "discard");
+    qdict_del(cbw_qdict, "cache");
+    qdict_extract_subqdict(cbw_qdict, NULL, "cache.");
+    qdict_del(cbw_qdict, "read-only");
+    qdict_del(cbw_qdict, "auto-read-only");
+    qdict_del(cbw_qdict, "force-share");
+    qdict_del(cbw_qdict, "detect-zeroes");
+
+    v = qobject_input_visitor_new_flat_confused(cbw_qdict, errp);
     if (!v) {
         goto out;
     }
 
-    visit_type_BlockDirtyBitmap(v, NULL, &bmp_param, errp);
-    if (!bmp_param) {
-        goto out;
-    }
-
-    *bitmap = block_dirty_bitmap_lookup(bmp_param->node, bmp_param->name, NULL,
-                                        errp);
-    if (!*bitmap) {
+    visit_type_BlockdevOptionsCbw(v, NULL, &opts, errp);
+    if (!opts) {
         goto out;
     }
 
-    ret = true;
+    /*
+     * Delete options which we are going to parse through BlockdevOptionsCbw
+     * object for original options.
+     */
+    qdict_extract_subqdict(options, NULL, "bitmap");
 
 out:
-    qapi_free_BlockDirtyBitmap(bmp_param);
     visit_free(v);
-    qobject_unref(bitmap_qdict);
+    qobject_unref(cbw_qdict);
 
-    return ret;
+    return opts;
 }
 
 static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
@@ -376,6 +380,12 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVCopyBeforeWriteState *s = bs->opaque;
     BdrvDirtyBitmap *bitmap = NULL;
     int64_t cluster_size;
+    g_autoptr(BlockdevOptionsCbw) opts = NULL;
+
+    opts = cbw_parse_options(options, errp);
+    if (!opts) {
+        return -EINVAL;
+    }
 
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
                                BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -390,8 +400,12 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    if (!cbw_parse_bitmap_option(options, &bitmap, errp)) {
-        return -EINVAL;
+    if (opts->has_bitmap) {
+        bitmap = block_dirty_bitmap_lookup(opts->bitmap->node,
+                                           opts->bitmap->name, NULL, errp);
+        if (!bitmap) {
+            return -EINVAL;
+        }
     }
 
     bs->total_sectors = bs->file->bs->total_sectors;
-- 
2.35.1

Re: [PATCH v2 1/7] block/copy-before-write: refactor option parsing

[Hanna Reitz]

On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
> We are going to add one more option of enum type. Let's refactor option
> parsing so that we can simply work with BlockdevOptionsCbw object.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
>   block/copy-before-write.c | 68 +++++++++++++++++++++++----------------
>   1 file changed, 41 insertions(+), 27 deletions(-)
>
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index a8a06fdc09..394e73b094 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -24,6 +24,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qapi/qmp/qjson.h"
>   
>   #include "sysemu/block-backend.h"
>   #include "qemu/cutils.h"
> @@ -328,46 +329,49 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c,
>       }
>   }
>   
> -static bool cbw_parse_bitmap_option(QDict *options, BdrvDirtyBitmap **bitmap,
> -                                    Error **errp)
> +static BlockdevOptionsCbw *cbw_parse_options(QDict *options, Error **errp)
>   {
> -    QDict *bitmap_qdict = NULL;
> -    BlockDirtyBitmap *bmp_param = NULL;
> +    QDict *cbw_qdict = NULL;
> +    BlockdevOptionsCbw *opts = NULL;
>       Visitor *v = NULL;
> -    bool ret = false;
>   
> -    *bitmap = NULL;
> +    cbw_qdict = qdict_clone_shallow(options);
>   
> -    qdict_extract_subqdict(options, &bitmap_qdict, "bitmap.");
> -    if (!qdict_size(bitmap_qdict)) {
> -        ret = true;
> -        goto out;
> -    }
> -
> -    v = qobject_input_visitor_new_flat_confused(bitmap_qdict, errp);
> +    /*
> +     * Delete BlockdevOptions base fields, that are not part of
> +     * BlockdevOptionsCbw.
> +     */
> +    qdict_del(cbw_qdict, "driver");
> +    qdict_del(cbw_qdict, "node-name");
> +    qdict_del(cbw_qdict, "discard");
> +    qdict_del(cbw_qdict, "cache");
> +    qdict_extract_subqdict(cbw_qdict, NULL, "cache.");
> +    qdict_del(cbw_qdict, "read-only");
> +    qdict_del(cbw_qdict, "auto-read-only");
> +    qdict_del(cbw_qdict, "force-share");
> +    qdict_del(cbw_qdict, "detect-zeroes");

Works in practice now, but seems a bit fragile.  If new fields are added 
to the base class, this will break.  (And I don’t know whether people 
will think of updating this when new fields are added to the base class.)

Would there be a problem if instead we parsed the full BlockdevOptions 
object here, asserting that .driver is BLOCKDEV_DRIVER_COPY_BEFORE_WRITE?

> +
> +    v = qobject_input_visitor_new_flat_confused(cbw_qdict, errp);
>       if (!v) {
>           goto out;
>       }
>   
> -    visit_type_BlockDirtyBitmap(v, NULL, &bmp_param, errp);
> -    if (!bmp_param) {
> -        goto out;
> -    }
> -
> -    *bitmap = block_dirty_bitmap_lookup(bmp_param->node, bmp_param->name, NULL,
> -                                        errp);
> -    if (!*bitmap) {
> +    visit_type_BlockdevOptionsCbw(v, NULL, &opts, errp);
> +    if (!opts) {
>           goto out;
>       }

Re: [PATCH v2 1/7] block/copy-before-write: refactor option parsing

[Vladimir Sementsov-Ogievskiy]

01.04.2022 13:50, Hanna Reitz wrote:
> On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
>> We are going to add one more option of enum type. Let's refactor option
>> parsing so that we can simply work with BlockdevOptionsCbw object.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>> ---
>>   block/copy-before-write.c | 68 +++++++++++++++++++++++----------------
>>   1 file changed, 41 insertions(+), 27 deletions(-)
>>
>> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
>> index a8a06fdc09..394e73b094 100644
>> --- a/block/copy-before-write.c
>> +++ b/block/copy-before-write.c
>> @@ -24,6 +24,7 @@
>>    */
>>   #include "qemu/osdep.h"
>> +#include "qapi/qmp/qjson.h"
>>   #include "sysemu/block-backend.h"
>>   #include "qemu/cutils.h"
>> @@ -328,46 +329,49 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c,
>>       }
>>   }
>> -static bool cbw_parse_bitmap_option(QDict *options, BdrvDirtyBitmap **bitmap,
>> -                                    Error **errp)
>> +static BlockdevOptionsCbw *cbw_parse_options(QDict *options, Error **errp)
>>   {
>> -    QDict *bitmap_qdict = NULL;
>> -    BlockDirtyBitmap *bmp_param = NULL;
>> +    QDict *cbw_qdict = NULL;
>> +    BlockdevOptionsCbw *opts = NULL;
>>       Visitor *v = NULL;
>> -    bool ret = false;
>> -    *bitmap = NULL;
>> +    cbw_qdict = qdict_clone_shallow(options);
>> -    qdict_extract_subqdict(options, &bitmap_qdict, "bitmap.");
>> -    if (!qdict_size(bitmap_qdict)) {
>> -        ret = true;
>> -        goto out;
>> -    }
>> -
>> -    v = qobject_input_visitor_new_flat_confused(bitmap_qdict, errp);
>> +    /*
>> +     * Delete BlockdevOptions base fields, that are not part of
>> +     * BlockdevOptionsCbw.
>> +     */
>> +    qdict_del(cbw_qdict, "driver");
>> +    qdict_del(cbw_qdict, "node-name");
>> +    qdict_del(cbw_qdict, "discard");
>> +    qdict_del(cbw_qdict, "cache");
>> +    qdict_extract_subqdict(cbw_qdict, NULL, "cache.");
>> +    qdict_del(cbw_qdict, "read-only");
>> +    qdict_del(cbw_qdict, "auto-read-only");
>> +    qdict_del(cbw_qdict, "force-share");
>> +    qdict_del(cbw_qdict, "detect-zeroes");
> 
> Works in practice now, but seems a bit fragile.  If new fields are added to the base class, this will break.  (And I don’t know whether people will think of updating this when new fields are added to the base class.)
> 
> Would there be a problem if instead we parsed the full BlockdevOptions object here, asserting that .driver is BLOCKDEV_DRIVER_COPY_BEFORE_WRITE?

Hmm. Thanks! Yes there is a problem with it,  as some options are already absorbed in bdrv_open_common(). But good news is that the only necessary absorbed option is "driver".

So, instead of asserting .driver, we temporary add "driver"="copy-before-write" into options, and then we can natively parse them as BlockdevOptions.

The following works for me (applied on the top of the series), will merge into v3:



diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 0ea5506f77..3d6c9fc055 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -375,40 +375,25 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c,
      }
  }
  
-static BlockdevOptionsCbw *cbw_parse_options(QDict *options, Error **errp)
+static BlockdevOptions *cbw_parse_options(QDict *options, Error **errp)
  {
-    QDict *cbw_qdict = NULL;
-    BlockdevOptionsCbw *opts = NULL;
+    BlockdevOptions *opts = NULL;
      Visitor *v = NULL;
  
-    cbw_qdict = qdict_clone_shallow(options);
+    qdict_put_str(options, "driver", "copy-before-write");
  
-    /*
-     * Delete BlockdevOptions base fields, that are not part of
-     * BlockdevOptionsCbw.
-     */
-    qdict_del(cbw_qdict, "driver");
-    qdict_del(cbw_qdict, "node-name");
-    qdict_del(cbw_qdict, "discard");
-    qdict_del(cbw_qdict, "cache");
-    qdict_extract_subqdict(cbw_qdict, NULL, "cache.");
-    qdict_del(cbw_qdict, "read-only");
-    qdict_del(cbw_qdict, "auto-read-only");
-    qdict_del(cbw_qdict, "force-share");
-    qdict_del(cbw_qdict, "detect-zeroes");
-
-    v = qobject_input_visitor_new_flat_confused(cbw_qdict, errp);
+    v = qobject_input_visitor_new_flat_confused(options, errp);
      if (!v) {
          goto out;
      }
  
-    visit_type_BlockdevOptionsCbw(v, NULL, &opts, errp);
+    visit_type_BlockdevOptions(v, NULL, &opts, errp);
      if (!opts) {
          goto out;
      }
  
      /*
-     * Delete options which we are going to parse through BlockdevOptionsCbw
+     * Delete options which we are going to parse through BlockdevOptions
       * object for original options.
       */
      qdict_extract_subqdict(options, NULL, "bitmap");
@@ -417,7 +402,7 @@ static BlockdevOptionsCbw *cbw_parse_options(QDict *options, Error **errp)
  
  out:
      visit_free(v);
-    qobject_unref(cbw_qdict);
+    qdict_del(options, "driver");
  
      return opts;
  }
@@ -428,12 +413,14 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
      BDRVCopyBeforeWriteState *s = bs->opaque;
      BdrvDirtyBitmap *bitmap = NULL;
      int64_t cluster_size;
-    g_autoptr(BlockdevOptionsCbw) opts = NULL;
+    g_autoptr(BlockdevOptions) full_opts = NULL;
+    BlockdevOptionsCbw *opts;
  
-    opts = cbw_parse_options(options, errp);
-    if (!opts) {
+    full_opts = cbw_parse_options(options, errp);
+    if (!full_opts) {
          return -EINVAL;
      }
+    opts = &full_opts->u.copy_before_write;
  
      bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
                                 BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,



-- 
Best regards,
Vladimir

[PATCH v2 2/7] block/copy-before-write: add on-cbw-error open parameter

[Vladimir Sementsov-Ogievskiy]

Currently, behavior on copy-before-write operation failure is simple:
report error to the guest.

Let's implement alternative behavior: break the whole copy-before-write
process (and corresponding backup job or NBD client) but keep guest
working. It's needed if we consider guest stability as more important.

The realisation is simple: on copy-before-write failure we immediately
continue guest write operation and set s->snapshot_ret variable which
will lead to all further and in-flight snapshot-API requests failure.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
---
 block/copy-before-write.c | 62 ++++++++++++++++++++++++++++++++++-----
 qapi/block-core.json      | 27 ++++++++++++++++-
 2 files changed, 81 insertions(+), 8 deletions(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 394e73b094..0614c3d08b 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -41,6 +41,7 @@
 typedef struct BDRVCopyBeforeWriteState {
     BlockCopyState *bcs;
     BdrvChild *target;
+    OnCbwError on_cbw_error;
 
     /*
      * @lock: protects access to @access_bitmap, @done_bitmap and
@@ -65,6 +66,14 @@ typedef struct BDRVCopyBeforeWriteState {
      * node. These areas must not be rewritten by guest.
      */
     BlockReqList frozen_read_reqs;
+
+    /*
+     * @snapshot_error is normally zero. But on first copy-before-write failure
+     * when @on_cbw_error == ON_CBW_ERROR_BREAK_SNAPSHOT, @snapshot_error takes
+     * value of this error (<0). After that all in-flight and further
+     * snaoshot-API requests will fail with that error.
+     */
+    int snapshot_error;
 } BDRVCopyBeforeWriteState;
 
 static coroutine_fn int cbw_co_preadv(
@@ -99,11 +108,25 @@ static coroutine_fn int cbw_do_copy_before_write(BlockDriverState *bs,
     end = QEMU_ALIGN_UP(offset + bytes, cluster_size);
 
     ret = block_copy(s->bcs, off, end - off, true);
-    if (ret < 0) {
+    if (ret < 0 && s->on_cbw_error == ON_CBW_ERROR_BREAK_GUEST_WRITE) {
         return ret;
     }
 
     WITH_QEMU_LOCK_GUARD(&s->lock) {
+        if (ret < 0) {
+            assert(s->on_cbw_error == ON_CBW_ERROR_BREAK_SNAPSHOT);
+            if (!s->snapshot_error) {
+                s->snapshot_error = ret;
+            }
+            /*
+             * No need to wait for s->frozen_read_reqs: they will fail anyway,
+             * as s->snapshot_error is set.
+             *
+             * We return 0, as error is handled. Guest operation should be
+             * continued.
+             */
+            return 0;
+        }
         bdrv_set_dirty_bitmap(s->done_bitmap, off, end - off);
         reqlist_wait_all(&s->frozen_read_reqs, off, end - off, &s->lock);
     }
@@ -176,6 +199,11 @@ static BlockReq *cbw_snapshot_read_lock(BlockDriverState *bs,
 
     QEMU_LOCK_GUARD(&s->lock);
 
+    if (s->snapshot_error) {
+        g_free(req);
+        return NULL;
+    }
+
     if (bdrv_dirty_bitmap_next_zero(s->access_bitmap, offset, bytes) != -1) {
         g_free(req);
         return NULL;
@@ -198,19 +226,26 @@ static BlockReq *cbw_snapshot_read_lock(BlockDriverState *bs,
     return req;
 }
 
-static void cbw_snapshot_read_unlock(BlockDriverState *bs, BlockReq *req)
+static int cbw_snapshot_read_unlock(BlockDriverState *bs, BlockReq *req)
 {
     BDRVCopyBeforeWriteState *s = bs->opaque;
 
     if (req->offset == -1 && req->bytes == -1) {
         g_free(req);
-        return;
+        /*
+         * No real need to read snapshot_error under mutex here: we are actually
+         * safe to ignore it and return 0, as this request was to s->target, and
+         * can't be influenced by guest write. But if we can new read negative
+         * s->snapshot_error let's return it, so that backup failed earlier.
+         */
+        return s->snapshot_error;
     }
 
     QEMU_LOCK_GUARD(&s->lock);
 
     reqlist_remove_req(req);
     g_free(req);
+    return s->snapshot_error;
 }
 
 static coroutine_fn int
@@ -219,7 +254,7 @@ cbw_co_preadv_snapshot(BlockDriverState *bs, int64_t offset, int64_t bytes,
 {
     BlockReq *req;
     BdrvChild *file;
-    int ret;
+    int ret, ret2;
 
     /* TODO: upgrade to async loop using AioTask */
     while (bytes) {
@@ -232,10 +267,13 @@ cbw_co_preadv_snapshot(BlockDriverState *bs, int64_t offset, int64_t bytes,
 
         ret = bdrv_co_preadv_part(file, offset, cur_bytes,
                                   qiov, qiov_offset, 0);
-        cbw_snapshot_read_unlock(bs, req);
+        ret2 = cbw_snapshot_read_unlock(bs, req);
         if (ret < 0) {
             return ret;
         }
+        if (ret2 < 0) {
+            return ret2;
+        }
 
         bytes -= cur_bytes;
         offset += cur_bytes;
@@ -253,7 +291,7 @@ cbw_co_snapshot_block_status(BlockDriverState *bs,
 {
     BDRVCopyBeforeWriteState *s = bs->opaque;
     BlockReq *req;
-    int ret;
+    int ret, ret2;
     int64_t cur_bytes;
     BdrvChild *child;
 
@@ -273,7 +311,14 @@ cbw_co_snapshot_block_status(BlockDriverState *bs,
         assert(ret & BDRV_BLOCK_ALLOCATED);
     }
 
-    cbw_snapshot_read_unlock(bs, req);
+    ret2 = cbw_snapshot_read_unlock(bs, req);
+
+    if (ret < 0) {
+        return ret;
+    }
+    if (ret2 < 0) {
+        return ret2;
+    }
 
     return ret;
 }
@@ -366,6 +411,7 @@ static BlockdevOptionsCbw *cbw_parse_options(QDict *options, Error **errp)
      * object for original options.
      */
     qdict_extract_subqdict(options, NULL, "bitmap");
+    qdict_del(options, "on-cbw-error");
 
 out:
     visit_free(v);
@@ -407,6 +453,8 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
             return -EINVAL;
         }
     }
+    s->on_cbw_error = opts->has_on_cbw_error ? opts->on_cbw_error :
+            ON_CBW_ERROR_BREAK_GUEST_WRITE;
 
     bs->total_sectors = bs->file->bs->total_sectors;
     bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e89f2dfb5b..3f08025114 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4162,6 +4162,27 @@
   'base': 'BlockdevOptionsGenericFormat',
   'data': { '*bottom': 'str' } }
 
+##
+# @OnCbwError:
+#
+# An enumeration of possible behaviors for copy-before-write operation
+# failures.
+#
+# @break-guest-write: report the error to the guest. This way the state
+#                     of copy-before-write process is kept OK and
+#                     copy-before-write filter continues to work normally.
+#
+# @break-snapshot: continue guest write. Since this, the snapshot state
+#                  provided by copy-before-write filter becomes broken.
+#                  So, all in-flight and all further snapshot-access
+#                  operations (through snapshot-access block driver)
+#                  will fail.
+#
+# Since: 7.0
+##
+{ 'enum': 'OnCbwError',
+  'data': [ 'break-guest-write', 'break-snapshot' ] }
+
 ##
 # @BlockdevOptionsCbw:
 #
@@ -4183,11 +4204,15 @@
 #          modifications (or removing) of specified bitmap doesn't
 #          influence the filter. (Since 7.0)
 #
+# @on-cbw-error: Behavior on failure of copy-before-write operation.
+#                Default is @break-guest-write. (Since 7.0)
+#
 # Since: 6.2
 ##
 { 'struct': 'BlockdevOptionsCbw',
   'base': 'BlockdevOptionsGenericFormat',
-  'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap' } }
+  'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
+            '*on-cbw-error': 'OnCbwError' } }
 
 ##
 # @BlockdevOptions:
-- 
2.35.1

Re: [PATCH v2 2/7] block/copy-before-write: add on-cbw-error open parameter

[Hanna Reitz]

On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
> Currently, behavior on copy-before-write operation failure is simple:
> report error to the guest.
>
> Let's implement alternative behavior: break the whole copy-before-write
> process (and corresponding backup job or NBD client) but keep guest
> working. It's needed if we consider guest stability as more important.
>
> The realisation is simple: on copy-before-write failure we immediately
> continue guest write operation and set s->snapshot_ret variable which
> will lead to all further and in-flight snapshot-API requests failure.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
>   block/copy-before-write.c | 62 ++++++++++++++++++++++++++++++++++-----
>   qapi/block-core.json      | 27 ++++++++++++++++-
>   2 files changed, 81 insertions(+), 8 deletions(-)
>
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index 394e73b094..0614c3d08b 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -41,6 +41,7 @@
>   typedef struct BDRVCopyBeforeWriteState {
>       BlockCopyState *bcs;
>       BdrvChild *target;
> +    OnCbwError on_cbw_error;
>   
>       /*
>        * @lock: protects access to @access_bitmap, @done_bitmap and
> @@ -65,6 +66,14 @@ typedef struct BDRVCopyBeforeWriteState {
>        * node. These areas must not be rewritten by guest.
>        */
>       BlockReqList frozen_read_reqs;
> +
> +    /*
> +     * @snapshot_error is normally zero. But on first copy-before-write failure
> +     * when @on_cbw_error == ON_CBW_ERROR_BREAK_SNAPSHOT, @snapshot_error takes
> +     * value of this error (<0). After that all in-flight and further
> +     * snaoshot-API requests will fail with that error.

*snapshot

> +     */
> +    int snapshot_error;
>   } BDRVCopyBeforeWriteState;
>   
>   static coroutine_fn int cbw_co_preadv(
> @@ -99,11 +108,25 @@ static coroutine_fn int cbw_do_copy_before_write(BlockDriverState *bs,
>       end = QEMU_ALIGN_UP(offset + bytes, cluster_size);

Wouldn’t it make sense to completely cease CBW if snapshot_error is 
non-zero?  (I.e. always returning 0 here, skipping block_copy().) You 
can’t read from it anyway anymore.  (Except from below the 
copy-before-write node, but users shouldn’t be doing this, because they 
can’t know which areas are valid to read and which aren’t.)

>       ret = block_copy(s->bcs, off, end - off, true);
> -    if (ret < 0) {
> +    if (ret < 0 && s->on_cbw_error == ON_CBW_ERROR_BREAK_GUEST_WRITE) {
>           return ret;
>       }
>   
>       WITH_QEMU_LOCK_GUARD(&s->lock) {
> +        if (ret < 0) {
> +            assert(s->on_cbw_error == ON_CBW_ERROR_BREAK_SNAPSHOT);
> +            if (!s->snapshot_error) {
> +                s->snapshot_error = ret;
> +            }
> +            /*
> +             * No need to wait for s->frozen_read_reqs: they will fail anyway,
> +             * as s->snapshot_error is set.
> +             *
> +             * We return 0, as error is handled. Guest operation should be
> +             * continued.
> +             */
> +            return 0;

Hm, OK.  Naively, it looks to me like we could save us this explanation 
and simplify the code just by unconditionally waiting here (I guess we 
could skip the wait if snapshot_error was non-zero before) and not 
checking snapshot_error in cbw_snapshot_read_unlock().  I don’t think 
not waiting here meaningfully saves time.

> +        }
>           bdrv_set_dirty_bitmap(s->done_bitmap, off, end - off);
>           reqlist_wait_all(&s->frozen_read_reqs, off, end - off, &s->lock);
>       }
> @@ -176,6 +199,11 @@ static BlockReq *cbw_snapshot_read_lock(BlockDriverState *bs,
>   
>       QEMU_LOCK_GUARD(&s->lock);
>   
> +    if (s->snapshot_error) {
> +        g_free(req);
> +        return NULL;
> +    }
> +
>       if (bdrv_dirty_bitmap_next_zero(s->access_bitmap, offset, bytes) != -1) {
>           g_free(req);
>           return NULL;
> @@ -198,19 +226,26 @@ static BlockReq *cbw_snapshot_read_lock(BlockDriverState *bs,
>       return req;
>   }
>   
> -static void cbw_snapshot_read_unlock(BlockDriverState *bs, BlockReq *req)
> +static int cbw_snapshot_read_unlock(BlockDriverState *bs, BlockReq *req)
>   {
>       BDRVCopyBeforeWriteState *s = bs->opaque;
>   
>       if (req->offset == -1 && req->bytes == -1) {
>           g_free(req);
> -        return;
> +        /*
> +         * No real need to read snapshot_error under mutex here: we are actually
> +         * safe to ignore it and return 0, as this request was to s->target, and
> +         * can't be influenced by guest write. But if we can new read negative
> +         * s->snapshot_error let's return it, so that backup failed earlier.
> +         */
> +        return s->snapshot_error;
>       }
>   
>       QEMU_LOCK_GUARD(&s->lock);
>   
>       reqlist_remove_req(req);
>       g_free(req);
> +    return s->snapshot_error;
>   }
>   
>   static coroutine_fn int
> @@ -219,7 +254,7 @@ cbw_co_preadv_snapshot(BlockDriverState *bs, int64_t offset, int64_t bytes,
>   {
>       BlockReq *req;
>       BdrvChild *file;
> -    int ret;
> +    int ret, ret2;
>   
>       /* TODO: upgrade to async loop using AioTask */
>       while (bytes) {
> @@ -232,10 +267,13 @@ cbw_co_preadv_snapshot(BlockDriverState *bs, int64_t offset, int64_t bytes,
>   
>           ret = bdrv_co_preadv_part(file, offset, cur_bytes,
>                                     qiov, qiov_offset, 0);
> -        cbw_snapshot_read_unlock(bs, req);
> +        ret2 = cbw_snapshot_read_unlock(bs, req);
>           if (ret < 0) {
>               return ret;
>           }
> +        if (ret2 < 0) {
> +            return ret2;
> +        }
>   
>           bytes -= cur_bytes;
>           offset += cur_bytes;
> @@ -253,7 +291,7 @@ cbw_co_snapshot_block_status(BlockDriverState *bs,
>   {
>       BDRVCopyBeforeWriteState *s = bs->opaque;
>       BlockReq *req;
> -    int ret;
> +    int ret, ret2;
>       int64_t cur_bytes;
>       BdrvChild *child;
>   
> @@ -273,7 +311,14 @@ cbw_co_snapshot_block_status(BlockDriverState *bs,
>           assert(ret & BDRV_BLOCK_ALLOCATED);
>       }
>   
> -    cbw_snapshot_read_unlock(bs, req);
> +    ret2 = cbw_snapshot_read_unlock(bs, req);
> +
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    if (ret2 < 0) {
> +        return ret2;
> +    }
>   
>       return ret;
>   }
> @@ -366,6 +411,7 @@ static BlockdevOptionsCbw *cbw_parse_options(QDict *options, Error **errp)
>        * object for original options.
>        */
>       qdict_extract_subqdict(options, NULL, "bitmap");
> +    qdict_del(options, "on-cbw-error");
>   
>   out:
>       visit_free(v);
> @@ -407,6 +453,8 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
>               return -EINVAL;
>           }
>       }
> +    s->on_cbw_error = opts->has_on_cbw_error ? opts->on_cbw_error :
> +            ON_CBW_ERROR_BREAK_GUEST_WRITE;
>   
>       bs->total_sectors = bs->file->bs->total_sectors;
>       bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e89f2dfb5b..3f08025114 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4162,6 +4162,27 @@
>     'base': 'BlockdevOptionsGenericFormat',
>     'data': { '*bottom': 'str' } }
>   
> +##
> +# @OnCbwError:
> +#
> +# An enumeration of possible behaviors for copy-before-write operation
> +# failures.
> +#
> +# @break-guest-write: report the error to the guest. This way the state
> +#                     of copy-before-write process is kept OK and

I’d be more verbose here: “This way, the guest will not be able to 
overwrite areas that cannot be backed up, so the backup remains valid.”

I like the bluntness of how these two options are named, by the way.  
That does clearly tell users what they’ll have to expect.

> +#                     copy-before-write filter continues to work normally.
> +#
> +# @break-snapshot: continue guest write. Since this, the snapshot state
> +#                  provided by copy-before-write filter becomes broken.

Maybe “Doing so will invalidate the backup snapshot”?

> +#                  So, all in-flight and all further snapshot-access
> +#                  operations (through snapshot-access block driver)
> +#                  will fail.
> +#
> +# Since: 7.0
> +##
> +{ 'enum': 'OnCbwError',
> +  'data': [ 'break-guest-write', 'break-snapshot' ] }
> +
>   ##
>   # @BlockdevOptionsCbw:
>   #
> @@ -4183,11 +4204,15 @@
>   #          modifications (or removing) of specified bitmap doesn't
>   #          influence the filter. (Since 7.0)
>   #
> +# @on-cbw-error: Behavior on failure of copy-before-write operation.
> +#                Default is @break-guest-write. (Since 7.0)

*7.1

> +#
>   # Since: 6.2
>   ##
>   { 'struct': 'BlockdevOptionsCbw',
>     'base': 'BlockdevOptionsGenericFormat',
> -  'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap' } }
> +  'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
> +            '*on-cbw-error': 'OnCbwError' } }
>   
>   ##
>   # @BlockdevOptions:

Re: [PATCH v2 2/7] block/copy-before-write: add on-cbw-error open parameter

[Vladimir Sementsov-Ogievskiy]

01.04.2022 14:58, Hanna Reitz wrote:
> On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
>> Currently, behavior on copy-before-write operation failure is simple:
>> report error to the guest.
>>
>> Let's implement alternative behavior: break the whole copy-before-write
>> process (and corresponding backup job or NBD client) but keep guest
>> working. It's needed if we consider guest stability as more important.
>>
>> The realisation is simple: on copy-before-write failure we immediately
>> continue guest write operation and set s->snapshot_ret variable which
>> will lead to all further and in-flight snapshot-API requests failure.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>> ---
>>   block/copy-before-write.c | 62 ++++++++++++++++++++++++++++++++++-----
>>   qapi/block-core.json      | 27 ++++++++++++++++-
>>   2 files changed, 81 insertions(+), 8 deletions(-)
>>
>> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
>> index 394e73b094..0614c3d08b 100644
>> --- a/block/copy-before-write.c
>> +++ b/block/copy-before-write.c
>> @@ -41,6 +41,7 @@
>>   typedef struct BDRVCopyBeforeWriteState {
>>       BlockCopyState *bcs;
>>       BdrvChild *target;
>> +    OnCbwError on_cbw_error;
>>       /*
>>        * @lock: protects access to @access_bitmap, @done_bitmap and
>> @@ -65,6 +66,14 @@ typedef struct BDRVCopyBeforeWriteState {
>>        * node. These areas must not be rewritten by guest.
>>        */
>>       BlockReqList frozen_read_reqs;
>> +
>> +    /*
>> +     * @snapshot_error is normally zero. But on first copy-before-write failure
>> +     * when @on_cbw_error == ON_CBW_ERROR_BREAK_SNAPSHOT, @snapshot_error takes
>> +     * value of this error (<0). After that all in-flight and further
>> +     * snaoshot-API requests will fail with that error.
> 
> *snapshot
> 
>> +     */
>> +    int snapshot_error;
>>   } BDRVCopyBeforeWriteState;
>>   static coroutine_fn int cbw_co_preadv(
>> @@ -99,11 +108,25 @@ static coroutine_fn int cbw_do_copy_before_write(BlockDriverState *bs,
>>       end = QEMU_ALIGN_UP(offset + bytes, cluster_size);
> 
> Wouldn’t it make sense to completely cease CBW if snapshot_error is non-zero?  (I.e. always returning 0 here, skipping block_copy().) You can’t read from it anyway anymore.  (Except from below the copy-before-write node, but users shouldn’t be doing this, because they can’t know which areas are valid to read and which aren’t.)

Agree, will do.

> 
>>       ret = block_copy(s->bcs, off, end - off, true);
>> -    if (ret < 0) {
>> +    if (ret < 0 && s->on_cbw_error == ON_CBW_ERROR_BREAK_GUEST_WRITE) {
>>           return ret;
>>       }
>>       WITH_QEMU_LOCK_GUARD(&s->lock) {
>> +        if (ret < 0) {
>> +            assert(s->on_cbw_error == ON_CBW_ERROR_BREAK_SNAPSHOT);
>> +            if (!s->snapshot_error) {
>> +                s->snapshot_error = ret;
>> +            }
>> +            /*
>> +             * No need to wait for s->frozen_read_reqs: they will fail anyway,
>> +             * as s->snapshot_error is set.
>> +             *
>> +             * We return 0, as error is handled. Guest operation should be
>> +             * continued.
>> +             */
>> +            return 0;
> 
> Hm, OK.  Naively, it looks to me like we could save us this explanation and simplify the code just by unconditionally waiting here (I guess we could skip the wait if snapshot_error was non-zero before) and not checking snapshot_error in cbw_snapshot_read_unlock().  I don’t think not waiting here meaningfully saves time.

Hmm. I tend to agree, this optimization doesn't seem to worth the complexity. Will drop it, we can implement it later if really needed.

> 
>> +        }
>>           bdrv_set_dirty_bitmap(s->done_bitmap, off, end - off);
>>           reqlist_wait_all(&s->frozen_read_reqs, off, end - off, &s->lock);
>>       }
>> @@ -176,6 +199,11 @@ static BlockReq *cbw_snapshot_read_lock(BlockDriverState *bs,
>>       QEMU_LOCK_GUARD(&s->lock);
>> +    if (s->snapshot_error) {
>> +        g_free(req);
>> +        return NULL;
>> +    }
>> +
>>       if (bdrv_dirty_bitmap_next_zero(s->access_bitmap, offset, bytes) != -1) {
>>           g_free(req);
>>           return NULL;
>> @@ -198,19 +226,26 @@ static BlockReq *cbw_snapshot_read_lock(BlockDriverState *bs,
>>       return req;
>>   }
>> -static void cbw_snapshot_read_unlock(BlockDriverState *bs, BlockReq *req)
>> +static int cbw_snapshot_read_unlock(BlockDriverState *bs, BlockReq *req)
>>   {
>>       BDRVCopyBeforeWriteState *s = bs->opaque;
>>       if (req->offset == -1 && req->bytes == -1) {
>>           g_free(req);
>> -        return;
>> +        /*
>> +         * No real need to read snapshot_error under mutex here: we are actually
>> +         * safe to ignore it and return 0, as this request was to s->target, and
>> +         * can't be influenced by guest write. But if we can new read negative
>> +         * s->snapshot_error let's return it, so that backup failed earlier.
>> +         */
>> +        return s->snapshot_error;
>>       }
>>       QEMU_LOCK_GUARD(&s->lock);
>>       reqlist_remove_req(req);
>>       g_free(req);
>> +    return s->snapshot_error;
>>   }
>>   static coroutine_fn int
>> @@ -219,7 +254,7 @@ cbw_co_preadv_snapshot(BlockDriverState *bs, int64_t offset, int64_t bytes,
>>   {
>>       BlockReq *req;
>>       BdrvChild *file;
>> -    int ret;
>> +    int ret, ret2;
>>       /* TODO: upgrade to async loop using AioTask */
>>       while (bytes) {
>> @@ -232,10 +267,13 @@ cbw_co_preadv_snapshot(BlockDriverState *bs, int64_t offset, int64_t bytes,
>>           ret = bdrv_co_preadv_part(file, offset, cur_bytes,
>>                                     qiov, qiov_offset, 0);
>> -        cbw_snapshot_read_unlock(bs, req);
>> +        ret2 = cbw_snapshot_read_unlock(bs, req);
>>           if (ret < 0) {
>>               return ret;
>>           }
>> +        if (ret2 < 0) {
>> +            return ret2;
>> +        }
>>           bytes -= cur_bytes;
>>           offset += cur_bytes;
>> @@ -253,7 +291,7 @@ cbw_co_snapshot_block_status(BlockDriverState *bs,
>>   {
>>       BDRVCopyBeforeWriteState *s = bs->opaque;
>>       BlockReq *req;
>> -    int ret;
>> +    int ret, ret2;
>>       int64_t cur_bytes;
>>       BdrvChild *child;
>> @@ -273,7 +311,14 @@ cbw_co_snapshot_block_status(BlockDriverState *bs,
>>           assert(ret & BDRV_BLOCK_ALLOCATED);
>>       }
>> -    cbw_snapshot_read_unlock(bs, req);
>> +    ret2 = cbw_snapshot_read_unlock(bs, req);
>> +
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +    if (ret2 < 0) {
>> +        return ret2;
>> +    }
>>       return ret;
>>   }
>> @@ -366,6 +411,7 @@ static BlockdevOptionsCbw *cbw_parse_options(QDict *options, Error **errp)
>>        * object for original options.
>>        */
>>       qdict_extract_subqdict(options, NULL, "bitmap");
>> +    qdict_del(options, "on-cbw-error");
>>   out:
>>       visit_free(v);
>> @@ -407,6 +453,8 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
>>               return -EINVAL;
>>           }
>>       }
>> +    s->on_cbw_error = opts->has_on_cbw_error ? opts->on_cbw_error :
>> +            ON_CBW_ERROR_BREAK_GUEST_WRITE;
>>       bs->total_sectors = bs->file->bs->total_sectors;
>>       bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index e89f2dfb5b..3f08025114 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -4162,6 +4162,27 @@
>>     'base': 'BlockdevOptionsGenericFormat',
>>     'data': { '*bottom': 'str' } }
>> +##
>> +# @OnCbwError:
>> +#
>> +# An enumeration of possible behaviors for copy-before-write operation
>> +# failures.
>> +#
>> +# @break-guest-write: report the error to the guest. This way the state
>> +#                     of copy-before-write process is kept OK and
> 
> I’d be more verbose here: “This way, the guest will not be able to overwrite areas that cannot be backed up, so the backup remains valid.”

Sounds good

> 
> I like the bluntness of how these two options are named, by the way. That does clearly tell users what they’ll have to expect.
> 
>> +#                     copy-before-write filter continues to work normally.
>> +#
>> +# @break-snapshot: continue guest write. Since this, the snapshot state
>> +#                  provided by copy-before-write filter becomes broken.
> 
> Maybe “Doing so will invalidate the backup snapshot”?

"invalidate" word was never clear for me.. In our block-layer for example "invalidate" is opposite to "inactivate".

something like:
   
    Doing so will make the provided snapshot state invalid and any backup or export process based on it will finally fail.

?

> 
>> +#                  So, all in-flight and all further snapshot-access
>> +#                  operations (through snapshot-access block driver)
>> +#                  will fail.
>> +#
>> +# Since: 7.0
>> +##
>> +{ 'enum': 'OnCbwError',
>> +  'data': [ 'break-guest-write', 'break-snapshot' ] }
>> +
>>   ##
>>   # @BlockdevOptionsCbw:
>>   #
>> @@ -4183,11 +4204,15 @@
>>   #          modifications (or removing) of specified bitmap doesn't
>>   #          influence the filter. (Since 7.0)
>>   #
>> +# @on-cbw-error: Behavior on failure of copy-before-write operation.
>> +#                Default is @break-guest-write. (Since 7.0)
> 
> *7.1
> 
>> +#
>>   # Since: 6.2
>>   ##
>>   { 'struct': 'BlockdevOptionsCbw',
>>     'base': 'BlockdevOptionsGenericFormat',
>> -  'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap' } }
>> +  'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
>> +            '*on-cbw-error': 'OnCbwError' } }
>>   ##
>>   # @BlockdevOptions:
> 


-- 
Best regards,
Vladimir

[PATCH v2 3/7] iotests: add copy-before-write: on-cbw-error tests

[Vladimir Sementsov-Ogievskiy]

Add tests for new option of copy-before-write filter: on-cbw-error.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
---
 tests/qemu-iotests/tests/copy-before-write    | 128 ++++++++++++++++++
 .../qemu-iotests/tests/copy-before-write.out  |   5 +
 2 files changed, 133 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/copy-before-write
 create mode 100644 tests/qemu-iotests/tests/copy-before-write.out

diff --git a/tests/qemu-iotests/tests/copy-before-write b/tests/qemu-iotests/tests/copy-before-write
new file mode 100755
index 0000000000..a32608f597
--- /dev/null
+++ b/tests/qemu-iotests/tests/copy-before-write
@@ -0,0 +1,128 @@
+#!/usr/bin/env python3
+# group: auto backup
+#
+# Copyright (c) 2022 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 os
+import re
+
+import iotests
+from iotests import qemu_img_create, qemu_io
+
+
+temp_img = os.path.join(iotests.test_dir, 'temp')
+source_img = os.path.join(iotests.test_dir, 'source')
+size = '1M'
+
+
+class TestCbwError(iotests.QMPTestCase):
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(temp_img)
+        os.remove(source_img)
+
+    def setUp(self):
+        qemu_img_create('-f', iotests.imgfmt, source_img, size)
+        qemu_img_create('-f', iotests.imgfmt, temp_img, size)
+        qemu_io('-c', 'write 0 1M', source_img)
+
+        self.vm = iotests.VM()
+        self.vm.launch()
+
+    def do_cbw_error(self, on_cbw_error):
+        result = self.vm.qmp('blockdev-add', {
+            'node-name': 'cbw',
+            'driver': 'copy-before-write',
+            'on-cbw-error': on_cbw_error,
+            'file': {
+                'driver': iotests.imgfmt,
+                'file': {
+                    'driver': 'file',
+                    'filename': source_img,
+                }
+            },
+            'target': {
+                'driver': iotests.imgfmt,
+                'file': {
+                    'driver': 'blkdebug',
+                    'image': {
+                        'driver': 'file',
+                        'filename': temp_img
+                    },
+                    'inject-error': [
+                        {
+                            'event': 'write_aio',
+                            'errno': 5,
+                            'immediately': False,
+                            'once': True
+                        }
+                    ]
+                }
+            }
+        })
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('blockdev-add', {
+            'node-name': 'access',
+            'driver': 'snapshot-access',
+            'file': 'cbw'
+        })
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.hmp_qemu_io('cbw', 'write 0 1M')
+        self.assert_qmp(result, 'return', '')
+
+        result = self.vm.hmp_qemu_io('access', 'read 0 1M')
+        self.assert_qmp(result, 'return', '')
+
+        self.vm.shutdown()
+        log = self.vm.get_log()
+        log = re.sub(r'^\[I \d+\.\d+\] OPENED\n', '', log)
+        log = re.sub(r'\[I \+\d+\.\d+\] CLOSED\n?$', '', log)
+        log = iotests.filter_qemu_io(log)
+        return log
+
+    def test_break_snapshot_on_cbw_error(self):
+        """break-snapshot behavior:
+        Guest write succeed, but further snapshot-read fails, as snapshot is
+        broken.
+        """
+        log = self.do_cbw_error('break-snapshot')
+
+        self.assertEqual(log, """\
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read failed: Permission denied
+""")
+
+    def test_break_guest_write_on_cbw_error(self):
+        """break-guest-write behavior:
+        Guest write fails, but snapshot-access continues working and further
+        snapshot-read succeeds.
+        """
+        log = self.do_cbw_error('break-guest-write')
+
+        self.assertEqual(log, """\
+write failed: Input/output error
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+""")
+
+
+if __name__ == '__main__':
+    iotests.main(supported_fmts=['qcow2'],
+                 supported_protocols=['file'])
diff --git a/tests/qemu-iotests/tests/copy-before-write.out b/tests/qemu-iotests/tests/copy-before-write.out
new file mode 100644
index 0000000000..fbc63e62f8
--- /dev/null
+++ b/tests/qemu-iotests/tests/copy-before-write.out
@@ -0,0 +1,5 @@
+..
+----------------------------------------------------------------------
+Ran 2 tests
+
+OK
-- 
2.35.1

Re: [PATCH v2 3/7] iotests: add copy-before-write: on-cbw-error tests

[Hanna Reitz]

On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
> Add tests for new option of copy-before-write filter: on-cbw-error.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
>   tests/qemu-iotests/tests/copy-before-write    | 128 ++++++++++++++++++
>   .../qemu-iotests/tests/copy-before-write.out  |   5 +
>   2 files changed, 133 insertions(+)
>   create mode 100755 tests/qemu-iotests/tests/copy-before-write
>   create mode 100644 tests/qemu-iotests/tests/copy-before-write.out

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

[PATCH v2 4/7] util: add qemu-co-timeout

[Vladimir Sementsov-Ogievskiy]

Add new API, to make a time limited call of the coroutine.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
---
 include/qemu/coroutine.h | 13 ++++++
 util/meson.build         |  1 +
 util/qemu-co-timeout.c   | 89 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 103 insertions(+)
 create mode 100644 util/qemu-co-timeout.c

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index c828a95ee0..8704b05da8 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -316,6 +316,19 @@ static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
     qemu_co_sleep_ns_wakeable(&w, type, ns);
 }
 
+typedef void CleanupFunc(void *opaque);
+/**
+ * Run entry in a coroutine and start timer. Wait for entry to finish or for
+ * timer to elapse, what happen first. If entry finished, return 0, if timer
+ * elapsed earlier, return -ETIMEDOUT.
+ *
+ * Be careful, entry execution is not canceled, user should handle it somehow.
+ * If @clean is provided, it's called after coroutine finish if timeout
+ * happened.
+ */
+int coroutine_fn qemu_co_timeout(CoroutineEntry *entry, void *opaque,
+                                 uint64_t timeout_ns, CleanupFunc clean);
+
 /**
  * Wake a coroutine if it is sleeping in qemu_co_sleep_ns. The timer will be
  * deleted. @sleep_state must be the variable whose address was given to
diff --git a/util/meson.build b/util/meson.build
index f6ee74ad0c..249891db72 100644
--- a/util/meson.build
+++ b/util/meson.build
@@ -83,6 +83,7 @@ if have_block
   util_ss.add(files('block-helpers.c'))
   util_ss.add(files('qemu-coroutine-sleep.c'))
   util_ss.add(files('qemu-co-shared-resource.c'))
+  util_ss.add(files('qemu-co-timeout.c'))
   util_ss.add(files('thread-pool.c', 'qemu-timer.c'))
   util_ss.add(files('readline.c'))
   util_ss.add(files('throttle.c'))
diff --git a/util/qemu-co-timeout.c b/util/qemu-co-timeout.c
new file mode 100644
index 0000000000..00cd335649
--- /dev/null
+++ b/util/qemu-co-timeout.c
@@ -0,0 +1,89 @@
+/*
+ * Helper functionality for distributing a fixed total amount of
+ * an abstract resource among multiple coroutines.
+ *
+ * Copyright (c) 2022 Virtuozzo International GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/coroutine.h"
+#include "block/aio.h"
+
+typedef struct QemuCoTimeoutState {
+    CoroutineEntry *entry;
+    void *opaque;
+    QemuCoSleep sleep_state;
+    bool marker;
+    CleanupFunc *clean;
+} QemuCoTimeoutState;
+
+static void coroutine_fn qemu_co_timeout_entry(void *opaque)
+{
+    QemuCoTimeoutState *s = opaque;
+
+    s->entry(s->opaque);
+
+    if (s->marker) {
+        assert(!s->sleep_state.to_wake);
+        /* .marker set by qemu_co_timeout, it have been failed */
+        if (s->clean) {
+            s->clean(s->opaque);
+        }
+        g_free(s);
+    } else {
+        s->marker = true;
+        qemu_co_sleep_wake(&s->sleep_state);
+    }
+}
+
+int coroutine_fn qemu_co_timeout(CoroutineEntry *entry, void *opaque,
+                                 uint64_t timeout_ns, CleanupFunc clean)
+{
+    QemuCoTimeoutState *s;
+    Coroutine *co;
+
+    if (timeout_ns == 0) {
+        entry(opaque);
+        return 0;
+    }
+
+    s = g_new(QemuCoTimeoutState, 1);
+    *s = (QemuCoTimeoutState) {
+        .entry = entry,
+        .opaque = opaque,
+        .clean = clean
+    };
+
+    co = qemu_coroutine_create(qemu_co_timeout_entry, s);
+
+    aio_co_enter(qemu_get_current_aio_context(), co);
+    qemu_co_sleep_ns_wakeable(&s->sleep_state, QEMU_CLOCK_REALTIME, timeout_ns);
+
+    if (s->marker) {
+        /* .marker set by qemu_co_timeout_entry, success */
+        g_free(s);
+        return 0;
+    }
+
+    /* Don't free s, as we can't cancel qemu_co_timeout_entry execution */
+    s->marker = true;
+    return -ETIMEDOUT;
+}
-- 
2.35.1

Re: [PATCH v2 4/7] util: add qemu-co-timeout

[Hanna Reitz]

On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
> Add new API, to make a time limited call of the coroutine.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
>   include/qemu/coroutine.h | 13 ++++++
>   util/meson.build         |  1 +
>   util/qemu-co-timeout.c   | 89 ++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 103 insertions(+)
>   create mode 100644 util/qemu-co-timeout.c

I don’t really understand what this does.  Intuitively, I would have 
assumed this makes the first yield of the respective coroutine not 
return when the timeout has elapsed, and instead, we switch back to 
qemu_co_timeout(), which returns to its callers.

But it looks like when this yield happens and we switch back to 
qemu_co_timeout(), the coroutine actually isn’t cancelled, and will just 
continue running, actually.  Is that right?  On first look, this looks 
like it’ll be quite difficult to think about what happens when this is 
used, because the coroutine in question is no longer run in sequence 
with its caller, but actually might run in parallel (even though it’s 
still a coroutine, so it’ll remain cooperative multitasking).

Re: [PATCH v2 4/7] util: add qemu-co-timeout

[Vladimir Sementsov-Ogievskiy]

01.04.2022 16:13, Hanna Reitz wrote:
> On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
>> Add new API, to make a time limited call of the coroutine.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>> ---
>>   include/qemu/coroutine.h | 13 ++++++
>>   util/meson.build         |  1 +
>>   util/qemu-co-timeout.c   | 89 ++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 103 insertions(+)
>>   create mode 100644 util/qemu-co-timeout.c
> 
> I don’t really understand what this does.  Intuitively, I would have assumed this makes the first yield of the respective coroutine not return when the timeout has elapsed, and instead, we switch back to qemu_co_timeout(), which returns to its callers.
> 
> But it looks like when this yield happens and we switch back to qemu_co_timeout(), the coroutine actually isn’t cancelled, and will just continue running, actually.  Is that right?  On first look, this looks like it’ll be quite difficult to think about what happens when this is used, because the coroutine in question is no longer run in sequence with its caller, but actually might run in parallel (even though it’s still a coroutine, so it’ll remain cooperative multitasking).
> 

Yes, the coroutine continue execution in parallel. That's a generic interface, and there is no way to "cancel" generic coroutine. So, caller should care about it.


-- 
Best regards,
Vladimir

[PATCH v2 5/7] block/block-copy: block_copy(): add timeout_ns parameter

[Vladimir Sementsov-Ogievskiy]

Add possibility to limit block_copy() call in time. To be used in the
next commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
---
 block/block-copy.c         | 26 +++++++++++++++++++-------
 block/copy-before-write.c  |  2 +-
 include/block/block-copy.h |  2 +-
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index ec46775ea5..b47cb188dd 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -883,10 +883,18 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state)
     return ret;
 }
 
+static void coroutine_fn block_copy_async_co_entry(void *opaque)
+{
+    block_copy_common(opaque);
+}
+
 int coroutine_fn block_copy(BlockCopyState *s, int64_t start, int64_t bytes,
-                            bool ignore_ratelimit)
+                            bool ignore_ratelimit, uint64_t timeout_ns)
 {
-    BlockCopyCallState call_state = {
+    int ret;
+    BlockCopyCallState *call_state = g_new(BlockCopyCallState, 1);
+
+    *call_state = (BlockCopyCallState) {
         .s = s,
         .offset = start,
         .bytes = bytes,
@@ -894,12 +902,16 @@ int coroutine_fn block_copy(BlockCopyState *s, int64_t start, int64_t bytes,
         .max_workers = BLOCK_COPY_MAX_WORKERS,
     };
 
-    return block_copy_common(&call_state);
-}
+    ret = qemu_co_timeout(block_copy_async_co_entry, call_state, timeout_ns,
+                          g_free);
+    if (ret < 0) {
+        /* Timeout. call_state will be freed by running coroutine. */
+        return ret;
+    }
 
-static void coroutine_fn block_copy_async_co_entry(void *opaque)
-{
-    block_copy_common(opaque);
+    ret = call_state->ret;
+
+    return ret;
 }
 
 BlockCopyCallState *block_copy_async(BlockCopyState *s,
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 0614c3d08b..7ef3f9f4c1 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -107,7 +107,7 @@ static coroutine_fn int cbw_do_copy_before_write(BlockDriverState *bs,
     off = QEMU_ALIGN_DOWN(offset, cluster_size);
     end = QEMU_ALIGN_UP(offset + bytes, cluster_size);
 
-    ret = block_copy(s->bcs, off, end - off, true);
+    ret = block_copy(s->bcs, off, end - off, true, 0);
     if (ret < 0 && s->on_cbw_error == ON_CBW_ERROR_BREAK_GUEST_WRITE) {
         return ret;
     }
diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index 68bbd344b2..1c9616cdee 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -40,7 +40,7 @@ int64_t block_copy_reset_unallocated(BlockCopyState *s,
                                      int64_t offset, int64_t *count);
 
 int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes,
-                            bool ignore_ratelimit);
+                            bool ignore_ratelimit, uint64_t timeout_ns);
 
 /*
  * Run block-copy in a coroutine, create corresponding BlockCopyCallState
-- 
2.35.1

Re: [PATCH v2 5/7] block/block-copy: block_copy(): add timeout_ns parameter

[Hanna Reitz]

On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
> Add possibility to limit block_copy() call in time. To be used in the
> next commit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
>   block/block-copy.c         | 26 +++++++++++++++++++-------
>   block/copy-before-write.c  |  2 +-
>   include/block/block-copy.h |  2 +-
>   3 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/block/block-copy.c b/block/block-copy.c
> index ec46775ea5..b47cb188dd 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c

[...]

> @@ -894,12 +902,16 @@ int coroutine_fn block_copy(BlockCopyState *s, int64_t start, int64_t bytes,
>           .max_workers = BLOCK_COPY_MAX_WORKERS,
>       };
>   
> -    return block_copy_common(&call_state);
> -}
> +    ret = qemu_co_timeout(block_copy_async_co_entry, call_state, timeout_ns,
> +                          g_free);

A direct path for timeout_ns == 0 might still be nice to have.

> +    if (ret < 0) {
> +        /* Timeout. call_state will be freed by running coroutine. */

Maybe assert(ret == -ETIMEDOUT);?

> +        return ret;

If I’m right in understanding how qemu_co_timeout() works, 
block_copy_common() will continue to run here.  Shouldn’t we at least 
cancel it by setting call_state->cancelled to true?

(Besides this, I think that letting block_copy_common() running in the 
background should be OK.  I’m not sure what the implications are if we 
do cancel the call here, while on-cbw-error is break-guest-write, 
though.  Should be fine, I guess, because block_copy_common() will still 
correctly keep track of what it has successfully copied and what it hasn’t?)

> +    }
>   
> -static void coroutine_fn block_copy_async_co_entry(void *opaque)
> -{
> -    block_copy_common(opaque);
> +    ret = call_state->ret;
> +
> +    return ret;

But here we still need to free call_state, right?

>   }
>   
>   BlockCopyCallState *block_copy_async(BlockCopyState *s,

Re: [PATCH v2 5/7] block/block-copy: block_copy(): add timeout_ns parameter

[Hanna Reitz]

On 01.04.22 15:16, Hanna Reitz wrote:
> On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
>> Add possibility to limit block_copy() call in time. To be used in the
>> next commit.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>> ---
>>   block/block-copy.c         | 26 +++++++++++++++++++-------
>>   block/copy-before-write.c  |  2 +-
>>   include/block/block-copy.h |  2 +-
>>   3 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/block-copy.c b/block/block-copy.c
>> index ec46775ea5..b47cb188dd 100644
>> --- a/block/block-copy.c
>> +++ b/block/block-copy.c
>
> [...]
>
>> @@ -894,12 +902,16 @@ int coroutine_fn block_copy(BlockCopyState *s, 
>> int64_t start, int64_t bytes,
>>           .max_workers = BLOCK_COPY_MAX_WORKERS,
>>       };
>>   -    return block_copy_common(&call_state);
>> -}
>> +    ret = qemu_co_timeout(block_copy_async_co_entry, call_state, 
>> timeout_ns,
>> +                          g_free);
>
> A direct path for timeout_ns == 0 might still be nice to have.

Ah, never mind, just saw that qemu_co_timeout() itself has a direct path 
for this.  Hadn’t noticed that before.

Re: [PATCH v2 5/7] block/block-copy: block_copy(): add timeout_ns parameter

[Vladimir Sementsov-Ogievskiy]

01.04.2022 16:16, Hanna Reitz wrote:
> On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
>> Add possibility to limit block_copy() call in time. To be used in the
>> next commit.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>> ---
>>   block/block-copy.c         | 26 +++++++++++++++++++-------
>>   block/copy-before-write.c  |  2 +-
>>   include/block/block-copy.h |  2 +-
>>   3 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/block-copy.c b/block/block-copy.c
>> index ec46775ea5..b47cb188dd 100644
>> --- a/block/block-copy.c
>> +++ b/block/block-copy.c
> 
> [...]
> 
>> @@ -894,12 +902,16 @@ int coroutine_fn block_copy(BlockCopyState *s, int64_t start, int64_t bytes,
>>           .max_workers = BLOCK_COPY_MAX_WORKERS,
>>       };
>> -    return block_copy_common(&call_state);
>> -}
>> +    ret = qemu_co_timeout(block_copy_async_co_entry, call_state, timeout_ns,
>> +                          g_free);
> 
> A direct path for timeout_ns == 0 might still be nice to have.
> 
>> +    if (ret < 0) {
>> +        /* Timeout. call_state will be freed by running coroutine. */
> 
> Maybe assert(ret == -ETIMEDOUT);?

OK

> 
>> +        return ret;
> 
> If I’m right in understanding how qemu_co_timeout() works, block_copy_common() will continue to run here.  Shouldn’t we at least cancel it by setting call_state->cancelled to true?

Agree

> 
> (Besides this, I think that letting block_copy_common() running in the background should be OK.  I’m not sure what the implications are if we do cancel the call here, while on-cbw-error is break-guest-write, though.  Should be fine, I guess, because block_copy_common() will still correctly keep track of what it has successfully copied and what it hasn’t?)

Hmm. I now think, that we should at least wait for such cancelled background requests before block_copy_state_free in cbw_close(). But in "[PATCH v5 00/45] Transactional block-graph modifying API" I want to detach children from CBW filter before calling .close().. So, possible solution is to wait for all cancelled requests on .bdrv_co_drain_begin().

Or alternatively, may be just increase bs->in_flight for CBW filter for each background cancelled request? And decrease when it finish. For this we should add a kind of callback to be called when timed-out coroutine entry finish.

> 
>> +    }
>> -static void coroutine_fn block_copy_async_co_entry(void *opaque)
>> -{
>> -    block_copy_common(opaque);
>> +    ret = call_state->ret;
>> +
>> +    return ret;
> 
> But here we still need to free call_state, right?
> 
>>   }
>>   BlockCopyCallState *block_copy_async(BlockCopyState *s,
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 5/7] block/block-copy: block_copy(): add timeout_ns parameter

[Hanna Reitz]

On 01.04.22 18:08, Vladimir Sementsov-Ogievskiy wrote:
> 01.04.2022 16:16, Hanna Reitz wrote:
>> On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
>>> Add possibility to limit block_copy() call in time. To be used in the
>>> next commit.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>>> ---
>>>   block/block-copy.c         | 26 +++++++++++++++++++-------
>>>   block/copy-before-write.c  |  2 +-
>>>   include/block/block-copy.h |  2 +-
>>>   3 files changed, 21 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/block/block-copy.c b/block/block-copy.c
>>> index ec46775ea5..b47cb188dd 100644
>>> --- a/block/block-copy.c
>>> +++ b/block/block-copy.c
>>
>> [...]
>>
>>> @@ -894,12 +902,16 @@ int coroutine_fn block_copy(BlockCopyState *s, 
>>> int64_t start, int64_t bytes,
>>>           .max_workers = BLOCK_COPY_MAX_WORKERS,
>>>       };
>>> -    return block_copy_common(&call_state);
>>> -}
>>> +    ret = qemu_co_timeout(block_copy_async_co_entry, call_state, 
>>> timeout_ns,
>>> +                          g_free);
>>
>> A direct path for timeout_ns == 0 might still be nice to have.
>>
>>> +    if (ret < 0) {
>>> +        /* Timeout. call_state will be freed by running coroutine. */
>>
>> Maybe assert(ret == -ETIMEDOUT);?
>
> OK
>
>>
>>> +        return ret;
>>
>> If I’m right in understanding how qemu_co_timeout() works, 
>> block_copy_common() will continue to run here.  Shouldn’t we at least 
>> cancel it by setting call_state->cancelled to true?
>
> Agree
>
>>
>> (Besides this, I think that letting block_copy_common() running in 
>> the background should be OK.  I’m not sure what the implications are 
>> if we do cancel the call here, while on-cbw-error is 
>> break-guest-write, though.  Should be fine, I guess, because 
>> block_copy_common() will still correctly keep track of what it has 
>> successfully copied and what it hasn’t?)
>
> Hmm. I now think, that we should at least wait for such cancelled 
> background requests before block_copy_state_free in cbw_close(). But 
> in "[PATCH v5 00/45] Transactional block-graph modifying API" I want 
> to detach children from CBW filter before calling .close().. So, 
> possible solution is to wait for all cancelled requests on 
> .bdrv_co_drain_begin().
>
> Or alternatively, may be just increase bs->in_flight for CBW filter 
> for each background cancelled request? And decrease when it finish. 
> For this we should add a kind of callback to be called when timed-out 
> coroutine entry finish.

in_flight sounds good to me.  That would automatically work for 
draining, right?

Re: [PATCH v2 5/7] block/block-copy: block_copy(): add timeout_ns parameter

[Vladimir Sementsov-Ogievskiy]

04.04.2022 17:39, Hanna Reitz wrote:
> On 01.04.22 18:08, Vladimir Sementsov-Ogievskiy wrote:
>> 01.04.2022 16:16, Hanna Reitz wrote:
>>> On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
>>>> Add possibility to limit block_copy() call in time. To be used in the
>>>> next commit.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>>>> ---
>>>>   block/block-copy.c         | 26 +++++++++++++++++++-------
>>>>   block/copy-before-write.c  |  2 +-
>>>>   include/block/block-copy.h |  2 +-
>>>>   3 files changed, 21 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/block/block-copy.c b/block/block-copy.c
>>>> index ec46775ea5..b47cb188dd 100644
>>>> --- a/block/block-copy.c
>>>> +++ b/block/block-copy.c
>>>
>>> [...]
>>>
>>>> @@ -894,12 +902,16 @@ int coroutine_fn block_copy(BlockCopyState *s, int64_t start, int64_t bytes,
>>>>           .max_workers = BLOCK_COPY_MAX_WORKERS,
>>>>       };
>>>> -    return block_copy_common(&call_state);
>>>> -}
>>>> +    ret = qemu_co_timeout(block_copy_async_co_entry, call_state, timeout_ns,
>>>> +                          g_free);
>>>
>>> A direct path for timeout_ns == 0 might still be nice to have.
>>>
>>>> +    if (ret < 0) {
>>>> +        /* Timeout. call_state will be freed by running coroutine. */
>>>
>>> Maybe assert(ret == -ETIMEDOUT);?
>>
>> OK
>>
>>>
>>>> +        return ret;
>>>
>>> If I’m right in understanding how qemu_co_timeout() works, block_copy_common() will continue to run here.  Shouldn’t we at least cancel it by setting call_state->cancelled to true?
>>
>> Agree
>>
>>>
>>> (Besides this, I think that letting block_copy_common() running in the background should be OK.  I’m not sure what the implications are if we do cancel the call here, while on-cbw-error is break-guest-write, though.  Should be fine, I guess, because block_copy_common() will still correctly keep track of what it has successfully copied and what it hasn’t?)
>>
>> Hmm. I now think, that we should at least wait for such cancelled background requests before block_copy_state_free in cbw_close(). But in "[PATCH v5 00/45] Transactional block-graph modifying API" I want to detach children from CBW filter before calling .close().. So, possible solution is to wait for all cancelled requests on .bdrv_co_drain_begin().
>>
>> Or alternatively, may be just increase bs->in_flight for CBW filter for each background cancelled request? And decrease when it finish. For this we should add a kind of callback to be called when timed-out coroutine entry finish.
> 
> in_flight sounds good to me.  That would automatically work for draining, right?
> 
> 

Yes it should

-- 
Best regards,
Vladimir

Re: [PATCH v2 5/7] block/block-copy: block_copy(): add timeout_ns parameter

[Vladimir Sementsov-Ogievskiy]

01.04.2022 16:16, Hanna Reitz wrote:
>> -static void coroutine_fn block_copy_async_co_entry(void *opaque)
>> -{
>> -    block_copy_common(opaque);
>> +    ret = call_state->ret;
>> +
>> +    return ret;
> 
> But here we still need to free call_state, right?

Right, will fix.

-- 
Best regards,
Vladimir

[PATCH v2 6/7] block/copy-before-write: implement cbw-timeout option

[Vladimir Sementsov-Ogievskiy]

In some scenarios, when copy-before-write operations lasts too long
time, it's better to cancel it.

Most useful would be to use the new option together with
on-cbw-error=break-snapshot: this way if cbw operation takes too long
time we'll just cancel backup process but do not disturb the guest too
much.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
---
 block/copy-before-write.c | 6 +++++-
 qapi/block-core.json      | 5 ++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 7ef3f9f4c1..0ea5506f77 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -42,6 +42,7 @@ typedef struct BDRVCopyBeforeWriteState {
     BlockCopyState *bcs;
     BdrvChild *target;
     OnCbwError on_cbw_error;
+    uint32_t cbw_timeout_ns;
 
     /*
      * @lock: protects access to @access_bitmap, @done_bitmap and
@@ -107,7 +108,7 @@ static coroutine_fn int cbw_do_copy_before_write(BlockDriverState *bs,
     off = QEMU_ALIGN_DOWN(offset, cluster_size);
     end = QEMU_ALIGN_UP(offset + bytes, cluster_size);
 
-    ret = block_copy(s->bcs, off, end - off, true, 0);
+    ret = block_copy(s->bcs, off, end - off, true, s->cbw_timeout_ns);
     if (ret < 0 && s->on_cbw_error == ON_CBW_ERROR_BREAK_GUEST_WRITE) {
         return ret;
     }
@@ -412,6 +413,7 @@ static BlockdevOptionsCbw *cbw_parse_options(QDict *options, Error **errp)
      */
     qdict_extract_subqdict(options, NULL, "bitmap");
     qdict_del(options, "on-cbw-error");
+    qdict_del(options, "cbw-timeout");
 
 out:
     visit_free(v);
@@ -455,6 +457,8 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
     }
     s->on_cbw_error = opts->has_on_cbw_error ? opts->on_cbw_error :
             ON_CBW_ERROR_BREAK_GUEST_WRITE;
+    s->cbw_timeout_ns = opts->has_cbw_timeout ?
+        opts->cbw_timeout * NANOSECONDS_PER_SECOND : 0;
 
     bs->total_sectors = bs->file->bs->total_sectors;
     bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3f08025114..e077506e0f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4207,12 +4207,15 @@
 # @on-cbw-error: Behavior on failure of copy-before-write operation.
 #                Default is @break-guest-write. (Since 7.0)
 #
+# @cbw-timeout: Zero means no limit. Non-zero sets the timeout in seconds
+#               for copy-before-write operation. Default 0. (Since 7.0)
+#
 # Since: 6.2
 ##
 { 'struct': 'BlockdevOptionsCbw',
   'base': 'BlockdevOptionsGenericFormat',
   'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
-            '*on-cbw-error': 'OnCbwError' } }
+            '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } }
 
 ##
 # @BlockdevOptions:
-- 
2.35.1

Re: [PATCH v2 6/7] block/copy-before-write: implement cbw-timeout option

[Hanna Reitz]

On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
> In some scenarios, when copy-before-write operations lasts too long
> time, it's better to cancel it.
>
> Most useful would be to use the new option together with
> on-cbw-error=break-snapshot: this way if cbw operation takes too long
> time we'll just cancel backup process but do not disturb the guest too
> much.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
>   block/copy-before-write.c | 6 +++++-
>   qapi/block-core.json      | 5 ++++-
>   2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index 7ef3f9f4c1..0ea5506f77 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -42,6 +42,7 @@ typedef struct BDRVCopyBeforeWriteState {
>       BlockCopyState *bcs;
>       BdrvChild *target;
>       OnCbwError on_cbw_error;
> +    uint32_t cbw_timeout_ns;
>   
>       /*
>        * @lock: protects access to @access_bitmap, @done_bitmap and
> @@ -107,7 +108,7 @@ static coroutine_fn int cbw_do_copy_before_write(BlockDriverState *bs,
>       off = QEMU_ALIGN_DOWN(offset, cluster_size);
>       end = QEMU_ALIGN_UP(offset + bytes, cluster_size);
>   
> -    ret = block_copy(s->bcs, off, end - off, true, 0);
> +    ret = block_copy(s->bcs, off, end - off, true, s->cbw_timeout_ns);
>       if (ret < 0 && s->on_cbw_error == ON_CBW_ERROR_BREAK_GUEST_WRITE) {
>           return ret;
>       }
> @@ -412,6 +413,7 @@ static BlockdevOptionsCbw *cbw_parse_options(QDict *options, Error **errp)
>        */
>       qdict_extract_subqdict(options, NULL, "bitmap");
>       qdict_del(options, "on-cbw-error");
> +    qdict_del(options, "cbw-timeout");
>   
>   out:
>       visit_free(v);
> @@ -455,6 +457,8 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
>       }
>       s->on_cbw_error = opts->has_on_cbw_error ? opts->on_cbw_error :
>               ON_CBW_ERROR_BREAK_GUEST_WRITE;
> +    s->cbw_timeout_ns = opts->has_cbw_timeout ?
> +        opts->cbw_timeout * NANOSECONDS_PER_SECOND : 0;
>   
>       bs->total_sectors = bs->file->bs->total_sectors;
>       bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 3f08025114..e077506e0f 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4207,12 +4207,15 @@
>   # @on-cbw-error: Behavior on failure of copy-before-write operation.
>   #                Default is @break-guest-write. (Since 7.0)
>   #
> +# @cbw-timeout: Zero means no limit. Non-zero sets the timeout in seconds
> +#               for copy-before-write operation. Default 0. (Since 7.0)

*7.1, but:

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

> +#
>   # Since: 6.2
>   ##
>   { 'struct': 'BlockdevOptionsCbw',
>     'base': 'BlockdevOptionsGenericFormat',
>     'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
> -            '*on-cbw-error': 'OnCbwError' } }
> +            '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } }
>   
>   ##
>   # @BlockdevOptions:

Re: [PATCH v2 6/7] block/copy-before-write: implement cbw-timeout option

[Hanna Reitz]

On 01.04.22 15:24, Hanna Reitz wrote:
> On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
>> In some scenarios, when copy-before-write operations lasts too long
>> time, it's better to cancel it.
>>
>> Most useful would be to use the new option together with
>> on-cbw-error=break-snapshot: this way if cbw operation takes too long
>> time we'll just cancel backup process but do not disturb the guest too
>> much.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>> ---
>>   block/copy-before-write.c | 6 +++++-
>>   qapi/block-core.json      | 5 ++++-
>>   2 files changed, 9 insertions(+), 2 deletions(-)
>>

[...]

>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 3f08025114..e077506e0f 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -4207,12 +4207,15 @@
>>   # @on-cbw-error: Behavior on failure of copy-before-write operation.
>>   #                Default is @break-guest-write. (Since 7.0)
>>   #
>> +# @cbw-timeout: Zero means no limit. Non-zero sets the timeout in 
>> seconds
>> +#               for copy-before-write operation. Default 0. (Since 7.0)
>
> *7.1, but:
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>

On second thought, perhaps we should make an explicit note that a 
timeout means an error?  E.g. “When a timeout occurs, the respective 
copy-before-write operation will fail, and the @on-cbw-error parameter 
will decide how this failure is handled.”

(Optional, R-b stands without it, too)

Re: [PATCH v2 6/7] block/copy-before-write: implement cbw-timeout option

[Vladimir Sementsov-Ogievskiy]

01.04.2022 16:28, Hanna Reitz wrote:
> On 01.04.22 15:24, Hanna Reitz wrote:
>> On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
>>> In some scenarios, when copy-before-write operations lasts too long
>>> time, it's better to cancel it.
>>>
>>> Most useful would be to use the new option together with
>>> on-cbw-error=break-snapshot: this way if cbw operation takes too long
>>> time we'll just cancel backup process but do not disturb the guest too
>>> much.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>>> ---
>>>   block/copy-before-write.c | 6 +++++-
>>>   qapi/block-core.json      | 5 ++++-
>>>   2 files changed, 9 insertions(+), 2 deletions(-)
>>>
> 
> [...]
> 
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 3f08025114..e077506e0f 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -4207,12 +4207,15 @@
>>>   # @on-cbw-error: Behavior on failure of copy-before-write operation.
>>>   #                Default is @break-guest-write. (Since 7.0)
>>>   #
>>> +# @cbw-timeout: Zero means no limit. Non-zero sets the timeout in seconds
>>> +#               for copy-before-write operation. Default 0. (Since 7.0)
>>
>> *7.1, but:
>>
>> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
> 
> On second thought, perhaps we should make an explicit note that a timeout means an error?  E.g. “When a timeout occurs, the respective copy-before-write operation will fail, and the @on-cbw-error parameter will decide how this failure is handled.”
> 

Agree, worth noting, will add.

-- 
Best regards,
Vladimir

[PATCH v2 7/7] iotests: copy-before-write: add cases for cbw-timeout option

[Vladimir Sementsov-Ogievskiy]

Add two simple test-cases: timeout failure with
break-snapshot-on-cbw-error behavior and similar with
break-guest-write-on-cbw-error behavior.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
---
 tests/qemu-iotests/tests/copy-before-write    | 78 +++++++++++++++++++
 .../qemu-iotests/tests/copy-before-write.out  |  4 +-
 2 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/tests/copy-before-write b/tests/qemu-iotests/tests/copy-before-write
index a32608f597..265299957c 100755
--- a/tests/qemu-iotests/tests/copy-before-write
+++ b/tests/qemu-iotests/tests/copy-before-write
@@ -122,6 +122,84 @@ read 1048576/1048576 bytes at offset 0
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 """)
 
+    def do_cbw_timeout(self, on_cbw_error):
+        result = self.vm.qmp('object-add', {
+            'qom-type': 'throttle-group',
+            'id': 'group0',
+            'limits': {'bps-write': 1}
+        })
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('blockdev-add', {
+            'node-name': 'cbw',
+            'driver': 'copy-before-write',
+            'on-cbw-error': on_cbw_error,
+            'cbw-timeout': 1,
+            'file': {
+                'driver': iotests.imgfmt,
+                'file': {
+                    'driver': 'file',
+                    'filename': source_img,
+                }
+            },
+            'target': {
+                'driver': 'throttle',
+                'throttle-group': 'group0',
+                'file': {
+                    'driver': 'qcow2',
+                    'file': {
+                        'driver': 'file',
+                        'filename': temp_img
+                    }
+                }
+            }
+        })
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('blockdev-add', {
+            'node-name': 'access',
+            'driver': 'snapshot-access',
+            'file': 'cbw'
+        })
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.hmp_qemu_io('cbw', 'write 0 512K')
+        self.assert_qmp(result, 'return', '')
+
+        # We need second write to trigger throttling
+        result = self.vm.hmp_qemu_io('cbw', 'write 512K 512K')
+        self.assert_qmp(result, 'return', '')
+
+        result = self.vm.hmp_qemu_io('access', 'read 0 1M')
+        self.assert_qmp(result, 'return', '')
+
+        self.vm.shutdown()
+        log = self.vm.get_log()
+        log = re.sub(r'^\[I \d+\.\d+\] OPENED\n', '', log)
+        log = re.sub(r'\[I \+\d+\.\d+\] CLOSED\n?$', '', log)
+        log = iotests.filter_qemu_io(log)
+        return log
+
+    def test_timeout_break_guest(self):
+        log = self.do_cbw_timeout('break-guest-write')
+        self.assertEqual(log, """\
+wrote 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+write failed: Connection timed out
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+""")
+
+    def test_timeout_break_snapshot(self):
+        log = self.do_cbw_timeout('break-snapshot')
+        self.assertEqual(log, """\
+wrote 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 524288/524288 bytes at offset 524288
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read failed: Permission denied
+""")
+
 
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2'],
diff --git a/tests/qemu-iotests/tests/copy-before-write.out b/tests/qemu-iotests/tests/copy-before-write.out
index fbc63e62f8..89968f35d7 100644
--- a/tests/qemu-iotests/tests/copy-before-write.out
+++ b/tests/qemu-iotests/tests/copy-before-write.out
@@ -1,5 +1,5 @@
-..
+....
 ----------------------------------------------------------------------
-Ran 2 tests
+Ran 4 tests
 
 OK
-- 
2.35.1

Re: [PATCH v2 7/7] iotests: copy-before-write: add cases for cbw-timeout option

[Hanna Reitz]

On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
> Add two simple test-cases: timeout failure with
> break-snapshot-on-cbw-error behavior and similar with
> break-guest-write-on-cbw-error behavior.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
>   tests/qemu-iotests/tests/copy-before-write    | 78 +++++++++++++++++++
>   .../qemu-iotests/tests/copy-before-write.out  |  4 +-
>   2 files changed, 80 insertions(+), 2 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v2 7/7] iotests: copy-before-write: add cases for cbw-timeout option

[Vladimir Sementsov-Ogievskiy]

01.04.2022 16:36, Hanna Reitz wrote:
> On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
>> Add two simple test-cases: timeout failure with
>> break-snapshot-on-cbw-error behavior and similar with
>> break-guest-write-on-cbw-error behavior.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>> ---
>>   tests/qemu-iotests/tests/copy-before-write    | 78 +++++++++++++++++++
>>   .../qemu-iotests/tests/copy-before-write.out  |  4 +-
>>   2 files changed, 80 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
> 

Thanks for reviewing!

-- 
Best regards,
Vladimir