qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] commit: Expose on-error option in QMP
@ 2020-02-14 20:08 Kevin Wolf
  2020-02-14 20:08 ` [PATCH 1/7] qapi: Document meaning of 'ignore' BlockdevOnError for jobs Kevin Wolf
                   ` (6 more replies)
  0 siblings, 7 replies; 15+ messages in thread
From: Kevin Wolf @ 2020-02-14 20:08 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz, nsoffer, jsnow

All other block jobs already provide a QMP option to control the error
handling policy. It's time to add it to commit, too.

Peter, I guess libvirt wants to expose this?

Nir, as you would probably assume, this is motivated by the recent
finding that VDSM has to preallocate the theoretical maximum for commit
operations because QEMU doesn't provide a way to reliably resize the
base image dynamically. If you ever want to improve that code, this will
enable you to do so from the QEMU side.

Kevin Wolf (7):
  qapi: Document meaning of 'ignore' BlockdevOnError for jobs
  commit: Remove unused bytes_written
  commit: Fix argument order for block_job_error_action()
  commit: Inline commit_populate()
  commit: Fix is_read for block_job_error_action()
  commit: Expose on-error option in QMP
  iotests: Test error handling policies with block-commit

 qapi/block-core.json       |   9 +-
 block/commit.c             |  37 ++---
 blockdev.c                 |   8 +-
 tests/qemu-iotests/040     | 283 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/040.out |   4 +-
 5 files changed, 309 insertions(+), 32 deletions(-)

-- 
2.20.1



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/7] qapi: Document meaning of 'ignore' BlockdevOnError for jobs
  2020-02-14 20:08 [PATCH 0/7] commit: Expose on-error option in QMP Kevin Wolf
@ 2020-02-14 20:08 ` Kevin Wolf
  2020-02-16 21:44   ` Ján Tomko
  2020-02-14 20:08 ` [PATCH 2/7] commit: Remove unused bytes_written Kevin Wolf
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Kevin Wolf @ 2020-02-14 20:08 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz, nsoffer, jsnow

It is not obvious what 'ignore' actually means for block jobs: It could
be continuing the job and returning success in the end despite the error
(no block job does this). It could also mean continuing and returning
failure in the end (this is what stream does). And it can mean retrying
the failed request later (this is what backup, commit and mirror do).

This (somewhat inconsistent) behaviour was introduced and described for
stream and mirror in commit ae586d6158. backup and commit were
introduced later and use the same model as mirror.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index ef94a29686..395d205fa8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1164,7 +1164,10 @@
 #          for jobs, cancel the job
 #
 # @ignore: ignore the error, only report a QMP event (BLOCK_IO_ERROR
-#          or BLOCK_JOB_ERROR)
+#          or BLOCK_JOB_ERROR). The backup, mirror and commit block jobs retry
+#          the failing request later and may still complete successfully. The
+#          stream block job continues to stream and will complete with an
+#          error.
 #
 # @enospc: same as @stop on ENOSPC, same as @report otherwise.
 #
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/7] commit: Remove unused bytes_written
  2020-02-14 20:08 [PATCH 0/7] commit: Expose on-error option in QMP Kevin Wolf
  2020-02-14 20:08 ` [PATCH 1/7] qapi: Document meaning of 'ignore' BlockdevOnError for jobs Kevin Wolf
