* [PATCH v6 1/4] copy-on-read: Support preadv/pwritev_part functions
2020-08-18 21:24 [PATCH v6 0/4] Apply COR-filter to the block-stream permanently Andrey Shinkevich
@ 2020-08-18 21:24 ` Andrey Shinkevich
2020-08-19 9:53 ` Vladimir Sementsov-Ogievskiy
2020-08-18 21:24 ` [PATCH v6 2/4] copy-on-read: add filter append/drop functions Andrey Shinkevich
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Andrey Shinkevich @ 2020-08-18 21:24 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, vsementsov, armbru, qemu-devel, andrey.shinkevich, den,
mreitz, jsnow
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>
---
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] 16+ messages in thread* [PATCH v6 2/4] copy-on-read: add filter append/drop functions
2020-08-18 21:24 [PATCH v6 0/4] Apply COR-filter to the block-stream permanently Andrey Shinkevich
2020-08-18 21:24 ` [PATCH v6 1/4] copy-on-read: Support preadv/pwritev_part functions Andrey Shinkevich
@ 2020-08-18 21:24 ` Andrey Shinkevich
2020-08-19 10:21 ` Vladimir Sementsov-Ogievskiy
2020-08-18 21:24 ` [PATCH v6 3/4] qapi: add filter-node-name to block-stream Andrey Shinkevich
2020-08-18 21:24 ` [PATCH v6 4/4] block: apply COR-filter to block-stream jobs Andrey Shinkevich
3 siblings, 1 reply; 16+ messages in thread
From: Andrey Shinkevich @ 2020-08-18 21:24 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, vsementsov, armbru, qemu-devel, andrey.shinkevich, den,
mreitz, jsnow
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 | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++
block/copy-on-read.h | 36 ++++++++++++++++++
2 files changed, 139 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..150d9b7 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,8 @@ 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;
+
return 0;
}
@@ -57,6 +69,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 +158,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 +183,83 @@ static void bdrv_copy_on_read_init(void)
bdrv_register(&bdrv_copy_on_read);
}
+
+static BlockDriverState *create_filter_node(BlockDriverState *bs,
+ const char *filter_node_name,
+ Error **errp)
+{
+ QDict *opts = qdict_new();
+
+ qdict_put_str(opts, "driver", "copy-on-read");
+ qdict_put_str(opts, "file", bdrv_get_node_name(bs));
+ if (filter_node_name) {
+ qdict_put_str(opts, "node-name", filter_node_name);
+ }
+
+ return bdrv_open(NULL, NULL, opts, BDRV_O_RDWR, errp);
+}
+
+
+BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
+ const char *filter_node_name,
+ Error **errp)
+{
+ BlockDriverState *cor_filter_bs;
+ BDRVStateCOR *state;
+ Error *local_err = NULL;
+
+ cor_filter_bs = create_filter_node(bs, filter_node_name, errp);
+ if (cor_filter_bs == NULL) {
+ error_prepend(errp, "Could not create filter node: ");
+ return NULL;
+ }
+
+ if (!filter_node_name) {
+ cor_filter_bs->implicit = true;
+ }
+
+ 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;
+ }
+
+ state = cor_filter_bs->opaque;
+ state->active = true;
+
+ 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..db03c6c
--- /dev/null
+++ b/block/copy-on-read.h
@@ -0,0 +1,36 @@
+/*
+ * 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 COPY_ON_READ_FILTER
+#define COPY_ON_READ_FILTER
+
+#include "block/block_int.h"
+#include "block/block-copy.h"
+
+BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
+ const char *filter_node_name,
+ Error **errp);
+void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs);
+
+#endif /* COPY_ON_READ_FILTER */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v6 2/4] copy-on-read: add filter append/drop functions
2020-08-18 21:24 ` [PATCH v6 2/4] copy-on-read: add filter append/drop functions Andrey Shinkevich
@ 2020-08-19 10:21 ` Vladimir Sementsov-Ogievskiy
2020-08-23 19:35 ` Andrey Shinkevich
0 siblings, 1 reply; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-08-19 10:21 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block
Cc: kwolf, armbru, qemu-devel, den, mreitz, jsnow
19.08.2020 00:24, 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 | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++
> block/copy-on-read.h | 36 ++++++++++++++++++
> 2 files changed, 139 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..150d9b7 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,8 @@ 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() here, as permissions will be
updated soon, when the filter node get its parent, yes?
Let's add at least a comment on this.
> +
> return 0;
> }
>
> @@ -57,6 +69,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 +158,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 +183,83 @@ static void bdrv_copy_on_read_init(void)
> bdrv_register(&bdrv_copy_on_read);
> }
>
> +
> +static BlockDriverState *create_filter_node(BlockDriverState *bs,
> + const char *filter_node_name,
> + Error **errp)
> +{
> + QDict *opts = qdict_new();
> +
> + qdict_put_str(opts, "driver", "copy-on-read");
> + qdict_put_str(opts, "file", bdrv_get_node_name(bs));
> + if (filter_node_name) {
> + qdict_put_str(opts, "node-name", filter_node_name);
> + }
> +
> + return bdrv_open(NULL, NULL, opts, BDRV_O_RDWR, errp);
> +}
> +
> +
> +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
> + const char *filter_node_name,
> + Error **errp)
> +{
> + BlockDriverState *cor_filter_bs;
> + BDRVStateCOR *state;
> + Error *local_err = NULL;
> +
> + cor_filter_bs = create_filter_node(bs, filter_node_name, errp);
> + if (cor_filter_bs == NULL) {
> + error_prepend(errp, "Could not create filter node: ");
> + return NULL;
> + }
> +
> + if (!filter_node_name) {
> + cor_filter_bs->implicit = true;
> + }
> +
> + 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;
> + }
> +
> + state = cor_filter_bs->opaque;
> + state->active = true;
hmm stop. it's already active, after create_filter_node, so you don't need to set it again, isn't it?
> +
> + 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..db03c6c
> --- /dev/null
> +++ b/block/copy-on-read.h
> @@ -0,0 +1,36 @@
> +/*
> + * 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 COPY_ON_READ_FILTER
> +#define COPY_ON_READ_FILTER
> +
> +#include "block/block_int.h"
> +#include "block/block-copy.h"
do you really need block-copy.h ?
> +
> +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
> + const char *filter_node_name,
> + Error **errp);
> +void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs);
> +
> +#endif /* COPY_ON_READ_FILTER */
>
Code seems correct to me, with extra "state->active = true;" dropped:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v6 2/4] copy-on-read: add filter append/drop functions
2020-08-19 10:21 ` Vladimir Sementsov-Ogievskiy
@ 2020-08-23 19:35 ` Andrey Shinkevich
2020-08-24 4:59 ` Andrey Shinkevich
0 siblings, 1 reply; 16+ messages in thread
From: Andrey Shinkevich @ 2020-08-23 19:35 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, armbru, qemu-devel, den, mreitz, jsnow
On 19.08.2020 13:21, Vladimir Sementsov-Ogievskiy wrote:
> 19.08.2020 00:24, 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 | 103
>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>> block/copy-on-read.h | 36 ++++++++++++++++++
>> 2 files changed, 139 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..150d9b7 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,8 @@ 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() here, as permissions
> will be
> updated soon, when the filter node get its parent, yes?
>
> Let's add at least a comment on this.
>
>> +
>> return 0;
>> }
>> @@ -57,6 +69,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 +158,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 +183,83 @@ static void bdrv_copy_on_read_init(void)
>> bdrv_register(&bdrv_copy_on_read);
>> }
>> +
>> +static BlockDriverState *create_filter_node(BlockDriverState *bs,
>> + const char
>> *filter_node_name,
>> + Error **errp)
>> +{
>> + QDict *opts = qdict_new();
>> +
>> + qdict_put_str(opts, "driver", "copy-on-read");
>> + qdict_put_str(opts, "file", bdrv_get_node_name(bs));
>> + if (filter_node_name) {
>> + qdict_put_str(opts, "node-name", filter_node_name);
>> + }
>> +
>> + return bdrv_open(NULL, NULL, opts, BDRV_O_RDWR, errp);
>> +}
>> +
>> +
>> +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
>> + const char *filter_node_name,
>> + Error **errp)
>> +{
>> + BlockDriverState *cor_filter_bs;
>> + BDRVStateCOR *state;
>> + Error *local_err = NULL;
>> +
>> + cor_filter_bs = create_filter_node(bs, filter_node_name, errp);
>> + if (cor_filter_bs == NULL) {
>> + error_prepend(errp, "Could not create filter node: ");
>> + return NULL;
>> + }
>> +
>> + if (!filter_node_name) {
>> + cor_filter_bs->implicit = true;
>> + }
>> +
>> + 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;
>> + }
>> +
>> + state = cor_filter_bs->opaque;
>> + state->active = true;
>
> hmm stop. it's already active, after create_filter_node, so you don't
> need to set it again, isn't it?
I will remove the extra assignment from the cor_open() for consistancy.
Andrey
>
>> +
>> + 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..db03c6c
>> --- /dev/null
>> +++ b/block/copy-on-read.h
>> @@ -0,0 +1,36 @@
>> +/*
>> + * 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 COPY_ON_READ_FILTER
>> +#define COPY_ON_READ_FILTER
>> +
>> +#include "block/block_int.h"
>> +#include "block/block-copy.h"
>
> do you really need block-copy.h ?
>
>> +
>> +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
>> + const char *filter_node_name,
>> + Error **errp);
>> +void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs);
>> +
>> +#endif /* COPY_ON_READ_FILTER */
>>
>
> Code seems correct to me, with extra "state->active = true;" dropped:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v6 2/4] copy-on-read: add filter append/drop functions
2020-08-23 19:35 ` Andrey Shinkevich
@ 2020-08-24 4:59 ` Andrey Shinkevich
0 siblings, 0 replies; 16+ messages in thread
From: Andrey Shinkevich @ 2020-08-24 4:59 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, armbru, qemu-devel, den, mreitz, jsnow
On 23.08.2020 22:35, Andrey Shinkevich wrote:
> On 19.08.2020 13:21, Vladimir Sementsov-Ogievskiy wrote:
>> 19.08.2020 00:24, 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 | 103
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>> block/copy-on-read.h | 36 ++++++++++++++++++
>>> 2 files changed, 139 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..150d9b7 100644
>>> --- a/block/copy-on-read.c
>>> +++ b/block/copy-on-read.c
>>> @@ -23,11 +23,21 @@
...
>>> +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
>>> + const char *filter_node_name,
>>> + Error **errp)
>>> +{
>>> + BlockDriverState *cor_filter_bs;
>>> + BDRVStateCOR *state;
>>> + Error *local_err = NULL;
>>> +
>>> + cor_filter_bs = create_filter_node(bs, filter_node_name, errp);
>>> + if (cor_filter_bs == NULL) {
>>> + error_prepend(errp, "Could not create filter node: ");
>>> + return NULL;
>>> + }
>>> +
>>> + if (!filter_node_name) {
>>> + cor_filter_bs->implicit = true;
>>> + }
>>> +
>>> + 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;
>>> + }
>>> +
>>> + state = cor_filter_bs->opaque;
>>> + state->active = true;
>>
>> hmm stop. it's already active, after create_filter_node, so you don't
>> need to set it again, isn't it?
>
>
> I will remove the extra assignment from the cor_open() for consistancy.
>
> Andrey
>
>
No, I won't. It is wrong to do that. COR-operations wouldn't work.
Andrey
>>
>>> +
>>> + return cor_filter_bs;
>>> +}
>>> +
...
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v6 3/4] qapi: add filter-node-name to block-stream
2020-08-18 21:24 [PATCH v6 0/4] Apply COR-filter to the block-stream permanently Andrey Shinkevich
2020-08-18 21:24 ` [PATCH v6 1/4] copy-on-read: Support preadv/pwritev_part functions Andrey Shinkevich
2020-08-18 21:24 ` [PATCH v6 2/4] copy-on-read: add filter append/drop functions Andrey Shinkevich
@ 2020-08-18 21:24 ` Andrey Shinkevich
2020-08-19 10:29 ` Vladimir Sementsov-Ogievskiy
2020-08-24 13:21 ` Markus Armbruster
2020-08-18 21:24 ` [PATCH v6 4/4] block: apply COR-filter to block-stream jobs Andrey Shinkevich
3 siblings, 2 replies; 16+ messages in thread
From: Andrey Shinkevich @ 2020-08-18 21:24 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, vsementsov, armbru, qemu-devel, andrey.shinkevich, den,
mreitz, jsnow
Provide the possibility to pass the 'filter-node-name' parameter to the
block-stream job as it is done for the commit block job. That will be
needed for further iotests implementations.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
block/monitor/block-hmp-cmds.c | 4 ++--
block/stream.c | 4 +++-
blockdev.c | 8 +++++++-
include/block/block_int.h | 7 ++++++-
qapi/block-core.json | 6 ++++++
5 files changed, 24 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 b9c1141..8bf6b6d 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 237fffb..800ecb3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2476,6 +2476,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)
@@ -2491,6 +2492,10 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
on_error = BLOCKDEV_ON_ERROR_REPORT;
}
+ if (!has_filter_node_name) {
+ filter_node_name = NULL;
+ }
+
bs = bdrv_lookup_bs(device, device, errp);
if (!bs) {
return;
@@ -2558,7 +2563,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 465a601..3efde33 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1122,6 +1122,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
@@ -1134,7 +1137,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 0b8ccd3..1db6ce1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2524,6 +2524,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.1)
+#
# @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.
@@ -2554,6 +2559,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] 16+ messages in thread* Re: [PATCH v6 3/4] qapi: add filter-node-name to block-stream
2020-08-18 21:24 ` [PATCH v6 3/4] qapi: add filter-node-name to block-stream Andrey Shinkevich
@ 2020-08-19 10:29 ` Vladimir Sementsov-Ogievskiy
2020-08-23 19:33 ` Andrey Shinkevich
2020-08-24 13:21 ` Markus Armbruster
1 sibling, 1 reply; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-08-19 10:29 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block
Cc: kwolf, armbru, qemu-devel, den, mreitz, jsnow
19.08.2020 00:24, Andrey Shinkevich wrote:
> Provide the possibility to pass the 'filter-node-name' parameter to the
> block-stream job as it is done for the commit block job. That will be
> needed for further iotests implementations.
and for users as well. I think, the last sentence may be dropped.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> block/monitor/block-hmp-cmds.c | 4 ++--
> block/stream.c | 4 +++-
> blockdev.c | 8 +++++++-
> include/block/block_int.h | 7 ++++++-
> qapi/block-core.json | 6 ++++++
> 5 files changed, 24 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 b9c1141..8bf6b6d 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 237fffb..800ecb3 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2476,6 +2476,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)
> @@ -2491,6 +2492,10 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
> on_error = BLOCKDEV_ON_ERROR_REPORT;
> }
>
> + if (!has_filter_node_name) {
> + filter_node_name = NULL;
> + }
this works automatically, you don't need to initialize it by hand
> +
> bs = bdrv_lookup_bs(device, device, errp);
> if (!bs) {
> return;
> @@ -2558,7 +2563,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 465a601..3efde33 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1122,6 +1122,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
> @@ -1134,7 +1137,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 0b8ccd3..1db6ce1 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2524,6 +2524,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.1)
> +#
> # @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.
> @@ -2554,6 +2559,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' } }
>
> ##
>
With extra "filter_node_name = NULL" dropped:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v6 3/4] qapi: add filter-node-name to block-stream
2020-08-19 10:29 ` Vladimir Sementsov-Ogievskiy
@ 2020-08-23 19:33 ` Andrey Shinkevich
0 siblings, 0 replies; 16+ messages in thread
From: Andrey Shinkevich @ 2020-08-23 19:33 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, armbru, qemu-devel, den, mreitz, jsnow
On 19.08.2020 13:29, Vladimir Sementsov-Ogievskiy wrote:
> 19.08.2020 00:24, Andrey Shinkevich wrote:
>> Provide the possibility to pass the 'filter-node-name' parameter to the
>> block-stream job as it is done for the commit block job. That will be
>> needed for further iotests implementations.
>
> and for users as well. I think, the last sentence may be dropped.
>
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>> block/monitor/block-hmp-cmds.c | 4 ++--
>> block/stream.c | 4 +++-
>> blockdev.c | 8 +++++++-
>> include/block/block_int.h | 7 ++++++-
>> qapi/block-core.json | 6 ++++++
>> 5 files changed, 24 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 b9c1141..8bf6b6d 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 237fffb..800ecb3 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2476,6 +2476,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)
>> @@ -2491,6 +2492,10 @@ void qmp_block_stream(bool has_job_id, const
>> char *job_id, const char *device,
>> on_error = BLOCKDEV_ON_ERROR_REPORT;
>> }
>> + if (!has_filter_node_name) {
>> + filter_node_name = NULL;
>> + }
>
> this works automatically, you don't need to initialize it by hand
In the current impementation of QEMU, it may look like an excess. I will
remove it. But the qmp_block_commit() has that check.
Andrey
>
>> +
>> bs = bdrv_lookup_bs(device, device, errp);
>> if (!bs) {
>> return;
>> @@ -2558,7 +2563,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 465a601..3efde33 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -1122,6 +1122,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
>> @@ -1134,7 +1137,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 0b8ccd3..1db6ce1 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2524,6 +2524,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.1)
>> +#
>> # @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.
>> @@ -2554,6 +2559,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' } }
>> ##
>>
>
>
> With extra "filter_node_name = NULL" dropped:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v6 3/4] qapi: add filter-node-name to block-stream
2020-08-18 21:24 ` [PATCH v6 3/4] qapi: add filter-node-name to block-stream Andrey Shinkevich
2020-08-19 10:29 ` Vladimir Sementsov-Ogievskiy
@ 2020-08-24 13:21 ` Markus Armbruster
1 sibling, 0 replies; 16+ messages in thread
From: Markus Armbruster @ 2020-08-24 13:21 UTC (permalink / raw)
To: Andrey Shinkevich
Cc: kwolf, vsementsov, qemu-block, qemu-devel, mreitz, den, jsnow
Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:
> Provide the possibility to pass the 'filter-node-name' parameter to the
> block-stream job as it is done for the commit block job. That will be
> needed for further iotests implementations.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0b8ccd3..1db6ce1 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2524,6 +2524,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.1)
It's (Since: 5.2) now.
> +#
> # @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.
> @@ -2554,6 +2559,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' } }
>
> ##
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v6 4/4] block: apply COR-filter to block-stream jobs
2020-08-18 21:24 [PATCH v6 0/4] Apply COR-filter to the block-stream permanently Andrey Shinkevich
` (2 preceding siblings ...)
2020-08-18 21:24 ` [PATCH v6 3/4] qapi: add filter-node-name to block-stream Andrey Shinkevich
@ 2020-08-18 21:24 ` Andrey Shinkevich
2020-08-19 10:46 ` Vladimir Sementsov-Ogievskiy
3 siblings, 1 reply; 16+ messages in thread
From: Andrey Shinkevich @ 2020-08-18 21:24 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, vsementsov, armbru, qemu-devel, andrey.shinkevich, den,
mreitz, jsnow
The patch completes the series with the COR-filter insertion to any
block-stream operation. It also makes changes to the iotests 030.
The test case 'test_stream_parallel' was deleted due to multiple
errors.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
block/stream.c | 76 ++++++++++++++++++++++++++++++++--------------
tests/qemu-iotests/030 | 50 +++---------------------------
tests/qemu-iotests/030.out | 4 +--
3 files changed, 61 insertions(+), 69 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 8bf6b6d..0b11979 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -19,6 +19,7 @@
#include "qapi/qmp/qerror.h"
#include "qemu/ratelimit.h"
#include "sysemu/block-backend.h"
+#include "block/copy-on-read.h"
enum {
/*
@@ -33,8 +34,11 @@ typedef struct StreamBlockJob {
BlockJob common;
BlockDriverState *base_overlay; /* COW overlay (stream from this) */
BlockDriverState *above_base; /* Node directly above the base */
+ BlockDriverState *cor_filter_bs;
+ BlockDriverState *target_bs;
BlockdevOnError on_error;
char *backing_file_str;
+ char *base_fmt;
bool bs_read_only;
bool chain_frozen;
} StreamBlockJob;
@@ -53,34 +57,26 @@ 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 *bs = s->target_bs;
BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_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)) {
- 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;
- }
- }
bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
- ret = bdrv_change_backing_file(unfiltered_bs, base_id, base_fmt);
+ ret = bdrv_change_backing_file(unfiltered_bs, s->backing_file_str,
+ s->base_fmt);
if (local_err) {
error_report_err(local_err);
return -EPERM;
@@ -94,7 +90,9 @@ static void stream_clean(Job *job)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockJob *bjob = &s->common;
- BlockDriverState *bs = blk_bs(bjob->blk);
+ BlockDriverState *bs = s->target_bs;
+
+ bdrv_cor_filter_drop(s->cor_filter_bs);
/* Reopen the image back in read-only mode if necessary */
if (s->bs_read_only) {
@@ -104,13 +102,14 @@ static void stream_clean(Job *job)
}
g_free(s->backing_file_str);
+ g_free(s->base_fmt);
}
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 *bs = s->target_bs;
BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
bool enable_cor = !bdrv_cow_child(s->base_overlay);
int64_t len;
@@ -231,6 +230,12 @@ void stream_start(const char *job_id, BlockDriverState *bs,
int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
BlockDriverState *base_overlay = bdrv_find_overlay(bs, base);
BlockDriverState *above_base;
+ BlockDriverState *cor_filter_bs = NULL;
+ char *base_fmt = NULL;
+
+ if (base && base->drv) {
+ base_fmt = g_strdup(base->drv->format_name);
+ }
if (!base_overlay) {
error_setg(errp, "'%s' is not in the backing chain of '%s'",
@@ -264,17 +269,36 @@ 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,
+ cor_filter_bs = bdrv_cor_filter_append(bs, filter_node_name, errp);
+ if (cor_filter_bs == NULL) {
+ goto fail;
+ }
+
+ 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
@@ -294,6 +318,9 @@ 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->base_fmt = base_fmt;
s->backing_file_str = g_strdup(backing_file_str);
s->bs_read_only = bs_read_only;
s->chain_frozen = true;
@@ -307,5 +334,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 1cdd7e2..fec9d89 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -221,60 +221,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
- 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=512*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',
@@ -287,7 +247,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
--
1.8.3.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v6 4/4] block: apply COR-filter to block-stream jobs
2020-08-18 21:24 ` [PATCH v6 4/4] block: apply COR-filter to block-stream jobs Andrey Shinkevich
@ 2020-08-19 10:46 ` Vladimir Sementsov-Ogievskiy
2020-08-23 19:28 ` Andrey Shinkevich
0 siblings, 1 reply; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-08-19 10:46 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block
Cc: kwolf, armbru, qemu-devel, den, mreitz, jsnow
19.08.2020 00:24, Andrey Shinkevich wrote:
> The patch completes the series with the COR-filter insertion to any
> block-stream operation. It also makes changes to the iotests 030.
> The test case 'test_stream_parallel' was deleted due to multiple
> errors.
"case deleted due to errors" is a bad reasoning.
If you remove the case, you should give detailed explanation, why it is
removed, what is the problem with it, what are the consequences, what is
not supported anymore.
Also, good to note here, that adding a filter here makes possible to
implement discarding already copied regions from backing files during
the job, to reduce disk over-usage.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> block/stream.c | 76 ++++++++++++++++++++++++++++++++--------------
> tests/qemu-iotests/030 | 50 +++---------------------------
> tests/qemu-iotests/030.out | 4 +--
> 3 files changed, 61 insertions(+), 69 deletions(-)
>
> diff --git a/block/stream.c b/block/stream.c
> index 8bf6b6d..0b11979 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -19,6 +19,7 @@
> #include "qapi/qmp/qerror.h"
> #include "qemu/ratelimit.h"
> #include "sysemu/block-backend.h"
> +#include "block/copy-on-read.h"
>
> enum {
> /*
> @@ -33,8 +34,11 @@ typedef struct StreamBlockJob {
> BlockJob common;
> BlockDriverState *base_overlay; /* COW overlay (stream from this) */
> BlockDriverState *above_base; /* Node directly above the base */
> + BlockDriverState *cor_filter_bs;
> + BlockDriverState *target_bs;
> BlockdevOnError on_error;
> char *backing_file_str;
> + char *base_fmt;
> bool bs_read_only;
> bool chain_frozen;
> } StreamBlockJob;
> @@ -53,34 +57,26 @@ 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 *bs = s->target_bs;
> BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
> BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_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)) {
> - 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;
> - }
> - }
> bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
> - ret = bdrv_change_backing_file(unfiltered_bs, base_id, base_fmt);
> + ret = bdrv_change_backing_file(unfiltered_bs, s->backing_file_str,
> + s->base_fmt);
> if (local_err) {
> error_report_err(local_err);
> return -EPERM;
> @@ -94,7 +90,9 @@ static void stream_clean(Job *job)
> {
> StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
> BlockJob *bjob = &s->common;
> - BlockDriverState *bs = blk_bs(bjob->blk);
> + BlockDriverState *bs = s->target_bs;
> +
> + bdrv_cor_filter_drop(s->cor_filter_bs);
>
> /* Reopen the image back in read-only mode if necessary */
> if (s->bs_read_only) {
> @@ -104,13 +102,14 @@ static void stream_clean(Job *job)
> }
>
> g_free(s->backing_file_str);
> + g_free(s->base_fmt);
> }
>
> 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 *bs = s->target_bs;
> BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
> bool enable_cor = !bdrv_cow_child(s->base_overlay);
> int64_t len;
> @@ -231,6 +230,12 @@ void stream_start(const char *job_id, BlockDriverState *bs,
> int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
> BlockDriverState *base_overlay = bdrv_find_overlay(bs, base);
> BlockDriverState *above_base;
> + BlockDriverState *cor_filter_bs = NULL;
> + char *base_fmt = NULL;
> +
> + if (base && base->drv) {
> + base_fmt = g_strdup(base->drv->format_name);
> + }
>
> if (!base_overlay) {
> error_setg(errp, "'%s' is not in the backing chain of '%s'",
> @@ -264,17 +269,36 @@ 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,
> + cor_filter_bs = bdrv_cor_filter_append(bs, filter_node_name, errp);
> + if (cor_filter_bs == NULL) {
> + goto fail;
> + }
> +
> + 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
> @@ -294,6 +318,9 @@ 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->base_fmt = base_fmt;
it's wrong to keep base_fmt during the job, as base may change
> s->backing_file_str = g_strdup(backing_file_str);
> s->bs_read_only = bs_read_only;
> s->chain_frozen = true;
> @@ -307,5 +334,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 1cdd7e2..fec9d89 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -221,60 +221,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
> - 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=512*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',
> @@ -287,7 +247,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
>
If we are going to drop this case.. Maybe, we should revert c624b015bf14f "block/stream: introduce a bottom node"? I see now, that it was bad idea..
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v6 4/4] block: apply COR-filter to block-stream jobs
2020-08-19 10:46 ` Vladimir Sementsov-Ogievskiy
@ 2020-08-23 19:28 ` Andrey Shinkevich
2020-08-24 8:20 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 16+ messages in thread
From: Andrey Shinkevich @ 2020-08-23 19:28 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, armbru, qemu-devel, den, mreitz, jsnow
On 19.08.2020 13:46, Vladimir Sementsov-Ogievskiy wrote:
> 19.08.2020 00:24, Andrey Shinkevich wrote:
>> The patch completes the series with the COR-filter insertion to any
>> block-stream operation. It also makes changes to the iotests 030.
>> The test case 'test_stream_parallel' was deleted due to multiple
>> errors.
>
...
>
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>> block/stream.c | 76
>> ++++++++++++++++++++++++++++++++--------------
>> tests/qemu-iotests/030 | 50 +++---------------------------
>> tests/qemu-iotests/030.out | 4 +--
>> 3 files changed, 61 insertions(+), 69 deletions(-)
>>
>> diff --git a/block/stream.c b/block/stream.c
>> index 8bf6b6d..0b11979 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -19,6 +19,7 @@
>> #include "qapi/qmp/qerror.h"
>> #include "qemu/ratelimit.h"
>> #include "sysemu/block-backend.h"
>> +#include "block/copy-on-read.h"
>> enum {
>> /*
>> @@ -33,8 +34,11 @@ typedef struct StreamBlockJob {
>> BlockJob common;
>> BlockDriverState *base_overlay; /* COW overlay (stream from
>> this) */
>> BlockDriverState *above_base; /* Node directly above the base */
>> + BlockDriverState *cor_filter_bs;
>> + BlockDriverState *target_bs;
>> BlockdevOnError on_error;
>> char *backing_file_str;
>> + char *base_fmt;
>> bool bs_read_only;
>> bool chain_frozen;
>> } StreamBlockJob;
>> @@ -53,34 +57,26 @@ 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 *bs = s->target_bs;
>> BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
>> BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_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)) {
>> - 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;
>> - }
>> - }
>> bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
>> - ret = bdrv_change_backing_file(unfiltered_bs, base_id,
>> base_fmt);
>> + ret = bdrv_change_backing_file(unfiltered_bs,
>> s->backing_file_str,
>> + s->base_fmt);
>> if (local_err) {
>> error_report_err(local_err);
>> return -EPERM;
>> @@ -94,7 +90,9 @@ static void stream_clean(Job *job)
>> {
>> StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>> BlockJob *bjob = &s->common;
>> - BlockDriverState *bs = blk_bs(bjob->blk);
>> + BlockDriverState *bs = s->target_bs;
>> +
>> + bdrv_cor_filter_drop(s->cor_filter_bs);
>> /* Reopen the image back in read-only mode if necessary */
>> if (s->bs_read_only) {
>> @@ -104,13 +102,14 @@ static void stream_clean(Job *job)
>> }
>> g_free(s->backing_file_str);
>> + g_free(s->base_fmt);
>> }
>> 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 *bs = s->target_bs;
>> BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
>> bool enable_cor = !bdrv_cow_child(s->base_overlay);
>> int64_t len;
>> @@ -231,6 +230,12 @@ void stream_start(const char *job_id,
>> BlockDriverState *bs,
>> int basic_flags = BLK_PERM_CONSISTENT_READ |
>> BLK_PERM_WRITE_UNCHANGED;
>> BlockDriverState *base_overlay = bdrv_find_overlay(bs, base);
>> BlockDriverState *above_base;
>> + BlockDriverState *cor_filter_bs = NULL;
>> + char *base_fmt = NULL;
>> +
>> + if (base && base->drv) {
>> + base_fmt = g_strdup(base->drv->format_name);
>> + }
>> if (!base_overlay) {
>> error_setg(errp, "'%s' is not in the backing chain of '%s'",
>> @@ -264,17 +269,36 @@ 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,
>> + cor_filter_bs = bdrv_cor_filter_append(bs, filter_node_name, errp);
>> + if (cor_filter_bs == NULL) {
>> + goto fail;
>> + }
>> +
>> + 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
>> @@ -294,6 +318,9 @@ 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->base_fmt = base_fmt;
>
> it's wrong to keep base_fmt during the job, as base may change
So, the format can differ from that of the backing_file_str given as the
input parameter of the stream_start()...
...while the backing_file_str path is written to the QCOW2 header on a disk.
Andrey
>
>> s->backing_file_str = g_strdup(backing_file_str);
>> s->bs_read_only = bs_read_only;
>> s->chain_frozen = true;
>> @@ -307,5 +334,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 1cdd7e2..fec9d89 100755
>> --- a/tests/qemu-iotests/030
>> +++ b/tests/qemu-iotests/030
>> @@ -221,60 +221,20 @@ class TestParallelOps(iotests.QMPTestCase):
>> for img in self.imgs:
>> os.remove(img)
...
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v6 4/4] block: apply COR-filter to block-stream jobs
2020-08-23 19:28 ` Andrey Shinkevich
@ 2020-08-24 8:20 ` Vladimir Sementsov-Ogievskiy
2020-08-24 8:49 ` Andrey Shinkevich
0 siblings, 1 reply; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-08-24 8:20 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block
Cc: kwolf, armbru, qemu-devel, den, mreitz, jsnow
23.08.2020 22:28, Andrey Shinkevich wrote:
> On 19.08.2020 13:46, Vladimir Sementsov-Ogievskiy wrote:
>> 19.08.2020 00:24, Andrey Shinkevich wrote:
>>> The patch completes the series with the COR-filter insertion to any
>>> block-stream operation. It also makes changes to the iotests 030.
>>> The test case 'test_stream_parallel' was deleted due to multiple
>>> errors.
>>
> ...
>>
>>>
>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>> ---
>>> block/stream.c | 76 ++++++++++++++++++++++++++++++++--------------
>>> tests/qemu-iotests/030 | 50 +++---------------------------
>>> tests/qemu-iotests/030.out | 4 +--
>>> 3 files changed, 61 insertions(+), 69 deletions(-)
>>>
>>> diff --git a/block/stream.c b/block/stream.c
>>> index 8bf6b6d..0b11979 100644
>>> --- a/block/stream.c
>>> +++ b/block/stream.c
>>> @@ -19,6 +19,7 @@
>>> #include "qapi/qmp/qerror.h"
>>> #include "qemu/ratelimit.h"
>>> #include "sysemu/block-backend.h"
>>> +#include "block/copy-on-read.h"
>>> enum {
>>> /*
>>> @@ -33,8 +34,11 @@ typedef struct StreamBlockJob {
>>> BlockJob common;
>>> BlockDriverState *base_overlay; /* COW overlay (stream from this) */
>>> BlockDriverState *above_base; /* Node directly above the base */
>>> + BlockDriverState *cor_filter_bs;
>>> + BlockDriverState *target_bs;
>>> BlockdevOnError on_error;
>>> char *backing_file_str;
>>> + char *base_fmt;
>>> bool bs_read_only;
>>> bool chain_frozen;
>>> } StreamBlockJob;
>>> @@ -53,34 +57,26 @@ 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 *bs = s->target_bs;
>>> BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
>>> BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_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)) {
>>> - 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;
>>> - }
>>> - }
>>> bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
>>> - ret = bdrv_change_backing_file(unfiltered_bs, base_id, base_fmt);
>>> + ret = bdrv_change_backing_file(unfiltered_bs, s->backing_file_str,
>>> + s->base_fmt);
>>> if (local_err) {
>>> error_report_err(local_err);
>>> return -EPERM;
>>> @@ -94,7 +90,9 @@ static void stream_clean(Job *job)
>>> {
>>> StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>>> BlockJob *bjob = &s->common;
>>> - BlockDriverState *bs = blk_bs(bjob->blk);
>>> + BlockDriverState *bs = s->target_bs;
>>> +
>>> + bdrv_cor_filter_drop(s->cor_filter_bs);
>>> /* Reopen the image back in read-only mode if necessary */
>>> if (s->bs_read_only) {
>>> @@ -104,13 +102,14 @@ static void stream_clean(Job *job)
>>> }
>>> g_free(s->backing_file_str);
>>> + g_free(s->base_fmt);
>>> }
>>> 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 *bs = s->target_bs;
>>> BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
>>> bool enable_cor = !bdrv_cow_child(s->base_overlay);
>>> int64_t len;
>>> @@ -231,6 +230,12 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>>> int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
>>> BlockDriverState *base_overlay = bdrv_find_overlay(bs, base);
>>> BlockDriverState *above_base;
>>> + BlockDriverState *cor_filter_bs = NULL;
>>> + char *base_fmt = NULL;
>>> +
>>> + if (base && base->drv) {
>>> + base_fmt = g_strdup(base->drv->format_name);
>>> + }
>>> if (!base_overlay) {
>>> error_setg(errp, "'%s' is not in the backing chain of '%s'",
>>> @@ -264,17 +269,36 @@ 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,
>>> + cor_filter_bs = bdrv_cor_filter_append(bs, filter_node_name, errp);
>>> + if (cor_filter_bs == NULL) {
>>> + goto fail;
>>> + }
>>> +
>>> + 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
>>> @@ -294,6 +318,9 @@ 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->base_fmt = base_fmt;
>>
>> it's wrong to keep base_fmt during the job, as base may change
>
>
> So, the format can differ from that of the backing_file_str given as the input parameter of the stream_start()...
>
> ...while the backing_file_str path is written to the QCOW2 header on a disk.
>
Or better, let's try to revert c624b015bf14f and freeze base again.
>
>
>>
>>> s->backing_file_str = g_strdup(backing_file_str);
>>> s->bs_read_only = bs_read_only;
>>> s->chain_frozen = true;
>>> @@ -307,5 +334,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 1cdd7e2..fec9d89 100755
>>> --- a/tests/qemu-iotests/030
>>> +++ b/tests/qemu-iotests/030
>>> @@ -221,60 +221,20 @@ class TestParallelOps(iotests.QMPTestCase):
>>> for img in self.imgs:
>>> os.remove(img)
> ...
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v6 4/4] block: apply COR-filter to block-stream jobs
2020-08-24 8:20 ` Vladimir Sementsov-Ogievskiy
@ 2020-08-24 8:49 ` Andrey Shinkevich
0 siblings, 0 replies; 16+ messages in thread
From: Andrey Shinkevich @ 2020-08-24 8:49 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, armbru, qemu-devel, den, mreitz, jsnow
[-- Attachment #1: Type: text/plain, Size: 9937 bytes --]
On 24.08.2020 11:20, Vladimir Sementsov-Ogievskiy wrote:
> 23.08.2020 22:28, Andrey Shinkevich wrote:
>> On 19.08.2020 13:46, Vladimir Sementsov-Ogievskiy wrote:
>>> 19.08.2020 00:24, Andrey Shinkevich wrote:
>>>> The patch completes the series with the COR-filter insertion to any
>>>> block-stream operation. It also makes changes to the iotests 030.
>>>> The test case 'test_stream_parallel' was deleted due to multiple
>>>> errors.
>>>
>> ...
>>>
>>>>
>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>> ---
>>>> block/stream.c | 76
>>>> ++++++++++++++++++++++++++++++++--------------
>>>> tests/qemu-iotests/030 | 50 +++---------------------------
>>>> tests/qemu-iotests/030.out | 4 +--
>>>> 3 files changed, 61 insertions(+), 69 deletions(-)
>>>>
>>>> diff --git a/block/stream.c b/block/stream.c
>>>> index 8bf6b6d..0b11979 100644
>>>> --- a/block/stream.c
>>>> +++ b/block/stream.c
>>>> @@ -19,6 +19,7 @@
>>>> #include "qapi/qmp/qerror.h"
>>>> #include "qemu/ratelimit.h"
>>>> #include "sysemu/block-backend.h"
>>>> +#include "block/copy-on-read.h"
>>>> enum {
>>>> /*
>>>> @@ -33,8 +34,11 @@ typedef struct StreamBlockJob {
>>>> BlockJob common;
>>>> BlockDriverState *base_overlay; /* COW overlay (stream from
>>>> this) */
>>>> BlockDriverState *above_base; /* Node directly above the
>>>> base */
>>>> + BlockDriverState *cor_filter_bs;
>>>> + BlockDriverState *target_bs;
>>>> BlockdevOnError on_error;
>>>> char *backing_file_str;
>>>> + char *base_fmt;
>>>> bool bs_read_only;
>>>> bool chain_frozen;
>>>> } StreamBlockJob;
>>>> @@ -53,34 +57,26 @@ 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 *bs = s->target_bs;
>>>> BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
>>>> BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_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)) {
>>>> - 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;
>>>> - }
>>>> - }
>>>> bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
>>>> - ret = bdrv_change_backing_file(unfiltered_bs, base_id,
>>>> base_fmt);
>>>> + ret = bdrv_change_backing_file(unfiltered_bs,
>>>> s->backing_file_str,
>>>> + s->base_fmt);
>>>> if (local_err) {
>>>> error_report_err(local_err);
>>>> return -EPERM;
>>>> @@ -94,7 +90,9 @@ static void stream_clean(Job *job)
>>>> {
>>>> StreamBlockJob *s = container_of(job, StreamBlockJob,
>>>> common.job);
>>>> BlockJob *bjob = &s->common;
>>>> - BlockDriverState *bs = blk_bs(bjob->blk);
>>>> + BlockDriverState *bs = s->target_bs;
>>>> +
>>>> + bdrv_cor_filter_drop(s->cor_filter_bs);
>>>> /* Reopen the image back in read-only mode if necessary */
>>>> if (s->bs_read_only) {
>>>> @@ -104,13 +102,14 @@ static void stream_clean(Job *job)
>>>> }
>>>> g_free(s->backing_file_str);
>>>> + g_free(s->base_fmt);
>>>> }
>>>> 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 *bs = s->target_bs;
>>>> BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
>>>> bool enable_cor = !bdrv_cow_child(s->base_overlay);
>>>> int64_t len;
>>>> @@ -231,6 +230,12 @@ void stream_start(const char *job_id,
>>>> BlockDriverState *bs,
>>>> int basic_flags = BLK_PERM_CONSISTENT_READ |
>>>> BLK_PERM_WRITE_UNCHANGED;
>>>> BlockDriverState *base_overlay = bdrv_find_overlay(bs, base);
>>>> BlockDriverState *above_base;
>>>> + BlockDriverState *cor_filter_bs = NULL;
>>>> + char *base_fmt = NULL;
>>>> +
>>>> + if (base && base->drv) {
>>>> + base_fmt = g_strdup(base->drv->format_name);
>>>> + }
>>>> if (!base_overlay) {
>>>> error_setg(errp, "'%s' is not in the backing chain of '%s'",
>>>> @@ -264,17 +269,36 @@ 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,
>>>> + cor_filter_bs = bdrv_cor_filter_append(bs, filter_node_name,
>>>> errp);
>>>> + if (cor_filter_bs == NULL) {
>>>> + goto fail;
>>>> + }
>>>> +
>>>> + 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
>>>> @@ -294,6 +318,9 @@ 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->base_fmt = base_fmt;
>>>
>>> it's wrong to keep base_fmt during the job, as base may change
>>
>>
>> So, the format can differ from that of the backing_file_str given as
>> the input parameter of the stream_start()...
>>
>> ...while the backing_file_str path is written to the QCOW2 header on
>> a disk.
>>
>
> Or better, let's try to revert c624b015bf14f and freeze base again.
The reversion won't help as the patch "stream: Deal with filters" did
that work already. We have to freeze the base again. I guess it will
discard the concept of the 'base_overlay' and the 'above_base'
introduced by Max in the series "block: Deal withfilters".
Andrey
>
>>
>>
>>>
>>>> s->backing_file_str = g_strdup(backing_file_str);
>>>> s->bs_read_only = bs_read_only;
>>>> s->chain_frozen = true;
>>>> @@ -307,5 +334,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 1cdd7e2..fec9d89 100755
>>>> --- a/tests/qemu-iotests/030
>>>> +++ b/tests/qemu-iotests/030
>>>> @@ -221,60 +221,20 @@ class TestParallelOps(iotests.QMPTestCase):
>>>> for img in self.imgs:
>>>> os.remove(img)
>> ...
>
>
[-- Attachment #2: Type: text/html, Size: 21934 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread