* [PATCH 0/3] block: copy-before-write: on-cbw-error behavior
@ 2022-03-01 20:59 Vladimir Sementsov-Ogievskiy
2022-03-01 20:59 ` [PATCH 1/3] block/copy-before-write: refactor option parsing Vladimir Sementsov-Ogievskiy
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-03-01 20:59 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, eblake, armbru, hreitz, kwolf, vsementsov, jsnow
Hi all!
Here is a new option for copy-before-write filter, to alter its behavior
on copy-before-write operation failure.
Based-on: <20220228113927.1852146-1-vsementsov@virtuozzo.com>
([PATCH v5 00/16] Make image fleecing more usable)
Vladimir Sementsov-Ogievskiy (3):
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
qapi/block-core.json | 27 +++-
block/copy-before-write.c | 123 ++++++++++++-----
tests/qemu-iotests/tests/copy-before-write | 128 ++++++++++++++++++
.../qemu-iotests/tests/copy-before-write.out | 5 +
4 files changed, 248 insertions(+), 35 deletions(-)
create mode 100755 tests/qemu-iotests/tests/copy-before-write
create mode 100644 tests/qemu-iotests/tests/copy-before-write.out
--
2.31.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/3] block/copy-before-write: refactor option parsing
2022-03-01 20:59 [PATCH 0/3] block: copy-before-write: on-cbw-error behavior Vladimir Sementsov-Ogievskiy
@ 2022-03-01 20:59 ` Vladimir Sementsov-Ogievskiy
2022-03-01 20:59 ` [PATCH 2/3] block/copy-before-write: add on-cbw-error open parameter Vladimir Sementsov-Ogievskiy
2022-03-01 20:59 ` [PATCH 3/3] iotests: add copy-before-write: on-cbw-error tests Vladimir Sementsov-Ogievskiy
2 siblings, 0 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-03-01 20:59 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, eblake, armbru, hreitz, kwolf, vsementsov, jsnow
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@virtuozzo.com>
---
block/copy-before-write.c | 66 ++++++++++++++++++++++++---------------
1 file changed, 40 insertions(+), 26 deletions(-)
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 0b6d26605c..56aa7577c3 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;
- }
+ /*
+ * 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(bitmap_qdict, errp);
+ 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) {
+ visit_type_BlockdevOptionsCbw(v, NULL, &opts, errp);
+ if (!opts) {
goto out;
}
- *bitmap = block_dirty_bitmap_lookup(bmp_param->node, bmp_param->name, NULL,
- errp);
- if (!*bitmap) {
- 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.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3] block/copy-before-write: add on-cbw-error open parameter
2022-03-01 20:59 [PATCH 0/3] block: copy-before-write: on-cbw-error behavior Vladimir Sementsov-Ogievskiy
2022-03-01 20:59 ` [PATCH 1/3] block/copy-before-write: refactor option parsing Vladimir Sementsov-Ogievskiy
@ 2022-03-01 20:59 ` Vladimir Sementsov-Ogievskiy
2022-03-09 15:43 ` Vladimir Sementsov-Ogievskiy
2022-03-01 20:59 ` [PATCH 3/3] iotests: add copy-before-write: on-cbw-error tests Vladimir Sementsov-Ogievskiy
2 siblings, 1 reply; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-03-01 20:59 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, eblake, armbru, hreitz, kwolf, vsementsov, jsnow
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@virtuozzo.com>
---
qapi/block-core.json | 27 ++++++++++++++++++-
block/copy-before-write.c | 57 +++++++++++++++++++++++++++++++++------
2 files changed, 75 insertions(+), 9 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f13b5ff942..e5206272aa 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4159,6 +4159,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:
#
@@ -4180,11 +4201,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:
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 56aa7577c3..e89cc9799c 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,9 +311,9 @@ 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);
- return ret;
+ return ret < 0 ? ret : ret2;
}
static int coroutine_fn cbw_co_pdiscard_snapshot(BlockDriverState *bs,
@@ -366,6 +404,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 +446,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 |
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3] iotests: add copy-before-write: on-cbw-error tests
2022-03-01 20:59 [PATCH 0/3] block: copy-before-write: on-cbw-error behavior Vladimir Sementsov-Ogievskiy
2022-03-01 20:59 ` [PATCH 1/3] block/copy-before-write: refactor option parsing Vladimir Sementsov-Ogievskiy
2022-03-01 20:59 ` [PATCH 2/3] block/copy-before-write: add on-cbw-error open parameter Vladimir Sementsov-Ogievskiy
@ 2022-03-01 20:59 ` Vladimir Sementsov-Ogievskiy
2 siblings, 0 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-03-01 20:59 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, eblake, armbru, hreitz, kwolf, vsementsov, jsnow
Add tests for new option of copy-before-write filter: on-cbw-error.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
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.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] block/copy-before-write: add on-cbw-error open parameter
2022-03-01 20:59 ` [PATCH 2/3] block/copy-before-write: add on-cbw-error open parameter Vladimir Sementsov-Ogievskiy
@ 2022-03-09 15:43 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-03-09 15:43 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, eblake, armbru, hreitz, kwolf, jsnow
01.03.2022 23:59, Vladimir Sementsov-Ogievskiy wrote:
> @@ -273,9 +311,9 @@ 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);
>
> - return ret;
> + return ret < 0 ? ret : ret2;
Oops. On success we should return ret, as it's a block-status
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-03-09 15:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-01 20:59 [PATCH 0/3] block: copy-before-write: on-cbw-error behavior Vladimir Sementsov-Ogievskiy
2022-03-01 20:59 ` [PATCH 1/3] block/copy-before-write: refactor option parsing Vladimir Sementsov-Ogievskiy
2022-03-01 20:59 ` [PATCH 2/3] block/copy-before-write: add on-cbw-error open parameter Vladimir Sementsov-Ogievskiy
2022-03-09 15:43 ` Vladimir Sementsov-Ogievskiy
2022-03-01 20:59 ` [PATCH 3/3] iotests: add copy-before-write: on-cbw-error tests Vladimir Sementsov-Ogievskiy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.