@ 2020-02-14 20:08 ` Kevin Wolf
  2020-02-16 21:45   ` Ján Tomko
  2020-02-14 20:08 ` [PATCH 3/7] commit: Fix argument order for block_job_error_action() Kevin Wolf
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Kevin Wolf @ 2020-02-14 20:08 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz, nsoffer, jsnow

The bytes_written variable is only ever written to, it serves no
purpose. This has actually been the case since the commit job was first
introduced in commit 747ff602636.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/commit.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 23c90b3b91..cce898a4f3 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -140,7 +140,6 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
     int ret = 0;
     int64_t n = 0; /* bytes */
     void *buf = NULL;
-    int bytes_written = 0;
     int64_t len, base_len;
 
     ret = len = blk_getlength(s->top);
@@ -180,7 +179,6 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
         trace_commit_one_iteration(s, offset, n, ret);
         if (copy) {
             ret = commit_populate(s->top, s->base, offset, n, buf);
-            bytes_written += n;
         }
         if (ret < 0) {
             BlockErrorAction action =
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/7] commit: Fix argument order for block_job_error_action()
  2020-02-14 20:08 [PATCH 0/7] commit: Expose on-error option in QMP Kevin Wolf
  2020-02-14 20:08 ` [PATCH 1/7] qapi: Document meaning of 'ignore' BlockdevOnError for jobs Kevin Wolf
  2020-02-14 20:08 ` [PATCH 2/7] commit: Remove unused bytes_written Kevin Wolf
@ 2020-02-14 20:08 ` Kevin Wolf
  2020-02-16 21:46   ` Ján Tomko
  2020-02-14 20:08 ` [PATCH 4/7] commit: Inline commit_populate() Kevin Wolf
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Kevin Wolf @ 2020-02-14 20:08 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz, nsoffer, jsnow

The block_job_error_action() error call in the commit job gives the
on_err and is_read arguments in the wrong order. Fix this.

(Of course, hard-coded is_read = false is wrong, too, but that's a
separate problem for a separate patch.)

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/commit.c b/block/commit.c
index cce898a4f3..8189f079d2 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -182,7 +182,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
         }
         if (ret < 0) {
             BlockErrorAction action =
-                block_job_error_action(&s->common, false, s->on_error, -ret);
+                block_job_error_action(&s->common, s->on_error, false, -ret);
             if (action == BLOCK_ERROR_ACTION_REPORT) {
                 goto out;
             } else {
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/7] commit: Inline commit_populate()
  2020-02-14 20:08 [PATCH 0/7] commit: Expose on-error option in QMP Kevin Wolf
                   ` (2 preceding siblings ...)
  2020-02-14 20:08 ` [PATCH 3/7] commit: Fix argument order for block_job_error_action() Kevin Wolf
@ 2020-02-14 20:08 ` Kevin Wolf
  2020-02-16 21:46   ` Ján Tomko
  2020-02-14 20:08 ` [PATCH 5/7] commit: Fix is_read for block_job_error_action() Kevin Wolf
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Kevin Wolf @ 2020-02-14 20:08 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz, nsoffer, jsnow

commit_populate() is a very short function and only called in a single
place. Its return value doesn't tell us whether an error happened while
reading or writing, which would be necessary for sending the right data
in the BLOCK_JOB_ERROR QMP event.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/commit.c | 28 ++++++----------------------
 1 file changed, 6 insertions(+), 22 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 8189f079d2..2992a1012f 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -43,27 +43,6 @@ typedef struct CommitBlockJob {
     char *backing_file_str;
 } CommitBlockJob;
 
-static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
-                                        int64_t offset, uint64_t bytes,
-                                        void *buf)
-{
-    int ret = 0;
-
-    assert(bytes < SIZE_MAX);
-
-    ret = blk_co_pread(bs, offset, bytes, buf, 0);
-    if (ret < 0) {
-        return ret;
-    }
-
-    ret = blk_co_pwrite(base, offset, bytes, buf, 0);
-    if (ret < 0) {
-        return ret;
-    }
-
-    return 0;
-}
-
 static int commit_prepare(Job *job)
 {
     CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
@@ -178,7 +157,12 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
         copy = (ret == 1);
         trace_commit_one_iteration(s, offset, n, ret);
         if (copy) {
-            ret = commit_populate(s->top, s->base, offset, n, buf);
+            assert(n < SIZE_MAX);
+
+            ret = blk_co_pread(s->top, offset, n, buf, 0);
+            if (ret >= 0) {
+                ret = blk_co_pwrite(s->base, offset, n, buf, 0);
+            }
         }
         if (ret < 0) {
             BlockErrorAction action =
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 5/7] commit: Fix is_read for block_job_error_action()
  2020-02-14 20:08 [PATCH 0/7] commit: Expose on-error option in QMP Kevin Wolf
                   ` (3 preceding siblings ...)
  2020-02-14 20:08 ` [PATCH 4/7] commit: Inline commit_populate() Kevin Wolf
@ 2020-02-14 20:08 ` Kevin Wolf
  2020-02-16 21:46   ` Ján Tomko
  2020-02-14 20:08 ` [PATCH 6/7] commit: Expose on-error option in QMP Kevin Wolf
  2020-02-14 20:08 ` [PATCH 7/7] iotests: Test error handling policies with block-commit Kevin Wolf
  6 siblings, 1 reply; 15+ messages in thread
From: Kevin Wolf @ 2020-02-14 20:08 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz, nsoffer, jsnow

block_job_error_action() needs to know if reading from the top node or
writing to the base node failed so that it can set the right 'operation'
in the BLOCK_JOB_ERROR QMP event.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/commit.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/commit.c b/block/commit.c
index 2992a1012f..8e672799af 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -143,6 +143,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
 
     for (offset = 0; offset < len; offset += n) {
         bool copy;
+        bool error_in_source = true;
 
         /* Note that even when no rate limit is applied we need to yield
          * with no pending I/O here so that bdrv_drain_all() returns.
@@ -162,11 +163,15 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
             ret = blk_co_pread(s->top, offset, n, buf, 0);
             if (ret >= 0) {
                 ret = blk_co_pwrite(s->base, offset, n, buf, 0);
+                if (ret < 0) {
+                    error_in_source = false;
+                }
             }
         }
         if (ret < 0) {
             BlockErrorAction action =
-                block_job_error_action(&s->common, s->on_error, false, -ret);
+                block_job_error_action(&s->common, s->on_error,
+                                       error_in_source, -ret);
             if (action == BLOCK_ERROR_ACTION_REPORT) {
                 goto out;
             } else {
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 6/7] commit: Expose on-error option in QMP
  2020-02-14 20:08 [PATCH 0/7] commit: Expose on-error option in QMP Kevin Wolf
                   ` (4 preceding siblings ...)
  2020-02-14 20:08 ` [PATCH 5/7] commit: Fix is_read for block_job_error_action() Kevin Wolf
@ 2020-02-14 20:08 ` Kevin Wolf
  2020-02-16 21:47   ` Ján Tomko
  2020-02-14 20:08 ` [PATCH 7/7] iotests: Test error handling policies with block-commit Kevin Wolf
  6 siblings, 1 reply; 15+ messages in thread
From: Kevin Wolf @ 2020-02-14 20:08 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz, nsoffer, jsnow

Now that the error handling in the common block job is fixed, we can
expose the on-error option in QMP instead of hard-coding it as 'report'
in qmp_block_commit().

This fulfills the promise that the old comment in that function made,
even if a bit later than expected: "This will be part of the QMP
command, if/when the BlockdevOnError change for blkmirror makes it in".

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json | 4 ++++
 blockdev.c           | 8 ++++----
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 395d205fa8..c69aece46e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1659,6 +1659,9 @@
 #
 # @speed:  the maximum speed, in bytes per second
 #
+# @on-error: the action to take on an error. 'ignore' means that the request
+#            should be retried. (default: report; Since: 5.0)
+#
 # @filter-node-name: the node name that should be assigned to the
 #                    filter driver that the commit job inserts into the graph
 #                    above @top. If this option is not given, a node name is
@@ -1695,6 +1698,7 @@
   'data': { '*job-id': 'str', 'device': 'str', '*base-node': 'str',
             '*base': 'str', '*top-node': 'str', '*top': 'str',
             '*backing-file': 'str', '*speed': 'int',
+            '*on-error': 'BlockdevOnError',
             '*filter-node-name': 'str',
             '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
 
diff --git a/blockdev.c b/blockdev.c
index c6a727cca9..374189a426 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3471,6 +3471,7 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
                       bool has_top, const char *top,
                       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,
@@ -3481,15 +3482,14 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
     BlockDriverState *base_bs, *top_bs;
     AioContext *aio_context;
     Error *local_err = NULL;
-    /* This will be part of the QMP command, if/when the
-     * BlockdevOnError change for blkmirror makes it in
-     */
-    BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
     int job_flags = JOB_DEFAULT;
 
     if (!has_speed) {
         speed = 0;
     }
+    if (!has_on_error) {
+        on_error = BLOCKDEV_ON_ERROR_REPORT;
+    }
     if (!has_filter_node_name) {
         filter_node_name = NULL;
     }
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 7/7] iotests: Test error handling policies with block-commit
  2020-02-14 20:08 [PATCH 0/7] commit: Expose on-error option in QMP Kevin Wolf
                   ` (5 preceding siblings ...)
  2020-02-14 20:08 ` [PATCH 6/7] commit: Expose on-error option in QMP Kevin Wolf
@ 2020-02-14 20:08 ` Kevin Wolf
  6 siblings, 0 replies; 15+ messages in thread
From: Kevin Wolf @ 2020-02-14 20:08 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz, nsoffer, jsnow

This tests both read failure (from the top node) and write failure (to
the base node) for on-error=report/stop/ignore.

As block-commit actually starts two different types of block jobs
(mirror.c for committing the active later, commit.c for intermediate
layers), all tests are run for both cases.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/040     | 283 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/040.out |   4 +-
 2 files changed, 285 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 2e7ee0e84f..32c82b4ec6 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -430,6 +430,289 @@ class TestReopenOverlay(ImageCommitTestCase):
     def test_reopen_overlay(self):
         self.run_commit_test(self.img1, self.img0)
 
+class TestErrorHandling(iotests.QMPTestCase):
+    image_len = 2 * 1024 * 1024
+
+    def setUp(self):
+        iotests.create_image(backing_img, self.image_len)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, mid_img)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % mid_img, test_img)
+
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x11 0 512k', mid_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x22 0 512k', test_img)
+
+        self.vm = iotests.VM()
+        self.vm.launch()
+
+        self.blkdebug_file = iotests.file_path("blkdebug.conf")
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(test_img)
+        os.remove(mid_img)
+        os.remove(backing_img)
+
+    def blockdev_add(self, **kwargs):
+        result = self.vm.qmp('blockdev-add', **kwargs)
+        self.assert_qmp(result, 'return', {})
+
+    def add_block_nodes(self, base_debug=None, mid_debug=None, top_debug=None):
+        self.blockdev_add(node_name='base-file', driver='file',
+                          filename=backing_img)
+        self.blockdev_add(node_name='mid-file', driver='file',
+                          filename=mid_img)
+        self.blockdev_add(node_name='top-file', driver='file',
+                          filename=test_img)
+
+        if base_debug:
+            self.blockdev_add(node_name='base-dbg', driver='blkdebug',
+                              image='base-file', inject_error=base_debug)
+        if mid_debug:
+            self.blockdev_add(node_name='mid-dbg', driver='blkdebug',
+                              image='mid-file', inject_error=mid_debug)
+        if top_debug:
+            self.blockdev_add(node_name='top-dbg', driver='blkdebug',
+                              image='top-file', inject_error=top_debug)
+
+        self.blockdev_add(node_name='base-fmt', driver='raw',
+                          file=('base-dbg' if base_debug else 'base-file'))
+        self.blockdev_add(node_name='mid-fmt', driver=iotests.imgfmt,
+                          file=('mid-dbg' if mid_debug else 'mid-file'),
+                          backing='base-fmt')
+        self.blockdev_add(node_name='top-fmt', driver=iotests.imgfmt,
+                          file=('top-dbg' if top_debug else 'top-file'),
+                          backing='mid-fmt')
+
+    def run_job(self, expected_events, error_pauses_job=False):
+        match_device = {'data': {'device': 'job0'}}
+        events = [
+            ('BLOCK_JOB_COMPLETED', match_device),
+            ('BLOCK_JOB_CANCELLED', match_device),
+            ('BLOCK_JOB_ERROR', match_device),
+            ('BLOCK_JOB_READY', match_device),
+        ]
+
+        completed = False
+        log = []
+        while not completed:
+            ev = self.vm.events_wait(events, timeout=5.0)
+            if ev['event'] == 'BLOCK_JOB_COMPLETED':
+                completed = True
+            elif ev['event'] == 'BLOCK_JOB_ERROR':
+                if error_pauses_job:
+                    result = self.vm.qmp('block-job-resume', device='job0')
+                    self.assert_qmp(result, 'return', {})
+            elif ev['event'] == 'BLOCK_JOB_READY':
+                result = self.vm.qmp('block-job-complete', device='job0')
+                self.assert_qmp(result, 'return', {})
+            else:
+                self.fail("Unexpected event: %s" % ev)
+            log.append(iotests.filter_qmp_event(ev))
+
+        self.maxDiff = None
+        self.assertEqual(expected_events, log)
+
+    def event_error(self, op, action):
+        return {
+            'event': 'BLOCK_JOB_ERROR',
+            'data': {'action': action, 'device': 'job0', 'operation': op},
+            'timestamp': {'microseconds': 'USECS', 'seconds': 'SECS'}
+        }
+
+    def event_ready(self):
+        return {
+            'event': 'BLOCK_JOB_READY',
+            'data': {'device': 'job0',
+                     'len': 524288,
+                     'offset': 524288,
+                     'speed': 0,
+                     'type': 'commit'},
+            'timestamp': {'microseconds': 'USECS', 'seconds': 'SECS'},
+        }
+
+    def event_completed(self, errmsg=None, active=True):
+        max_len = 524288 if active else self.image_len
+        data = {
+            'device': 'job0',
+            'len': max_len,
+            'offset': 0 if errmsg else max_len,
+            'speed': 0,
+            'type': 'commit'
+        }
+        if errmsg:
+            data['error'] = errmsg
+
+        return {
+            'event': 'BLOCK_JOB_COMPLETED',
+            'data': data,
+            'timestamp': {'microseconds': 'USECS', 'seconds': 'SECS'},
+        }
+
+    def blkdebug_event(self, event, is_raw=False):
+        if event:
+            return [{
+                'event': event,
+                'sector': 512 if is_raw else 1024,
+                'once': True,
+            }]
+        return None
+
+    def prepare_and_start_job(self, on_error, active=True,
+                              top_event=None, mid_event=None, base_event=None):
+
+        top_debug = self.blkdebug_event(top_event)
+        mid_debug = self.blkdebug_event(mid_event)
+        base_debug = self.blkdebug_event(base_event, True)
+
+        self.add_block_nodes(top_debug=top_debug, mid_debug=mid_debug,
+                             base_debug=base_debug)
+
+        result = self.vm.qmp('block-commit', job_id='job0', device='top-fmt',
+                             top_node='top-fmt' if active else 'mid-fmt',
+                             base_node='mid-fmt' if active else 'base-fmt',
+                             on_error=on_error)
+        self.assert_qmp(result, 'return', {})
+
+    def testActiveReadErrorReport(self):
+        self.prepare_and_start_job('report', top_event='read_aio')
+        self.run_job([
+            self.event_error('read', 'report'),
+            self.event_completed('Input/output error')
+        ])
+
+        self.vm.shutdown()
+        self.assertFalse(iotests.compare_images(test_img, mid_img),
+                         'target image matches source after error')
+
+    def testActiveReadErrorStop(self):
+        self.prepare_and_start_job('stop', top_event='read_aio')
+        self.run_job([
+            self.event_error('read', 'stop'),
+            self.event_ready(),
+            self.event_completed()
+        ], error_pauses_job=True)
+
+        self.vm.shutdown()
+        self.assertTrue(iotests.compare_images(test_img, mid_img),
+                        'target image does not match source after commit')
+
+    def testActiveReadErrorIgnore(self):
+        self.prepare_and_start_job('ignore', top_event='read_aio')
+        self.run_job([
+            self.event_error('read', 'ignore'),
+            self.event_ready(),
+            self.event_completed()
+        ])
+
+        # For commit, 'ignore' actually means retry, so this will succeed
+        self.vm.shutdown()
+        self.assertTrue(iotests.compare_images(test_img, mid_img),
+                        'target image does not match source after commit')
+
+    def testActiveWriteErrorReport(self):
+        self.prepare_and_start_job('report', mid_event='write_aio')
+        self.run_job([
+            self.event_error('write', 'report'),
+            self.event_completed('Input/output error')
+        ])
+
+        self.vm.shutdown()
+        self.assertFalse(iotests.compare_images(test_img, mid_img),
+                         'target image matches source after error')
+
+    def testActiveWriteErrorStop(self):
+        self.prepare_and_start_job('stop', mid_event='write_aio')
+        self.run_job([
+            self.event_error('write', 'stop'),
+            self.event_ready(),
+            self.event_completed()
+        ], error_pauses_job=True)
+
+        self.vm.shutdown()
+        self.assertTrue(iotests.compare_images(test_img, mid_img),
+                        'target image does not match source after commit')
+
+    def testActiveWriteErrorIgnore(self):
+        self.prepare_and_start_job('ignore', mid_event='write_aio')
+        self.run_job([
+            self.event_error('write', 'ignore'),
+            self.event_ready(),
+            self.event_completed()
+        ])
+
+        # For commit, 'ignore' actually means retry, so this will succeed
+        self.vm.shutdown()
+        self.assertTrue(iotests.compare_images(test_img, mid_img),
+                        'target image does not match source after commit')
+
+    def testIntermediateReadErrorReport(self):
+        self.prepare_and_start_job('report', active=False, mid_event='read_aio')
+        self.run_job([
+            self.event_error('read', 'report'),
+            self.event_completed('Input/output error', active=False)
+        ])
+
+        self.vm.shutdown()
+        self.assertFalse(iotests.compare_images(mid_img, backing_img, fmt2='raw'),
+                         'target image matches source after error')
+
+    def testIntermediateReadErrorStop(self):
+        self.prepare_and_start_job('stop', active=False, mid_event='read_aio')
+        self.run_job([
+            self.event_error('read', 'stop'),
+            self.event_completed(active=False)
+        ], error_pauses_job=True)
+
+        self.vm.shutdown()
+        self.assertTrue(iotests.compare_images(mid_img, backing_img, fmt2='raw'),
+                        'target image does not match source after commit')
+
+    def testIntermediateReadErrorIgnore(self):
+        self.prepare_and_start_job('ignore', active=False, mid_event='read_aio')
+        self.run_job([
+            self.event_error('read', 'ignore'),
+            self.event_completed(active=False)
+        ])
+
+        # For commit, 'ignore' actually means retry, so this will succeed
+        self.vm.shutdown()
+        self.assertTrue(iotests.compare_images(mid_img, backing_img, fmt2='raw'),
+                        'target image does not match source after commit')
+
+    def testIntermediateWriteErrorReport(self):
+        self.prepare_and_start_job('report', active=False, base_event='write_aio')
+        self.run_job([
+            self.event_error('write', 'report'),
+            self.event_completed('Input/output error', active=False)
+        ])
+
+        self.vm.shutdown()
+        self.assertFalse(iotests.compare_images(mid_img, backing_img, fmt2='raw'),
+                         'target image matches source after error')
+
+    def testIntermediateWriteErrorStop(self):
+        self.prepare_and_start_job('stop', active=False, base_event='write_aio')
+        self.run_job([
+            self.event_error('write', 'stop'),
+            self.event_completed(active=False)
+        ], error_pauses_job=True)
+
+        self.vm.shutdown()
+        self.assertTrue(iotests.compare_images(mid_img, backing_img, fmt2='raw'),
+                        'target image does not match source after commit')
+
+    def testIntermediateWriteErrorIgnore(self):
+        self.prepare_and_start_job('ignore', active=False, base_event='write_aio')
+        self.run_job([
+            self.event_error('write', 'ignore'),
+            self.event_completed(active=False)
+        ])
+
+        # For commit, 'ignore' actually means retry, so this will succeed
+        self.vm.shutdown()
+        self.assertTrue(iotests.compare_images(mid_img, backing_img, fmt2='raw'),
+                        'target image does not match source after commit')
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'],
                  supported_protocols=['file'])
diff --git a/tests/qemu-iotests/040.out b/tests/qemu-iotests/040.out
index 220a5fa82c..6a917130b6 100644
--- a/tests/qemu-iotests/040.out
+++ b/tests/qemu-iotests/040.out
@@ -1,5 +1,5 @@
-...............................................
+...........................................................
 ----------------------------------------------------------------------
-Ran 47 tests
+Ran 59 tests
 
 OK
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/7] qapi: Document meaning of 'ignore' BlockdevOnError for jobs
  2020-02-14 20:08 ` [PATCH 1/7] qapi: Document meaning of 'ignore' BlockdevOnError for jobs Kevin Wolf
@ 2020-02-16 21:44   ` Ján Tomko
  2020-02-17  9:58     ` Kevin Wolf
  0 siblings, 1 reply; 15+ messages in thread
From: Ján Tomko @ 2020-02-16 21:44 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: pkrempa, qemu-block, qemu-devel, mreitz, nsoffer, jsnow

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

On Fri, Feb 14, 2020 at 09:08:06PM +0100, Kevin Wolf wrote:
>It is not obvious what 'ignore' actually means for block jobs: It could
>be continuing the job and returning success in the end despite the error
>(no block job does this). It could also mean continuing and returning
>failure in the end (this is what stream does). And it can mean retrying
>the failed request later (this is what backup, commit and mirror do).
>
>This (somewhat inconsistent) behaviour was introduced and described for
>stream and mirror in commit ae586d6158. backup and commit were

fatal: ambiguous argument 'ae586d6158': unknown revision or path not in the working tree.

>introduced later and use the same model as mirror.
>
>Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>---
> qapi/block-core.json | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/7] commit: Remove unused bytes_written
  2020-02-14 20:08 ` [PATCH 2/7] commit: Remove unused bytes_written Kevin Wolf
@ 2020-02-16 21:45   ` Ján Tomko
  0 siblings, 0 replies; 15+ messages in thread
From: Ján Tomko @ 2020-02-16 21:45 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: pkrempa, qemu-block, qemu-devel, mreitz, nsoffer, jsnow

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

On Fri, Feb 14, 2020 at 09:08:07PM +0100, Kevin Wolf wrote:
>The bytes_written variable is only ever written to, it serves no
>purpose. This has actually been the case since the commit job was first
>introduced in commit 747ff602636.
>
>Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>---
> block/commit.c | 2 --
> 1 file changed, 2 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/7] commit: Fix argument order for block_job_error_action()
  2020-02-14 20:08 ` [PATCH 3/7] commit: Fix argument order for block_job_error_action() Kevin Wolf
@ 2020-02-16 21:46   ` Ján Tomko
  0 siblings, 0 replies; 15+ messages in thread
From: Ján Tomko @ 2020-02-16 21:46 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: pkrempa, qemu-block, qemu-devel, mreitz, nsoffer, jsnow

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

On Fri, Feb 14, 2020 at 09:08:08PM +0100, Kevin Wolf wrote:
>The block_job_error_action() error call in the commit job gives the
>on_err and is_read arguments in the wrong order. Fix this.
>
>(Of course, hard-coded is_read = false is wrong, too, but that's a
>separate problem for a separate patch.)
>
>Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>---
> block/commit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/7] commit: Inline commit_populate()
  2020-02-14 20:08 ` [PATCH 4/7] commit: Inline commit_populate() Kevin Wolf
@ 2020-02-16 21:46   ` Ján Tomko
  0 siblings, 0 replies; 15+ messages in thread
