* [PATCH v5 1/6] qcow2: Fix dangling pointer after reopen for 'file'
2021-07-06 11:23 [PATCH v5 0/6] Make blockdev-reopen stable Kevin Wolf
@ 2021-07-06 11:23 ` Kevin Wolf
2021-07-06 13:12 ` Vladimir Sementsov-Ogievskiy
2021-07-06 11:23 ` [PATCH v5 2/6] block: Add bdrv_reopen_queue_free() Kevin Wolf
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2021-07-06 11:23 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, berto, qemu-devel, mreitz
Without an external data file, s->data_file is a second pointer with the
same value as bs->file. When changing bs->file to a different BdrvChild
and freeing the old BdrvChild, s->data_file must also be updated,
otherwise it points to freed memory and causes crashes.
This problem was caught by iotests case 245.
Fixes: df2b7086f169239ebad5d150efa29c9bb6d4f820
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/block/qcow2.c b/block/qcow2.c
index ee4530cdbd..cb459ef6a6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -962,6 +962,7 @@ static bool read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
}
typedef struct Qcow2ReopenState {
+ bool had_data_file;
Qcow2Cache *l2_table_cache;
Qcow2Cache *refcount_block_cache;
int l2_slice_size; /* Number of entries in a slice of the L2 table */
@@ -1932,6 +1933,8 @@ static int qcow2_reopen_prepare(BDRVReopenState *state,
r = g_new0(Qcow2ReopenState, 1);
state->opaque = r;
+ r->had_data_file = has_data_file(state->bs);
+
ret = qcow2_update_options_prepare(state->bs, r, state->options,
state->flags, errp);
if (ret < 0) {
@@ -1966,7 +1969,18 @@ fail:
static void qcow2_reopen_commit(BDRVReopenState *state)
{
+ BDRVQcow2State *s = state->bs->opaque;
+ Qcow2ReopenState *r = state->opaque;
+
qcow2_update_options_commit(state->bs, state->opaque);
+ if (!r->had_data_file && s->data_file != state->bs->file) {
+ /*
+ * If s->data_file is just a second pointer to bs->file (which is the
+ * case without an external data file), it may need to be updated.
+ */
+ s->data_file = state->bs->file;
+ assert(!has_data_file(state->bs));
+ }
g_free(state->opaque);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v5 1/6] qcow2: Fix dangling pointer after reopen for 'file'
2021-07-06 11:23 ` [PATCH v5 1/6] qcow2: Fix dangling pointer after reopen for 'file' Kevin Wolf
@ 2021-07-06 13:12 ` Vladimir Sementsov-Ogievskiy
2021-07-06 13:43 ` Kevin Wolf
0 siblings, 1 reply; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-07-06 13:12 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: mreitz, berto, qemu-devel
06.07.2021 14:23, Kevin Wolf wrote:
> Without an external data file, s->data_file is a second pointer with the
> same value as bs->file. When changing bs->file to a different BdrvChild
> and freeing the old BdrvChild, s->data_file must also be updated,
> otherwise it points to freed memory and causes crashes.
>
> This problem was caught by iotests case 245.
>
> Fixes: df2b7086f169239ebad5d150efa29c9bb6d4f820
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Still, some ideas below:
> ---
> block/qcow2.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index ee4530cdbd..cb459ef6a6 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -962,6 +962,7 @@ static bool read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
> }
>
> typedef struct Qcow2ReopenState {
> + bool had_data_file;
> Qcow2Cache *l2_table_cache;
> Qcow2Cache *refcount_block_cache;
> int l2_slice_size; /* Number of entries in a slice of the L2 table */
> @@ -1932,6 +1933,8 @@ static int qcow2_reopen_prepare(BDRVReopenState *state,
> r = g_new0(Qcow2ReopenState, 1);
> state->opaque = r;
>
> + r->had_data_file = has_data_file(state->bs);
> +
So, during reopen, at some moment s->data_file become invalid. So, we shouldn't rely on it..
Maybe we need
s->data_file = NULL;
here..
> ret = qcow2_update_options_prepare(state->bs, r, state->options,
> state->flags, errp);
> if (ret < 0) {
> @@ -1966,7 +1969,18 @@ fail:
>
> static void qcow2_reopen_commit(BDRVReopenState *state)
> {
> + BDRVQcow2State *s = state->bs->opaque;
> + Qcow2ReopenState *r = state->opaque;
> +
> qcow2_update_options_commit(state->bs, state->opaque);
Worth doing
assert(r->had_data_file == has_data_file(state->bs));
here, to be double sure?
> + if (!r->had_data_file && s->data_file != state->bs->file) {
> + /*
> + * If s->data_file is just a second pointer to bs->file (which is the
> + * case without an external data file), it may need to be updated.
> + */
> + s->data_file = state->bs->file;
> + assert(!has_data_file(state->bs));
> + }
> g_free(state->opaque);
> }
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 1/6] qcow2: Fix dangling pointer after reopen for 'file'
2021-07-06 13:12 ` Vladimir Sementsov-Ogievskiy
@ 2021-07-06 13:43 ` Kevin Wolf
0 siblings, 0 replies; 14+ messages in thread
From: Kevin Wolf @ 2021-07-06 13:43 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: berto, qemu-devel, qemu-block, mreitz
Am 06.07.2021 um 15:12 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 06.07.2021 14:23, Kevin Wolf wrote:
> > Without an external data file, s->data_file is a second pointer with the
> > same value as bs->file. When changing bs->file to a different BdrvChild
> > and freeing the old BdrvChild, s->data_file must also be updated,
> > otherwise it points to freed memory and causes crashes.
> >
> > This problem was caught by iotests case 245.
> >
> > Fixes: df2b7086f169239ebad5d150efa29c9bb6d4f820
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Still, some ideas below:
>
> > ---
> > block/qcow2.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index ee4530cdbd..cb459ef6a6 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -962,6 +962,7 @@ static bool read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
> > }
> > typedef struct Qcow2ReopenState {
> > + bool had_data_file;
> > Qcow2Cache *l2_table_cache;
> > Qcow2Cache *refcount_block_cache;
> > int l2_slice_size; /* Number of entries in a slice of the L2 table */
> > @@ -1932,6 +1933,8 @@ static int qcow2_reopen_prepare(BDRVReopenState *state,
> > r = g_new0(Qcow2ReopenState, 1);
> > state->opaque = r;
> > + r->had_data_file = has_data_file(state->bs);
> > +
>
> So, during reopen, at some moment s->data_file become invalid. So, we
> shouldn't rely on it..
>
> Maybe we need
>
> s->data_file = NULL;
>
> here..
"need" is a strong word, but I guess we shouldn't access it between
prepare and commit, so I agree setting it to NULL would make bugs in
this area very visible.
In fact, we wouldn't even need r->had_data_file then because commit
could just set s->data_file = state->bs->file if it's NULL.
> > ret = qcow2_update_options_prepare(state->bs, r, state->options,
> > state->flags, errp);
> > if (ret < 0) {
> > @@ -1966,7 +1969,18 @@ fail:
> > static void qcow2_reopen_commit(BDRVReopenState *state)
> > {
> > + BDRVQcow2State *s = state->bs->opaque;
> > + Qcow2ReopenState *r = state->opaque;
> > +
> > qcow2_update_options_commit(state->bs, state->opaque);
>
> Worth doing
>
> assert(r->had_data_file == has_data_file(state->bs));
>
> here, to be double sure?
This would be wrong because at the time that this runs, state->bs->file
is already updated and this is what has_data_file() checks against. So
you can't use has_data_file() any more until it's synced again with the
code below.
In fact, this is why I even added r->had_data_file. At first I directly
used has_data_file(state->bs) here and watched it break.
> > + if (!r->had_data_file && s->data_file != state->bs->file) {
> > + /*
> > + * If s->data_file is just a second pointer to bs->file (which is the
> > + * case without an external data file), it may need to be updated.
> > + */
> > + s->data_file = state->bs->file;
> > + assert(!has_data_file(state->bs));
> > + }
> > g_free(state->opaque);
> > }
Kevin
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 2/6] block: Add bdrv_reopen_queue_free()
2021-07-06 11:23 [PATCH v5 0/6] Make blockdev-reopen stable Kevin Wolf
2021-07-06 11:23 ` [PATCH v5 1/6] qcow2: Fix dangling pointer after reopen for 'file' Kevin Wolf
@ 2021-07-06 11:23 ` Kevin Wolf
2021-07-06 13:18 ` Vladimir Sementsov-Ogievskiy
2021-07-06 11:23 ` [PATCH v5 3/6] block: Acquire AioContexts during bdrv_reopen_multiple() Kevin Wolf
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2021-07-06 11:23 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, berto, qemu-devel, mreitz
From: Alberto Garcia <berto@igalia.com>
Move the code to free a BlockReopenQueue to a separate function.
It will be used in a subsequent patch.
[ kwolf: Also free explicit_options and options, and explicitly
qobject_ref() the value when it continues to be used. This avoids
memory leaks as we saw them in two recent patches. ]
Signed-off-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block.h | 1 +
block.c | 22 ++++++++++++++++------
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 7ec77ecb1a..6d42992985 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -386,6 +386,7 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
BlockDriverState *bs, QDict *options,
bool keep_old_opts);
+void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue);
int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
Error **errp);
diff --git a/block.c b/block.c
index acd35cb0cb..ee9b46e95e 100644
--- a/block.c
+++ b/block.c
@@ -4095,6 +4095,19 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
NULL, 0, keep_old_opts);
}
+void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue)
+{
+ if (bs_queue) {
+ BlockReopenQueueEntry *bs_entry, *next;
+ QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+ qobject_unref(bs_entry->state.explicit_options);
+ qobject_unref(bs_entry->state.options);
+ g_free(bs_entry);
+ }
+ g_free(bs_queue);
+ }
+}
+
/*
* Reopen multiple BlockDriverStates atomically & transactionally.
*
@@ -4197,15 +4210,10 @@ abort:
if (bs_entry->prepared) {
bdrv_reopen_abort(&bs_entry->state);
}
- qobject_unref(bs_entry->state.explicit_options);
- qobject_unref(bs_entry->state.options);
}
cleanup:
- QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
- g_free(bs_entry);
- }
- g_free(bs_queue);
+ bdrv_reopen_queue_free(bs_queue);
return ret;
}
@@ -4573,6 +4581,8 @@ static void bdrv_reopen_commit(BDRVReopenState *reopen_state)
/* set BDS specific flags now */
qobject_unref(bs->explicit_options);
qobject_unref(bs->options);
+ qobject_ref(reopen_state->explicit_options);
+ qobject_ref(reopen_state->options);
bs->explicit_options = reopen_state->explicit_options;
bs->options = reopen_state->options;
--
2.31.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/6] block: Add bdrv_reopen_queue_free()
2021-07-06 11:23 ` [PATCH v5 2/6] block: Add bdrv_reopen_queue_free() Kevin Wolf
@ 2021-07-06 13:18 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-07-06 13:18 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: mreitz, berto, qemu-devel
06.07.2021 14:23, Kevin Wolf wrote:
> From: Alberto Garcia<berto@igalia.com>
>
> Move the code to free a BlockReopenQueue to a separate function.
> It will be used in a subsequent patch.
>
> [ kwolf: Also free explicit_options and options, and explicitly
> qobject_ref() the value when it continues to be used. This avoids
> memory leaks as we saw them in two recent patches. ]
Hmm, not clear from the context which two patches you mean
>
> Signed-off-by: Alberto Garcia<berto@igalia.com>
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 3/6] block: Acquire AioContexts during bdrv_reopen_multiple()
2021-07-06 11:23 [PATCH v5 0/6] Make blockdev-reopen stable Kevin Wolf
2021-07-06 11:23 ` [PATCH v5 1/6] qcow2: Fix dangling pointer after reopen for 'file' Kevin Wolf
2021-07-06 11:23 ` [PATCH v5 2/6] block: Add bdrv_reopen_queue_free() Kevin Wolf
@ 2021-07-06 11:23 ` Kevin Wolf
2021-07-06 15:17 ` Vladimir Sementsov-Ogievskiy
2021-07-06 11:23 ` [PATCH v5 4/6] block: Support multiple reopening with x-blockdev-reopen Kevin Wolf
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2021-07-06 11:23 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, berto, qemu-devel, mreitz
As the BlockReopenQueue can contain nodes in multiple AioContexts, only
one of which may be locked when AIO_WAIT_WHILE() can be called, we can't
let the caller lock the right contexts. Instead, individually lock the
AioContext of a single node when iterating the queue.
Reintroduce bdrv_reopen() as a wrapper for reopening a single node that
drains the node and temporarily drops the AioContext lock for
bdrv_reopen_multiple().
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block.h | 2 ++
block.c | 49 ++++++++++++++++++++++++++++++++++++-------
block/replication.c | 7 +++++++
blockdev.c | 5 +++++
qemu-io-cmds.c | 7 +------
5 files changed, 57 insertions(+), 13 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 6d42992985..3477290f9a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -388,6 +388,8 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
bool keep_old_opts);
void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue);
int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
+int bdrv_reopen(BlockDriverState *bs, QDict *opts, bool keep_old_opts,
+ Error **errp);
int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
Error **errp);
int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
diff --git a/block.c b/block.c
index ee9b46e95e..06fb69df5c 100644
--- a/block.c
+++ b/block.c
@@ -4124,19 +4124,26 @@ void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue)
*
* All affected nodes must be drained between bdrv_reopen_queue() and
* bdrv_reopen_multiple().
+ *
+ * To be called from the main thread, with all other AioContexts unlocked.
*/
int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
{
int ret = -1;
BlockReopenQueueEntry *bs_entry, *next;
+ AioContext *ctx;
Transaction *tran = tran_new();
g_autoptr(GHashTable) found = NULL;
g_autoptr(GSList) refresh_list = NULL;
+ assert(qemu_get_current_aio_context() == qemu_get_aio_context());
assert(bs_queue != NULL);
QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
+ ctx = bdrv_get_aio_context(bs_entry->state.bs);
+ aio_context_acquire(ctx);
ret = bdrv_flush(bs_entry->state.bs);
+ aio_context_release(ctx);
if (ret < 0) {
error_setg_errno(errp, -ret, "Error flushing drive");
goto abort;
@@ -4145,7 +4152,10 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
assert(bs_entry->state.bs->quiesce_counter > 0);
+ ctx = bdrv_get_aio_context(bs_entry->state.bs);
+ aio_context_acquire(ctx);
ret = bdrv_reopen_prepare(&bs_entry->state, bs_queue, tran, errp);
+ aio_context_release(ctx);
if (ret < 0) {
goto abort;
}
@@ -4188,7 +4198,10 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
* to first element.
*/
QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
+ ctx = bdrv_get_aio_context(bs_entry->state.bs);
+ aio_context_acquire(ctx);
bdrv_reopen_commit(&bs_entry->state);
+ aio_context_release(ctx);
}
tran_commit(tran);
@@ -4197,7 +4210,10 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
BlockDriverState *bs = bs_entry->state.bs;
if (bs->drv->bdrv_reopen_commit_post) {
+ ctx = bdrv_get_aio_context(bs);
+ aio_context_acquire(ctx);
bs->drv->bdrv_reopen_commit_post(&bs_entry->state);
+ aio_context_release(ctx);
}
}
@@ -4208,7 +4224,10 @@ abort:
tran_abort(tran);
QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
if (bs_entry->prepared) {
+ ctx = bdrv_get_aio_context(bs_entry->state.bs);
+ aio_context_acquire(ctx);
bdrv_reopen_abort(&bs_entry->state);
+ aio_context_release(ctx);
}
}
@@ -4218,23 +4237,39 @@ cleanup:
return ret;
}
-int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
- Error **errp)
+int bdrv_reopen(BlockDriverState *bs, QDict *opts, bool keep_old_opts,
+ Error **errp)
{
- int ret;
+ AioContext *ctx = bdrv_get_aio_context(bs);
BlockReopenQueue *queue;
- QDict *opts = qdict_new();
-
- qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
+ int ret;
bdrv_subtree_drained_begin(bs);
- queue = bdrv_reopen_queue(NULL, bs, opts, true);
+ if (ctx != qemu_get_aio_context()) {
+ aio_context_release(ctx);
+ }
+
+ queue = bdrv_reopen_queue(NULL, bs, opts, keep_old_opts);
ret = bdrv_reopen_multiple(queue, errp);
+
+ if (ctx != qemu_get_aio_context()) {
+ aio_context_acquire(ctx);
+ }
bdrv_subtree_drained_end(bs);
return ret;
}
+int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
+ Error **errp)
+{
+ QDict *opts = qdict_new();
+
+ qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
+
+ return bdrv_reopen(bs, opts, true, errp);
+}
+
/*
* Take a BDRVReopenState and check if the value of 'backing' in the
* reopen_state->options QDict is valid or not.
diff --git a/block/replication.c b/block/replication.c
index 52163f2d1f..774e15df16 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -390,7 +390,14 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable,
}
if (reopen_queue) {
+ AioContext *ctx = bdrv_get_aio_context(bs);
+ if (ctx != qemu_get_aio_context()) {
+ aio_context_release(ctx);
+ }
bdrv_reopen_multiple(reopen_queue, errp);
+ if (ctx != qemu_get_aio_context()) {
+ aio_context_acquire(ctx);
+ }
}
bdrv_subtree_drained_end(s->hidden_disk->bs);
diff --git a/blockdev.c b/blockdev.c
index f08192deda..f657d090d3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3593,8 +3593,13 @@ void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp)
ctx = bdrv_get_aio_context(bs);
aio_context_acquire(ctx);
bdrv_subtree_drained_begin(bs);
+ aio_context_release(ctx);
+
queue = bdrv_reopen_queue(NULL, bs, qdict, false);
bdrv_reopen_multiple(queue, errp);
+
+ ctx = bdrv_get_aio_context(bs);
+ aio_context_acquire(ctx);
bdrv_subtree_drained_end(bs);
aio_context_release(ctx);
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index e8d862a426..46593d632d 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2116,8 +2116,6 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
bool writethrough = !blk_enable_write_cache(blk);
bool has_rw_option = false;
bool has_cache_option = false;
-
- BlockReopenQueue *brq;
Error *local_err = NULL;
while ((c = getopt(argc, argv, "c:o:rw")) != -1) {
@@ -2210,10 +2208,7 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
qdict_put_bool(opts, BDRV_OPT_CACHE_NO_FLUSH, flags & BDRV_O_NO_FLUSH);
}
- bdrv_subtree_drained_begin(bs);
- brq = bdrv_reopen_queue(NULL, bs, opts, true);
- bdrv_reopen_multiple(brq, &local_err);
- bdrv_subtree_drained_end(bs);
+ bdrv_reopen(bs, opts, true, &local_err);
if (local_err) {
error_report_err(local_err);
--
2.31.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v5 3/6] block: Acquire AioContexts during bdrv_reopen_multiple()
2021-07-06 11:23 ` [PATCH v5 3/6] block: Acquire AioContexts during bdrv_reopen_multiple() Kevin Wolf
@ 2021-07-06 15:17 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-07-06 15:17 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: mreitz, berto, qemu-devel
06.07.2021 14:23, Kevin Wolf wrote:
> As the BlockReopenQueue can contain nodes in multiple AioContexts, only
> one of which may be locked when AIO_WAIT_WHILE() can be called, we can't
> let the caller lock the right contexts. Instead, individually lock the
> AioContext of a single node when iterating the queue.
>
> Reintroduce bdrv_reopen() as a wrapper for reopening a single node that
> drains the node and temporarily drops the AioContext lock for
> bdrv_reopen_multiple().
>
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
I don't have the whole picture in mind. But looks clear:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 4/6] block: Support multiple reopening with x-blockdev-reopen
2021-07-06 11:23 [PATCH v5 0/6] Make blockdev-reopen stable Kevin Wolf
` (2 preceding siblings ...)
2021-07-06 11:23 ` [PATCH v5 3/6] block: Acquire AioContexts during bdrv_reopen_multiple() Kevin Wolf
@ 2021-07-06 11:23 ` Kevin Wolf
2021-07-06 15:45 ` Vladimir Sementsov-Ogievskiy
2021-07-06 11:23 ` [PATCH v5 5/6] iotests: Test reopening multiple devices at the same time Kevin Wolf
2021-07-06 11:23 ` [PATCH v5 6/6] block: Make blockdev-reopen stable API Kevin Wolf
5 siblings, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2021-07-06 11:23 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, berto, qemu-devel, mreitz
From: Alberto Garcia <berto@igalia.com>
[ kwolf: Fixed AioContext locking ]
Signed-off-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-core.json | 18 +++--
blockdev.c | 81 ++++++++++---------
tests/qemu-iotests/155 | 9 ++-
tests/qemu-iotests/165 | 4 +-
tests/qemu-iotests/245 | 27 ++++---
tests/qemu-iotests/248 | 2 +-
tests/qemu-iotests/248.out | 2 +-
tests/qemu-iotests/296 | 9 ++-
tests/qemu-iotests/298 | 4 +-
.../tests/remove-bitmap-from-backing | 18 +++--
10 files changed, 99 insertions(+), 75 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4a46552816..052520331e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4221,13 +4221,15 @@
##
# @x-blockdev-reopen:
#
-# Reopens a block device using the given set of options. Any option
-# not specified will be reset to its default value regardless of its
-# previous status. If an option cannot be changed or a particular
+# Reopens one or more block devices using the given set of options.
+# Any option not specified will be reset to its default value regardless
+# of its previous status. If an option cannot be changed or a particular
# driver does not support reopening then the command will return an
-# error.
+# error. All devices in the list are reopened in one transaction, so
+# if one of them fails then the whole transaction is cancelled.
#
-# The top-level @node-name option (from BlockdevOptions) must be
+# The command receives a list of block devices to reopen. For each one
+# of them, the top-level @node-name option (from BlockdevOptions) must be
# specified and is used to select the block device to be reopened.
# Other @node-name options must be either omitted or set to the
# current name of the appropriate node. This command won't change any
@@ -4247,8 +4249,8 @@
#
# 4) NULL: the current child (if any) is detached.
#
-# Options (1) and (2) are supported in all cases, but at the moment
-# only @backing allows replacing or detaching an existing child.
+# Options (1) and (2) are supported in all cases. Option (3) is
+# supported for @file and @backing, and option (4) for @backing only.
#
# Unlike with blockdev-add, the @backing option must always be present
# unless the node being reopened does not have a backing file and its
@@ -4258,7 +4260,7 @@
# Since: 4.0
##
{ 'command': 'x-blockdev-reopen',
- 'data': 'BlockdevOptions', 'boxed': true }
+ 'data': { 'options': ['BlockdevOptions'] } }
##
# @blockdev-del:
diff --git a/blockdev.c b/blockdev.c
index f657d090d3..1e8c946828 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3560,51 +3560,60 @@ fail:
visit_free(v);
}
-void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp)
-{
- BlockDriverState *bs;
- AioContext *ctx;
- QObject *obj;
- Visitor *v = qobject_output_visitor_new(&obj);
- BlockReopenQueue *queue;
- QDict *qdict;
-
- /* Check for the selected node name */
- if (!options->has_node_name) {
- error_setg(errp, "node-name not specified");
- goto fail;
- }
+void qmp_x_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
+{
+ BlockReopenQueue *queue = NULL;
+ GSList *drained = NULL;
+
+ /* Add each one of the BDS that we want to reopen to the queue */
+ for (; reopen_list != NULL; reopen_list = reopen_list->next) {
+ BlockdevOptions *options = reopen_list->value;
+ BlockDriverState *bs;
+ AioContext *ctx;
+ QObject *obj;
+ Visitor *v;
+ QDict *qdict;
+
+ /* Check for the selected node name */
+ if (!options->has_node_name) {
+ error_setg(errp, "node-name not specified");
+ goto fail;
+ }
- bs = bdrv_find_node(options->node_name);
- if (!bs) {
- error_setg(errp, "Failed to find node with node-name='%s'",
+ bs = bdrv_find_node(options->node_name);
+ if (!bs) {
+ error_setg(errp, "Failed to find node with node-name='%s'",
options->node_name);
- goto fail;
- }
+ goto fail;
+ }
- /* Put all options in a QDict and flatten it */
- visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
- visit_complete(v, &obj);
- qdict = qobject_to(QDict, obj);
+ /* Put all options in a QDict and flatten it */
+ v = qobject_output_visitor_new(&obj);
+ visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
+ visit_complete(v, &obj);
+ visit_free(v);
- qdict_flatten(qdict);
+ qdict = qobject_to(QDict, obj);
- /* Perform the reopen operation */
- ctx = bdrv_get_aio_context(bs);
- aio_context_acquire(ctx);
- bdrv_subtree_drained_begin(bs);
- aio_context_release(ctx);
+ qdict_flatten(qdict);
- queue = bdrv_reopen_queue(NULL, bs, qdict, false);
- bdrv_reopen_multiple(queue, errp);
+ ctx = bdrv_get_aio_context(bs);
+ aio_context_acquire(ctx);
- ctx = bdrv_get_aio_context(bs);
- aio_context_acquire(ctx);
- bdrv_subtree_drained_end(bs);
- aio_context_release(ctx);
+ bdrv_subtree_drained_begin(bs);
+ queue = bdrv_reopen_queue(queue, bs, qdict, false);
+ drained = g_slist_prepend(drained, bs);
+
+ aio_context_release(ctx);
+ }
+
+ /* Perform the reopen operation */
+ bdrv_reopen_multiple(queue, errp);
+ queue = NULL;
fail:
- visit_free(v);
+ bdrv_reopen_queue_free(queue);
+ g_slist_free_full(drained, (GDestroyNotify) bdrv_subtree_drained_end);
}
void qmp_blockdev_del(const char *node_name, Error **errp)
diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index bafef9dd9a..3400b0312a 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -261,9 +261,12 @@ class TestBlockdevMirrorReopen(MirrorBaseClass):
result = self.vm.qmp('blockdev-add', node_name="backing",
driver="null-co")
self.assert_qmp(result, 'return', {})
- result = self.vm.qmp('x-blockdev-reopen', node_name="target",
- driver=iotests.imgfmt, file="target-file",
- backing="backing")
+ result = self.vm.qmp('x-blockdev-reopen', options = [{
+ 'node-name': "target",
+ 'driver': iotests.imgfmt,
+ 'file': "target-file",
+ 'backing': "backing"
+ }])
self.assert_qmp(result, 'return', {})
class TestBlockdevMirrorReopenIothread(TestBlockdevMirrorReopen):
diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index abc4ffadd5..57aa88ecae 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -137,7 +137,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
assert sha256_1 == self.getSha256()
# Reopen to RW
- result = self.vm.qmp('x-blockdev-reopen', **{
+ result = self.vm.qmp('x-blockdev-reopen', options = [{
'node-name': 'node0',
'driver': iotests.imgfmt,
'file': {
@@ -145,7 +145,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
'filename': disk
},
'read-only': False
- })
+ }])
self.assert_qmp(result, 'return', {})
# Check that bitmap is reopened to RW and we can write to it.
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 0295129cbb..b81fde8f12 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -85,8 +85,18 @@ class TestBlockdevReopen(iotests.QMPTestCase):
"Expected output of %d qemu-io commands, found %d" %
(found, self.total_io_cmds))
- # Run x-blockdev-reopen with 'opts' but applying 'newopts'
- # on top of it. The original 'opts' dict is unmodified
+ # Run x-blockdev-reopen on a list of block devices
+ def reopenMultiple(self, opts, errmsg = None):
+ result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, options = opts)
+ if errmsg:
+ self.assert_qmp(result, 'error/class', 'GenericError')
+ self.assert_qmp(result, 'error/desc', errmsg)
+ else:
+ self.assert_qmp(result, 'return', {})
+
+ # Run x-blockdev-reopen on a single block device (specified by
+ # 'opts') but applying 'newopts' on top of it. The original 'opts'
+ # dict is unmodified
def reopen(self, opts, newopts = {}, errmsg = None):
opts = copy.deepcopy(opts)
@@ -101,12 +111,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
subdict = opts[prefix]
subdict[key] = value
- result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, **opts)
- if errmsg:
- self.assert_qmp(result, 'error/class', 'GenericError')
- self.assert_qmp(result, 'error/desc', errmsg)
- else:
- self.assert_qmp(result, 'return', {})
+ self.reopenMultiple([ opts ], errmsg)
# Run query-named-block-nodes and return the specified entry
@@ -142,10 +147,10 @@ class TestBlockdevReopen(iotests.QMPTestCase):
# We cannot change any of these
self.reopen(opts, {'node-name': 'not-found'}, "Failed to find node with node-name='not-found'")
self.reopen(opts, {'node-name': ''}, "Failed to find node with node-name=''")
- self.reopen(opts, {'node-name': None}, "Invalid parameter type for 'node-name', expected: string")
+ self.reopen(opts, {'node-name': None}, "Invalid parameter type for 'options[0].node-name', expected: string")
self.reopen(opts, {'driver': 'raw'}, "Cannot change the option 'driver'")
self.reopen(opts, {'driver': ''}, "Invalid parameter ''")
- self.reopen(opts, {'driver': None}, "Invalid parameter type for 'driver', expected: string")
+ self.reopen(opts, {'driver': None}, "Invalid parameter type for 'options[0].driver', expected: string")
self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor node-name='not-found'")
self.reopen(opts, {'file': ''}, "Cannot find device='' nor node-name=''")
self.reopen(opts, {'file': None}, "Invalid parameter type for 'file', expected: BlockdevRef")
@@ -154,7 +159,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
self.reopen(opts, {'file.filename': hd_path[1]}, "Cannot change the option 'filename'")
self.reopen(opts, {'file.aio': 'native'}, "Cannot change the option 'aio'")
self.reopen(opts, {'file.locking': 'off'}, "Cannot change the option 'locking'")
- self.reopen(opts, {'file.filename': None}, "Invalid parameter type for 'file.filename', expected: string")
+ self.reopen(opts, {'file.filename': None}, "Invalid parameter type for 'options[0].file.filename', expected: string")
# node-name is optional in BlockdevOptions, but x-blockdev-reopen needs it
del opts['node-name']
diff --git a/tests/qemu-iotests/248 b/tests/qemu-iotests/248
index 4daaed1530..03911333c4 100755
--- a/tests/qemu-iotests/248
+++ b/tests/qemu-iotests/248
@@ -63,7 +63,7 @@ vm.get_qmp_events()
del blockdev_opts['file']['size']
vm.qmp_log('x-blockdev-reopen', filters=[filter_qmp_testfiles],
- **blockdev_opts)
+ options = [ blockdev_opts ])
vm.qmp_log('block-job-resume', device='drive0')
vm.event_wait('JOB_STATUS_CHANGE', timeout=1.0,
diff --git a/tests/qemu-iotests/248.out b/tests/qemu-iotests/248.out
index 369b25bf26..893f625347 100644
--- a/tests/qemu-iotests/248.out
+++ b/tests/qemu-iotests/248.out
@@ -2,7 +2,7 @@
{"return": {}}
{"execute": "blockdev-mirror", "arguments": {"device": "drive0", "on-target-error": "enospc", "sync": "full", "target": "target"}}
{"return": {}}
-{"execute": "x-blockdev-reopen", "arguments": {"driver": "qcow2", "file": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-target"}}, "node-name": "target"}}
+{"execute": "x-blockdev-reopen", "arguments": {"options": [{"driver": "qcow2", "file": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-target"}}, "node-name": "target"}]}}
{"return": {}}
{"execute": "block-job-resume", "arguments": {"device": "drive0"}}
{"return": {}}
diff --git a/tests/qemu-iotests/296 b/tests/qemu-iotests/296
index 7c65e987a1..74b74511b6 100755
--- a/tests/qemu-iotests/296
+++ b/tests/qemu-iotests/296
@@ -120,8 +120,7 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
command = 'x-blockdev-reopen' if reOpen else 'blockdev-add'
- result = vm.qmp(command, **
- {
+ opts = {
'driver': iotests.imgfmt,
'node-name': id,
'read-only': readOnly,
@@ -131,7 +130,11 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
'filename': test_img,
}
}
- )
+
+ if reOpen:
+ result = vm.qmp(command, options = [ opts ])
+ else:
+ result = vm.qmp(command, **opts)
self.assert_qmp(result, 'return', {})
diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
index d535946b5f..4efdb35b91 100755
--- a/tests/qemu-iotests/298
+++ b/tests/qemu-iotests/298
@@ -98,7 +98,7 @@ class TestPreallocateFilter(TestPreallocateBase):
self.check_big()
def test_reopen_opts(self):
- result = self.vm.qmp('x-blockdev-reopen', **{
+ result = self.vm.qmp('x-blockdev-reopen', options = [{
'node-name': 'disk',
'driver': iotests.imgfmt,
'file': {
@@ -112,7 +112,7 @@ class TestPreallocateFilter(TestPreallocateBase):
'filename': disk
}
}
- })
+ }])
self.assert_qmp(result, 'return', {})
self.vm.hmp_qemu_io('drive0', 'write 0 1M')
diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing b/tests/qemu-iotests/tests/remove-bitmap-from-backing
index 0ea4c36507..0b07f7e836 100755
--- a/tests/qemu-iotests/tests/remove-bitmap-from-backing
+++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing
@@ -41,13 +41,15 @@ log('Trying to remove persistent bitmap from r-o base node, should fail:')
vm.qmp_log('block-dirty-bitmap-remove', node='base', name='bitmap0')
new_base_opts = {
- 'node-name': 'base',
- 'driver': 'qcow2',
- 'file': {
- 'driver': 'file',
- 'filename': base
- },
- 'read-only': False
+ 'options': [{
+ 'node-name': 'base',
+ 'driver': 'qcow2',
+ 'file': {
+ 'driver': 'file',
+ 'filename': base
+ },
+ 'read-only': False
+ }]
}
# Don't want to bother with filtering qmp_log for reopen command
@@ -58,7 +60,7 @@ if result != {'return': {}}:
log('Remove persistent bitmap from base node reopened to RW:')
vm.qmp_log('block-dirty-bitmap-remove', node='base', name='bitmap0')
-new_base_opts['read-only'] = True
+new_base_opts['options'][0]['read-only'] = True
result = vm.qmp('x-blockdev-reopen', **new_base_opts)
if result != {'return': {}}:
log('Failed to reopen: ' + str(result))
--
2.31.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v5 4/6] block: Support multiple reopening with x-blockdev-reopen
2021-07-06 11:23 ` [PATCH v5 4/6] block: Support multiple reopening with x-blockdev-reopen Kevin Wolf
@ 2021-07-06 15:45 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-07-06 15:45 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: mreitz, berto, qemu-devel
06.07.2021 14:23, Kevin Wolf wrote:
> From: Alberto Garcia <berto@igalia.com>
>
> [ kwolf: Fixed AioContext locking ]
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> qapi/block-core.json | 18 +++--
> blockdev.c | 81 ++++++++++---------
> tests/qemu-iotests/155 | 9 ++-
> tests/qemu-iotests/165 | 4 +-
> tests/qemu-iotests/245 | 27 ++++---
> tests/qemu-iotests/248 | 2 +-
> tests/qemu-iotests/248.out | 2 +-
> tests/qemu-iotests/296 | 9 ++-
[..]
>
> ##
> # @blockdev-del:
> diff --git a/blockdev.c b/blockdev.c
> index f657d090d3..1e8c946828 100644
> --- a/blockdev.c
> +++ b/blockdev.c
[..]
>
> - bs = bdrv_find_node(options->node_name);
> - if (!bs) {
> - error_setg(errp, "Failed to find node with node-name='%s'",
> + bs = bdrv_find_node(options->node_name);
> + if (!bs) {
> + error_setg(errp, "Failed to find node with node-name='%s'",
> options->node_name);
indentation broken
> - goto fail;
> - }
> + goto fail;
> + }
>
> - /* Put all options in a QDict and flatten it */
> - visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
> - visit_complete(v, &obj);
> - qdict = qobject_to(QDict, obj);
> + /* Put all options in a QDict and flatten it */
> + v = qobject_output_visitor_new(&obj);
> + visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
> + visit_complete(v, &obj);
> + visit_free(v);
[..]
> + # Run x-blockdev-reopen on a list of block devices
> + def reopenMultiple(self, opts, errmsg = None):
> + result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, options = opts)
I don't really care if this doesn't break iotest 297, but PEP8 doesn't like whitespaces around '=' for named arguments..
> + if errmsg:
> + self.assert_qmp(result, 'error/class', 'GenericError')
> + self.assert_qmp(result, 'error/desc', errmsg)
> + else:
> + self.assert_qmp(result, 'return', {})
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 5/6] iotests: Test reopening multiple devices at the same time
2021-07-06 11:23 [PATCH v5 0/6] Make blockdev-reopen stable Kevin Wolf
` (3 preceding siblings ...)
2021-07-06 11:23 ` [PATCH v5 4/6] block: Support multiple reopening with x-blockdev-reopen Kevin Wolf
@ 2021-07-06 11:23 ` Kevin Wolf
2021-07-06 15:50 ` Vladimir Sementsov-Ogievskiy
2021-07-06 11:23 ` [PATCH v5 6/6] block: Make blockdev-reopen stable API Kevin Wolf
5 siblings, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2021-07-06 11:23 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, berto, qemu-devel, mreitz
From: Alberto Garcia <berto@igalia.com>
This test swaps the images used by two active block devices.
This is now possible thanks to the new ability to run
x-blockdev-reopen on multiple devices at the same time.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/qemu-iotests/245 | 47 ++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/245.out | 4 ++--
2 files changed, 49 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index b81fde8f12..6eff352099 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -649,6 +649,53 @@ class TestBlockdevReopen(iotests.QMPTestCase):
'-c', 'read -P 0x40 0x40008 1',
'-c', 'read -P 0x80 0x40010 1', hd_path[0])
+ # Swap the disk images of two active block devices
+ def test_swap_files(self):
+ # Add hd0 and hd2 (none of them with backing files)
+ opts0 = hd_opts(0)
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts0)
+ self.assert_qmp(result, 'return', {})
+
+ opts2 = hd_opts(2)
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts2)
+ self.assert_qmp(result, 'return', {})
+
+ # Write different data to both block devices
+ self.run_qemu_io("hd0", "write -P 0xa0 0 1k")
+ self.run_qemu_io("hd2", "write -P 0xa2 0 1k")
+
+ # Check that the data reads correctly
+ self.run_qemu_io("hd0", "read -P 0xa0 0 1k")
+ self.run_qemu_io("hd2", "read -P 0xa2 0 1k")
+
+ # It's not possible to make a block device use an image that
+ # is already being used by the other device.
+ self.reopen(opts0, {'file': 'hd2-file'},
+ "Permission conflict on node 'hd2-file': permissions "
+ "'write, resize' are both required by node 'hd2' (uses "
+ "node 'hd2-file' as 'file' child) and unshared by node "
+ "'hd0' (uses node 'hd2-file' as 'file' child).")
+ self.reopen(opts2, {'file': 'hd0-file'},
+ "Permission conflict on node 'hd0-file': permissions "
+ "'write, resize' are both required by node 'hd0' (uses "
+ "node 'hd0-file' as 'file' child) and unshared by node "
+ "'hd2' (uses node 'hd0-file' as 'file' child).")
+
+ # But we can swap the images if we reopen both devices at the
+ # same time
+ opts0['file'] = 'hd2-file'
+ opts2['file'] = 'hd0-file'
+ self.reopenMultiple([opts0, opts2])
+ self.run_qemu_io("hd0", "read -P 0xa2 0 1k")
+ self.run_qemu_io("hd2", "read -P 0xa0 0 1k")
+
+ # And we can of course come back to the original state
+ opts0['file'] = 'hd0-file'
+ opts2['file'] = 'hd2-file'
+ self.reopenMultiple([opts0, opts2])
+ self.run_qemu_io("hd0", "read -P 0xa0 0 1k")
+ self.run_qemu_io("hd2", "read -P 0xa2 0 1k")
+
# Misc reopen tests with different block drivers
@iotests.skip_if_unsupported(['quorum', 'throttle'])
def test_misc_drivers(self):
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index daf1e51922..4eced19294 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -17,8 +17,8 @@ read 1/1 bytes at offset 262152
read 1/1 bytes at offset 262160
1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-..............
+...............
----------------------------------------------------------------------
-Ran 24 tests
+Ran 25 tests
OK
--
2.31.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v5 5/6] iotests: Test reopening multiple devices at the same time
2021-07-06 11:23 ` [PATCH v5 5/6] iotests: Test reopening multiple devices at the same time Kevin Wolf
@ 2021-07-06 15:50 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-07-06 15:50 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: mreitz, berto, qemu-devel
06.07.2021 14:23, Kevin Wolf wrote:
> From: Alberto Garcia<berto@igalia.com>
>
> This test swaps the images used by two active block devices.
>
> This is now possible thanks to the new ability to run
> x-blockdev-reopen on multiple devices at the same time.
>
> Signed-off-by: Alberto Garcia<berto@igalia.com>
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 6/6] block: Make blockdev-reopen stable API
2021-07-06 11:23 [PATCH v5 0/6] Make blockdev-reopen stable Kevin Wolf
` (4 preceding siblings ...)
2021-07-06 11:23 ` [PATCH v5 5/6] iotests: Test reopening multiple devices at the same time Kevin Wolf
@ 2021-07-06 11:23 ` Kevin Wolf
2021-07-06 15:51 ` Vladimir Sementsov-Ogievskiy
5 siblings, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2021-07-06 11:23 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, berto, qemu-devel, mreitz
From: Alberto Garcia <berto@igalia.com>
This patch drops the 'x-' prefix from x-blockdev-reopen.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-core.json | 6 +++---
blockdev.c | 2 +-
tests/qemu-iotests/155 | 2 +-
tests/qemu-iotests/165 | 2 +-
tests/qemu-iotests/245 | 10 +++++-----
tests/qemu-iotests/248 | 2 +-
tests/qemu-iotests/248.out | 2 +-
tests/qemu-iotests/296 | 2 +-
tests/qemu-iotests/298 | 2 +-
tests/qemu-iotests/tests/remove-bitmap-from-backing | 4 ++--
10 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 052520331e..2eb399f0d4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4219,7 +4219,7 @@
{ 'command': 'blockdev-add', 'data': 'BlockdevOptions', 'boxed': true }
##
-# @x-blockdev-reopen:
+# @blockdev-reopen:
#
# Reopens one or more block devices using the given set of options.
# Any option not specified will be reset to its default value regardless
@@ -4257,9 +4257,9 @@
# image does not have a default backing file name as part of its
# metadata.
#
-# Since: 4.0
+# Since: 6.0
##
-{ 'command': 'x-blockdev-reopen',
+{ 'command': 'blockdev-reopen',
'data': { 'options': ['BlockdevOptions'] } }
##
diff --git a/blockdev.c b/blockdev.c
index 1e8c946828..7639f2108e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3560,7 +3560,7 @@ fail:
visit_free(v);
}
-void qmp_x_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
+void qmp_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
{
BlockReopenQueue *queue = NULL;
GSList *drained = NULL;
diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index 3400b0312a..fec43d662d 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -261,7 +261,7 @@ class TestBlockdevMirrorReopen(MirrorBaseClass):
result = self.vm.qmp('blockdev-add', node_name="backing",
driver="null-co")
self.assert_qmp(result, 'return', {})
- result = self.vm.qmp('x-blockdev-reopen', options = [{
+ result = self.vm.qmp('blockdev-reopen', options = [{
'node-name': "target",
'driver': iotests.imgfmt,
'file': "target-file",
diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index 57aa88ecae..92a431315b 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -137,7 +137,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
assert sha256_1 == self.getSha256()
# Reopen to RW
- result = self.vm.qmp('x-blockdev-reopen', options = [{
+ result = self.vm.qmp('blockdev-reopen', options = [{
'node-name': 'node0',
'driver': iotests.imgfmt,
'file': {
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 6eff352099..28a116a6aa 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1,7 +1,7 @@
#!/usr/bin/env python3
# group: rw
#
-# Test cases for the QMP 'x-blockdev-reopen' command
+# Test cases for the QMP 'blockdev-reopen' command
#
# Copyright (C) 2018-2019 Igalia, S.L.
# Author: Alberto Garcia <berto@igalia.com>
@@ -85,16 +85,16 @@ class TestBlockdevReopen(iotests.QMPTestCase):
"Expected output of %d qemu-io commands, found %d" %
(found, self.total_io_cmds))
- # Run x-blockdev-reopen on a list of block devices
+ # Run blockdev-reopen on a list of block devices
def reopenMultiple(self, opts, errmsg = None):
- result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, options = opts)
+ result = self.vm.qmp('blockdev-reopen', conv_keys = False, options = opts)
if errmsg:
self.assert_qmp(result, 'error/class', 'GenericError')
self.assert_qmp(result, 'error/desc', errmsg)
else:
self.assert_qmp(result, 'return', {})
- # Run x-blockdev-reopen on a single block device (specified by
+ # Run blockdev-reopen on a single block device (specified by
# 'opts') but applying 'newopts' on top of it. The original 'opts'
# dict is unmodified
def reopen(self, opts, newopts = {}, errmsg = None):
@@ -161,7 +161,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
self.reopen(opts, {'file.locking': 'off'}, "Cannot change the option 'locking'")
self.reopen(opts, {'file.filename': None}, "Invalid parameter type for 'options[0].file.filename', expected: string")
- # node-name is optional in BlockdevOptions, but x-blockdev-reopen needs it
+ # node-name is optional in BlockdevOptions, but blockdev-reopen needs it
del opts['node-name']
self.reopen(opts, {}, "node-name not specified")
diff --git a/tests/qemu-iotests/248 b/tests/qemu-iotests/248
index 03911333c4..2ec2416e8a 100755
--- a/tests/qemu-iotests/248
+++ b/tests/qemu-iotests/248
@@ -62,7 +62,7 @@ vm.event_wait('JOB_STATUS_CHANGE', timeout=3.0,
vm.get_qmp_events()
del blockdev_opts['file']['size']
-vm.qmp_log('x-blockdev-reopen', filters=[filter_qmp_testfiles],
+vm.qmp_log('blockdev-reopen', filters=[filter_qmp_testfiles],
options = [ blockdev_opts ])
vm.qmp_log('block-job-resume', device='drive0')
diff --git a/tests/qemu-iotests/248.out b/tests/qemu-iotests/248.out
index 893f625347..66e94ccd7e 100644
--- a/tests/qemu-iotests/248.out
+++ b/tests/qemu-iotests/248.out
@@ -2,7 +2,7 @@
{"return": {}}
{"execute": "blockdev-mirror", "arguments": {"device": "drive0", "on-target-error": "enospc", "sync": "full", "target": "target"}}
{"return": {}}
-{"execute": "x-blockdev-reopen", "arguments": {"options": [{"driver": "qcow2", "file": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-target"}}, "node-name": "target"}]}}
+{"execute": "blockdev-reopen", "arguments": {"options": [{"driver": "qcow2", "file": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-target"}}, "node-name": "target"}]}}
{"return": {}}
{"execute": "block-job-resume", "arguments": {"device": "drive0"}}
{"return": {}}
diff --git a/tests/qemu-iotests/296 b/tests/qemu-iotests/296
index 74b74511b6..9206ddb954 100755
--- a/tests/qemu-iotests/296
+++ b/tests/qemu-iotests/296
@@ -118,7 +118,7 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
def openImageQmp(self, vm, id, file, secret,
readOnly = False, reOpen = False):
- command = 'x-blockdev-reopen' if reOpen else 'blockdev-add'
+ command = 'blockdev-reopen' if reOpen else 'blockdev-add'
opts = {
'driver': iotests.imgfmt,
diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
index 4efdb35b91..b4d8bd9b55 100755
--- a/tests/qemu-iotests/298
+++ b/tests/qemu-iotests/298
@@ -98,7 +98,7 @@ class TestPreallocateFilter(TestPreallocateBase):
self.check_big()
def test_reopen_opts(self):
- result = self.vm.qmp('x-blockdev-reopen', options = [{
+ result = self.vm.qmp('blockdev-reopen', options = [{
'node-name': 'disk',
'driver': iotests.imgfmt,
'file': {
diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing b/tests/qemu-iotests/tests/remove-bitmap-from-backing
index 0b07f7e836..8d48fc0f3c 100755
--- a/tests/qemu-iotests/tests/remove-bitmap-from-backing
+++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing
@@ -53,7 +53,7 @@ new_base_opts = {
}
# Don't want to bother with filtering qmp_log for reopen command
-result = vm.qmp('x-blockdev-reopen', **new_base_opts)
+result = vm.qmp('blockdev-reopen', **new_base_opts)
if result != {'return': {}}:
log('Failed to reopen: ' + str(result))
@@ -61,7 +61,7 @@ log('Remove persistent bitmap from base node reopened to RW:')
vm.qmp_log('block-dirty-bitmap-remove', node='base', name='bitmap0')
new_base_opts['options'][0]['read-only'] = True
-result = vm.qmp('x-blockdev-reopen', **new_base_opts)
+result = vm.qmp('blockdev-reopen', **new_base_opts)
if result != {'return': {}}:
log('Failed to reopen: ' + str(result))
--
2.31.1
^ permalink raw reply related [flat|nested] 14+ messages in thread