* [PATCH v10 0/9] Apply COR-filter to the block-stream permanently
@ 2020-09-29 12:38 Andrey Shinkevich via
2020-09-29 12:38 ` [PATCH v10 1/9] copy-on-read: Support preadv/pwritev_part functions Andrey Shinkevich via
` (8 more replies)
0 siblings, 9 replies; 25+ messages in thread
From: Andrey Shinkevich via @ 2020-09-29 12:38 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake,
den, vsementsov, andrey.shinkevich
Despite the patch "freeze link to base node..." has been removed from the
series in the current version 9, the iotest case test_stream_parallel does
not pass after the COR-filter is inserted into the backing chain. As the
test case may not be initialized, it does not make a sense and was removed
again.
The check with bdrv_is_allocated_above() takes place in the COR-filter and
in the block-stream job both. An optimization of the block-stream job based
on the filter functionality may be made in a separate series.
v10:
02: The missed new file block/copy-on-read.h added
v9:
02: Refactored.
04: Base node name is used instead of the file name.
05: New implementation based on Max' review.
06: New.
07: New. The patch "freeze link to base node..." was deleted.
08: New.
09: The filter node options are initialized.
The v8 Message-Id:
<1598633579-221780-1-git-send-email-andrey.shinkevich@virtuozzo.com>
Andrey Shinkevich (9):
copy-on-read: Support preadv/pwritev_part functions
copy-on-read: add filter append/drop functions
qapi: add filter-node-name to block-stream
copy-on-read: pass base node name to COR driver
copy-on-read: limit guest COR activity to base in COR driver
copy-on-read: skip non-guest reads if no copy needed
stream: skip filters when writing backing file name to QCOW2 header
block: remove unused backing-file name parameter
block: apply COR-filter to block-stream jobs
block/copy-on-read.c | 165 ++++++++++++++++++++++++++++++++++++++---
block/copy-on-read.h | 35 +++++++++
block/io.c | 2 +-
block/monitor/block-hmp-cmds.c | 6 +-
block/stream.c | 112 +++++++++++++++++-----------
blockdev.c | 21 +-----
include/block/block_int.h | 9 ++-
qapi/block-core.json | 23 ++----
tests/qemu-iotests/030 | 51 ++-----------
tests/qemu-iotests/030.out | 4 +-
tests/qemu-iotests/141.out | 2 +-
tests/qemu-iotests/245 | 19 +++--
12 files changed, 302 insertions(+), 147 deletions(-)
create mode 100644 block/copy-on-read.h
--
1.8.3.1
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v10 1/9] copy-on-read: Support preadv/pwritev_part functions
2020-09-29 12:38 [PATCH v10 0/9] Apply COR-filter to the block-stream permanently Andrey Shinkevich via
@ 2020-09-29 12:38 ` Andrey Shinkevich via
2020-09-29 12:38 ` [PATCH v10 2/9] copy-on-read: add filter append/drop functions Andrey Shinkevich via
` (7 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Andrey Shinkevich via @ 2020-09-29 12:38 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake,
den, vsementsov, andrey.shinkevich
Add support for the recently introduced functions
bdrv_co_preadv_part()
and
bdrv_co_pwritev_part()
to the COR-filter driver.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/copy-on-read.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 2816e61..cb03e0f 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -74,21 +74,25 @@ static int64_t cor_getlength(BlockDriverState *bs)
}
-static int coroutine_fn cor_co_preadv(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov,
+ size_t qiov_offset,
+ int flags)
{
- return bdrv_co_preadv(bs->file, offset, bytes, qiov,
- flags | BDRV_REQ_COPY_ON_READ);
+ return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
+ flags | BDRV_REQ_COPY_ON_READ);
}
-static int coroutine_fn cor_co_pwritev(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+static int coroutine_fn cor_co_pwritev_part(BlockDriverState *bs,
+ uint64_t offset,
+ uint64_t bytes,
+ QEMUIOVector *qiov,
+ size_t qiov_offset, int flags)
{
-
- return bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
+ return bdrv_co_pwritev_part(bs->file, offset, bytes, qiov, qiov_offset,
+ flags);
}
@@ -137,8 +141,8 @@ static BlockDriver bdrv_copy_on_read = {
.bdrv_getlength = cor_getlength,
- .bdrv_co_preadv = cor_co_preadv,
- .bdrv_co_pwritev = cor_co_pwritev,
+ .bdrv_co_preadv_part = cor_co_preadv_part,
+ .bdrv_co_pwritev_part = cor_co_pwritev_part,
.bdrv_co_pwrite_zeroes = cor_co_pwrite_zeroes,
.bdrv_co_pdiscard = cor_co_pdiscard,
.bdrv_co_pwritev_compressed = cor_co_pwritev_compressed,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v10 2/9] copy-on-read: add filter append/drop functions
2020-09-29 12:38 [PATCH v10 0/9] Apply COR-filter to the block-stream permanently Andrey Shinkevich via
2020-09-29 12:38 ` [PATCH v10 1/9] copy-on-read: Support preadv/pwritev_part functions Andrey Shinkevich via
@ 2020-09-29 12:38 ` Andrey Shinkevich via
2020-10-05 13:34 ` Vladimir Sementsov-Ogievskiy
2020-09-29 12:38 ` [PATCH v10 3/9] qapi: add filter-node-name to block-stream Andrey Shinkevich via
` (6 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Andrey Shinkevich via @ 2020-09-29 12:38 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake,
den, vsementsov, andrey.shinkevich
Provide API for the COR-filter insertion/removal.
Also, drop the filter child permissions for an inactive state when the
filter node is being removed.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
block/copy-on-read.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++
block/copy-on-read.h | 35 ++++++++++++++++++++++
2 files changed, 119 insertions(+)
create mode 100644 block/copy-on-read.h
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index cb03e0f..3c8231f 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -23,11 +23,21 @@
#include "qemu/osdep.h"
#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;
+} 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 +52,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 +74,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 +163,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,
@@ -159,4 +188,59 @@ static void bdrv_copy_on_read_init(void)
bdrv_register(&bdrv_copy_on_read);
}
+
+BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
+ QDict *node_options,
+ int flags, Error **errp)
+{
+ BlockDriverState *cor_filter_bs;
+ Error *local_err = NULL;
+
+ cor_filter_bs = bdrv_open(NULL, NULL, node_options, flags, errp);
+ if (cor_filter_bs == NULL) {
+ error_prepend(errp, "Could not create COR-filter node: ");
+ return NULL;
+ }
+
+ bdrv_drained_begin(bs);
+ bdrv_replace_node(bs, cor_filter_bs, &local_err);
+ bdrv_drained_end(bs);
+
+ if (local_err) {
+ bdrv_unref(cor_filter_bs);
+ error_propagate(errp, local_err);
+ return NULL;
+ }
+
+ return cor_filter_bs;
+}
+
+
+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);
+}
+
+
block_init(bdrv_copy_on_read_init);
diff --git a/block/copy-on-read.h b/block/copy-on-read.h
new file mode 100644
index 0000000..d6f2422
--- /dev/null
+++ b/block/copy-on-read.h
@@ -0,0 +1,35 @@
+/*
+ * 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"
+
+BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
+ QDict *node_options,
+ int flags, Error **errp);
+void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs);
+
+#endif /* BLOCK_COPY_ON_READ */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v10 3/9] qapi: add filter-node-name to block-stream
2020-09-29 12:38 [PATCH v10 0/9] Apply COR-filter to the block-stream permanently Andrey Shinkevich via
2020-09-29 12:38 ` [PATCH v10 1/9] copy-on-read: Support preadv/pwritev_part functions Andrey Shinkevich via
2020-09-29 12:38 ` [PATCH v10 2/9] copy-on-read: add filter append/drop functions Andrey Shinkevich via
@ 2020-09-29 12:38 ` Andrey Shinkevich via
2020-09-29 12:38 ` [PATCH v10 4/9] copy-on-read: pass base node name to COR driver Andrey Shinkevich via
` (5 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Andrey Shinkevich via @ 2020-09-29 12:38 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake,
den, vsementsov, andrey.shinkevich
Provide the possibility to pass the 'filter-node-name' parameter to the
block-stream job as it is done for the commit block job.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/monitor/block-hmp-cmds.c | 4 ++--
block/stream.c | 4 +++-
blockdev.c | 4 +++-
include/block/block_int.h | 7 ++++++-
qapi/block-core.json | 6 ++++++
5 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 4d3db5e..4e66775 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -507,8 +507,8 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
false, NULL, qdict_haskey(qdict, "speed"), speed, true,
- BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
- &error);
+ BLOCKDEV_ON_ERROR_REPORT, false, NULL, false, false, false,
+ false, &error);
hmp_handle_error(mon, error);
}
diff --git a/block/stream.c b/block/stream.c
index 8ce6729..e0540ee 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -221,7 +221,9 @@ static const BlockJobDriver stream_job_driver = {
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
int creation_flags, int64_t speed,
- BlockdevOnError on_error, Error **errp)
+ BlockdevOnError on_error,
+ const char *filter_node_name,
+ Error **errp)
{
StreamBlockJob *s;
BlockDriverState *iter;
diff --git a/blockdev.c b/blockdev.c
index bebd3ba..d719c47 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2489,6 +2489,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)
@@ -2571,7 +2572,8 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
}
stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
- job_flags, has_speed ? speed : 0, on_error, &local_err);
+ job_flags, has_speed ? speed : 0, on_error,
+ filter_node_name, &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto out;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 38cad9d..f782737 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1134,6 +1134,9 @@ int is_windows_drive(const char *filename);
* See @BlockJobCreateFlags
* @speed: The maximum speed, in bytes per second, or 0 for unlimited.
* @on_error: The action to take upon error.
+ * @filter_node_name: The node name that should be assigned to the filter
+ * driver that the commit job inserts into the graph above @bs. NULL means
+ * that a node name should be autogenerated.
* @errp: Error object.
*
* Start a streaming operation on @bs. Clusters that are unallocated
@@ -1146,7 +1149,9 @@ int is_windows_drive(const char *filename);
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
int creation_flags, int64_t speed,
- BlockdevOnError on_error, Error **errp);
+ BlockdevOnError on_error,
+ const char *filter_node_name,
+ Error **errp);
/**
* commit_start:
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3c16f1e..32fb097 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2533,6 +2533,11 @@
# 'stop' and 'enospc' can only be used if the block device
# supports io-status (see BlockInfo). Since 1.3.
#
+# @filter-node-name: the node name that should be assigned to the
+# filter driver that the stream job inserts into the graph
+# above @device. If this option is not given, a node name is
+# autogenerated. (Since: 5.2)
+#
# @auto-finalize: When false, this job will wait in a PENDING state after it has
# finished its work, waiting for @block-job-finalize before
# making any block graph changes.
@@ -2563,6 +2568,7 @@
'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
'*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
'*on-error': 'BlockdevOnError',
+ '*filter-node-name': 'str',
'*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
##
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v10 4/9] copy-on-read: pass base node name to COR driver
2020-09-29 12:38 [PATCH v10 0/9] Apply COR-filter to the block-stream permanently Andrey Shinkevich via
` (2 preceding siblings ...)
2020-09-29 12:38 ` [PATCH v10 3/9] qapi: add filter-node-name to block-stream Andrey Shinkevich via
@ 2020-09-29 12:38 ` Andrey Shinkevich via
2020-10-05 14:50 ` Vladimir Sementsov-Ogievskiy
2020-09-29 12:38 ` [PATCH v10 5/9] copy-on-read: limit guest COR activity to base in " Andrey Shinkevich via
` (4 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Andrey Shinkevich via @ 2020-09-29 12:38 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake,
den, vsementsov, andrey.shinkevich
To limit the guest's COR operations by the base node in the backing
chain during stream job, pass the base node name to the copy-on-read
driver. The rest of the functionality will be implemented in the patch
that follows.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
block/copy-on-read.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 3c8231f..e04092f 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -24,19 +24,23 @@
#include "block/block_int.h"
#include "qemu/module.h"
#include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qdict.h"
#include "block/copy-on-read.h"
typedef struct BDRVStateCOR {
bool active;
+ BlockDriverState *base_bs;
} BDRVStateCOR;
static int cor_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
+ BlockDriverState *base_bs = NULL;
BDRVStateCOR *state = bs->opaque;
+ const char *base_node = qdict_get_try_str(options, "base");
bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -52,7 +56,16 @@ 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 (base_node) {
+ qdict_del(options, "base");
+ base_bs = bdrv_lookup_bs(NULL, base_node, errp);
+ if (!base_bs) {
+ error_setg(errp, QERR_BASE_NOT_FOUND, base_node);
+ return -EINVAL;
+ }
+ }
state->active = true;
+ state->base_bs = base_bs;
/*
* We don't need to call bdrv_child_refresh_perms() now as the permissions
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v10 5/9] copy-on-read: limit guest COR activity to base in COR driver
2020-09-29 12:38 [PATCH v10 0/9] Apply COR-filter to the block-stream permanently Andrey Shinkevich via
` (3 preceding siblings ...)
2020-09-29 12:38 ` [PATCH v10 4/9] copy-on-read: pass base node name to COR driver Andrey Shinkevich via
@ 2020-09-29 12:38 ` Andrey Shinkevich via
2020-10-05 14:58 ` Vladimir Sementsov-Ogievskiy
2020-09-29 12:38 ` [PATCH v10 6/9] copy-on-read: skip non-guest reads if no copy needed Andrey Shinkevich via
` (3 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Andrey Shinkevich via @ 2020-09-29 12:38 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake,
den, vsementsov, andrey.shinkevich
Limit the guest's COR operations by the base node in the backing chain
when the base node name is given. It will be useful for a block stream
job when the COR-filter is applied.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
block/copy-on-read.c | 38 ++++++++++++++++++++++++++++++++++++--
1 file changed, 36 insertions(+), 2 deletions(-)
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index e04092f..f53f7e0 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -121,8 +121,42 @@ static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
size_t qiov_offset,
int flags)
{
- return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
- flags | BDRV_REQ_COPY_ON_READ);
+ int64_t n = 0;
+ int64_t size = offset + bytes;
+ int local_flags;
+ int ret;
+ BDRVStateCOR *state = bs->opaque;
+
+ if (!state->base_bs) {
+ return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
+ flags | BDRV_REQ_COPY_ON_READ);
+ }
+
+ while (offset < size) {
+ local_flags = flags;
+
+ /* In case of failure, try to copy-on-read anyway */
+ ret = bdrv_is_allocated(bs->file->bs, offset, bytes, &n);
+ if (!ret) {
+ ret = bdrv_is_allocated_above(bdrv_cow_bs(bs->file->bs),
+ state->base_bs, false, offset, n, &n);
+ if (ret > 0) {
+ local_flags |= BDRV_REQ_COPY_ON_READ;
+ }
+ }
+
+ ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset,
+ local_flags);
+ if (ret < 0) {
+ return ret;
+ }
+
+ offset += n;
+ qiov_offset += n;
+ bytes -= n;
+ }
+
+ return 0;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v10 6/9] copy-on-read: skip non-guest reads if no copy needed
2020-09-29 12:38 [PATCH v10 0/9] Apply COR-filter to the block-stream permanently Andrey Shinkevich via
` (4 preceding siblings ...)
2020-09-29 12:38 ` [PATCH v10 5/9] copy-on-read: limit guest COR activity to base in " Andrey Shinkevich via
@ 2020-09-29 12:38 ` Andrey Shinkevich via
2020-10-07 10:06 ` Vladimir Sementsov-Ogievskiy
2020-09-29 12:38 ` [PATCH v10 7/9] stream: skip filters when writing backing file name to QCOW2 header Andrey Shinkevich via
` (2 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Andrey Shinkevich via @ 2020-09-29 12:38 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake,
den, vsementsov, andrey.shinkevich
If the flag BDRV_REQ_PREFETCH was set, pass it further to the
COR-driver to skip unneeded reading. It can be taken into account for
the COR-algorithms optimization. That check is being made during the
block stream job by the moment.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
block/copy-on-read.c | 14 ++++++++++----
block/io.c | 2 +-
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index f53f7e0..5389dca 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -145,10 +145,16 @@ 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;
+ if ((flags & BDRV_REQ_PREFETCH) &
+ !(local_flags & BDRV_REQ_COPY_ON_READ)) {
+ /* Skip non-guest reads if no copy needed */
+ } else {
+
+ ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset,
+ local_flags);
+ if (ret < 0) {
+ return ret;
+ }
}
offset += n;
diff --git a/block/io.c b/block/io.c
index 11df188..62b75a5 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1388,7 +1388,7 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
qemu_iovec_init_buf(&local_qiov, bounce_buffer, pnum);
ret = bdrv_driver_preadv(bs, cluster_offset, pnum,
- &local_qiov, 0, 0);
+ &local_qiov, 0, flags & BDRV_REQ_PREFETCH);
if (ret < 0) {
goto err;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v10 7/9] stream: skip filters when writing backing file name to QCOW2 header
2020-09-29 12:38 [PATCH v10 0/9] Apply COR-filter to the block-stream permanently Andrey Shinkevich via
` (5 preceding siblings ...)
2020-09-29 12:38 ` [PATCH v10 6/9] copy-on-read: skip non-guest reads if no copy needed Andrey Shinkevich via
@ 2020-09-29 12:38 ` Andrey Shinkevich via
2020-10-07 10:14 ` Vladimir Sementsov-Ogievskiy
2020-09-29 12:38 ` [PATCH v10 8/9] block: remove unused backing-file name parameter Andrey Shinkevich via
2020-09-29 12:38 ` [PATCH v10 9/9] block: apply COR-filter to block-stream jobs Andrey Shinkevich via
8 siblings, 1 reply; 25+ messages in thread
From: Andrey Shinkevich via @ 2020-09-29 12:38 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake,
den, vsementsov, andrey.shinkevich
Avoid writing a filter JSON-name to QCOW2 image when the backing file
is changed after the block stream job.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
block/stream.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index e0540ee..b0719e9 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
BlockDriverState *bs = blk_bs(bjob->blk);
BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
+ BlockDriverState *base_metadata = 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 (base_metadata) {
+ base_id = base_metadata->filename;
+ if (base_metadata->drv) {
+ base_fmt = base_metadata->drv->format_name;
}
}
bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v10 8/9] block: remove unused backing-file name parameter
2020-09-29 12:38 [PATCH v10 0/9] Apply COR-filter to the block-stream permanently Andrey Shinkevich via
` (6 preceding siblings ...)
2020-09-29 12:38 ` [PATCH v10 7/9] stream: skip filters when writing backing file name to QCOW2 header Andrey Shinkevich via
@ 2020-09-29 12:38 ` Andrey Shinkevich via
2020-10-07 10:21 ` Vladimir Sementsov-Ogievskiy
2020-09-29 12:38 ` [PATCH v10 9/9] block: apply COR-filter to block-stream jobs Andrey Shinkevich via
8 siblings, 1 reply; 25+ messages in thread
From: Andrey Shinkevich via @ 2020-09-29 12:38 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake,
den, vsementsov, andrey.shinkevich
The block stream QMP parameter backing-file is in use no more. It
designates a backing file name to set in QCOW2 image header after the
block stream job finished. The base file name is used instead.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
block/monitor/block-hmp-cmds.c | 2 +-
block/stream.c | 6 +-----
blockdev.c | 17 +----------------
include/block/block_int.h | 2 +-
qapi/block-core.json | 17 +----------------
5 files changed, 5 insertions(+), 39 deletions(-)
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 4e66775..5f19499 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -506,7 +506,7 @@ 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,
+ 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 b0719e9..fe2663f 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -34,7 +34,6 @@ typedef struct StreamBlockJob {
BlockDriverState *base_overlay; /* COW overlay (stream from this) */
BlockDriverState *above_base; /* Node directly above the base */
BlockdevOnError on_error;
- char *backing_file_str;
bool bs_read_only;
bool chain_frozen;
} StreamBlockJob;
@@ -103,8 +102,6 @@ static void stream_clean(Job *job)
blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
bdrv_reopen_set_read_only(bs, true, NULL);
}
-
- g_free(s->backing_file_str);
}
static int coroutine_fn stream_run(Job *job, Error **errp)
@@ -220,7 +217,7 @@ static const BlockJobDriver stream_job_driver = {
};
void stream_start(const char *job_id, BlockDriverState *bs,
- BlockDriverState *base, const char *backing_file_str,
+ BlockDriverState *base,
int creation_flags, int64_t speed,
BlockdevOnError on_error,
const char *filter_node_name,
@@ -295,7 +292,6 @@ 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->bs_read_only = bs_read_only;
s->chain_frozen = true;
diff --git a/blockdev.c b/blockdev.c
index d719c47..b223601 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2486,7 +2486,6 @@ out:
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_speed, int64_t speed,
bool has_on_error, BlockdevOnError on_error,
bool has_filter_node_name, const char *filter_node_name,
@@ -2498,7 +2497,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) {
@@ -2526,7 +2524,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) {
@@ -2541,7 +2538,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 */
@@ -2553,17 +2549,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
}
}
- /* if we are streaming the entire chain, the result will have no backing
- * file, and specifying one is therefore an error */
- if (base_bs == NULL && has_backing_file) {
- error_setg(errp, "backing file specified, but streaming the "
- "entire chain");
- 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;
}
@@ -2571,7 +2556,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,
job_flags, has_speed ? speed : 0, on_error,
filter_node_name, &local_err);
if (local_err) {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f782737..bbe2ee6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1147,7 +1147,7 @@ int is_windows_drive(const char *filename);
* BlockDriverState.
*/
void stream_start(const char *job_id, BlockDriverState *bs,
- BlockDriverState *base, const char *backing_file_str,
+ BlockDriverState *base,
int creation_flags, int64_t speed,
BlockdevOnError on_error,
const char *filter_node_name,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 32fb097..6358606 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2512,21 +2512,6 @@
# @base-node: the node name of the backing file.
# It cannot be set if @base is also set. (Since 2.8)
#
-# @backing-file: The backing file string to write into the top
-# image. This filename is not validated.
-#
-# If a pathname string is such that it cannot be
-# resolved by QEMU, that means that subsequent QMP or
-# HMP commands must use node-names for the image in
-# question, as filename lookup methods will fail.
-#
-# If not specified, QEMU will automatically determine
-# the backing file string to use, or error out if there
-# is no obvious choice. Care should be taken when
-# specifying the string, to specify a valid filename or
-# protocol.
-# (Since 2.1)
-#
# @speed: the maximum speed, in bytes per second
#
# @on-error: the action to take on an error (default report).
@@ -2566,7 +2551,7 @@
##
{ 'command': 'block-stream',
'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
- '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
+ '*base-node': 'str', '*speed': 'int',
'*on-error': 'BlockdevOnError',
'*filter-node-name': 'str',
'*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v10 9/9] block: apply COR-filter to block-stream jobs
2020-09-29 12:38 [PATCH v10 0/9] Apply COR-filter to the block-stream permanently Andrey Shinkevich via
` (7 preceding siblings ...)
2020-09-29 12:38 ` [PATCH v10 8/9] block: remove unused backing-file name parameter Andrey Shinkevich via
@ 2020-09-29 12:38 ` Andrey Shinkevich via
2020-10-07 17:27 ` Andrey Shinkevich
8 siblings, 1 reply; 25+ messages in thread
From: Andrey Shinkevich via @ 2020-09-29 12:38 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake,
den, vsementsov, andrey.shinkevich
This patch completes the series with the COR-filter insertion for
block-stream operations. Adding the filter makes it possible for copied
regions to be discarded in backing files during the block-stream job,
what will reduce the disk overuse.
The COR-filter insertion incurs changes in the iotests case
245:test_block_stream_4 that reopens the backing chain during a
block-stream job. There are changes in the iotests #030 as well.
The iotests case 030:test_stream_parallel was deleted due to multiple
conflicts between the concurrent job operations over the same backing
chain. The base backing node for one job is the top node for another
job. It may change due to the filter node inserted into the backing
chain while both jobs are running. Another issue is that the parts of
the backing chain are being frozen by the running job and may not be
changed by the concurrent job when needed. The concept of the parallel
jobs with common nodes is considered vital no more.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
block/stream.c | 93 ++++++++++++++++++++++++++++++----------------
tests/qemu-iotests/030 | 51 +++----------------------
tests/qemu-iotests/030.out | 4 +-
tests/qemu-iotests/141.out | 2 +-
tests/qemu-iotests/245 | 19 +++++++---
5 files changed, 83 insertions(+), 86 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index fe2663f..240b3dc 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -17,8 +17,10 @@
#include "block/blockjob_int.h"
#include "qapi/error.h"
#include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qdict.h"
#include "qemu/ratelimit.h"
#include "sysemu/block-backend.h"
+#include "block/copy-on-read.h"
enum {
/*
@@ -33,6 +35,8 @@ typedef struct StreamBlockJob {
BlockJob common;
BlockDriverState *base_overlay; /* COW overlay (stream from this) */
BlockDriverState *above_base; /* Node directly above the base */
+ BlockDriverState *cor_filter_bs;
+ BlockDriverState *target_bs;
BlockdevOnError on_error;
bool bs_read_only;
bool chain_frozen;
@@ -52,23 +56,20 @@ static void stream_abort(Job *job)
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
if (s->chain_frozen) {
- BlockJob *bjob = &s->common;
- bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->above_base);
+ bdrv_unfreeze_backing_chain(s->cor_filter_bs, s->above_base);
}
}
static int stream_prepare(Job *job)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
- BlockJob *bjob = &s->common;
- BlockDriverState *bs = blk_bs(bjob->blk);
- BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
+ BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
BlockDriverState *base_metadata = bdrv_skip_filters(base);
Error *local_err = NULL;
int ret = 0;
- bdrv_unfreeze_backing_chain(bs, s->above_base);
+ bdrv_unfreeze_backing_chain(s->cor_filter_bs, s->above_base);
s->chain_frozen = false;
if (bdrv_cow_child(unfiltered_bs)) {
@@ -94,13 +95,14 @@ static void stream_clean(Job *job)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockJob *bjob = &s->common;
- BlockDriverState *bs = blk_bs(bjob->blk);
+
+ bdrv_cor_filter_drop(s->cor_filter_bs);
/* 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);
}
}
@@ -108,9 +110,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockBackend *blk = s->common.blk;
- BlockDriverState *bs = blk_bs(blk);
- BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
- bool enable_cor = !bdrv_cow_child(s->base_overlay);
+ BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
int64_t len;
int64_t offset = 0;
uint64_t delay_ns = 0;
@@ -122,21 +122,12 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
return 0;
}
- len = bdrv_getlength(bs);
+ len = bdrv_getlength(s->target_bs);
if (len < 0) {
return len;
}
job_progress_set_remaining(&s->common.job, len);
- /* Turn on copy-on-read for the whole block device so that guest read
- * requests help us make progress. Only do this when copying the entire
- * backing chain since the copy-on-read operation does not take base into
- * account.
- */
- if (enable_cor) {
- bdrv_enable_copy_on_read(bs);
- }
-
for ( ; offset < len; offset += n) {
bool copy;
int ret;
@@ -195,10 +186,6 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
}
}
- if (enable_cor) {
- bdrv_disable_copy_on_read(bs);
- }
-
/* Do not remove the backing file if an error was there but ignored. */
return error;
}
@@ -228,6 +215,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
bool bs_read_only;
int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
BlockDriverState *base_overlay = bdrv_find_overlay(bs, base);
+ BlockDriverState *cor_filter_bs = NULL;
BlockDriverState *above_base;
if (!base_overlay) {
@@ -262,17 +250,51 @@ void stream_start(const char *job_id, BlockDriverState *bs,
}
}
- /* Prevent concurrent jobs trying to modify the graph structure here, we
- * already have our own plans. Also don't allow resize as the image size is
- * queried only at the job start and then cached. */
- s = block_job_create(job_id, &stream_job_driver, NULL, bs,
- basic_flags | BLK_PERM_GRAPH_MOD,
- basic_flags | BLK_PERM_WRITE,
+ QDict *opts = qdict_new();
+
+ qdict_put_str(opts, "driver", "copy-on-read");
+ qdict_put_str(opts, "file", bdrv_get_node_name(bs));
+ if (base) {
+ qdict_put_str(opts, "base", base->node_name);
+ }
+ if (filter_node_name) {
+ qdict_put_str(opts, "node-name", filter_node_name);
+ }
+
+ cor_filter_bs = bdrv_cor_filter_append(bs, opts, BDRV_O_RDWR, errp);
+ if (cor_filter_bs == NULL) {
+ goto fail;
+ }
+
+ if (!filter_node_name) {
+ cor_filter_bs->implicit = true;
+ }
+
+ if (bdrv_freeze_backing_chain(cor_filter_bs, bs, errp) < 0) {
+ bdrv_cor_filter_drop(cor_filter_bs);
+ cor_filter_bs = NULL;
+ goto fail;
+ }
+
+ s = block_job_create(job_id, &stream_job_driver, NULL, cor_filter_bs,
+ BLK_PERM_CONSISTENT_READ,
+ basic_flags | BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD,
speed, creation_flags, NULL, NULL, errp);
if (!s) {
goto fail;
}
+ /*
+ * Prevent concurrent jobs trying to modify the graph structure here, we
+ * already have our own plans. Also don't allow resize as the image size is
+ * queried only at the job start and then cached.
+ */
+ if (block_job_add_bdrv(&s->common, "active node", bs,
+ basic_flags | BLK_PERM_GRAPH_MOD,
+ basic_flags | BLK_PERM_WRITE, &error_abort)) {
+ goto fail;
+ }
+
/* Block all intermediate nodes between bs and base, because they will
* disappear from the chain after this operation. The streaming job reads
* every block only once, assuming that it doesn't change, so forbid writes
@@ -292,6 +314,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
s->base_overlay = base_overlay;
s->above_base = above_base;
+ s->cor_filter_bs = cor_filter_bs;
+ s->target_bs = bs;
s->bs_read_only = bs_read_only;
s->chain_frozen = true;
@@ -304,5 +328,10 @@ fail:
if (bs_read_only) {
bdrv_reopen_set_read_only(bs, true, NULL);
}
- bdrv_unfreeze_backing_chain(bs, above_base);
+ if (cor_filter_bs) {
+ bdrv_unfreeze_backing_chain(cor_filter_bs, above_base);
+ bdrv_cor_filter_drop(cor_filter_bs);
+ } else {
+ bdrv_unfreeze_backing_chain(bs, above_base);
+ }
}
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index dcb4b5d..0064590 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -227,61 +227,20 @@ class TestParallelOps(iotests.QMPTestCase):
for img in self.imgs:
os.remove(img)
- # Test that it's possible to run several block-stream operations
- # in parallel in the same snapshot chain
- @unittest.skipIf(os.environ.get('QEMU_CHECK_BLOCK_AUTO'), 'disabled in CI')
- def test_stream_parallel(self):
- self.assert_no_active_block_jobs()
-
- # Check that the maps don't match before the streaming operations
- for i in range(2, self.num_imgs, 2):
- self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[i]),
- qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[i-1]),
- 'image file map matches backing file before streaming')
-
- # Create all streaming jobs
- pending_jobs = []
- for i in range(2, self.num_imgs, 2):
- node_name = 'node%d' % i
- job_id = 'stream-%s' % node_name
- pending_jobs.append(job_id)
- result = self.vm.qmp('block-stream', device=node_name, job_id=job_id, base=self.imgs[i-2], speed=1024)
- self.assert_qmp(result, 'return', {})
-
- for job in pending_jobs:
- result = self.vm.qmp('block-job-set-speed', device=job, speed=0)
- self.assert_qmp(result, 'return', {})
-
- # Wait for all jobs to be finished.
- while len(pending_jobs) > 0:
- for event in self.vm.get_qmp_events(wait=True):
- if event['event'] == 'BLOCK_JOB_COMPLETED':
- job_id = self.dictpath(event, 'data/device')
- self.assertTrue(job_id in pending_jobs)
- self.assert_qmp_absent(event, 'data/error')
- pending_jobs.remove(job_id)
-
- self.assert_no_active_block_jobs()
- self.vm.shutdown()
-
- # Check that all maps match now
- for i in range(2, self.num_imgs, 2):
- self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i]),
- qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]),
- 'image file map does not match backing file after streaming')
-
# Test that it's not possible to perform two block-stream
# operations if there are nodes involved in both.
def test_overlapping_1(self):
self.assert_no_active_block_jobs()
# Set a speed limit to make sure that this job blocks the rest
- result = self.vm.qmp('block-stream', device='node4', job_id='stream-node4', base=self.imgs[1], speed=1024*1024)
+ result = self.vm.qmp('block-stream', device='node4',
+ job_id='stream-node4', base=self.imgs[1],
+ filter_node_name='stream-filter', speed=1024*1024)
self.assert_qmp(result, 'return', {})
result = self.vm.qmp('block-stream', device='node5', job_id='stream-node5', base=self.imgs[2])
self.assert_qmp(result, 'error/desc',
- "Node 'node4' is busy: block device is in use by block job: stream")
+ "Node 'stream-filter' is busy: block device is in use by block job: stream")
result = self.vm.qmp('block-stream', device='node3', job_id='stream-node3', base=self.imgs[2])
self.assert_qmp(result, 'error/desc',
@@ -294,7 +253,7 @@ class TestParallelOps(iotests.QMPTestCase):
# block-commit should also fail if it touches nodes used by the stream job
result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[4], job_id='commit-node4')
self.assert_qmp(result, 'error/desc',
- "Node 'node4' is busy: block device is in use by block job: stream")
+ "Node 'stream-filter' is busy: block device is in use by block job: stream")
result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[1], top=self.imgs[3], job_id='commit-node1')
self.assert_qmp(result, 'error/desc',
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index 6d9bee1..5eb508d 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-...........................
+..........................
----------------------------------------------------------------------
-Ran 27 tests
+Ran 26 tests
OK
diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out
index 08e0aec..028a16f 100644
--- a/tests/qemu-iotests/141.out
+++ b/tests/qemu-iotests/141.out
@@ -99,7 +99,7 @@ wrote 1048576/1048576 bytes at offset 0
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}
{'execute': 'blockdev-del', 'arguments': {'node-name': 'drv0'}}
-{"error": {"class": "GenericError", "desc": "Node drv0 is in use"}}
+{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: stream"}}
{'execute': 'block-job-cancel', 'arguments': {'device': 'job0'}}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "job0"}}
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index e60c832..940e85a 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -899,17 +899,26 @@ class TestBlockdevReopen(iotests.QMPTestCase):
# make hd1 read-only and block-stream requires it to be read-write
# (Which error message appears depends on whether the stream job is
# already done with copying at this point.)
- self.reopen(opts, {},
+ # As the COR-filter node is inserted into the backing chain with the
+ # 'block-stream' operation, we move the options to their proper nodes.
+ opts = hd_opts(1)
+ opts['backing'] = hd_opts(2)
+ opts['backing']['backing'] = None
+ self.reopen(opts, {'read-only': True},
["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"])
# We can't remove hd2 while the stream job is ongoing
- opts['backing']['backing'] = None
- self.reopen(opts, {'backing.read-only': False}, "Cannot change 'backing' link from 'hd1' to 'hd2'")
+ opts['backing'] = None
+ self.reopen(opts, {'read-only': False},
+ "Cannot change 'backing' link from 'hd1' to 'hd2'")
- # We can detach hd1 from hd0 because it doesn't affect the stream job
+ # We can't detach hd1 from hd0 because there is the COR-filter implicit
+ # node in between.
+ opts = hd_opts(0)
opts['backing'] = None
- self.reopen(opts)
+ self.reopen(opts, {},
+ "Cannot change backing link if 'hd0' has an implicit backing file")
self.vm.run_job('stream0', auto_finalize = False, auto_dismiss = True)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v10 2/9] copy-on-read: add filter append/drop functions
2020-09-29 12:38 ` [PATCH v10 2/9] copy-on-read: add filter append/drop functions Andrey Shinkevich via
@ 2020-10-05 13:34 ` Vladimir Sementsov-Ogievskiy
2020-10-05 16:23 ` Andrey Shinkevich
0 siblings, 1 reply; 25+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-05 13:34 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake, den
29.09.2020 15:38, Andrey Shinkevich wrote:
> Provide API for the COR-filter insertion/removal.
> Also, drop the filter child permissions for an inactive state when the
> filter node is being removed.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> block/copy-on-read.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> block/copy-on-read.h | 35 ++++++++++++++++++++++
> 2 files changed, 119 insertions(+)
> create mode 100644 block/copy-on-read.h
>
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index cb03e0f..3c8231f 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -23,11 +23,21 @@
> #include "qemu/osdep.h"
> #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;
> +} 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 +52,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 +74,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 +163,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,
> @@ -159,4 +188,59 @@ static void bdrv_copy_on_read_init(void)
> bdrv_register(&bdrv_copy_on_read);
> }
>
> +
> +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
> + QDict *node_options,
> + int flags, Error **errp)
Ok, now function can add ~any filter, not only COR.. But it's a pair for bdrv_cor_filter_drop(), and with "active" hack we don't want make the functions generic I think. So it's OK for now to keep function here and named _cor_.
> +{
> + BlockDriverState *cor_filter_bs;
> + Error *local_err = NULL;
> +
> + cor_filter_bs = bdrv_open(NULL, NULL, node_options, flags, errp);
> + if (cor_filter_bs == NULL) {
> + error_prepend(errp, "Could not create COR-filter node: ");
> + return NULL;
> + }
You've dropped setting ->implicit field if filter_node_name not specified. Probably caller now can do it.. I don't really care about implicit case, so it's OK for me if it works with iotests.
So,
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v10 4/9] copy-on-read: pass base node name to COR driver
2020-09-29 12:38 ` [PATCH v10 4/9] copy-on-read: pass base node name to COR driver Andrey Shinkevich via
@ 2020-10-05 14:50 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 25+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-05 14:50 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake, den
29.09.2020 15:38, Andrey Shinkevich wrote:
> To limit the guest's COR operations by the base node in the backing
Better to drop "guest's " word. We don't to limit the guest in any
> chain during stream job, pass the base node name to the copy-on-read
> driver. The rest of the functionality will be implemented in the patch
> that follows.
>
Oops. Seems we want bottom-node, not base, in according with stream job
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> block/copy-on-read.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index 3c8231f..e04092f 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -24,19 +24,23 @@
> #include "block/block_int.h"
> #include "qemu/module.h"
> #include "qapi/error.h"
> +#include "qapi/qmp/qerror.h"
> #include "qapi/qmp/qdict.h"
> #include "block/copy-on-read.h"
>
>
> typedef struct BDRVStateCOR {
> bool active;
> + BlockDriverState *base_bs;
> } BDRVStateCOR;
>
>
> static int cor_open(BlockDriverState *bs, QDict *options, int flags,
> Error **errp)
> {
> + BlockDriverState *base_bs = NULL;
> BDRVStateCOR *state = bs->opaque;
> + const char *base_node = qdict_get_try_str(options, "base");
>
> bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
> BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
> @@ -52,7 +56,16 @@ 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 (base_node) {
> + qdict_del(options, "base");
> + base_bs = bdrv_lookup_bs(NULL, base_node, errp);
> + if (!base_bs) {
> + error_setg(errp, QERR_BASE_NOT_FOUND, base_node);
> + return -EINVAL;
> + }
> + }
> state->active = true;
> + state->base_bs = base_bs;
>
> /*
> * We don't need to call bdrv_child_refresh_perms() now as the permissions
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v10 5/9] copy-on-read: limit guest COR activity to base in COR driver
2020-09-29 12:38 ` [PATCH v10 5/9] copy-on-read: limit guest COR activity to base in " Andrey Shinkevich via
@ 2020-10-05 14:58 ` Vladimir Sementsov-Ogievskiy
2020-10-05 16:45 ` Andrey Shinkevich
2020-10-05 18:18 ` Andrey Shinkevich
0 siblings, 2 replies; 25+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-05 14:58 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake, den
29.09.2020 15:38, Andrey Shinkevich wrote:
> Limit the guest's COR operations by the base node in the backing chain
> when the base node name is given. It will be useful for a block stream
> job when the COR-filter is applied.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> block/copy-on-read.c | 38 ++++++++++++++++++++++++++++++++++++--
> 1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index e04092f..f53f7e0 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -121,8 +121,42 @@ static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
> size_t qiov_offset,
> int flags)
> {
> - return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
> - flags | BDRV_REQ_COPY_ON_READ);
> + int64_t n = 0;
> + int64_t size = offset + bytes;
> + int local_flags;
> + int ret;
> + BDRVStateCOR *state = bs->opaque;
> +
> + if (!state->base_bs) {
> + return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
> + flags | BDRV_REQ_COPY_ON_READ);
> + }
> +
> + while (offset < size) {
> + local_flags = flags;
> +
> + /* In case of failure, try to copy-on-read anyway */
But you add the flag only in case of success.. On any failure of furhter is*allocated calls we should set the flag.
> + ret = bdrv_is_allocated(bs->file->bs, offset, bytes, &n);
> + if (!ret) {
> + ret = bdrv_is_allocated_above(bdrv_cow_bs(bs->file->bs),
> + state->base_bs, false, offset, n, &n);
> + if (ret > 0) {
> + local_flags |= BDRV_REQ_COPY_ON_READ;
> + }
> + }
> +
> + 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;
> }
>
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v10 2/9] copy-on-read: add filter append/drop functions
2020-10-05 13:34 ` Vladimir Sementsov-Ogievskiy
@ 2020-10-05 16:23 ` Andrey Shinkevich
0 siblings, 0 replies; 25+ messages in thread
From: Andrey Shinkevich @ 2020-10-05 16:23 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake, den
On 05.10.2020 16:34, Vladimir Sementsov-Ogievskiy wrote:
> 29.09.2020 15:38, Andrey Shinkevich wrote:
>> Provide API for the COR-filter insertion/removal.
>> Also, drop the filter child permissions for an inactive state when the
>> filter node is being removed.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>> block/copy-on-read.c | 84
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> block/copy-on-read.h | 35 ++++++++++++++++++++++
>> 2 files changed, 119 insertions(+)
>> create mode 100644 block/copy-on-read.h
>>
>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>> index cb03e0f..3c8231f 100644
>> --- a/block/copy-on-read.c
>> +++ b/block/copy-on-read.c
>> @@ -23,11 +23,21 @@
>> #include "qemu/osdep.h"
>> #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;
>> +} 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 +52,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 +74,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 +163,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,
>> @@ -159,4 +188,59 @@ static void bdrv_copy_on_read_init(void)
>> bdrv_register(&bdrv_copy_on_read);
>> }
>> +
>> +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
>> + QDict *node_options,
>> + int flags, Error **errp)
>
>
> Ok, now function can add ~any filter, not only COR.. But it's a pair for
> bdrv_cor_filter_drop(), and with "active" hack we don't want make the
> functions generic I think. So it's OK for now to keep function here and
> named _cor_.
>
>> +{
>> + BlockDriverState *cor_filter_bs;
>> + Error *local_err = NULL;
>> +
>> + cor_filter_bs = bdrv_open(NULL, NULL, node_options, flags, errp);
>> + if (cor_filter_bs == NULL) {
>> + error_prepend(errp, "Could not create COR-filter node: ");
>> + return NULL;
>> + }
>
> You've dropped setting ->implicit field if filter_node_name not
> specified. Probably caller now can do it.. I don't really care about
> implicit case, so it's OK for me if it works with iotests.
Thank you for your R-B. The idea behind setting the 'implicit' member by
a caller is to prepare the code for the node replacement by a function
at the block generic layer in future. In the scope of this series, that
may be better to keep it here.
Andrey
>
> So,
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v10 5/9] copy-on-read: limit guest COR activity to base in COR driver
2020-10-05 14:58 ` Vladimir Sementsov-Ogievskiy
@ 2020-10-05 16:45 ` Andrey Shinkevich
2020-10-05 18:18 ` Andrey Shinkevich
1 sibling, 0 replies; 25+ messages in thread
From: Andrey Shinkevich @ 2020-10-05 16:45 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake, den
On 05.10.2020 17:58, Vladimir Sementsov-Ogievskiy wrote:
> 29.09.2020 15:38, Andrey Shinkevich wrote:
>> Limit the guest's COR operations by the base node in the backing chain
>> when the base node name is given. It will be useful for a block stream
>> job when the COR-filter is applied.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>> block/copy-on-read.c | 38 ++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>> index e04092f..f53f7e0 100644
>> --- a/block/copy-on-read.c
>> +++ b/block/copy-on-read.c
>> @@ -121,8 +121,42 @@ static int coroutine_fn
>> cor_co_preadv_part(BlockDriverState *bs,
>> size_t qiov_offset,
>> int flags)
>> {
>> - return bdrv_co_preadv_part(bs->file, offset, bytes, qiov,
>> qiov_offset,
>> - flags | BDRV_REQ_COPY_ON_READ);
>> + int64_t n = 0;
>> + int64_t size = offset + bytes;
>> + int local_flags;
>> + int ret;
>> + BDRVStateCOR *state = bs->opaque;
>> +
>> + if (!state->base_bs) {
>> + return bdrv_co_preadv_part(bs->file, offset, bytes, qiov,
>> qiov_offset,
>> + flags | BDRV_REQ_COPY_ON_READ);
>> + }
>> +
>> + while (offset < size) {
>> + local_flags = flags;
>> +
>> + /* In case of failure, try to copy-on-read anyway */
>
> But you add the flag only in case of success.. On any failure of furhter
> is*allocated calls we should set the flag.
Yes, thanks.
Andrey
>
>> + ret = bdrv_is_allocated(bs->file->bs, offset, bytes, &n);
>> + if (!ret) {
>> + ret = bdrv_is_allocated_above(bdrv_cow_bs(bs->file->bs),
>> + state->base_bs, false,
>> offset, n, &n);
>> + if (ret > 0) {
>> + local_flags |= BDRV_REQ_COPY_ON_READ;
>> + }
>> + }
>> +
>> + 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;
>> }
>>
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v10 5/9] copy-on-read: limit guest COR activity to base in COR driver
2020-10-05 14:58 ` Vladimir Sementsov-Ogievskiy
2020-10-05 16:45 ` Andrey Shinkevich
@ 2020-10-05 18:18 ` Andrey Shinkevich
1 sibling, 0 replies; 25+ messages in thread
From: Andrey Shinkevich @ 2020-10-05 18:18 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake, den
On 05.10.2020 17:58, Vladimir Sementsov-Ogievskiy wrote:
> 29.09.2020 15:38, Andrey Shinkevich wrote:
>> Limit the guest's COR operations by the base node in the backing chain
>> when the base node name is given. It will be useful for a block stream
>> job when the COR-filter is applied.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>> block/copy-on-read.c | 38 ++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>> index e04092f..f53f7e0 100644
>> --- a/block/copy-on-read.c
>> +++ b/block/copy-on-read.c
>> @@ -121,8 +121,42 @@ static int coroutine_fn
>> cor_co_preadv_part(BlockDriverState *bs,
>> size_t qiov_offset,
>> int flags)
>> {
>> - return bdrv_co_preadv_part(bs->file, offset, bytes, qiov,
>> qiov_offset,
>> - flags | BDRV_REQ_COPY_ON_READ);
>> + int64_t n = 0;
>> + int64_t size = offset + bytes;
>> + int local_flags;
>> + int ret;
>> + BDRVStateCOR *state = bs->opaque;
>> +
>> + if (!state->base_bs) {
>> + return bdrv_co_preadv_part(bs->file, offset, bytes, qiov,
>> qiov_offset,
>> + flags | BDRV_REQ_COPY_ON_READ);
>> + }
>> +
>> + while (offset < size) {
>> + local_flags = flags;
>> +
>> + /* In case of failure, try to copy-on-read anyway */
>
> But you add the flag only in case of success.. On any failure of furhter
> is*allocated calls we should set the flag.
>
Actually, myself would prefer returning the error instead.
Andrey
>> + ret = bdrv_is_allocated(bs->file->bs, offset, bytes, &n);
>> + if (!ret) {
>> + ret = bdrv_is_allocated_above(bdrv_cow_bs(bs->file->bs),
>> + state->base_bs, false,
>> offset, n, &n);
>> + if (ret > 0) {
>> + local_flags |= BDRV_REQ_COPY_ON_READ;
>> + }
>> + }
>> +
>> + 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;
>> }
>>
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v10 6/9] copy-on-read: skip non-guest reads if no copy needed
2020-09-29 12:38 ` [PATCH v10 6/9] copy-on-read: skip non-guest reads if no copy needed Andrey Shinkevich via
@ 2020-10-07 10:06 ` Vladimir Sementsov-Ogievskiy
2020-10-07 19:01 ` Andrey Shinkevich
0 siblings, 1 reply; 25+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-07 10:06 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake, den
29.09.2020 15:38, Andrey Shinkevich wrote:
> If the flag BDRV_REQ_PREFETCH was set, pass it further to the
> COR-driver to skip unneeded reading. It can be taken into account for
> the COR-algorithms optimization. That check is being made during the
> block stream job by the moment.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> block/copy-on-read.c | 14 ++++++++++----
> block/io.c | 2 +-
> 2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index f53f7e0..5389dca 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -145,10 +145,16 @@ 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;
> + if ((flags & BDRV_REQ_PREFETCH) &
BDRV_REQ_PREFETCH is documented to be only used with BDRV_REQ_COPY_ON_READ. But here
BDRV_REQ_COPY_ON_READ appears intermediately. We should change documentation in block.h
in a separate patch (and probably code in bdrv_aligned_preadv())
> + !(local_flags & BDRV_REQ_COPY_ON_READ)) {
> + /* Skip non-guest reads if no copy needed */
> + } else {
> +
extra new-line ?
> + ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset,
> + local_flags);
> + if (ret < 0) {
> + return ret;
> + }
> }
>
> offset += n;
> diff --git a/block/io.c b/block/io.c
> index 11df188..62b75a5 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1388,7 +1388,7 @@ static int coroutine_fn jk(BdrvChild *child,
> qemu_iovec_init_buf(&local_qiov, bounce_buffer, pnum);
>
> ret = bdrv_driver_preadv(bs, cluster_offset, pnum,
> - &local_qiov, 0, 0);
> + &local_qiov, 0, flags & BDRV_REQ_PREFETCH);
Why? In this place we want to read. We'll write back the data a few lines below. What will we write,
if underlying driver decide to do nothing because of BDRV_REQ_PREFETCH?
> if (ret < 0) {
> goto err;
> }
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v10 7/9] stream: skip filters when writing backing file name to QCOW2 header
2020-09-29 12:38 ` [PATCH v10 7/9] stream: skip filters when writing backing file name to QCOW2 header Andrey Shinkevich via
@ 2020-10-07 10:14 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 25+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-07 10:14 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake, den
29.09.2020 15:38, Andrey Shinkevich wrote:
> Avoid writing a filter JSON-name to QCOW2 image when the backing file
> is changed after the block stream job.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> block/stream.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/block/stream.c b/block/stream.c
> index e0540ee..b0719e9 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
> BlockDriverState *bs = blk_bs(bjob->blk);
> BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
> BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
> + BlockDriverState *base_metadata = bdrv_skip_filters(base);
Could we call it base_unfiltered in according with all such things?
> 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;
Seems backing_file_str becomes unused and we should drop it. So, actually,
this patch fix two problems:
- do not save backing_file_str at stream start (as base may change during the job)
- avoid json filters in final qcow2 image metadata
> - if (base->drv) {
> - base_fmt = base->drv->format_name;
> + if (base_metadata) {
> + base_id = base_metadata->filename;
> + if (base_metadata->drv) {
> + base_fmt = base_metadata->drv->format_name;
> }
> }
> bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v10 8/9] block: remove unused backing-file name parameter
2020-09-29 12:38 ` [PATCH v10 8/9] block: remove unused backing-file name parameter Andrey Shinkevich via
@ 2020-10-07 10:21 ` Vladimir Sementsov-Ogievskiy
2020-10-07 19:32 ` Andrey Shinkevich
0 siblings, 1 reply; 25+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-07 10:21 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake, den
29.09.2020 15:38, Andrey Shinkevich wrote:
> The block stream QMP parameter backing-file is in use no more. It
> designates a backing file name to set in QCOW2 image header after the
> block stream job finished. The base file name is used instead.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
We can't just remove it without a deprecation period of three releases.
So actually, in a previous patch, we should implement new behavior for
automatic backing-file detection if this parameter is unspecified. Amd
keep old behavior for backing-file-name if it is given.
Hmm. Or, probably, we can use direct base for base-filename? And in cases
when we should skip filters (for example of parallel jobs) user should
specify backing-file explicitly?
> ---
> block/monitor/block-hmp-cmds.c | 2 +-
> block/stream.c | 6 +-----
> blockdev.c | 17 +----------------
> include/block/block_int.h | 2 +-
> qapi/block-core.json | 17 +----------------
> 5 files changed, 5 insertions(+), 39 deletions(-)
>
> diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
> index 4e66775..5f19499 100644
> --- a/block/monitor/block-hmp-cmds.c
> +++ b/block/monitor/block-hmp-cmds.c
> @@ -506,7 +506,7 @@ 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,
> + 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 b0719e9..fe2663f 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -34,7 +34,6 @@ typedef struct StreamBlockJob {
> BlockDriverState *base_overlay; /* COW overlay (stream from this) */
> BlockDriverState *above_base; /* Node directly above the base */
> BlockdevOnError on_error;
> - char *backing_file_str;
> bool bs_read_only;
> bool chain_frozen;
> } StreamBlockJob;
> @@ -103,8 +102,6 @@ static void stream_clean(Job *job)
> blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
> bdrv_reopen_set_read_only(bs, true, NULL);
> }
> -
> - g_free(s->backing_file_str);
> }
>
> static int coroutine_fn stream_run(Job *job, Error **errp)
> @@ -220,7 +217,7 @@ static const BlockJobDriver stream_job_driver = {
> };
>
> void stream_start(const char *job_id, BlockDriverState *bs,
> - BlockDriverState *base, const char *backing_file_str,
> + BlockDriverState *base,
> int creation_flags, int64_t speed,
> BlockdevOnError on_error,
> const char *filter_node_name,
> @@ -295,7 +292,6 @@ 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->bs_read_only = bs_read_only;
> s->chain_frozen = true;
>
> diff --git a/blockdev.c b/blockdev.c
> index d719c47..b223601 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2486,7 +2486,6 @@ out:
> 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_speed, int64_t speed,
> bool has_on_error, BlockdevOnError on_error,
> bool has_filter_node_name, const char *filter_node_name,
> @@ -2498,7 +2497,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) {
> @@ -2526,7 +2524,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) {
> @@ -2541,7 +2538,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 */
> @@ -2553,17 +2549,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
> }
> }
>
> - /* if we are streaming the entire chain, the result will have no backing
> - * file, and specifying one is therefore an error */
> - if (base_bs == NULL && has_backing_file) {
> - error_setg(errp, "backing file specified, but streaming the "
> - "entire chain");
> - 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;
> }
> @@ -2571,7 +2556,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,
> job_flags, has_speed ? speed : 0, on_error,
> filter_node_name, &local_err);
> if (local_err) {
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f782737..bbe2ee6 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1147,7 +1147,7 @@ int is_windows_drive(const char *filename);
> * BlockDriverState.
> */
> void stream_start(const char *job_id, BlockDriverState *bs,
> - BlockDriverState *base, const char *backing_file_str,
> + BlockDriverState *base,
> int creation_flags, int64_t speed,
> BlockdevOnError on_error,
> const char *filter_node_name,
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 32fb097..6358606 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2512,21 +2512,6 @@
> # @base-node: the node name of the backing file.
> # It cannot be set if @base is also set. (Since 2.8)
> #
> -# @backing-file: The backing file string to write into the top
> -# image. This filename is not validated.
> -#
> -# If a pathname string is such that it cannot be
> -# resolved by QEMU, that means that subsequent QMP or
> -# HMP commands must use node-names for the image in
> -# question, as filename lookup methods will fail.
> -#
> -# If not specified, QEMU will automatically determine
> -# the backing file string to use, or error out if there
> -# is no obvious choice. Care should be taken when
> -# specifying the string, to specify a valid filename or
> -# protocol.
> -# (Since 2.1)
> -#
> # @speed: the maximum speed, in bytes per second
> #
> # @on-error: the action to take on an error (default report).
> @@ -2566,7 +2551,7 @@
> ##
> { 'command': 'block-stream',
> 'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
> - '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
> + '*base-node': 'str', '*speed': 'int',
> '*on-error': 'BlockdevOnError',
> '*filter-node-name': 'str',
> '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v10 9/9] block: apply COR-filter to block-stream jobs
2020-09-29 12:38 ` [PATCH v10 9/9] block: apply COR-filter to block-stream jobs Andrey Shinkevich via
@ 2020-10-07 17:27 ` Andrey Shinkevich
2020-10-07 17:39 ` Andrey Shinkevich
0 siblings, 1 reply; 25+ messages in thread
From: Andrey Shinkevich @ 2020-10-07 17:27 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake,
den, vsementsov
On 29.09.2020 15:38, Andrey Shinkevich wrote:
> This patch completes the series with the COR-filter insertion for
> block-stream operations. Adding the filter makes it possible for copied
> regions to be discarded in backing files during the block-stream job,
> what will reduce the disk overuse.
> The COR-filter insertion incurs changes in the iotests case
> 245:test_block_stream_4 that reopens the backing chain during a
> block-stream job. There are changes in the iotests #030 as well.
> The iotests case 030:test_stream_parallel was deleted due to multiple
> conflicts between the concurrent job operations over the same backing
> chain. The base backing node for one job is the top node for another
> job. It may change due to the filter node inserted into the backing
> chain while both jobs are running. Another issue is that the parts of
> the backing chain are being frozen by the running job and may not be
> changed by the concurrent job when needed. The concept of the parallel
> jobs with common nodes is considered vital no more.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> block/stream.c | 93 ++++++++++++++++++++++++++++++----------------
> tests/qemu-iotests/030 | 51 +++----------------------
> tests/qemu-iotests/030.out | 4 +-
> tests/qemu-iotests/141.out | 2 +-
> tests/qemu-iotests/245 | 19 +++++++---
> 5 files changed, 83 insertions(+), 86 deletions(-)
>
> diff --git a/block/stream.c b/block/stream.c
> index fe2663f..240b3dc 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -17,8 +17,10 @@
One more change missed, as we use the COR-filter:
@@ -47,8 +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);
+ return blk_co_preadv(blk, offset, bytes, NULL, 0);
}
Andrey
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v10 9/9] block: apply COR-filter to block-stream jobs
2020-10-07 17:27 ` Andrey Shinkevich
@ 2020-10-07 17:39 ` Andrey Shinkevich
0 siblings, 0 replies; 25+ messages in thread
From: Andrey Shinkevich @ 2020-10-07 17:39 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake,
den, vsementsov
On 07.10.2020 20:27, Andrey Shinkevich wrote:
>
> On 29.09.2020 15:38, Andrey Shinkevich wrote:
>> This patch completes the series with the COR-filter insertion for
>> block-stream operations. Adding the filter makes it possible for copied
>> regions to be discarded in backing files during the block-stream job,
>> what will reduce the disk overuse.
>> The COR-filter insertion incurs changes in the iotests case
>> 245:test_block_stream_4 that reopens the backing chain during a
>> block-stream job. There are changes in the iotests #030 as well.
>> The iotests case 030:test_stream_parallel was deleted due to multiple
>> conflicts between the concurrent job operations over the same backing
>> chain. The base backing node for one job is the top node for another
>> job. It may change due to the filter node inserted into the backing
>> chain while both jobs are running. Another issue is that the parts of
>> the backing chain are being frozen by the running job and may not be
>> changed by the concurrent job when needed. The concept of the parallel
>> jobs with common nodes is considered vital no more.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>> block/stream.c | 93
>> ++++++++++++++++++++++++++++++----------------
>> tests/qemu-iotests/030 | 51 +++----------------------
>> tests/qemu-iotests/030.out | 4 +-
>> tests/qemu-iotests/141.out | 2 +-
>> tests/qemu-iotests/245 | 19 +++++++---
>> 5 files changed, 83 insertions(+), 86 deletions(-)
>>
>> diff --git a/block/stream.c b/block/stream.c
>> index fe2663f..240b3dc 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -17,8 +17,10 @@
>
>
> One more change missed, as we use the COR-filter:
>
> @@ -47,8 +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);
+ return blk_co_preadv(blk, offset, bytes, NULL, BDRV_REQ_PREFETCH);
Sorry, with the only flag BDRV_REQ_PREFETCH set.
A change in the comment at the flag BDRV_REQ_PREFETCH is coming with a
separate patch as Vladimir suggested.
Andrey
> + return blk_co_preadv(blk, offset, bytes, NULL, 0);
> }
>
> Andrey
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v10 6/9] copy-on-read: skip non-guest reads if no copy needed
2020-10-07 10:06 ` Vladimir Sementsov-Ogievskiy
@ 2020-10-07 19:01 ` Andrey Shinkevich
2020-10-07 19:28 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 25+ messages in thread
From: Andrey Shinkevich @ 2020-10-07 19:01 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake, den
On 07.10.2020 13:06, Vladimir Sementsov-Ogievskiy wrote:
> 29.09.2020 15:38, Andrey Shinkevich wrote:
>> If the flag BDRV_REQ_PREFETCH was set, pass it further to the
>> COR-driver to skip unneeded reading. It can be taken into account for
>> the COR-algorithms optimization. That check is being made during the
>> block stream job by the moment.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>> block/copy-on-read.c | 14 ++++++++++----
>> block/io.c | 2 +-
>> 2 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>> index f53f7e0..5389dca 100644
>> --- a/block/copy-on-read.c
>> +++ b/block/copy-on-read.c
>> @@ -145,10 +145,16 @@ 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;
>> + if ((flags & BDRV_REQ_PREFETCH) &
>
> BDRV_REQ_PREFETCH is documented to be only used with
> BDRV_REQ_COPY_ON_READ. But here
> BDRV_REQ_COPY_ON_READ appears intermediately. We should change
> documentation in block.h
> in a separate patch (and probably code in bdrv_aligned_preadv())
>
OK, we will come here without the BDRV_REQ_PREFETCH flag set.
To differ between guest reads and the stream job ones, we would set it
here by checking for the qiov NULL pointer:
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 4e3b1c5..df2c2ab 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -144,6 +144,9 @@ static int coroutine_fn
cor_co_preadv_part(BlockDriverState *bs,
n, &n);
if (ret) {
local_flags |= BDRV_REQ_COPY_ON_READ;
+ if (!qiov) {
+ local_flags |= BDRV_REQ_PREFETCH;
+ }
}
}
Andrey
>> + !(local_flags & BDRV_REQ_COPY_ON_READ)) {
>> + /* Skip non-guest reads if no copy needed */
>> + } else {
>> +
>
> extra new-line ?
>
>> + ret = bdrv_co_preadv_part(bs->file, offset, n, qiov,
>> qiov_offset,
>> + local_flags);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> }
>> offset += n;
>> diff --git a/block/io.c b/block/io.c
>> index 11df188..62b75a5 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -1388,7 +1388,7 @@ static int coroutine_fn jk(BdrvChild *child,
>> qemu_iovec_init_buf(&local_qiov, bounce_buffer, pnum);
>> ret = bdrv_driver_preadv(bs, cluster_offset, pnum,
>> - &local_qiov, 0, 0);
>> + &local_qiov, 0, flags &
>> BDRV_REQ_PREFETCH);
>
> Why? In this place we want to read. We'll write back the data a few
> lines below. What will we write,
> if underlying driver decide to do nothing because of BDRV_REQ_PREFETCH?
>
See my comment above please.
>> if (ret < 0) {
>> goto err;
>> }
>>
>
>
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v10 6/9] copy-on-read: skip non-guest reads if no copy needed
2020-10-07 19:01 ` Andrey Shinkevich
@ 2020-10-07 19:28 ` Vladimir Sementsov-Ogievskiy
2020-10-09 12:56 ` Andrey Shinkevich
0 siblings, 1 reply; 25+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-07 19:28 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake, den
07.10.2020 22:01, Andrey Shinkevich wrote:
>
> On 07.10.2020 13:06, Vladimir Sementsov-Ogievskiy wrote:
>> 29.09.2020 15:38, Andrey Shinkevich wrote:
>>> If the flag BDRV_REQ_PREFETCH was set, pass it further to the
>>> COR-driver to skip unneeded reading. It can be taken into account for
>>> the COR-algorithms optimization. That check is being made during the
>>> block stream job by the moment.
>>>
>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>> ---
>>> block/copy-on-read.c | 14 ++++++++++----
>>> block/io.c | 2 +-
>>> 2 files changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>>> index f53f7e0..5389dca 100644
>>> --- a/block/copy-on-read.c
>>> +++ b/block/copy-on-read.c
>>> @@ -145,10 +145,16 @@ 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;
>>> + if ((flags & BDRV_REQ_PREFETCH) &
>>
>> BDRV_REQ_PREFETCH is documented to be only used with BDRV_REQ_COPY_ON_READ. But here
>> BDRV_REQ_COPY_ON_READ appears intermediately. We should change documentation in block.h
>> in a separate patch (and probably code in bdrv_aligned_preadv())
>>
>
> OK, we will come here without the BDRV_REQ_PREFETCH flag set.
flag BDRV_REQ_PREFETCH should be set in stream job. Where should it be handled, I don't follow?
> To differ between guest reads and the stream job ones, we would set it here by checking for the qiov NULL pointer:
>
>
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index 4e3b1c5..df2c2ab 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -144,6 +144,9 @@ static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
> n, &n);
> if (ret) {
> local_flags |= BDRV_REQ_COPY_ON_READ;
> + if (!qiov) {
> + local_flags |= BDRV_REQ_PREFETCH;
if qiov is NULL, this means that flags must include BDRV_REQ_PREFETCH. local_flags should inherit flags I think.
> + }
> }
> }
>
> Andrey
>
>>> + !(local_flags & BDRV_REQ_COPY_ON_READ)) {
>>> + /* Skip non-guest reads if no copy needed */
>>> + } else {
>>> +
>>
>> extra new-line ?
>>
>>> + ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset,
>>> + local_flags);
>>> + if (ret < 0) {
>>> + return ret;
>>> + }
>>> }
>>> offset += n;
>>> diff --git a/block/io.c b/block/io.c
>>> index 11df188..62b75a5 100644
>>> --- a/block/io.c
>>> +++ b/block/io.c
>>> @@ -1388,7 +1388,7 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
>>> qemu_iovec_init_buf(&local_qiov, bounce_buffer, pnum);
>>> ret = bdrv_driver_preadv(bs, cluster_offset, pnum,
>>> - &local_qiov, 0, 0);
>>> + &local_qiov, 0, flags & BDRV_REQ_PREFETCH);
>>
>> Why? In this place we want to read. We'll write back the data a few lines below. What will we write,
>> if underlying driver decide to do nothing because of BDRV_REQ_PREFETCH?
>>
>
> See my comment above please.
Anyway, BDRV_REQ_PREFETCH here is wrong. You should not pass any qiov, if you set BDRV_REQ_PREFETCH flag.
If we come to bdrv_co_do_copy_on_readv, it means that we have COPY_ON_READ flag. And therefore, we will handle
PREFETCH and COPY_ON_READ here in generic layer. And therefore, we shouldn't pass them to driver.
On the contrary, if we have PREFETCH flag in bdrv_co_aligned_preadv, but don't have COPY_ON_READ in the same time,
this means that we must pass PREFETCH flag to the driver if it supports it. And do nothing if driver
doesn't support PREFETCH. That's how I see it.
>
>>> if (ret < 0) {
>>> goto err;
>>> }
>>>
>>
>>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v10 8/9] block: remove unused backing-file name parameter
2020-10-07 10:21 ` Vladimir Sementsov-Ogievskiy
@ 2020-10-07 19:32 ` Andrey Shinkevich
0 siblings, 0 replies; 25+ messages in thread
From: Andrey Shinkevich @ 2020-10-07 19:32 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake, den
On 07.10.2020 13:21, Vladimir Sementsov-Ogievskiy wrote:
> 29.09.2020 15:38, Andrey Shinkevich wrote:
>> The block stream QMP parameter backing-file is in use no more. It
>> designates a backing file name to set in QCOW2 image header after the
>> block stream job finished. The base file name is used instead.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>
> We can't just remove it without a deprecation period of three releases.
It has not been in use for a long. It's time.
>
> So actually, in a previous patch, we should implement new behavior for
> automatic backing-file detection if this parameter is unspecified. Amd
> keep old behavior for backing-file-name if it is given.
>
> Hmm. Or, probably, we can use direct base for base-filename? And in cases
> when we should skip filters (for example of parallel jobs) user should
> specify backing-file explicitly?
The backing_file_str is always specified if the base is specified and is
always equal to the base->filename. So, the user's backing file name is
always NULL for the stream job. Furthermore, it is not checked for being
the correct backing node and can lead to a wrong record in the QCOW2 header.
Andrey
>
>> ---
>> block/monitor/block-hmp-cmds.c | 2 +-
>> block/stream.c | 6 +-----
>> blockdev.c | 17 +----------------
>> include/block/block_int.h | 2 +-
>> qapi/block-core.json | 17 +----------------
>> 5 files changed, 5 insertions(+), 39 deletions(-)
>>
>> diff --git a/block/monitor/block-hmp-cmds.c
>> b/block/monitor/block-hmp-cmds.c
>> index 4e66775..5f19499 100644
>> --- a/block/monitor/block-hmp-cmds.c
>> +++ b/block/monitor/block-hmp-cmds.c
>> @@ -506,7 +506,7 @@ 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,
>> + 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 b0719e9..fe2663f 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -34,7 +34,6 @@ typedef struct StreamBlockJob {
>> BlockDriverState *base_overlay; /* COW overlay (stream from
>> this) */
>> BlockDriverState *above_base; /* Node directly above the base */
>> BlockdevOnError on_error;
>> - char *backing_file_str;
>> bool bs_read_only;
>> bool chain_frozen;
>> } StreamBlockJob;
>> @@ -103,8 +102,6 @@ static void stream_clean(Job *job)
>> blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
>> bdrv_reopen_set_read_only(bs, true, NULL);
>> }
>> -
>> - g_free(s->backing_file_str);
>> }
>> static int coroutine_fn stream_run(Job *job, Error **errp)
>> @@ -220,7 +217,7 @@ static const BlockJobDriver stream_job_driver = {
>> };
>> void stream_start(const char *job_id, BlockDriverState *bs,
>> - BlockDriverState *base, const char *backing_file_str,
>> + BlockDriverState *base,
>> int creation_flags, int64_t speed,
>> BlockdevOnError on_error,
>> const char *filter_node_name,
>> @@ -295,7 +292,6 @@ 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->bs_read_only = bs_read_only;
>> s->chain_frozen = true;
>> diff --git a/blockdev.c b/blockdev.c
>> index d719c47..b223601 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2486,7 +2486,6 @@ out:
>> 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_speed, int64_t speed,
>> bool has_on_error, BlockdevOnError on_error,
>> bool has_filter_node_name, const char
>> *filter_node_name,
>> @@ -2498,7 +2497,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) {
>> @@ -2526,7 +2524,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) {
>> @@ -2541,7 +2538,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 */
>> @@ -2553,17 +2549,6 @@ void qmp_block_stream(bool has_job_id, const
>> char *job_id, const char *device,
>> }
>> }
>> - /* if we are streaming the entire chain, the result will have no
>> backing
>> - * file, and specifying one is therefore an error */
>> - if (base_bs == NULL && has_backing_file) {
>> - error_setg(errp, "backing file specified, but streaming the "
>> - "entire chain");
>> - 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;
>> }
>> @@ -2571,7 +2556,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,
>> job_flags, has_speed ? speed : 0, on_error,
>> filter_node_name, &local_err);
>> if (local_err) {
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index f782737..bbe2ee6 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -1147,7 +1147,7 @@ int is_windows_drive(const char *filename);
>> * BlockDriverState.
>> */
>> void stream_start(const char *job_id, BlockDriverState *bs,
>> - BlockDriverState *base, const char *backing_file_str,
>> + BlockDriverState *base,
>> int creation_flags, int64_t speed,
>> BlockdevOnError on_error,
>> const char *filter_node_name,
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 32fb097..6358606 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2512,21 +2512,6 @@
>> # @base-node: the node name of the backing file.
>> # It cannot be set if @base is also set. (Since 2.8)
>> #
>> -# @backing-file: The backing file string to write into the top
>> -# image. This filename is not validated.
>> -#
>> -# If a pathname string is such that it cannot be
>> -# resolved by QEMU, that means that subsequent QMP or
>> -# HMP commands must use node-names for the image in
>> -# question, as filename lookup methods will fail.
>> -#
>> -# If not specified, QEMU will automatically determine
>> -# the backing file string to use, or error out if there
>> -# is no obvious choice. Care should be taken when
>> -# specifying the string, to specify a valid filename or
>> -# protocol.
>> -# (Since 2.1)
>> -#
>> # @speed: the maximum speed, in bytes per second
>> #
>> # @on-error: the action to take on an error (default report).
>> @@ -2566,7 +2551,7 @@
>> ##
>> { 'command': 'block-stream',
>> 'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
>> - '*base-node': 'str', '*backing-file': 'str', '*speed':
>> 'int',
>> + '*base-node': 'str', '*speed': 'int',
>> '*on-error': 'BlockdevOnError',
>> '*filter-node-name': 'str',
>> '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>>
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v10 6/9] copy-on-read: skip non-guest reads if no copy needed
2020-10-07 19:28 ` Vladimir Sementsov-Ogievskiy
@ 2020-10-09 12:56 ` Andrey Shinkevich
0 siblings, 0 replies; 25+ messages in thread
From: Andrey Shinkevich @ 2020-10-09 12:56 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: qemu-devel, kwolf, mreitz, stefanha, fam, jsnow, armbru, eblake, den
On 07.10.2020 22:28, Vladimir Sementsov-Ogievskiy wrote:
> 07.10.2020 22:01, Andrey Shinkevich wrote:
>>
>> On 07.10.2020 13:06, Vladimir Sementsov-Ogievskiy wrote:
>>> 29.09.2020 15:38, Andrey Shinkevich wrote:
>>>> If the flag BDRV_REQ_PREFETCH was set, pass it further to the
>>>> COR-driver to skip unneeded reading. It can be taken into account for
>>>> the COR-algorithms optimization. That check is being made during the
>>>> block stream job by the moment.
>>>>
>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>> ---
>>>> block/copy-on-read.c | 14 ++++++++++----
>>>> block/io.c | 2 +-
>>>> 2 files changed, 11 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>>>> index f53f7e0..5389dca 100644
>>>> --- a/block/copy-on-read.c
>>>> +++ b/block/copy-on-read.c
>>>> @@ -145,10 +145,16 @@ 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;
>>>> + if ((flags & BDRV_REQ_PREFETCH) &
>>>
>>> BDRV_REQ_PREFETCH is documented to be only used with
>>> BDRV_REQ_COPY_ON_READ. But here
>>> BDRV_REQ_COPY_ON_READ appears intermediately. We should change
>>> documentation in block.h
>>> in a separate patch (and probably code in bdrv_aligned_preadv())
>>>
>>
>> OK, we will come here without the BDRV_REQ_PREFETCH flag set.
>
> flag BDRV_REQ_PREFETCH should be set in stream job. Where should it be
> handled, I don't follow?
>
If we leave block/io.c unchanged in this patch, what I'm agreeing with,
we'll come to the COR-driver with the hardcoded flags = 0 :
#4 0x000055a22bb480cf in cor_co_preadv_part (bs=0x55a22d593710,
offset=0, bytes=524288, qiov=0x0, qiov_offset=0, flags=0) at
../block/copy-on-read.c:149
#5 0x000055a22badcb1d in bdrv_driver_preadv (bs=0x55a22d593710,
offset=0, bytes=524288, qiov=0x0, qiov_offset=0, flags=0) at
../block/io.c:1129
#6 0x000055a22baddc81 in bdrv_aligned_preadv (child=0x55a22d814780,
req=0x7f8c1abffce0, offset=0, bytes=524288, align=1, qiov=0x0,
qiov_offset=0, flags=512) at ../block/io.c:1515
#7 0x000055a22bade59a in bdrv_co_preadv_part (child=0x55a22d814780,
offset=0, bytes=524288, qiov=0x0, qiov_offset=0,
flags=BDRV_REQ_PREFETCH) at ../block/io.c:1757
#8 0x000055a22bade3d2 in bdrv_co_preadv (child=0x55a22d814780,
offset=0, bytes=524288, qiov=0x0, flags=BDRV_REQ_PREFETCH) at
../block/io.c:1715
#9 0x000055a22baf5d09 in blk_do_preadv (blk=0x55a22d818c00, offset=0,
bytes=524288, qiov=0x0, flags=BDRV_REQ_PREFETCH) at
../block/block-backend.c:1211
#10 0x000055a22baf5d61 in blk_co_preadv (blk=0x55a22d818c00, offset=0,
bytes=524288, qiov=0x0, flags=BDRV_REQ_PREFETCH) at
../block/block-backend.c:1223
#11 0x000055a22bab4eba in stream_populate (blk=0x55a22d818c00, offset=0,
bytes=524288) at ../block/stream.c:50
#12 0x000055a22bab52c2 in stream_run (job=0x55a22d810a20,
errp=0x55a22d810aa0) at ../block/stream.c:162
#13 0x000055a22bab79f0 in job_co_entry (opaque=0x55a22d810a20) at
../job.c:908
So, the only way for the COR-filter driver to differ between guests
reads and the stream job is to check the qiov pointer for NULL and reset
the flags as appropriate. This is what I am going to do in the next version.
Andrey
>> To differ between guest reads and the stream job ones, we would set it
>> here by checking for the qiov NULL pointer:
>>
>>
>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>> index 4e3b1c5..df2c2ab 100644
>> --- a/block/copy-on-read.c
>> +++ b/block/copy-on-read.c
>> @@ -144,6 +144,9 @@ static int coroutine_fn
>> cor_co_preadv_part(BlockDriverState *bs,
>> n, &n);
>> if (ret) {
>> local_flags |= BDRV_REQ_COPY_ON_READ;
>> + if (!qiov) {
>> + local_flags |= BDRV_REQ_PREFETCH;
>
> if qiov is NULL, this means that flags must include BDRV_REQ_PREFETCH.
> local_flags should inherit flags I think.
>
>> + }
>> }
>> }
>>
>> Andrey
>>
>>>> + !(local_flags & BDRV_REQ_COPY_ON_READ)) {
>>>> + /* Skip non-guest reads if no copy needed */
>>>> + } else {
>>>> +
>>>
>>> extra new-line ?
>>>
>>>> + ret = bdrv_co_preadv_part(bs->file, offset, n, qiov,
>>>> qiov_offset,
>>>> + local_flags);
>>>> + if (ret < 0) {
>>>> + return ret;
>>>> + }
>>>> }
>>>> offset += n;
>>>> diff --git a/block/io.c b/block/io.c
>>>> index 11df188..62b75a5 100644
>>>> --- a/block/io.c
>>>> +++ b/block/io.c
>>>> @@ -1388,7 +1388,7 @@ static int coroutine_fn
>>>> bdrv_co_do_copy_on_readv(BdrvChild *child,
>>>> qemu_iovec_init_buf(&local_qiov, bounce_buffer, pnum);
>>>> ret = bdrv_driver_preadv(bs, cluster_offset, pnum,
>>>> - &local_qiov, 0, 0);
>>>> + &local_qiov, 0, flags &
>>>> BDRV_REQ_PREFETCH);
>>>
>>> Why? In this place we want to read. We'll write back the data a few
>>> lines below. What will we write,
>>> if underlying driver decide to do nothing because of BDRV_REQ_PREFETCH?
>>>
>>
>> See my comment above please.
>
> Anyway, BDRV_REQ_PREFETCH here is wrong. You should not pass any qiov,
> if you set BDRV_REQ_PREFETCH flag.
>
> If we come to bdrv_co_do_copy_on_readv, it means that we have
> COPY_ON_READ flag. And therefore, we will handle
> PREFETCH and COPY_ON_READ here in generic layer. And therefore, we
> shouldn't pass them to driver.
>
> On the contrary, if we have PREFETCH flag in bdrv_co_aligned_preadv, but
> don't have COPY_ON_READ in the same time,
> this means that we must pass PREFETCH flag to the driver if it supports
> it. And do nothing if driver
> doesn't support PREFETCH. That's how I see it.
>
>>
>>>> if (ret < 0) {
>>>> goto err;
>>>> }
>>>>
>>>
>>>
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2020-10-09 12:58 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-29 12:38 [PATCH v10 0/9] Apply COR-filter to the block-stream permanently Andrey Shinkevich via
2020-09-29 12:38 ` [PATCH v10 1/9] copy-on-read: Support preadv/pwritev_part functions Andrey Shinkevich via
2020-09-29 12:38 ` [PATCH v10 2/9] copy-on-read: add filter append/drop functions Andrey Shinkevich via
2020-10-05 13:34 ` Vladimir Sementsov-Ogievskiy
2020-10-05 16:23 ` Andrey Shinkevich
2020-09-29 12:38 ` [PATCH v10 3/9] qapi: add filter-node-name to block-stream Andrey Shinkevich via
2020-09-29 12:38 ` [PATCH v10 4/9] copy-on-read: pass base node name to COR driver Andrey Shinkevich via
2020-10-05 14:50 ` Vladimir Sementsov-Ogievskiy
2020-09-29 12:38 ` [PATCH v10 5/9] copy-on-read: limit guest COR activity to base in " Andrey Shinkevich via
2020-10-05 14:58 ` Vladimir Sementsov-Ogievskiy
2020-10-05 16:45 ` Andrey Shinkevich
2020-10-05 18:18 ` Andrey Shinkevich
2020-09-29 12:38 ` [PATCH v10 6/9] copy-on-read: skip non-guest reads if no copy needed Andrey Shinkevich via
2020-10-07 10:06 ` Vladimir Sementsov-Ogievskiy
2020-10-07 19:01 ` Andrey Shinkevich
2020-10-07 19:28 ` Vladimir Sementsov-Ogievskiy
2020-10-09 12:56 ` Andrey Shinkevich
2020-09-29 12:38 ` [PATCH v10 7/9] stream: skip filters when writing backing file name to QCOW2 header Andrey Shinkevich via
2020-10-07 10:14 ` Vladimir Sementsov-Ogievskiy
2020-09-29 12:38 ` [PATCH v10 8/9] block: remove unused backing-file name parameter Andrey Shinkevich via
2020-10-07 10:21 ` Vladimir Sementsov-Ogievskiy
2020-10-07 19:32 ` Andrey Shinkevich
2020-09-29 12:38 ` [PATCH v10 9/9] block: apply COR-filter to block-stream jobs Andrey Shinkevich via
2020-10-07 17:27 ` Andrey Shinkevich
2020-10-07 17:39 ` Andrey Shinkevich
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.