From: Ján Tomko @ 2020-02-16 21:46 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: pkrempa, qemu-block, qemu-devel, mreitz, nsoffer, jsnow

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

On Fri, Feb 14, 2020 at 09:08:09PM +0100, Kevin Wolf wrote:
>commit_populate() is a very short function and only called in a single
>place. Its return value doesn't tell us whether an error happened while
>reading or writing, which would be necessary for sending the right data
>in the BLOCK_JOB_ERROR QMP event.
>
>Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>---
> block/commit.c | 28 ++++++----------------------
> 1 file changed, 6 insertions(+), 22 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/7] commit: Fix is_read for block_job_error_action()
  2020-02-14 20:08 ` [PATCH 5/7] commit: Fix is_read for block_job_error_action() Kevin Wolf
@ 2020-02-16 21:46   ` Ján Tomko
  0 siblings, 0 replies; 15+ messages in thread
From: Ján Tomko @ 2020-02-16 21:46 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: pkrempa, qemu-block, qemu-devel, mreitz, nsoffer, jsnow

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

On Fri, Feb 14, 2020 at 09:08:10PM +0100, Kevin Wolf wrote:
>block_job_error_action() needs to know if reading from the top node or
>writing to the base node failed so that it can set the right 'operation'
>in the BLOCK_JOB_ERROR QMP event.
>
>Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>---
> block/commit.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 6/7] commit: Expose on-error option in QMP
  2020-02-14 20:08 ` [PATCH 6/7] commit: Expose on-error option in QMP Kevin Wolf
@ 2020-02-16 21:47   ` Ján Tomko
  0 siblings, 0 replies; 15+ messages in thread
