* [PATCH v15 00/13] Apply COR-filter to the block-stream permanently
@ 2020-12-16 6:16 Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 01/13] copy-on-read: support preadv/pwritev_part functions Vladimir Sementsov-Ogievskiy
` (13 more replies)
0 siblings, 14 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-16 6:16 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, qemu-devel, armbru, stefanha, den,
andrey.shinkevich, mreitz, jsnow
Hi all!
Here is a new version of cor-filter in block-stream series. Main change
is freezing the chain in cor-filter itself.
v15:
02: s/ = / = /
add Max's r-b
03: add Max's r-b
04: since: 6.0
indent comment
add Max's r-b
05: changed commit msg
wording
document the default
since: 6.0
use bdrv_find_node(), fix errp overwriting
freeze the chain
check bottom is not filter
ref bottom_bs
06: limit to qcow2 to not care
use qemu-img rebase -u -b ''
07: use assert instead of abort
add Max's r-b
08: add Max's r-b
09: changed commit msg (was "stream: skip filters when writing backing file name to QCOW2 header")
keep mostly same logic for the case when backing-file is specified, don't do bdrv_find_backing_image()
10: don't restrict backing-file for now
11: add Max's r-b
12: add Max's r-b
13: chain is now frozen in filter, so the logic changed around add/remove fitlter
Andrey Shinkevich (10):
copy-on-read: support preadv/pwritev_part functions
block: add API function to insert a node
copy-on-read: add filter drop function
qapi: add filter-node-name to block-stream
qapi: copy-on-read filter: add 'bottom' option
iotests: add #310 to test bottom node in COR driver
block: include supported_read_flags into BDS structure
copy-on-read: skip non-guest reads if no copy needed
stream: rework backing-file changing
block: apply COR-filter to block-stream jobs
Vladimir Sementsov-Ogievskiy (3):
qapi: block-stream: add "bottom" argument
iotests: 30: prepare to COR filter insertion by stream job
block/stream: add s->target_bs
qapi/block-core.json | 38 ++++++-
block/copy-on-read.h | 32 ++++++
include/block/block.h | 10 +-
include/block/block_int.h | 12 ++-
block.c | 25 +++++
block/copy-on-read.c | 184 +++++++++++++++++++++++++++++---
block/io.c | 10 +-
block/monitor/block-hmp-cmds.c | 7 +-
block/stream.c | 185 ++++++++++++++++++++-------------
blockdev.c | 69 +++++++++---
tests/qemu-iotests/030 | 12 ++-
tests/qemu-iotests/141.out | 2 +-
tests/qemu-iotests/245 | 20 ++--
tests/qemu-iotests/310 | 116 +++++++++++++++++++++
tests/qemu-iotests/310.out | 15 +++
tests/qemu-iotests/group | 1 +
16 files changed, 608 insertions(+), 130 deletions(-)
create mode 100644 block/copy-on-read.h
create mode 100755 tests/qemu-iotests/310
create mode 100644 tests/qemu-iotests/310.out
--
2.25.4
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v15 01/13] copy-on-read: support preadv/pwritev_part functions
2020-12-16 6:16 [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Vladimir Sementsov-Ogievskiy
@ 2020-12-16 6:16 ` Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 02/13] block: add API function to insert a node Vladimir Sementsov-Ogievskiy
` (12 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-16 6:16 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, qemu-devel, armbru, stefanha, den,
andrey.shinkevich, mreitz, jsnow
From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Add support for the recently introduced functions
bdrv_co_preadv_part()
and
bdrv_co_pwritev_part()
to the COR-filter driver.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/copy-on-read.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 2816e61afe..cb03e0f2d3 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -74,21 +74,25 @@ static int64_t cor_getlength(BlockDriverState *bs)
}
-static int coroutine_fn cor_co_preadv(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov,
+ size_t qiov_offset,
+ int flags)
{
- return bdrv_co_preadv(bs->file, offset, bytes, qiov,
- flags | BDRV_REQ_COPY_ON_READ);
+ return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
+ flags | BDRV_REQ_COPY_ON_READ);
}
-static int coroutine_fn cor_co_pwritev(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+static int coroutine_fn cor_co_pwritev_part(BlockDriverState *bs,
+ uint64_t offset,
+ uint64_t bytes,
+ QEMUIOVector *qiov,
+ size_t qiov_offset, int flags)
{
-
- return bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
+ return bdrv_co_pwritev_part(bs->file, offset, bytes, qiov, qiov_offset,
+ flags);
}
@@ -137,8 +141,8 @@ static BlockDriver bdrv_copy_on_read = {
.bdrv_getlength = cor_getlength,
- .bdrv_co_preadv = cor_co_preadv,
- .bdrv_co_pwritev = cor_co_pwritev,
+ .bdrv_co_preadv_part = cor_co_preadv_part,
+ .bdrv_co_pwritev_part = cor_co_pwritev_part,
.bdrv_co_pwrite_zeroes = cor_co_pwrite_zeroes,
.bdrv_co_pdiscard = cor_co_pdiscard,
.bdrv_co_pwritev_compressed = cor_co_pwritev_compressed,
--
2.25.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v15 02/13] block: add API function to insert a node
2020-12-16 6:16 [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 01/13] copy-on-read: support preadv/pwritev_part functions Vladimir Sementsov-Ogievskiy
@ 2020-12-16 6:16 ` Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 03/13] copy-on-read: add filter drop function Vladimir Sementsov-Ogievskiy
` (11 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-16 6:16 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, qemu-devel, armbru, stefanha, den,
andrey.shinkevich, mreitz, jsnow
From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Provide API for insertion a node to backing chain.
Suggested-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
include/block/block.h | 2 ++
block.c | 25 +++++++++++++++++++++++++
2 files changed, 27 insertions(+)
diff --git a/include/block/block.h b/include/block/block.h
index 5b81e33e94..8ea794959b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -360,6 +360,8 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
Error **errp);
void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
Error **errp);
+BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
+ int flags, Error **errp);
int bdrv_parse_aio(const char *mode, int *flags);
int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);
diff --git a/block.c b/block.c
index 8f177504d4..8ed9c5eaf9 100644
--- a/block.c
+++ b/block.c
@@ -4704,6 +4704,31 @@ static void bdrv_delete(BlockDriverState *bs)
g_free(bs);
}
+BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
+ int flags, Error **errp)
+{
+ BlockDriverState *new_node_bs;
+ Error *local_err = NULL;
+
+ new_node_bs = bdrv_open(NULL, NULL, node_options, flags, errp);
+ if (new_node_bs == NULL) {
+ error_prepend(errp, "Could not create node: ");
+ return NULL;
+ }
+
+ bdrv_drained_begin(bs);
+ bdrv_replace_node(bs, new_node_bs, &local_err);
+ bdrv_drained_end(bs);
+
+ if (local_err) {
+ bdrv_unref(new_node_bs);
+ error_propagate(errp, local_err);
+ return NULL;
+ }
+
+ return new_node_bs;
+}
+
/*
* Run consistency checks on an image
*
--
2.25.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v15 03/13] copy-on-read: add filter drop function
2020-12-16 6:16 [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 01/13] copy-on-read: support preadv/pwritev_part functions Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 02/13] block: add API function to insert a node Vladimir Sementsov-Ogievskiy
@ 2020-12-16 6:16 ` Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 04/13] qapi: add filter-node-name to block-stream Vladimir Sementsov-Ogievskiy
` (10 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-16 6:16 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, qemu-devel, armbru, stefanha, den,
andrey.shinkevich, mreitz, jsnow
From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Provide API for the COR-filter removal. Also, drop the filter child
permissions for an inactive state when the filter node is being
removed.
To insert the filter, the block generic layer function
bdrv_insert_node() can be used.
The new function bdrv_cor_filter_drop() may be considered as an
intermediate solution before the QEMU permission update system has
overhauled. Then we are able to implement the API function
bdrv_remove_node() on the block generic layer.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/copy-on-read.h | 32 +++++++++++++++++++++++++
block/copy-on-read.c | 56 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 88 insertions(+)
create mode 100644 block/copy-on-read.h
diff --git a/block/copy-on-read.h b/block/copy-on-read.h
new file mode 100644
index 0000000000..7bf405dccd
--- /dev/null
+++ b/block/copy-on-read.h
@@ -0,0 +1,32 @@
+/*
+ * Copy-on-read filter block driver
+ *
+ * The filter driver performs Copy-On-Read (COR) operations
+ *
+ * Copyright (c) 2018-2020 Virtuozzo International GmbH.
+ *
+ * Author:
+ * Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef BLOCK_COPY_ON_READ
+#define BLOCK_COPY_ON_READ
+
+#include "block/block_int.h"
+
+void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs);
+
+#endif /* BLOCK_COPY_ON_READ */
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index cb03e0f2d3..618c4c4f43 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -23,11 +23,20 @@
#include "qemu/osdep.h"
#include "block/block_int.h"
#include "qemu/module.h"
+#include "qapi/error.h"
+#include "block/copy-on-read.h"
+
+
+typedef struct BDRVStateCOR {
+ bool active;
+} BDRVStateCOR;
static int cor_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
+ BDRVStateCOR *state = bs->opaque;
+
bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
false, errp);
@@ -42,6 +51,13 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags,
((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
bs->file->bs->supported_zero_flags);
+ state->active = true;
+
+ /*
+ * We don't need to call bdrv_child_refresh_perms() now as the permissions
+ * will be updated later when the filter node gets its parent.
+ */
+
return 0;
}
@@ -57,6 +73,17 @@ static void cor_child_perm(BlockDriverState *bs, BdrvChild *c,
uint64_t perm, uint64_t shared,
uint64_t *nperm, uint64_t *nshared)
{
+ BDRVStateCOR *s = bs->opaque;
+
+ if (!s->active) {
+ /*
+ * While the filter is being removed
+ */
+ *nperm = 0;
+ *nshared = BLK_PERM_ALL;
+ return;
+ }
+
*nperm = perm & PERM_PASSTHROUGH;
*nshared = (shared & PERM_PASSTHROUGH) | PERM_UNCHANGED;
@@ -135,6 +162,7 @@ static void cor_lock_medium(BlockDriverState *bs, bool locked)
static BlockDriver bdrv_copy_on_read = {
.format_name = "copy-on-read",
+ .instance_size = sizeof(BDRVStateCOR),
.bdrv_open = cor_open,
.bdrv_child_perm = cor_child_perm,
@@ -154,6 +182,34 @@ static BlockDriver bdrv_copy_on_read = {
.is_filter = true,
};
+
+void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs)
+{
+ BdrvChild *child;
+ BlockDriverState *bs;
+ BDRVStateCOR *s = cor_filter_bs->opaque;
+
+ child = bdrv_filter_child(cor_filter_bs);
+ if (!child) {
+ return;
+ }
+ bs = child->bs;
+
+ /* Retain the BDS until we complete the graph change. */
+ bdrv_ref(bs);
+ /* Hold a guest back from writing while permissions are being reset. */
+ bdrv_drained_begin(bs);
+ /* Drop permissions before the graph change. */
+ s->active = false;
+ bdrv_child_refresh_perms(cor_filter_bs, child, &error_abort);
+ bdrv_replace_node(cor_filter_bs, bs, &error_abort);
+
+ bdrv_drained_end(bs);
+ bdrv_unref(bs);
+ bdrv_unref(cor_filter_bs);
+}
+
+
static void bdrv_copy_on_read_init(void)
{
bdrv_register(&bdrv_copy_on_read);
--
2.25.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v15 04/13] qapi: add filter-node-name to block-stream
2020-12-16 6:16 [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2020-12-16 6:16 ` [PATCH v15 03/13] copy-on-read: add filter drop function Vladimir Sementsov-Ogievskiy
@ 2020-12-16 6:16 ` Vladimir Sementsov-Ogievskiy
2020-12-22 15:24 ` Max Reitz
2020-12-16 6:16 ` [PATCH v15 05/13] qapi: copy-on-read filter: add 'bottom' option Vladimir Sementsov-Ogievskiy
` (9 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-16 6:16 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, qemu-devel, armbru, stefanha, den,
andrey.shinkevich, mreitz, jsnow
From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Provide the possibility to pass the 'filter-node-name' parameter to the
block-stream job as it is done for the commit block job.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[vsementsov: comment indentation, s/Since: 5.2/Since: 6.0/]
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
qapi/block-core.json | 6 ++++++
include/block/block_int.h | 7 ++++++-
block/monitor/block-hmp-cmds.c | 4 ++--
block/stream.c | 4 +++-
blockdev.c | 4 +++-
5 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 04c5196e59..6050cf3c39 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2543,6 +2543,11 @@
# 'stop' and 'enospc' can only be used if the block device
# supports io-status (see BlockInfo). Since 1.3.
#
+# @filter-node-name: the node name that should be assigned to the
+# filter driver that the stream job inserts into the graph
+# above @device. If this option is not given, a node name is
+# autogenerated. (Since: 6.0)
+#
# @auto-finalize: When false, this job will wait in a PENDING state after it has
# finished its work, waiting for @block-job-finalize before
# making any block graph changes.
@@ -2573,6 +2578,7 @@
'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
'*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
'*on-error': 'BlockdevOnError',
+ '*filter-node-name': 'str',
'*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
##
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1eeafc118c..6f778e2517 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1142,6 +1142,9 @@ int is_windows_drive(const char *filename);
* See @BlockJobCreateFlags
* @speed: The maximum speed, in bytes per second, or 0 for unlimited.
* @on_error: The action to take upon error.
+ * @filter_node_name: The node name that should be assigned to the filter
+ * driver that the commit job inserts into the graph above
+ * @bs. NULL means that a node name should be autogenerated.
* @errp: Error object.
*
* Start a streaming operation on @bs. Clusters that are unallocated
@@ -1154,7 +1157,9 @@ int is_windows_drive(const char *filename);
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
int creation_flags, int64_t speed,
- BlockdevOnError on_error, Error **errp);
+ BlockdevOnError on_error,
+ const char *filter_node_name,
+ Error **errp);
/**
* commit_start:
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index d15a2be827..e8a58f326e 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -508,8 +508,8 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
false, NULL, qdict_haskey(qdict, "speed"), speed, true,
- BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
- &error);
+ BLOCKDEV_ON_ERROR_REPORT, false, NULL, false, false, false,
+ false, &error);
hmp_handle_error(mon, error);
}
diff --git a/block/stream.c b/block/stream.c
index 236384f2f7..6e281c71ac 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -221,7 +221,9 @@ static const BlockJobDriver stream_job_driver = {
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
int creation_flags, int64_t speed,
- BlockdevOnError on_error, Error **errp)
+ BlockdevOnError on_error,
+ const char *filter_node_name,
+ Error **errp)
{
StreamBlockJob *s;
BlockDriverState *iter;
diff --git a/blockdev.c b/blockdev.c
index 412354b4b6..c290cb1dca 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2501,6 +2501,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
bool has_backing_file, const char *backing_file,
bool has_speed, int64_t speed,
bool has_on_error, BlockdevOnError on_error,
+ bool has_filter_node_name, const char *filter_node_name,
bool has_auto_finalize, bool auto_finalize,
bool has_auto_dismiss, bool auto_dismiss,
Error **errp)
@@ -2583,7 +2584,8 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
}
stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
- job_flags, has_speed ? speed : 0, on_error, &local_err);
+ job_flags, has_speed ? speed : 0, on_error,
+ filter_node_name, &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto out;
--
2.25.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v15 05/13] qapi: copy-on-read filter: add 'bottom' option
2020-12-16 6:16 [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2020-12-16 6:16 ` [PATCH v15 04/13] qapi: add filter-node-name to block-stream Vladimir Sementsov-Ogievskiy
@ 2020-12-16 6:16 ` Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 06/13] iotests: add #310 to test bottom node in COR driver Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-16 6:16 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, qemu-devel, armbru, stefanha, den,
andrey.shinkevich, mreitz, jsnow
From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Add an option to limit copy-on-read operations to specified sub-chain
of backing-chain, to make copy-on-read filter useful for block-stream
job.
Suggested-by: Max Reitz <mreitz@redhat.com>
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[vsementsov: change subject, modified to freeze the chain,
do some fixes]
---
qapi/block-core.json | 20 ++++++++-
block/copy-on-read.c | 98 +++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 115 insertions(+), 3 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6050cf3c39..b8094a5ec7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3942,6 +3942,24 @@
'data': { 'throttle-group': 'str',
'file' : 'BlockdevRef'
} }
+
+##
+# @BlockdevOptionsCor:
+#
+# Driver specific block device options for the copy-on-read driver.
+#
+# @bottom: The name of a non-filter node (allocation-bearing layer) that
+# limits the COR operations in the backing chain (inclusive), so
+# that no data below this node will be copied by this filter.
+# If option is absent, the limit is not applied, so that data
+# from all backing layers may be copied.
+#
+# Since: 6.0
+##
+{ 'struct': 'BlockdevOptionsCor',
+ 'base': 'BlockdevOptionsGenericFormat',
+ 'data': { '*bottom': 'str' } }
+
##
# @BlockdevOptions:
#
@@ -3994,7 +4012,7 @@
'bochs': 'BlockdevOptionsGenericFormat',
'cloop': 'BlockdevOptionsGenericFormat',
'compress': 'BlockdevOptionsGenericFormat',
- 'copy-on-read':'BlockdevOptionsGenericFormat',
+ 'copy-on-read':'BlockdevOptionsCor',
'dmg': 'BlockdevOptionsGenericFormat',
'file': 'BlockdevOptionsFile',
'ftp': 'BlockdevOptionsCurlFtp',
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 618c4c4f43..71560984f6 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -24,18 +24,24 @@
#include "block/block_int.h"
#include "qemu/module.h"
#include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
#include "block/copy-on-read.h"
typedef struct BDRVStateCOR {
bool active;
+ BlockDriverState *bottom_bs;
+ bool chain_frozen;
} BDRVStateCOR;
static int cor_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
+ BlockDriverState *bottom_bs = NULL;
BDRVStateCOR *state = bs->opaque;
+ /* Find a bottom node name, if any */
+ const char *bottom_node = qdict_get_try_str(options, "bottom");
bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -51,7 +57,38 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags,
((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
bs->file->bs->supported_zero_flags);
+ if (bottom_node) {
+ bottom_bs = bdrv_find_node(bottom_node);
+ if (!bottom_bs) {
+ error_setg(errp, "Bottom node '%s' not found", bottom_node);
+ qdict_del(options, "bottom");
+ return -EINVAL;
+ }
+ qdict_del(options, "bottom");
+
+ if (!bottom_bs->drv) {
+ error_setg(errp, "Bottom node '%s' not opened", bottom_node);
+ return -EINVAL;
+ }
+
+ if (bottom_bs->drv->is_filter) {
+ error_setg(errp, "Bottom node '%s' is a filter", bottom_node);
+ return -EINVAL;
+ }
+
+ if (bdrv_freeze_backing_chain(bs, bottom_bs, errp) < 0) {
+ return -EINVAL;
+ }
+ state->chain_frozen = true;
+
+ /*
+ * We do freeze the chain, so it shouldn't be removed. Still, storing a
+ * pointer worth bdrv_ref().
+ */
+ bdrv_ref(bottom_bs);
+ }
state->active = true;
+ state->bottom_bs = bottom_bs;
/*
* We don't need to call bdrv_child_refresh_perms() now as the permissions
@@ -107,8 +144,46 @@ static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
size_t qiov_offset,
int flags)
{
- return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
- flags | BDRV_REQ_COPY_ON_READ);
+ int64_t n;
+ int local_flags;
+ int ret;
+ BDRVStateCOR *state = bs->opaque;
+
+ if (!state->bottom_bs) {
+ return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
+ flags | BDRV_REQ_COPY_ON_READ);
+ }
+
+ while (bytes) {
+ local_flags = flags;
+
+ /* In case of failure, try to copy-on-read anyway */
+ ret = bdrv_is_allocated(bs->file->bs, offset, bytes, &n);
+ if (ret <= 0) {
+ ret = bdrv_is_allocated_above(bdrv_backing_chain_next(bs->file->bs),
+ state->bottom_bs, true, offset,
+ n, &n);
+ if (ret > 0 || ret < 0) {
+ local_flags |= BDRV_REQ_COPY_ON_READ;
+ }
+ /* Finish earlier if the end of a backing file has been reached */
+ if (n == 0) {
+ break;
+ }
+ }
+
+ ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset,
+ local_flags);
+ if (ret < 0) {
+ return ret;
+ }
+
+ offset += n;
+ qiov_offset += n;
+ bytes -= n;
+ }
+
+ return 0;
}
@@ -160,11 +235,25 @@ static void cor_lock_medium(BlockDriverState *bs, bool locked)
}
+static void cor_close(BlockDriverState *bs)
+{
+ BDRVStateCOR *s = bs->opaque;
+
+ if (s->chain_frozen) {
+ s->chain_frozen = false;
+ bdrv_unfreeze_backing_chain(bs, s->bottom_bs);
+ }
+
+ bdrv_unref(s->bottom_bs);
+}
+
+
static BlockDriver bdrv_copy_on_read = {
.format_name = "copy-on-read",
.instance_size = sizeof(BDRVStateCOR),
.bdrv_open = cor_open,
+ .bdrv_close = cor_close,
.bdrv_child_perm = cor_child_perm,
.bdrv_getlength = cor_getlength,
@@ -201,6 +290,11 @@ void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs)
bdrv_drained_begin(bs);
/* Drop permissions before the graph change. */
s->active = false;
+ /* unfreeze, as otherwise bdrv_replace_node() will fail */
+ if (s->chain_frozen) {
+ s->chain_frozen = false;
+ bdrv_unfreeze_backing_chain(cor_filter_bs, s->bottom_bs);
+ }
bdrv_child_refresh_perms(cor_filter_bs, child, &error_abort);
bdrv_replace_node(cor_filter_bs, bs, &error_abort);
--
2.25.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v15 06/13] iotests: add #310 to test bottom node in COR driver
2020-12-16 6:16 [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2020-12-16 6:16 ` [PATCH v15 05/13] qapi: copy-on-read filter: add 'bottom' option Vladimir Sementsov-Ogievskiy
@ 2020-12-16 6:16 ` Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 07/13] block: include supported_read_flags into BDS structure Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-16 6:16 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, qemu-devel, armbru, stefanha, den,
andrey.shinkevich, mreitz, jsnow
From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
The test case #310 is similar to #216 by Max Reitz. The difference is
that the test #310 involves a bottom node to the COR filter driver.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[vsementsov: detach backing to test reads from top, limit to qcow2]
---
tests/qemu-iotests/310 | 116 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/310.out | 15 +++++
tests/qemu-iotests/group | 1 +
3 files changed, 132 insertions(+)
create mode 100755 tests/qemu-iotests/310
create mode 100644 tests/qemu-iotests/310.out
diff --git a/tests/qemu-iotests/310 b/tests/qemu-iotests/310
new file mode 100755
index 0000000000..a35e8e14f5
--- /dev/null
+++ b/tests/qemu-iotests/310
@@ -0,0 +1,116 @@
+#!/usr/bin/env python3
+#
+# Copy-on-read tests using a COR filter with a bottom node
+#
+# Copyright (C) 2018 Red Hat, Inc.
+# Copyright (c) 2020 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import iotests
+from iotests import log, qemu_img, qemu_io_silent
+
+# Need backing file support
+iotests.script_initialize(supported_fmts=['qcow2'],
+ supported_platforms=['linux'])
+
+log('')
+log('=== Copy-on-read across nodes ===')
+log('')
+
+# This test is similar to the 216 one by Max Reitz <mreitz@redhat.com>
+# The difference is that this test case involves a bottom node to the
+# COR filter driver.
+
+with iotests.FilePath('base.img') as base_img_path, \
+ iotests.FilePath('mid.img') as mid_img_path, \
+ iotests.FilePath('top.img') as top_img_path, \
+ iotests.VM() as vm:
+
+ log('--- Setting up images ---')
+ log('')
+
+ assert qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M') == 0
+ assert qemu_io_silent(base_img_path, '-c', 'write -P 1 0M 1M') == 0
+ assert qemu_io_silent(base_img_path, '-c', 'write -P 1 3M 1M') == 0
+ assert qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
+ '-F', iotests.imgfmt, mid_img_path) == 0
+ assert qemu_io_silent(mid_img_path, '-c', 'write -P 3 2M 1M') == 0
+ assert qemu_io_silent(mid_img_path, '-c', 'write -P 3 4M 1M') == 0
+ assert qemu_img('create', '-f', iotests.imgfmt, '-b', mid_img_path,
+ '-F', iotests.imgfmt, top_img_path) == 0
+ assert qemu_io_silent(top_img_path, '-c', 'write -P 2 1M 1M') == 0
+
+# 0 1 2 3 4
+# top 2
+# mid 3 3
+# base 1 1
+
+ log('Done')
+
+ log('')
+ log('--- Doing COR ---')
+ log('')
+
+ vm.launch()
+
+ log(vm.qmp('blockdev-add',
+ node_name='node0',
+ driver='copy-on-read',
+ bottom='node2',
+ file={
+ 'driver': iotests.imgfmt,
+ 'file': {
+ 'driver': 'file',
+ 'filename': top_img_path
+ },
+ 'backing': {
+ 'node-name': 'node2',
+ 'driver': iotests.imgfmt,
+ 'file': {
+ 'driver': 'file',
+ 'filename': mid_img_path
+ },
+ 'backing': {
+ 'driver': iotests.imgfmt,
+ 'file': {
+ 'driver': 'file',
+ 'filename': base_img_path
+ }
+ },
+ }
+ }))
+
+ # Trigger COR
+ log(vm.qmp('human-monitor-command',
+ command_line='qemu-io node0 "read 0 5M"'))
+
+ vm.shutdown()
+
+ log('')
+ log('--- Checking COR result ---')
+ log('')
+
+ # Detach backing to check that we can read the data from the top level now
+ assert qemu_img('rebase', '-u', '-b', '', '-f', iotests.imgfmt,
+ top_img_path,) == 0
+
+ assert qemu_io_silent(top_img_path, '-c', 'read -P 0 0 1M') == 0
+ assert qemu_io_silent(top_img_path, '-c', 'read -P 2 1M 1M') == 0
+ assert qemu_io_silent(top_img_path, '-c', 'read -P 3 2M 1M') == 0
+ assert qemu_io_silent(top_img_path, '-c', 'read -P 0 3M 1M') == 0
+ assert qemu_io_silent(top_img_path, '-c', 'read -P 3 4M 1M') == 0
+
+ log('Done')
diff --git a/tests/qemu-iotests/310.out b/tests/qemu-iotests/310.out
new file mode 100644
index 0000000000..a70aa5cdae
--- /dev/null
+++ b/tests/qemu-iotests/310.out
@@ -0,0 +1,15 @@
+
+=== Copy-on-read across nodes ===
+
+--- Setting up images ---
+
+Done
+
+--- Doing COR ---
+
+{"return": {}}
+{"return": ""}
+
+--- Checking COR result ---
+
+Done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 9a8394b4cd..9fa72cf442 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -317,3 +317,4 @@
307 rw quick export
308 rw
309 rw auto quick
+310 rw quick
--
2.25.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v15 07/13] block: include supported_read_flags into BDS structure
2020-12-16 6:16 [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2020-12-16 6:16 ` [PATCH v15 06/13] iotests: add #310 to test bottom node in COR driver Vladimir Sementsov-Ogievskiy
@ 2020-12-16 6:16 ` Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 08/13] copy-on-read: skip non-guest reads if no copy needed Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-16 6:16 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, qemu-devel, armbru, stefanha, den,
andrey.shinkevich, mreitz, jsnow
From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Add the new member supported_read_flags to the BlockDriverState
structure. It will control the flags set for copy-on-read operations.
Make the block generic layer evaluate supported read flags before they
go to a block driver.
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[vsementsov: use assert instead of abort]
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
include/block/block_int.h | 4 ++++
block/io.c | 10 ++++++++--
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 6f778e2517..1f56443440 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -881,6 +881,10 @@ struct BlockDriverState {
/* I/O Limits */
BlockLimits bl;
+ /*
+ * Flags honored during pread
+ */
+ unsigned int supported_read_flags;
/* Flags honored during pwrite (so far: BDRV_REQ_FUA,
* BDRV_REQ_WRITE_UNCHANGED).
* If a driver does not support BDRV_REQ_WRITE_UNCHANGED, those
diff --git a/block/io.c b/block/io.c
index 24205f5168..851fe53604 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1431,6 +1431,9 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
if (flags & BDRV_REQ_COPY_ON_READ) {
int64_t pnum;
+ /* The flag BDRV_REQ_COPY_ON_READ has reached its addressee */
+ flags &= ~BDRV_REQ_COPY_ON_READ;
+
ret = bdrv_is_allocated(bs, offset, bytes, &pnum);
if (ret < 0) {
goto out;
@@ -1452,9 +1455,11 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
goto out;
}
+ assert(!(flags & ~bs->supported_read_flags));
+
max_bytes = ROUND_UP(MAX(0, total_bytes - offset), align);
if (bytes <= max_bytes && bytes <= max_transfer) {
- ret = bdrv_driver_preadv(bs, offset, bytes, qiov, qiov_offset, 0);
+ ret = bdrv_driver_preadv(bs, offset, bytes, qiov, qiov_offset, flags);
goto out;
}
@@ -1467,7 +1472,8 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
ret = bdrv_driver_preadv(bs, offset + bytes - bytes_remaining,
num, qiov,
- qiov_offset + bytes - bytes_remaining, 0);
+ qiov_offset + bytes - bytes_remaining,
+ flags);
max_bytes -= num;
} else {
num = bytes_remaining;
--
2.25.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v15 08/13] copy-on-read: skip non-guest reads if no copy needed
2020-12-16 6:16 [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2020-12-16 6:16 ` [PATCH v15 07/13] block: include supported_read_flags into BDS structure Vladimir Sementsov-Ogievskiy
@ 2020-12-16 6:16 ` Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 09/13] stream: rework backing-file changing Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-16 6:16 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, qemu-devel, armbru, stefanha, den,
andrey.shinkevich, mreitz, jsnow
From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
If the flag BDRV_REQ_PREFETCH was set, skip idling read/write
operations in COR-driver. It can be taken into account for the
COR-algorithms optimization. That check is being made during the
block stream job by the moment.
Add the BDRV_REQ_PREFETCH flag to the supported_read_flags of the
COR-filter.
block: Modify the comment for the flag BDRV_REQ_PREFETCH as we are
going to use it alone and pass it to the COR-filter driver for further
processing.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
include/block/block.h | 8 +++++---
block/copy-on-read.c | 14 ++++++++++----
2 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 8ea794959b..f652f31406 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -81,9 +81,11 @@ typedef enum {
BDRV_REQ_NO_FALLBACK = 0x100,
/*
- * BDRV_REQ_PREFETCH may be used only together with BDRV_REQ_COPY_ON_READ
- * on read request and means that caller doesn't really need data to be
- * written to qiov parameter which may be NULL.
+ * BDRV_REQ_PREFETCH makes sense only in the context of copy-on-read
+ * (i.e., together with the BDRV_REQ_COPY_ON_READ flag or when a COR
+ * filter is involved), in which case it signals that the COR operation
+ * need not read the data into memory (qiov) but only ensure they are
+ * copied to the top layer (i.e., that COR operation is done).
*/
BDRV_REQ_PREFETCH = 0x200,
/* Mask of valid flags */
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 71560984f6..9cad9e1b8c 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -50,6 +50,8 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags,
return -EINVAL;
}
+ bs->supported_read_flags = BDRV_REQ_PREFETCH;
+
bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
(BDRV_REQ_FUA & bs->file->bs->supported_write_flags);
@@ -172,10 +174,14 @@ static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
}
}
- ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset,
- local_flags);
- if (ret < 0) {
- return ret;
+ /* Skip if neither read nor write are needed */
+ if ((local_flags & (BDRV_REQ_PREFETCH | BDRV_REQ_COPY_ON_READ)) !=
+ BDRV_REQ_PREFETCH) {
+ ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset,
+ local_flags);
+ if (ret < 0) {
+ return ret;
+ }
}
offset += n;
--
2.25.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v15 09/13] stream: rework backing-file changing
2020-12-16 6:16 [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Vladimir Sementsov-Ogievskiy
` (7 preceding siblings ...)
2020-12-16 6:16 ` [PATCH v15 08/13] copy-on-read: skip non-guest reads if no copy needed Vladimir Sementsov-Ogievskiy
@ 2020-12-16 6:16 ` Vladimir Sementsov-Ogievskiy
2020-12-22 15:59 ` Max Reitz
2020-12-16 6:17 ` [PATCH v15 10/13] qapi: block-stream: add "bottom" argument Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-16 6:16 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, qemu-devel, armbru, stefanha, den,
andrey.shinkevich, mreitz, jsnow
From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Stream in stream_prepare calls bdrv_change_backing_file() to change
backing-file in the metadata of bs.
It may use either backing-file parameter given by user or just take
filename of base on job start.
Backing file format is determined by base on job finish.
There are some problems with this design, we solve only two by this
patch:
1. Consider scenario with backing-file unset. Current concept of stream
supports changing of the base during the job (we don't freeze link to
the base). So, we should not save base filename at job start,
- let's determine name of the base on job finish.
2. Using direct base to determine filename and format is not very good:
base node may be a filter, so its filename may be JSON, and format_name
is not good for storing into qcow2 metadata as backing file format.
- let's use unfiltered_base
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[vsementsov: change commit subject, change logic in stream_prepare]
---
block/stream.c | 9 +++++----
blockdev.c | 8 +-------
2 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 6e281c71ac..6a525a5edf 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
BlockDriverState *bs = blk_bs(bjob->blk);
BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
+ BlockDriverState *unfiltered_base = bdrv_skip_filters(base);
Error *local_err = NULL;
int ret = 0;
@@ -73,10 +74,10 @@ static int stream_prepare(Job *job)
if (bdrv_cow_child(unfiltered_bs)) {
const char *base_id = NULL, *base_fmt = NULL;
- if (base) {
- base_id = s->backing_file_str;
- if (base->drv) {
- base_fmt = base->drv->format_name;
+ if (unfiltered_base) {
+ base_id = s->backing_file_str ?: unfiltered_base->filename;
+ if (unfiltered_base->drv) {
+ base_fmt = unfiltered_base->drv->format_name;
}
}
bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
diff --git a/blockdev.c b/blockdev.c
index c290cb1dca..b58f36fc31 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2510,7 +2510,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
BlockDriverState *base_bs = NULL;
AioContext *aio_context;
Error *local_err = NULL;
- const char *base_name = NULL;
int job_flags = JOB_DEFAULT;
if (!has_on_error) {
@@ -2538,7 +2537,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
goto out;
}
assert(bdrv_get_aio_context(base_bs) == aio_context);
- base_name = base;
}
if (has_base_node) {
@@ -2553,7 +2551,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
}
assert(bdrv_get_aio_context(base_bs) == aio_context);
bdrv_refresh_filename(base_bs);
- base_name = base_bs->filename;
}
/* Check for op blockers in the whole chain between bs and base */
@@ -2573,9 +2570,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
goto out;
}
- /* backing_file string overrides base bs filename */
- base_name = has_backing_file ? backing_file : base_name;
-
if (has_auto_finalize && !auto_finalize) {
job_flags |= JOB_MANUAL_FINALIZE;
}
@@ -2583,7 +2577,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
job_flags |= JOB_MANUAL_DISMISS;
}
- stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
+ stream_start(has_job_id ? job_id : NULL, bs, base_bs, backing_file,
job_flags, has_speed ? speed : 0, on_error,
filter_node_name, &local_err);
if (local_err) {
--
2.25.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v15 10/13] qapi: block-stream: add "bottom" argument
2020-12-16 6:16 [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Vladimir Sementsov-Ogievskiy
` (8 preceding siblings ...)
2020-12-16 6:16 ` [PATCH v15 09/13] stream: rework backing-file changing Vladimir Sementsov-Ogievskiy
@ 2020-12-16 6:17 ` Vladimir Sementsov-Ogievskiy
2020-12-22 16:07 ` Max Reitz
2020-12-16 6:17 ` [PATCH v15 11/13] iotests: 30: prepare to COR filter insertion by stream job Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-16 6:17 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, qemu-devel, armbru, stefanha, den,
andrey.shinkevich, mreitz, jsnow
The code already don't freeze base node and we try to make it prepared
for the situation when base node is changed during the operation. In
other words, block-stream doesn't own base node.
Let's introduce a new interface which should replace the current one,
which will in better relations with the code. Specifying bottom node
instead of base, and requiring it to be non-filter gives us the
following benefits:
- drop difference between above_base and base_overlay, which will be
renamed to just bottom, when old interface dropped
- clean way to work with parallel streams/commits on the same backing
chain, which otherwise become a problem when we introduce a filter
for stream job
- cleaner interface. Nobody will surprised the fact that base node may
disappear during block-stream, when there is no word about "base" in
the interface.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
qapi/block-core.json | 12 ++++---
include/block/block_int.h | 1 +
block/monitor/block-hmp-cmds.c | 3 +-
block/stream.c | 50 +++++++++++++++++++---------
blockdev.c | 59 ++++++++++++++++++++++++++++------
5 files changed, 94 insertions(+), 31 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b8094a5ec7..cb0066fd5c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2517,10 +2517,14 @@
# @device: the device or node name of the top image
#
# @base: the common backing file name.
-# It cannot be set if @base-node is also set.
+# It cannot be set if @base-node or @bottom is also set.
#
# @base-node: the node name of the backing file.
-# It cannot be set if @base is also set. (Since 2.8)
+# It cannot be set if @base or @bottom is also set. (Since 2.8)
+#
+# @bottom: the last node in the chain that should be streamed into
+# top. It cannot be set if @base or @base-node is also set.
+# It cannot be filter node. (Since 6.0)
#
# @backing-file: The backing file string to write into the top
# image. This filename is not validated.
@@ -2576,8 +2580,8 @@
##
{ 'command': 'block-stream',
'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
- '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
- '*on-error': 'BlockdevOnError',
+ '*base-node': 'str', '*backing-file': 'str', '*bottom': 'str',
+ '*speed': 'int', '*on-error': 'BlockdevOnError',
'*filter-node-name': 'str',
'*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1f56443440..4b8aa61fb4 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1160,6 +1160,7 @@ int is_windows_drive(const char *filename);
*/
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
+ BlockDriverState *bottom,
int creation_flags, int64_t speed,
BlockdevOnError on_error,
const char *filter_node_name,
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index e8a58f326e..afd75ab628 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -507,7 +507,8 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
int64_t speed = qdict_get_try_int(qdict, "speed", 0);
qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
- false, NULL, qdict_haskey(qdict, "speed"), speed, true,
+ false, NULL, false, NULL,
+ qdict_haskey(qdict, "speed"), speed, true,
BLOCKDEV_ON_ERROR_REPORT, false, NULL, false, false, false,
false, &error);
diff --git a/block/stream.c b/block/stream.c
index 6a525a5edf..045d6bc76b 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -221,6 +221,7 @@ static const BlockJobDriver stream_job_driver = {
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
+ BlockDriverState *bottom,
int creation_flags, int64_t speed,
BlockdevOnError on_error,
const char *filter_node_name,
@@ -230,25 +231,42 @@ void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *iter;
bool bs_read_only;
int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
- BlockDriverState *base_overlay = bdrv_find_overlay(bs, base);
+ BlockDriverState *base_overlay;
BlockDriverState *above_base;
- if (!base_overlay) {
- error_setg(errp, "'%s' is not in the backing chain of '%s'",
- base->node_name, bs->node_name);
- return;
- }
+ assert(!(base && bottom));
+ assert(!(backing_file_str && bottom));
+
+ if (bottom) {
+ /*
+ * New simple interface. The code is written in terms of old interface
+ * with @base parameter (still, it doesn't freeze link to base, so in
+ * this mean old code is correct for new interface). So, for now, just
+ * emulate base_overlay and above_base. Still, when old interface
+ * finally removed, we should refactor code to use only "bottom", but
+ * not "*base*" things.
+ */
+ assert(!bottom->drv->is_filter);
+ base_overlay = above_base = bottom;
+ } else {
+ base_overlay = bdrv_find_overlay(bs, base);
+ if (!base_overlay) {
+ error_setg(errp, "'%s' is not in the backing chain of '%s'",
+ base->node_name, bs->node_name);
+ return;
+ }
- /*
- * Find the node directly above @base. @base_overlay is a COW overlay, so
- * it must have a bdrv_cow_child(), but it is the immediate overlay of
- * @base, so between the two there can only be filters.
- */
- above_base = base_overlay;
- if (bdrv_cow_bs(above_base) != base) {
- above_base = bdrv_cow_bs(above_base);
- while (bdrv_filter_bs(above_base) != base) {
- above_base = bdrv_filter_bs(above_base);
+ /*
+ * Find the node directly above @base. @base_overlay is a COW overlay,
+ * so it must have a bdrv_cow_child(), but it is the immediate overlay
+ * of @base, so between the two there can only be filters.
+ */
+ above_base = base_overlay;
+ if (bdrv_cow_bs(above_base) != base) {
+ above_base = bdrv_cow_bs(above_base);
+ while (bdrv_filter_bs(above_base) != base) {
+ above_base = bdrv_filter_bs(above_base);
+ }
}
}
diff --git a/blockdev.c b/blockdev.c
index b58f36fc31..18699d3cda 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2499,6 +2499,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
bool has_base, const char *base,
bool has_base_node, const char *base_node,
bool has_backing_file, const char *backing_file,
+ bool has_bottom, const char *bottom,
bool has_speed, int64_t speed,
bool has_on_error, BlockdevOnError on_error,
bool has_filter_node_name, const char *filter_node_name,
@@ -2506,12 +2507,31 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
bool has_auto_dismiss, bool auto_dismiss,
Error **errp)
{
- BlockDriverState *bs, *iter;
+ BlockDriverState *bs, *iter, *iter_end;
BlockDriverState *base_bs = NULL;
+ BlockDriverState *bottom_bs = NULL;
AioContext *aio_context;
Error *local_err = NULL;
int job_flags = JOB_DEFAULT;
+ if (has_base && has_base_node) {
+ error_setg(errp, "'base' and 'base-node' cannot be specified "
+ "at the same time");
+ return;
+ }
+
+ if (has_base && has_bottom) {
+ error_setg(errp, "'base' and 'bottom' cannot be specified "
+ "at the same time");
+ return;
+ }
+
+ if (has_bottom && has_base_node) {
+ error_setg(errp, "'bottom' and 'base-node' cannot be specified "
+ "at the same time");
+ return;
+ }
+
if (!has_on_error) {
on_error = BLOCKDEV_ON_ERROR_REPORT;
}
@@ -2524,12 +2544,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
- if (has_base && has_base_node) {
- error_setg(errp, "'base' and 'base-node' cannot be specified "
- "at the same time");
- goto out;
- }
-
if (has_base) {
base_bs = bdrv_find_backing_image(bs, base);
if (base_bs == NULL) {
@@ -2553,8 +2567,33 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
bdrv_refresh_filename(base_bs);
}
- /* Check for op blockers in the whole chain between bs and base */
- for (iter = bs; iter && iter != base_bs;
+ if (has_bottom) {
+ bottom_bs = bdrv_lookup_bs(NULL, bottom, errp);
+ if (!bottom_bs) {
+ goto out;
+ }
+ if (!bottom_bs->drv) {
+ error_setg(errp, "Node '%s' is not open", bottom);
+ goto out;
+ }
+ if (bottom_bs->drv->is_filter) {
+ error_setg(errp, "Node '%s' is a filter, use a non-filter node "
+ "as 'bottom'", bottom);
+ goto out;
+ }
+ if (!bdrv_chain_contains(bs, bottom_bs)) {
+ error_setg(errp, "Node '%s' is not in a chain starting from '%s'",
+ bottom, device);
+ goto out;
+ }
+ assert(bdrv_get_aio_context(bottom_bs) == aio_context);
+ }
+
+ /*
+ * Check for op blockers in the whole chain between bs and base (or bottom)
+ */
+ iter_end = has_bottom ? bdrv_filter_or_cow_bs(bottom_bs) : base_bs;
+ for (iter = bs; iter && iter != iter_end;
iter = bdrv_filter_or_cow_bs(iter))
{
if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
@@ -2578,7 +2617,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
}
stream_start(has_job_id ? job_id : NULL, bs, base_bs, backing_file,
- job_flags, has_speed ? speed : 0, on_error,
+ bottom_bs, job_flags, has_speed ? speed : 0, on_error,
filter_node_name, &local_err);
if (local_err) {
error_propagate(errp, local_err);
--
2.25.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v15 11/13] iotests: 30: prepare to COR filter insertion by stream job
2020-12-16 6:16 [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Vladimir Sementsov-Ogievskiy
` (9 preceding siblings ...)
2020-12-16 6:17 ` [PATCH v15 10/13] qapi: block-stream: add "bottom" argument Vladimir Sementsov-Ogievskiy
@ 2020-12-16 6:17 ` Vladimir Sementsov-Ogievskiy
2020-12-16 6:17 ` [PATCH v15 12/13] block/stream: add s->target_bs Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-16 6:17 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, qemu-devel, armbru, stefanha, den,
andrey.shinkevich, mreitz, jsnow
test_stream_parallel run parallel stream jobs, intersecting so that top
of one is base of another. It's OK now, but it would be a problem if
insert the filter, as one job will want to use another job's filter as
above_base node.
Correct thing to do is move to new interface: "bottom" argument instead
of base. This guarantees that jobs don't intersect by their actions.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/030 | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index dcb4b5d6a6..bd8cf9cff7 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -245,7 +245,9 @@ class TestParallelOps(iotests.QMPTestCase):
node_name = 'node%d' % i
job_id = 'stream-%s' % node_name
pending_jobs.append(job_id)
- result = self.vm.qmp('block-stream', device=node_name, job_id=job_id, base=self.imgs[i-2], speed=1024)
+ result = self.vm.qmp('block-stream', device=node_name,
+ job_id=job_id, bottom=f'node{i-1}',
+ speed=1024)
self.assert_qmp(result, 'return', {})
for job in pending_jobs:
--
2.25.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v15 12/13] block/stream: add s->target_bs
2020-12-16 6:16 [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Vladimir Sementsov-Ogievskiy
` (10 preceding siblings ...)
2020-12-16 6:17 ` [PATCH v15 11/13] iotests: 30: prepare to COR filter insertion by stream job Vladimir Sementsov-Ogievskiy
@ 2020-12-16 6:17 ` Vladimir Sementsov-Ogievskiy
2020-12-16 6:17 ` [PATCH v15 13/13] block: apply COR-filter to block-stream jobs Vladimir Sementsov-Ogievskiy
2021-01-05 16:08 ` [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Max Reitz
13 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-16 6:17 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, qemu-devel, armbru, stefanha, den,
andrey.shinkevich, mreitz, jsnow
Add a direct link to target bs for convenience and to simplify
following commit which will insert COR filter above target bs.
This is a part of original commit written by Andrey.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/stream.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 045d6bc76b..626dfa2b22 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -33,6 +33,7 @@ typedef struct StreamBlockJob {
BlockJob common;
BlockDriverState *base_overlay; /* COW overlay (stream from this) */
BlockDriverState *above_base; /* Node directly above the base */
+ BlockDriverState *target_bs;
BlockdevOnError on_error;
char *backing_file_str;
bool bs_read_only;
@@ -53,23 +54,20 @@ static void stream_abort(Job *job)
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
if (s->chain_frozen) {
- BlockJob *bjob = &s->common;
- bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->above_base);
+ bdrv_unfreeze_backing_chain(s->target_bs, s->above_base);
}
}
static int stream_prepare(Job *job)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
- BlockJob *bjob = &s->common;
- BlockDriverState *bs = blk_bs(bjob->blk);
- BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
+ BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
BlockDriverState *unfiltered_base = bdrv_skip_filters(base);
Error *local_err = NULL;
int ret = 0;
- bdrv_unfreeze_backing_chain(bs, s->above_base);
+ bdrv_unfreeze_backing_chain(s->target_bs, s->above_base);
s->chain_frozen = false;
if (bdrv_cow_child(unfiltered_bs)) {
@@ -95,13 +93,12 @@ static void stream_clean(Job *job)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockJob *bjob = &s->common;
- BlockDriverState *bs = blk_bs(bjob->blk);
/* Reopen the image back in read-only mode if necessary */
if (s->bs_read_only) {
/* Give up write permissions before making it read-only */
blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
- bdrv_reopen_set_read_only(bs, true, NULL);
+ bdrv_reopen_set_read_only(s->target_bs, true, NULL);
}
g_free(s->backing_file_str);
@@ -111,8 +108,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockBackend *blk = s->common.blk;
- BlockDriverState *bs = blk_bs(blk);
- BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
+ BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
bool enable_cor = !bdrv_cow_child(s->base_overlay);
int64_t len;
int64_t offset = 0;
@@ -125,7 +121,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
return 0;
}
- len = bdrv_getlength(bs);
+ len = bdrv_getlength(s->target_bs);
if (len < 0) {
return len;
}
@@ -137,7 +133,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
* account.
*/
if (enable_cor) {
- bdrv_enable_copy_on_read(bs);
+ bdrv_enable_copy_on_read(s->target_bs);
}
for ( ; offset < len; offset += n) {
@@ -199,7 +195,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
}
if (enable_cor) {
- bdrv_disable_copy_on_read(bs);
+ bdrv_disable_copy_on_read(s->target_bs);
}
/* Do not remove the backing file if an error was there but ignored. */
@@ -314,6 +310,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
s->base_overlay = base_overlay;
s->above_base = above_base;
s->backing_file_str = g_strdup(backing_file_str);
+ s->target_bs = bs;
s->bs_read_only = bs_read_only;
s->chain_frozen = true;
--
2.25.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v15 13/13] block: apply COR-filter to block-stream jobs
2020-12-16 6:16 [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Vladimir Sementsov-Ogievskiy
` (11 preceding siblings ...)
2020-12-16 6:17 ` [PATCH v15 12/13] block/stream: add s->target_bs Vladimir Sementsov-Ogievskiy
@ 2020-12-16 6:17 ` Vladimir Sementsov-Ogievskiy
2020-12-22 16:20 ` Max Reitz
2021-01-05 15:30 ` Max Reitz
2021-01-05 16:08 ` [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Max Reitz
13 siblings, 2 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-16 6:17 UTC (permalink / raw)
To: qemu-block
Cc: fam, kwolf, vsementsov, qemu-devel, armbru, stefanha, den,
andrey.shinkevich, mreitz, jsnow
From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
This patch completes the series with the COR-filter applied to
block-stream operations.
Adding the filter makes it possible in future implement discarding
copied regions in backing files during the block-stream job, to reduce
the disk overuse (we need control on permissions).
Also, the filter now is smart enough to do copy-on-read with specified
base, so we have benefit on guest reads even when doing block-stream of
the part of the backing chain.
Several iotests are slightly modified due to filter insertion.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/stream.c | 105 ++++++++++++++++++++++---------------
tests/qemu-iotests/030 | 8 +--
tests/qemu-iotests/141.out | 2 +-
tests/qemu-iotests/245 | 20 ++++---
4 files changed, 80 insertions(+), 55 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 626dfa2b22..1fa742b0db 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -17,8 +17,10 @@
#include "block/blockjob_int.h"
#include "qapi/error.h"
#include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qdict.h"
#include "qemu/ratelimit.h"
#include "sysemu/block-backend.h"
+#include "block/copy-on-read.h"
enum {
/*
@@ -33,11 +35,11 @@ typedef struct StreamBlockJob {
BlockJob common;
BlockDriverState *base_overlay; /* COW overlay (stream from this) */
BlockDriverState *above_base; /* Node directly above the base */
+ BlockDriverState *cor_filter_bs;
BlockDriverState *target_bs;
BlockdevOnError on_error;
char *backing_file_str;
bool bs_read_only;
- bool chain_frozen;
} StreamBlockJob;
static int coroutine_fn stream_populate(BlockBackend *blk,
@@ -45,17 +47,7 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
{
assert(bytes < SIZE_MAX);
- return blk_co_preadv(blk, offset, bytes, NULL,
- BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH);
-}
-
-static void stream_abort(Job *job)
-{
- StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
-
- if (s->chain_frozen) {
- bdrv_unfreeze_backing_chain(s->target_bs, s->above_base);
- }
+ return blk_co_preadv(blk, offset, bytes, NULL, BDRV_REQ_PREFETCH);
}
static int stream_prepare(Job *job)
@@ -67,8 +59,9 @@ static int stream_prepare(Job *job)
Error *local_err = NULL;
int ret = 0;
- bdrv_unfreeze_backing_chain(s->target_bs, s->above_base);
- s->chain_frozen = false;
+ /* We should drop filter at this point, as filter hold the backing chain */
+ bdrv_cor_filter_drop(s->cor_filter_bs);
+ s->cor_filter_bs = NULL;
if (bdrv_cow_child(unfiltered_bs)) {
const char *base_id = NULL, *base_fmt = NULL;
@@ -94,6 +87,11 @@ static void stream_clean(Job *job)
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockJob *bjob = &s->common;
+ if (s->cor_filter_bs) {
+ bdrv_cor_filter_drop(s->cor_filter_bs);
+ s->cor_filter_bs = NULL;
+ }
+
/* Reopen the image back in read-only mode if necessary */
if (s->bs_read_only) {
/* Give up write permissions before making it read-only */
@@ -109,7 +107,6 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockBackend *blk = s->common.blk;
BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
- bool enable_cor = !bdrv_cow_child(s->base_overlay);
int64_t len;
int64_t offset = 0;
uint64_t delay_ns = 0;
@@ -127,15 +124,6 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
}
job_progress_set_remaining(&s->common.job, len);
- /* Turn on copy-on-read for the whole block device so that guest read
- * requests help us make progress. Only do this when copying the entire
- * backing chain since the copy-on-read operation does not take base into
- * account.
- */
- if (enable_cor) {
- bdrv_enable_copy_on_read(s->target_bs);
- }
-
for ( ; offset < len; offset += n) {
bool copy;
int ret;
@@ -194,10 +182,6 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
}
}
- if (enable_cor) {
- bdrv_disable_copy_on_read(s->target_bs);
- }
-
/* Do not remove the backing file if an error was there but ignored. */
return error;
}
@@ -209,7 +193,6 @@ static const BlockJobDriver stream_job_driver = {
.free = block_job_free,
.run = stream_run,
.prepare = stream_prepare,
- .abort = stream_abort,
.clean = stream_clean,
.user_resume = block_job_user_resume,
},
@@ -228,7 +211,9 @@ void stream_start(const char *job_id, BlockDriverState *bs,
bool bs_read_only;
int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
BlockDriverState *base_overlay;
+ BlockDriverState *cor_filter_bs = NULL;
BlockDriverState *above_base;
+ QDict *opts;
assert(!(base && bottom));
assert(!(backing_file_str && bottom));
@@ -266,30 +251,62 @@ void stream_start(const char *job_id, BlockDriverState *bs,
}
}
- if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
- return;
- }
-
/* Make sure that the image is opened in read-write mode */
bs_read_only = bdrv_is_read_only(bs);
if (bs_read_only) {
- if (bdrv_reopen_set_read_only(bs, false, errp) != 0) {
- bs_read_only = false;
- goto fail;
+ int ret;
+ /* Hold the chain during reopen */
+ if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
+ return;
+ }
+
+ ret = bdrv_reopen_set_read_only(bs, false, errp);
+
+ /* failure, or cor-filter will hold the chain */
+ bdrv_unfreeze_backing_chain(bs, above_base);
+
+ if (ret < 0) {
+ return;
}
}
- /* Prevent concurrent jobs trying to modify the graph structure here, we
- * already have our own plans. Also don't allow resize as the image size is
- * queried only at the job start and then cached. */
- s = block_job_create(job_id, &stream_job_driver, NULL, bs,
- basic_flags | BLK_PERM_GRAPH_MOD,
+ opts = qdict_new();
+
+ qdict_put_str(opts, "driver", "copy-on-read");
+ qdict_put_str(opts, "file", bdrv_get_node_name(bs));
+ /* Pass the base_overlay node name as 'bottom' to COR driver */
+ qdict_put_str(opts, "bottom", base_overlay->node_name);
+ if (filter_node_name) {
+ qdict_put_str(opts, "node-name", filter_node_name);
+ }
+
+ cor_filter_bs = bdrv_insert_node(bs, opts, BDRV_O_RDWR, errp);
+ if (!cor_filter_bs) {
+ goto fail;
+ }
+
+ if (!filter_node_name) {
+ cor_filter_bs->implicit = true;
+ }
+
+ s = block_job_create(job_id, &stream_job_driver, NULL, cor_filter_bs,
+ BLK_PERM_CONSISTENT_READ,
basic_flags | BLK_PERM_WRITE,
speed, creation_flags, NULL, NULL, errp);
if (!s) {
goto fail;
}
+ /*
+ * Prevent concurrent jobs trying to modify the graph structure here, we
+ * already have our own plans. Also don't allow resize as the image size is
+ * queried only at the job start and then cached.
+ */
+ if (block_job_add_bdrv(&s->common, "active node", bs, 0,
+ basic_flags | BLK_PERM_WRITE, &error_abort)) {
+ goto fail;
+ }
+
/* Block all intermediate nodes between bs and base, because they will
* disappear from the chain after this operation. The streaming job reads
* every block only once, assuming that it doesn't change, so forbid writes
@@ -310,9 +327,9 @@ void stream_start(const char *job_id, BlockDriverState *bs,
s->base_overlay = base_overlay;
s->above_base = above_base;
s->backing_file_str = g_strdup(backing_file_str);
+ s->cor_filter_bs = cor_filter_bs;
s->target_bs = bs;
s->bs_read_only = bs_read_only;
- s->chain_frozen = true;
s->on_error = on_error;
trace_stream_start(bs, base, s);
@@ -320,8 +337,10 @@ void stream_start(const char *job_id, BlockDriverState *bs,
return;
fail:
+ if (cor_filter_bs) {
+ bdrv_cor_filter_drop(cor_filter_bs);
+ }
if (bs_read_only) {
bdrv_reopen_set_read_only(bs, true, NULL);
}
- bdrv_unfreeze_backing_chain(bs, above_base);
}
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index bd8cf9cff7..c576d55d07 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -278,12 +278,14 @@ class TestParallelOps(iotests.QMPTestCase):
self.assert_no_active_block_jobs()
# Set a speed limit to make sure that this job blocks the rest
- result = self.vm.qmp('block-stream', device='node4', job_id='stream-node4', base=self.imgs[1], speed=1024*1024)
+ result = self.vm.qmp('block-stream', device='node4',
+ job_id='stream-node4', base=self.imgs[1],
+ filter_node_name='stream-filter', speed=1024*1024)
self.assert_qmp(result, 'return', {})
result = self.vm.qmp('block-stream', device='node5', job_id='stream-node5', base=self.imgs[2])
self.assert_qmp(result, 'error/desc',
- "Node 'node4' is busy: block device is in use by block job: stream")
+ "Node 'stream-filter' is busy: block device is in use by block job: stream")
result = self.vm.qmp('block-stream', device='node3', job_id='stream-node3', base=self.imgs[2])
self.assert_qmp(result, 'error/desc',
@@ -296,7 +298,7 @@ class TestParallelOps(iotests.QMPTestCase):
# block-commit should also fail if it touches nodes used by the stream job
result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[4], job_id='commit-node4')
self.assert_qmp(result, 'error/desc',
- "Node 'node4' is busy: block device is in use by block job: stream")
+ "Node 'stream-filter' is busy: block device is in use by block job: stream")
result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[1], top=self.imgs[3], job_id='commit-node1')
self.assert_qmp(result, 'error/desc',
diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out
index 08e0aecd65..028a16f365 100644
--- a/tests/qemu-iotests/141.out
+++ b/tests/qemu-iotests/141.out
@@ -99,7 +99,7 @@ wrote 1048576/1048576 bytes at offset 0
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}
{'execute': 'blockdev-del', 'arguments': {'node-name': 'drv0'}}
-{"error": {"class": "GenericError", "desc": "Node drv0 is in use"}}
+{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: stream"}}
{'execute': 'block-job-cancel', 'arguments': {'device': 'job0'}}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "job0"}}
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index e60c8326d3..432e837e6c 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -892,20 +892,24 @@ class TestBlockdevReopen(iotests.QMPTestCase):
# hd1 <- hd0
result = self.vm.qmp('block-stream', conv_keys = True, job_id = 'stream0',
- device = 'hd1', auto_finalize = False)
+ device = 'hd1', filter_node_name='cor',
+ auto_finalize = False)
self.assert_qmp(result, 'return', {})
- # We can't reopen with the original options because that would
- # make hd1 read-only and block-stream requires it to be read-write
- # (Which error message appears depends on whether the stream job is
- # already done with copying at this point.)
+ # We can't reopen with the original options because there is a filter
+ # inserted by stream job above hd1.
self.reopen(opts, {},
- ["Can't set node 'hd1' to r/o with copy-on-read enabled",
- "Cannot make block node read-only, there is a writer on it"])
+ "Cannot change the option 'backing.backing.file.node-name'")
+
+ # We can't reopen hd1 to read-only, as block-stream requires it to be
+ # read-write
+ self.reopen(opts['backing'], {'read-only': True},
+ "Cannot make block node read-only, there is a writer on it")
# We can't remove hd2 while the stream job is ongoing
opts['backing']['backing'] = None
- self.reopen(opts, {'backing.read-only': False}, "Cannot change 'backing' link from 'hd1' to 'hd2'")
+ self.reopen(opts['backing'], {'read-only': False},
+ "Cannot change 'backing' link from 'hd1' to 'hd2'")
# We can detach hd1 from hd0 because it doesn't affect the stream job
opts['backing'] = None
--
2.25.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v15 04/13] qapi: add filter-node-name to block-stream
2020-12-16 6:16 ` [PATCH v15 04/13] qapi: add filter-node-name to block-stream Vladimir Sementsov-Ogievskiy
@ 2020-12-22 15:24 ` Max Reitz
0 siblings, 0 replies; 28+ messages in thread
From: Max Reitz @ 2020-12-22 15:24 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, qemu-devel, armbru, stefanha, den, andrey.shinkevich, jsnow
On 16.12.20 07:16, Vladimir Sementsov-Ogievskiy wrote:
> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>
> Provide the possibility to pass the 'filter-node-name' parameter to the
> block-stream job as it is done for the commit block job.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> [vsementsov: comment indentation, s/Since: 5.2/Since: 6.0/]
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> qapi/block-core.json | 6 ++++++
> include/block/block_int.h | 7 ++++++-
> block/monitor/block-hmp-cmds.c | 4 ++--
> block/stream.c | 4 +++-
> blockdev.c | 4 +++-
> 5 files changed, 20 insertions(+), 5 deletions(-)
[...]
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 1eeafc118c..6f778e2517 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1142,6 +1142,9 @@ int is_windows_drive(const char *filename);
> * See @BlockJobCreateFlags
> * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> * @on_error: The action to take upon error.
> + * @filter_node_name: The node name that should be assigned to the filter
> + * driver that the commit job inserts into the graph above
Only now noticed s/commit/stream/
(I’ll fix it)
Max
> + * @bs. NULL means that a node name should be autogenerated.
> * @errp: Error object.
> *
> * Start a streaming operation on @bs. Clusters that are unallocated
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v15 09/13] stream: rework backing-file changing
2020-12-16 6:16 ` [PATCH v15 09/13] stream: rework backing-file changing Vladimir Sementsov-Ogievskiy
@ 2020-12-22 15:59 ` Max Reitz
2020-12-22 17:53 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 28+ messages in thread
From: Max Reitz @ 2020-12-22 15:59 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, qemu-devel, armbru, stefanha, den, andrey.shinkevich, jsnow
On 16.12.20 07:16, Vladimir Sementsov-Ogievskiy wrote:
> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>
> Stream in stream_prepare calls bdrv_change_backing_file() to change
> backing-file in the metadata of bs.
>
> It may use either backing-file parameter given by user or just take
> filename of base on job start.
>
> Backing file format is determined by base on job finish.
>
> There are some problems with this design, we solve only two by this
> patch:
>
> 1. Consider scenario with backing-file unset. Current concept of stream
> supports changing of the base during the job (we don't freeze link to
> the base). So, we should not save base filename at job start,
>
> - let's determine name of the base on job finish.
>
> 2. Using direct base to determine filename and format is not very good:
> base node may be a filter, so its filename may be JSON, and format_name
> is not good for storing into qcow2 metadata as backing file format.
>
> - let's use unfiltered_base
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> [vsementsov: change commit subject, change logic in stream_prepare]
> ---
> block/stream.c | 9 +++++----
> blockdev.c | 8 +-------
> 2 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/block/stream.c b/block/stream.c
> index 6e281c71ac..6a525a5edf 100644
> --- a/block/stream.c
> +++ b/block/stream.c
[...]
> @@ -73,10 +74,10 @@ static int stream_prepare(Job *job)
>
> if (bdrv_cow_child(unfiltered_bs)) {
> const char *base_id = NULL, *base_fmt = NULL;
> - if (base) {
> - base_id = s->backing_file_str;
> - if (base->drv) {
> - base_fmt = base->drv->format_name;
> + if (unfiltered_base) {
> + base_id = s->backing_file_str ?: unfiltered_base->filename;
> + if (unfiltered_base->drv) {
> + base_fmt = unfiltered_base->drv->format_name;
> }
> }
> bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
I think I preferred the v14 behavior of not setting a backing file
format if backing_file_str is nowhere to be found in the current backing
chain. (I just noticed, I had a typo in my reply to v14, though; the
“continuing on with setting a backing_fmt” should have read “continuing
on *without* setting a backing_fmt”...)
Anyway, this is still an improvement on the pre-patch behavior, so:
Reviewed-by: Max Reitz <mreitz@redhat.com>
(And as we discussed, the best would be for the user to specify a
backing format through a yet-to-be-added option.)
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v15 10/13] qapi: block-stream: add "bottom" argument
2020-12-16 6:17 ` [PATCH v15 10/13] qapi: block-stream: add "bottom" argument Vladimir Sementsov-Ogievskiy
@ 2020-12-22 16:07 ` Max Reitz
2020-12-22 18:00 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 28+ messages in thread
From: Max Reitz @ 2020-12-22 16:07 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, qemu-devel, armbru, stefanha, den, andrey.shinkevich, jsnow
On 16.12.20 07:17, Vladimir Sementsov-Ogievskiy wrote:
> The code already don't freeze base node and we try to make it prepared
> for the situation when base node is changed during the operation. In
> other words, block-stream doesn't own base node.
>
> Let's introduce a new interface which should replace the current one,
> which will in better relations with the code. Specifying bottom node
> instead of base, and requiring it to be non-filter gives us the
> following benefits:
>
> - drop difference between above_base and base_overlay, which will be
> renamed to just bottom, when old interface dropped
>
> - clean way to work with parallel streams/commits on the same backing
> chain, which otherwise become a problem when we introduce a filter
> for stream job
>
> - cleaner interface. Nobody will surprised the fact that base node may
> disappear during block-stream, when there is no word about "base" in
> the interface.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> qapi/block-core.json | 12 ++++---
> include/block/block_int.h | 1 +
> block/monitor/block-hmp-cmds.c | 3 +-
> block/stream.c | 50 +++++++++++++++++++---------
> blockdev.c | 59 ++++++++++++++++++++++++++++------
> 5 files changed, 94 insertions(+), 31 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b8094a5ec7..cb0066fd5c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2517,10 +2517,14 @@
> # @device: the device or node name of the top image
> #
> # @base: the common backing file name.
> -# It cannot be set if @base-node is also set.
> +# It cannot be set if @base-node or @bottom is also set.
> #
> # @base-node: the node name of the backing file.
> -# It cannot be set if @base is also set. (Since 2.8)
> +# It cannot be set if @base or @bottom is also set. (Since 2.8)
> +#
> +# @bottom: the last node in the chain that should be streamed into
> +# top. It cannot be set if @base or @base-node is also set.
> +# It cannot be filter node. (Since 6.0)
As far as I can make out, one of the results of our discussion on v14
was that when using backing-file + bottom, we want to require the user
to specify backing-fmt as well. Now, backing-fmt isn’t present yet.
Doesn’t that mean we have to make bottom + backing-file an error until
we have backing-fmt (like it was in v14)?
Or do you consider the change to patch 9 sufficient to make at least the
case work for which backing-file was purportedly introduced, i.e. FD
passing? (Patch 9’s new version will just take the format of the base
post-streaming, which would be most likely a correct guess when using FD
passing.)
(I.e., now with patch 9 being more liberal in guessing, perhaps you
decided to no longer make backing-fmt mandatory after all.)
Max
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v15 13/13] block: apply COR-filter to block-stream jobs
2020-12-16 6:17 ` [PATCH v15 13/13] block: apply COR-filter to block-stream jobs Vladimir Sementsov-Ogievskiy
@ 2020-12-22 16:20 ` Max Reitz
2020-12-22 18:07 ` Vladimir Sementsov-Ogievskiy
2021-01-05 15:30 ` Max Reitz
1 sibling, 1 reply; 28+ messages in thread
From: Max Reitz @ 2020-12-22 16:20 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, qemu-devel, armbru, stefanha, den, andrey.shinkevich, jsnow
On 16.12.20 07:17, Vladimir Sementsov-Ogievskiy wrote:
> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>
> This patch completes the series with the COR-filter applied to
> block-stream operations.
>
> Adding the filter makes it possible in future implement discarding
> copied regions in backing files during the block-stream job, to reduce
> the disk overuse (we need control on permissions).
>
> Also, the filter now is smart enough to do copy-on-read with specified
> base, so we have benefit on guest reads even when doing block-stream of
> the part of the backing chain.
>
> Several iotests are slightly modified due to filter insertion.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/stream.c | 105 ++++++++++++++++++++++---------------
> tests/qemu-iotests/030 | 8 +--
> tests/qemu-iotests/141.out | 2 +-
> tests/qemu-iotests/245 | 20 ++++---
> 4 files changed, 80 insertions(+), 55 deletions(-)
>
> diff --git a/block/stream.c b/block/stream.c
> index 626dfa2b22..1fa742b0db 100644
> --- a/block/stream.c
> +++ b/block/stream.c
[...]
> @@ -266,30 +251,62 @@ void stream_start(const char *job_id, BlockDriverState *bs,
[...]
> /* Make sure that the image is opened in read-write mode */
> bs_read_only = bdrv_is_read_only(bs);
> if (bs_read_only) {
> - if (bdrv_reopen_set_read_only(bs, false, errp) != 0) {
> - bs_read_only = false;
> - goto fail;
> + int ret;
> + /* Hold the chain during reopen */
> + if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
> + return;
> + }
> +
> + ret = bdrv_reopen_set_read_only(bs, false, errp);
> +
> + /* failure, or cor-filter will hold the chain */
> + bdrv_unfreeze_backing_chain(bs, above_base);
> +
> + if (ret < 0) {
Shouldn’t we keep the “bs_read_only = false;” here?
(All the rest of this patch looks good.)
Max
> + return;
> }
> }
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v15 09/13] stream: rework backing-file changing
2020-12-22 15:59 ` Max Reitz
@ 2020-12-22 17:53 ` Vladimir Sementsov-Ogievskiy
2021-01-05 12:51 ` Max Reitz
0 siblings, 1 reply; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-22 17:53 UTC (permalink / raw)
To: Max Reitz, qemu-block
Cc: fam, kwolf, qemu-devel, armbru, stefanha, den, andrey.shinkevich, jsnow
22.12.2020 18:59, Max Reitz wrote:
> On 16.12.20 07:16, Vladimir Sementsov-Ogievskiy wrote:
>> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>
>> Stream in stream_prepare calls bdrv_change_backing_file() to change
>> backing-file in the metadata of bs.
>>
>> It may use either backing-file parameter given by user or just take
>> filename of base on job start.
>>
>> Backing file format is determined by base on job finish.
>>
>> There are some problems with this design, we solve only two by this
>> patch:
>>
>> 1. Consider scenario with backing-file unset. Current concept of stream
>> supports changing of the base during the job (we don't freeze link to
>> the base). So, we should not save base filename at job start,
>>
>> - let's determine name of the base on job finish.
>>
>> 2. Using direct base to determine filename and format is not very good:
>> base node may be a filter, so its filename may be JSON, and format_name
>> is not good for storing into qcow2 metadata as backing file format.
>>
>> - let's use unfiltered_base
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> [vsementsov: change commit subject, change logic in stream_prepare]
>> ---
>> block/stream.c | 9 +++++----
>> blockdev.c | 8 +-------
>> 2 files changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/block/stream.c b/block/stream.c
>> index 6e281c71ac..6a525a5edf 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>
> [...]
>
>> @@ -73,10 +74,10 @@ static int stream_prepare(Job *job)
>> if (bdrv_cow_child(unfiltered_bs)) {
>> const char *base_id = NULL, *base_fmt = NULL;
>> - if (base) {
>> - base_id = s->backing_file_str;
>> - if (base->drv) {
>> - base_fmt = base->drv->format_name;
>> + if (unfiltered_base) {
>> + base_id = s->backing_file_str ?: unfiltered_base->filename;
>> + if (unfiltered_base->drv) {
>> + base_fmt = unfiltered_base->drv->format_name;
>> }
>> }
>> bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
>
> I think I preferred the v14 behavior of not setting a backing file format if backing_file_str is nowhere to be found in the current backing chain. (I just noticed, I had a typo in my reply to v14, though; the “continuing on with setting a backing_fmt” should have read “continuing on *without* setting a backing_fmt”...)
>
> Anyway, this is still an improvement on the pre-patch behavior, so:
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> (And as we discussed, the best would be for the user to specify a backing format through a yet-to-be-added option.)
>
We discussed that the original aim of backing_file_str arg is something like fd-passing, when qemu doesn't know real name. In that way what was done in v14 is a degradation: we'll never find such name in a backing chain. And acutally, using format of backing file is a correct thing.
So, as I understand now:
We set backing file to the node which is the new backing-bs (maybe, skipping some filters). Nobody should set backing in qcow2 metadata to something absolutely different. So, using format_name of backing bs (skipping filters) is a correct thing.
We want to support cases when qemu doens't know real file-names. So, trying to check filename, or search it in a backing chain is wrong idea..
Hmm, or when we search backing name, we really track what was written in backing_file field of some qcow2 image in a chain, so it should be something correct? Hmm, then may be v14 was OK.. But in this case again, user should not pass backing_file, but we can just use backing_file field of cow-parent of the base at job start.. Oh. Anyway, we can adjust it later in a separate series.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v15 10/13] qapi: block-stream: add "bottom" argument
2020-12-22 16:07 ` Max Reitz
@ 2020-12-22 18:00 ` Vladimir Sementsov-Ogievskiy
2020-12-22 18:11 ` Vladimir Sementsov-Ogievskiy
2021-01-05 12:51 ` Max Reitz
0 siblings, 2 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-22 18:00 UTC (permalink / raw)
To: Max Reitz, qemu-block
Cc: fam, kwolf, qemu-devel, armbru, stefanha, den, andrey.shinkevich, jsnow
22.12.2020 19:07, Max Reitz wrote:
> On 16.12.20 07:17, Vladimir Sementsov-Ogievskiy wrote:
>> The code already don't freeze base node and we try to make it prepared
>> for the situation when base node is changed during the operation. In
>> other words, block-stream doesn't own base node.
>>
>> Let's introduce a new interface which should replace the current one,
>> which will in better relations with the code. Specifying bottom node
>> instead of base, and requiring it to be non-filter gives us the
>> following benefits:
>>
>> - drop difference between above_base and base_overlay, which will be
>> renamed to just bottom, when old interface dropped
>>
>> - clean way to work with parallel streams/commits on the same backing
>> chain, which otherwise become a problem when we introduce a filter
>> for stream job
>>
>> - cleaner interface. Nobody will surprised the fact that base node may
>> disappear during block-stream, when there is no word about "base" in
>> the interface.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> qapi/block-core.json | 12 ++++---
>> include/block/block_int.h | 1 +
>> block/monitor/block-hmp-cmds.c | 3 +-
>> block/stream.c | 50 +++++++++++++++++++---------
>> blockdev.c | 59 ++++++++++++++++++++++++++++------
>> 5 files changed, 94 insertions(+), 31 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index b8094a5ec7..cb0066fd5c 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2517,10 +2517,14 @@
>> # @device: the device or node name of the top image
>> #
>> # @base: the common backing file name.
>> -# It cannot be set if @base-node is also set.
>> +# It cannot be set if @base-node or @bottom is also set.
>> #
>> # @base-node: the node name of the backing file.
>> -# It cannot be set if @base is also set. (Since 2.8)
>> +# It cannot be set if @base or @bottom is also set. (Since 2.8)
>> +#
>> +# @bottom: the last node in the chain that should be streamed into
>> +# top. It cannot be set if @base or @base-node is also set.
>> +# It cannot be filter node. (Since 6.0)
>
> As far as I can make out, one of the results of our discussion on v14 was that when using backing-file + bottom, we want to require the user to specify backing-fmt as well. Now, backing-fmt isn’t present yet. Doesn’t that mean we have to make bottom + backing-file an error until we have backing-fmt (like it was in v14)?
See my answer on 09. I just have some doubts around backing-fmt and decided to keep it as is.
I don't think that we really need backing-fmt. We shouldn't have use-cases when backing-fmt is set to something another than final base node. Therefore, using format_name of final base node is a correct thing to do. So, I don't see the reason now for introducing new option.
>
> Or do you consider the change to patch 9 sufficient to make at least the case work for which backing-file was purportedly introduced, i.e. FD passing? (Patch 9’s new version will just take the format of the base post-streaming, which would be most likely a correct guess when using FD passing.)
Yes, I decided just to keep logic around backing-fmt as is. Hmm, definitely I should have described my decision in cover letter, for you not being surprised.
>
> (I.e., now with patch 9 being more liberal in guessing, perhaps you decided to no longer make backing-fmt mandatory after all.)
>
> Max
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v15 13/13] block: apply COR-filter to block-stream jobs
2020-12-22 16:20 ` Max Reitz
@ 2020-12-22 18:07 ` Vladimir Sementsov-Ogievskiy
2021-01-05 12:52 ` Max Reitz
0 siblings, 1 reply; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-22 18:07 UTC (permalink / raw)
To: Max Reitz, qemu-block
Cc: fam, kwolf, qemu-devel, armbru, stefanha, den, andrey.shinkevich, jsnow
22.12.2020 19:20, Max Reitz wrote:
> On 16.12.20 07:17, Vladimir Sementsov-Ogievskiy wrote:
>> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>
>> This patch completes the series with the COR-filter applied to
>> block-stream operations.
>>
>> Adding the filter makes it possible in future implement discarding
>> copied regions in backing files during the block-stream job, to reduce
>> the disk overuse (we need control on permissions).
>>
>> Also, the filter now is smart enough to do copy-on-read with specified
>> base, so we have benefit on guest reads even when doing block-stream of
>> the part of the backing chain.
>>
>> Several iotests are slightly modified due to filter insertion.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> block/stream.c | 105 ++++++++++++++++++++++---------------
>> tests/qemu-iotests/030 | 8 +--
>> tests/qemu-iotests/141.out | 2 +-
>> tests/qemu-iotests/245 | 20 ++++---
>> 4 files changed, 80 insertions(+), 55 deletions(-)
>>
>> diff --git a/block/stream.c b/block/stream.c
>> index 626dfa2b22..1fa742b0db 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>
> [...]
>
>> @@ -266,30 +251,62 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>
> [...]
>
>> /* Make sure that the image is opened in read-write mode */
>> bs_read_only = bdrv_is_read_only(bs);
>> if (bs_read_only) {
>> - if (bdrv_reopen_set_read_only(bs, false, errp) != 0) {
>> - bs_read_only = false;
>> - goto fail;
>> + int ret;
>> + /* Hold the chain during reopen */
>> + if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
>> + return;
>> + }
>> +
>> + ret = bdrv_reopen_set_read_only(bs, false, errp);
>> +
>> + /* failure, or cor-filter will hold the chain */
>> + bdrv_unfreeze_backing_chain(bs, above_base);
>> +
>> + if (ret < 0) {
>
> Shouldn’t we keep the “bs_read_only = false;” here?
>
No, as we don't goto fail. (pre-patch, we goto fail here, and don't want fail: code path to reopend back to RW (as reopening to RO is failed anyway (and we hope it's transactional enough)))
> (All the rest of this patch looks good.)
>
> Max
>
>> + return;
>> }
>> }
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v15 10/13] qapi: block-stream: add "bottom" argument
2020-12-22 18:00 ` Vladimir Sementsov-Ogievskiy
@ 2020-12-22 18:11 ` Vladimir Sementsov-Ogievskiy
2021-01-05 12:51 ` Max Reitz
1 sibling, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-12-22 18:11 UTC (permalink / raw)
To: Max Reitz, qemu-block
Cc: fam, kwolf, qemu-devel, armbru, stefanha, den, andrey.shinkevich, jsnow
22.12.2020 21:00, Vladimir Sementsov-Ogievskiy wrote:
> We shouldn't have use-cases when backing-fmt is set to something another than final base node.
I mean, we shouldn't have use-cases when backing-file is [...]
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v15 09/13] stream: rework backing-file changing
2020-12-22 17:53 ` Vladimir Sementsov-Ogievskiy
@ 2021-01-05 12:51 ` Max Reitz
0 siblings, 0 replies; 28+ messages in thread
From: Max Reitz @ 2021-01-05 12:51 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, qemu-devel, armbru, stefanha, den, andrey.shinkevich, jsnow
On 22.12.20 18:53, Vladimir Sementsov-Ogievskiy wrote:
> 22.12.2020 18:59, Max Reitz wrote:
>> On 16.12.20 07:16, Vladimir Sementsov-Ogievskiy wrote:
>>> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>
>>> Stream in stream_prepare calls bdrv_change_backing_file() to change
>>> backing-file in the metadata of bs.
>>>
>>> It may use either backing-file parameter given by user or just take
>>> filename of base on job start.
>>>
>>> Backing file format is determined by base on job finish.
>>>
>>> There are some problems with this design, we solve only two by this
>>> patch:
>>>
>>> 1. Consider scenario with backing-file unset. Current concept of stream
>>> supports changing of the base during the job (we don't freeze link to
>>> the base). So, we should not save base filename at job start,
>>>
>>> - let's determine name of the base on job finish.
>>>
>>> 2. Using direct base to determine filename and format is not very good:
>>> base node may be a filter, so its filename may be JSON, and format_name
>>> is not good for storing into qcow2 metadata as backing file format.
>>>
>>> - let's use unfiltered_base
>>>
>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> [vsementsov: change commit subject, change logic in stream_prepare]
>>> ---
>>> block/stream.c | 9 +++++----
>>> blockdev.c | 8 +-------
>>> 2 files changed, 6 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/block/stream.c b/block/stream.c
>>> index 6e281c71ac..6a525a5edf 100644
>>> --- a/block/stream.c
>>> +++ b/block/stream.c
>>
>> [...]
>>
>>> @@ -73,10 +74,10 @@ static int stream_prepare(Job *job)
>>> if (bdrv_cow_child(unfiltered_bs)) {
>>> const char *base_id = NULL, *base_fmt = NULL;
>>> - if (base) {
>>> - base_id = s->backing_file_str;
>>> - if (base->drv) {
>>> - base_fmt = base->drv->format_name;
>>> + if (unfiltered_base) {
>>> + base_id = s->backing_file_str ?: unfiltered_base->filename;
>>> + if (unfiltered_base->drv) {
>>> + base_fmt = unfiltered_base->drv->format_name;
>>> }
>>> }
>>> bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
>>
>> I think I preferred the v14 behavior of not setting a backing file
>> format if backing_file_str is nowhere to be found in the current
>> backing chain. (I just noticed, I had a typo in my reply to v14,
>> though; the “continuing on with setting a backing_fmt” should have
>> read “continuing on *without* setting a backing_fmt”...)
>>
>> Anyway, this is still an improvement on the pre-patch behavior, so:
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>> (And as we discussed, the best would be for the user to specify a
>> backing format through a yet-to-be-added option.)
>>
>
> We discussed that the original aim of backing_file_str arg is something
> like fd-passing, when qemu doesn't know real name. In that way what was
> done in v14 is a degradation: we'll never find such name in a backing
> chain. And acutally, using format of backing file is a correct thing.
It was my understanding that this was an example use case; other users
might want to use backing-file for something else entirely. (I imagined
using e.g. a completely different file in a different format.)
OTOH, considering that “something else entirely” simply doesn’t work
(because the driver has to be something from the backing chain), my
imagination was just too wild. If anyone should ever want to follow up
on it, I expect them to complain, and we’ll worry about it then.
> So, as I understand now:
>
> We set backing file to the node which is the new backing-bs (maybe,
> skipping some filters). Nobody should set backing in qcow2 metadata to
> something absolutely different. So, using format_name of backing bs
> (skipping filters) is a correct thing.
>
> We want to support cases when qemu doens't know real file-names. So,
> trying to check filename, or search it in a backing chain is wrong idea..
>
> Hmm, or when we search backing name, we really track what was written in
> backing_file field of some qcow2 image in a chain, so it should be
> something correct?
If it does something else than what people want it to do, they’ll
complain. :)
(It isn’t like this patch is breaking anything that would work right now.)
So I agree that we don’t need backing-fmt now. (Or maybe ever.)
Max
Max
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v15 10/13] qapi: block-stream: add "bottom" argument
2020-12-22 18:00 ` Vladimir Sementsov-Ogievskiy
2020-12-22 18:11 ` Vladimir Sementsov-Ogievskiy
@ 2021-01-05 12:51 ` Max Reitz
1 sibling, 0 replies; 28+ messages in thread
From: Max Reitz @ 2021-01-05 12:51 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, qemu-devel, armbru, stefanha, den, andrey.shinkevich, jsnow
On 22.12.20 19:00, Vladimir Sementsov-Ogievskiy wrote:
> 22.12.2020 19:07, Max Reitz wrote:
>> On 16.12.20 07:17, Vladimir Sementsov-Ogievskiy wrote:
>>> The code already don't freeze base node and we try to make it prepared
>>> for the situation when base node is changed during the operation. In
>>> other words, block-stream doesn't own base node.
>>>
>>> Let's introduce a new interface which should replace the current one,
>>> which will in better relations with the code. Specifying bottom node
>>> instead of base, and requiring it to be non-filter gives us the
>>> following benefits:
>>>
>>> - drop difference between above_base and base_overlay, which will be
>>> renamed to just bottom, when old interface dropped
>>>
>>> - clean way to work with parallel streams/commits on the same backing
>>> chain, which otherwise become a problem when we introduce a filter
>>> for stream job
>>>
>>> - cleaner interface. Nobody will surprised the fact that base node may
>>> disappear during block-stream, when there is no word about "base" in
>>> the interface.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>> qapi/block-core.json | 12 ++++---
>>> include/block/block_int.h | 1 +
>>> block/monitor/block-hmp-cmds.c | 3 +-
>>> block/stream.c | 50 +++++++++++++++++++---------
>>> blockdev.c | 59 ++++++++++++++++++++++++++++------
>>> 5 files changed, 94 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index b8094a5ec7..cb0066fd5c 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -2517,10 +2517,14 @@
>>> # @device: the device or node name of the top image
>>> #
>>> # @base: the common backing file name.
>>> -# It cannot be set if @base-node is also set.
>>> +# It cannot be set if @base-node or @bottom is also set.
>>> #
>>> # @base-node: the node name of the backing file.
>>> -# It cannot be set if @base is also set. (Since 2.8)
>>> +# It cannot be set if @base or @bottom is also set.
>>> (Since 2.8)
>>> +#
>>> +# @bottom: the last node in the chain that should be streamed into
>>> +# top. It cannot be set if @base or @base-node is also set.
>>> +# It cannot be filter node. (Since 6.0)
>>
>> As far as I can make out, one of the results of our discussion on v14
>> was that when using backing-file + bottom, we want to require the user
>> to specify backing-fmt as well. Now, backing-fmt isn’t present yet.
>> Doesn’t that mean we have to make bottom + backing-file an error until
>> we have backing-fmt (like it was in v14)?
>
> See my answer on 09. I just have some doubts around backing-fmt and
> decided to keep it as is.
>
> I don't think that we really need backing-fmt. We shouldn't have
> use-cases when backing-fmt is set to something another than final base
> node. Therefore, using format_name of final base node is a correct thing
> to do. So, I don't see the reason now for introducing new option.
Yup, yup, all good.
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v15 13/13] block: apply COR-filter to block-stream jobs
2020-12-22 18:07 ` Vladimir Sementsov-Ogievskiy
@ 2021-01-05 12:52 ` Max Reitz
0 siblings, 0 replies; 28+ messages in thread
From: Max Reitz @ 2021-01-05 12:52 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, qemu-devel, armbru, stefanha, den, andrey.shinkevich, jsnow
On 22.12.20 19:07, Vladimir Sementsov-Ogievskiy wrote:
> 22.12.2020 19:20, Max Reitz wrote:
>> On 16.12.20 07:17, Vladimir Sementsov-Ogievskiy wrote:
>>> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>
>>> This patch completes the series with the COR-filter applied to
>>> block-stream operations.
>>>
>>> Adding the filter makes it possible in future implement discarding
>>> copied regions in backing files during the block-stream job, to reduce
>>> the disk overuse (we need control on permissions).
>>>
>>> Also, the filter now is smart enough to do copy-on-read with specified
>>> base, so we have benefit on guest reads even when doing block-stream of
>>> the part of the backing chain.
>>>
>>> Several iotests are slightly modified due to filter insertion.
>>>
>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>> block/stream.c | 105 ++++++++++++++++++++++---------------
>>> tests/qemu-iotests/030 | 8 +--
>>> tests/qemu-iotests/141.out | 2 +-
>>> tests/qemu-iotests/245 | 20 ++++---
>>> 4 files changed, 80 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/block/stream.c b/block/stream.c
>>> index 626dfa2b22..1fa742b0db 100644
>>> --- a/block/stream.c
>>> +++ b/block/stream.c
>>
>> [...]
>>
>>> @@ -266,30 +251,62 @@ void stream_start(const char *job_id,
>>> BlockDriverState *bs,
>>
>> [...]
>>
>>> /* Make sure that the image is opened in read-write mode */
>>> bs_read_only = bdrv_is_read_only(bs);
>>> if (bs_read_only) {
>>> - if (bdrv_reopen_set_read_only(bs, false, errp) != 0) {
>>> - bs_read_only = false;
>>> - goto fail;
>>> + int ret;
>>> + /* Hold the chain during reopen */
>>> + if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
>>> + return;
>>> + }
>>> +
>>> + ret = bdrv_reopen_set_read_only(bs, false, errp);
>>> +
>>> + /* failure, or cor-filter will hold the chain */
>>> + bdrv_unfreeze_backing_chain(bs, above_base);
>>> +
>>> + if (ret < 0) {
>>
>> Shouldn’t we keep the “bs_read_only = false;” here?
>>
>
> No, as we don't goto fail.
Ah, right, then it won’t do anything.
> (pre-patch, we goto fail here, and don't want
> fail: code path to reopend back to RW (as reopening to RO is failed
> anyway (and we hope it's transactional enough)))
That’s why we had bs_read_only = false; pre-patch, so the reopen back to
RW is skipped.
And with this patch, we don’t need anything else from the “fail” path
(freezing is done by the filter, and the filter doesn’t exist yet), so
it’s correct to condense the “bs_read_only = false; goto fail;” into a
plain “return”.
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v15 13/13] block: apply COR-filter to block-stream jobs
2020-12-16 6:17 ` [PATCH v15 13/13] block: apply COR-filter to block-stream jobs Vladimir Sementsov-Ogievskiy
2020-12-22 16:20 ` Max Reitz
@ 2021-01-05 15:30 ` Max Reitz
1 sibling, 0 replies; 28+ messages in thread
From: Max Reitz @ 2021-01-05 15:30 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, qemu-devel, armbru, stefanha, den, andrey.shinkevich, jsnow
On 16.12.20 07:17, Vladimir Sementsov-Ogievskiy wrote:
> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>
> This patch completes the series with the COR-filter applied to
> block-stream operations.
>
> Adding the filter makes it possible in future implement discarding
> copied regions in backing files during the block-stream job, to reduce
> the disk overuse (we need control on permissions).
>
> Also, the filter now is smart enough to do copy-on-read with specified
> base, so we have benefit on guest reads even when doing block-stream of
> the part of the backing chain.
>
> Several iotests are slightly modified due to filter insertion.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/stream.c | 105 ++++++++++++++++++++++---------------
> tests/qemu-iotests/030 | 8 +--
> tests/qemu-iotests/141.out | 2 +-
> tests/qemu-iotests/245 | 20 ++++---
> 4 files changed, 80 insertions(+), 55 deletions(-)
[...]
> diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out
> index 08e0aecd65..028a16f365 100644
> --- a/tests/qemu-iotests/141.out
> +++ b/tests/qemu-iotests/141.out
> @@ -99,7 +99,7 @@ wrote 1048576/1048576 bytes at offset 0
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}
> {'execute': 'blockdev-del', 'arguments': {'node-name': 'drv0'}}
> -{"error": {"class": "GenericError", "desc": "Node drv0 is in use"}}
> +{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: stream"}}
> {'execute': 'block-job-cancel', 'arguments': {'device': 'job0'}}
> {"return": {}}
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "job0"}}
Amusing side note: This context matches two places in 141.out. With
0e720781282 (which requires two contextual whitespace tweaks), the file
gets longer, so the line number doesn’t match anymore. git then tries
to apply the context to the first match (which is closer to line 99),
which is wrong.
First time I had something like that happen, actually.
Max
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v15 00/13] Apply COR-filter to the block-stream permanently
2020-12-16 6:16 [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Vladimir Sementsov-Ogievskiy
` (12 preceding siblings ...)
2020-12-16 6:17 ` [PATCH v15 13/13] block: apply COR-filter to block-stream jobs Vladimir Sementsov-Ogievskiy
@ 2021-01-05 16:08 ` Max Reitz
2021-01-08 10:24 ` Vladimir Sementsov-Ogievskiy
13 siblings, 1 reply; 28+ messages in thread
From: Max Reitz @ 2021-01-05 16:08 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, qemu-devel, armbru, stefanha, den, andrey.shinkevich, jsnow
On 16.12.20 07:16, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here is a new version of cor-filter in block-stream series. Main change
> is freezing the chain in cor-filter itself.
Thanks, applied to my block branch:
https://git.xanclic.moe/XanClic/qemu/commits/branch/block
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v15 00/13] Apply COR-filter to the block-stream permanently
2021-01-05 16:08 ` [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Max Reitz
@ 2021-01-08 10:24 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-01-08 10:24 UTC (permalink / raw)
To: Max Reitz, qemu-block
Cc: fam, kwolf, qemu-devel, armbru, stefanha, den, andrey.shinkevich, jsnow
05.01.2021 19:08, Max Reitz wrote:
> On 16.12.20 07:16, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here is a new version of cor-filter in block-stream series. Main change
>> is freezing the chain in cor-filter itself.
>
> Thanks, applied to my block branch:
>
> https://git.xanclic.moe/XanClic/qemu/commits/branch/block
>
Great! Thanks a lot!
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2021-01-08 10:25 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-16 6:16 [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 01/13] copy-on-read: support preadv/pwritev_part functions Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 02/13] block: add API function to insert a node Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 03/13] copy-on-read: add filter drop function Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 04/13] qapi: add filter-node-name to block-stream Vladimir Sementsov-Ogievskiy
2020-12-22 15:24 ` Max Reitz
2020-12-16 6:16 ` [PATCH v15 05/13] qapi: copy-on-read filter: add 'bottom' option Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 06/13] iotests: add #310 to test bottom node in COR driver Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 07/13] block: include supported_read_flags into BDS structure Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 08/13] copy-on-read: skip non-guest reads if no copy needed Vladimir Sementsov-Ogievskiy
2020-12-16 6:16 ` [PATCH v15 09/13] stream: rework backing-file changing Vladimir Sementsov-Ogievskiy
2020-12-22 15:59 ` Max Reitz
2020-12-22 17:53 ` Vladimir Sementsov-Ogievskiy
2021-01-05 12:51 ` Max Reitz
2020-12-16 6:17 ` [PATCH v15 10/13] qapi: block-stream: add "bottom" argument Vladimir Sementsov-Ogievskiy
2020-12-22 16:07 ` Max Reitz
2020-12-22 18:00 ` Vladimir Sementsov-Ogievskiy
2020-12-22 18:11 ` Vladimir Sementsov-Ogievskiy
2021-01-05 12:51 ` Max Reitz
2020-12-16 6:17 ` [PATCH v15 11/13] iotests: 30: prepare to COR filter insertion by stream job Vladimir Sementsov-Ogievskiy
2020-12-16 6:17 ` [PATCH v15 12/13] block/stream: add s->target_bs Vladimir Sementsov-Ogievskiy
2020-12-16 6:17 ` [PATCH v15 13/13] block: apply COR-filter to block-stream jobs Vladimir Sementsov-Ogievskiy
2020-12-22 16:20 ` Max Reitz
2020-12-22 18:07 ` Vladimir Sementsov-Ogievskiy
2021-01-05 12:52 ` Max Reitz
2021-01-05 15:30 ` Max Reitz
2021-01-05 16:08 ` [PATCH v15 00/13] Apply COR-filter to the block-stream permanently Max Reitz
2021-01-08 10:24 ` 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.