From: Ján Tomko @ 2020-02-16 21:47 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: pkrempa, qemu-block, qemu-devel, mreitz, nsoffer, jsnow

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

On Fri, Feb 14, 2020 at 09:08:11PM +0100, Kevin Wolf wrote:
>Now that the error handling in the common block job is fixed, we can
>expose the on-error option in QMP instead of hard-coding it as 'report'
>in qmp_block_commit().
>
>This fulfills the promise that the old comment in that function made,
>even if a bit later than expected: "This will be part of the QMP
>command, if/when the BlockdevOnError change for blkmirror makes it in".
>
>Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>---
> qapi/block-core.json | 4 ++++
> blockdev.c           | 8 ++++----
> 2 files changed, 8 insertions(+), 4 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/7] qapi: Document meaning of 'ignore' BlockdevOnError for jobs
  2020-02-16 21:44   ` Ján Tomko
@ 2020-02-17  9:58     ` Kevin Wolf
  0 siblings, 0 replies; 15+ messages in thread
From: Kevin Wolf @ 2020-02-17  9:58 UTC (permalink / raw)
  To: Ján Tomko; +Cc: pkrempa, qemu-block, qemu-devel, mreitz, nsoffer, jsnow

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

Am 16.02.2020 um 22:44 hat Ján Tomko geschrieben:
> On Fri, Feb 14, 2020 at 09:08:06PM +0100, Kevin Wolf wrote:
> > It is not obvious what 'ignore' actually means for block jobs: It could
> > be continuing the job and returning success in the end despite the error
> > (no block job does this). It could also mean continuing and returning
> > failure in the end (this is what stream does). And it can mean retrying
> > the failed request later (this is what backup, commit and mirror do).
> > 
> > This (somewhat inconsistent) behaviour was introduced and described for
> > stream and mirror in commit ae586d6158. backup and commit were
> 
> fatal: ambiguous argument 'ae586d6158': unknown revision or path not in the working tree.

Oops, thanks for catching this. Not sure how this happened, but
32c81a4a6ec is the correct commit.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-02-17  9:59 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-14 20:08 [PATCH 0/7] commit: Expose on-error option in QMP Kevin Wolf
2020-02-14 20:08 ` [PATCH 1/7] qapi: Document meaning of 'ignore' BlockdevOnError for jobs Kevin Wolf
2020-02-16 21:44   ` Ján Tomko
2020-02-17  9:58     ` Kevin Wolf
2020-02-14 20:08 ` [PATCH 2/7] commit: Remove unused bytes_written Kevin Wolf
2020-02-16 21:45   ` Ján Tomko
2020-02-14 20:08 ` [PATCH 3/7] commit: Fix argument order for block_job_error_action() Kevin Wolf
2020-02-16 21:46   ` Ján Tomko
2020-02-14 20:08 ` [PATCH 4/7] commit: Inline commit_populate() Kevin Wolf
2020-02-16 21:46   ` Ján Tomko
2020-02-14 20:08 ` [PATCH 5/7] commit: Fix is_read for block_job_error_action() Kevin Wolf
2020-02-16 21:46   ` Ján Tomko
2020-02-14 20:08 ` [PATCH 6/7] commit: Expose on-error option in QMP Kevin Wolf
2020-02-16 21:47   ` Ján Tomko
2020-02-14 20:08 ` [PATCH 7/7] iotests: Test error handling policies with block-commit Kevin Wolf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).