* [PATCH v4 01/19] blockdev: Allow external snapshots everywhere
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 02/19] blockdev: Allow resizing everywhere Max Reitz
` (18 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
There is no good reason why we would allow external snapshots only on
the first non-filter node in a chain. Parent BDSs should not care
whether their child is replaced by a snapshot. (If they do care, they
should announce that via freezing the chain, which is checked in
bdrv_append() through bdrv_set_backing_hd().)
Before we had bdrv_is_first_non_filter() here (since 212a5a8f095), there
was a special function bdrv_check_ext_snapshot() that allowed snapshots
by default, but block drivers could override this. Only blkverify did
so, however.
It is not clear to me why blkverify would do so; maybe just so that the
testee block driver would not be replaced. The introducing commit
f6186f49e2c does not explain why. Maybe because 08b24cfe376 would have
been the correct solution? (Which adds a .supports_backing check.)
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
blockdev.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 43d931fd79..1745d740ec 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1592,11 +1592,6 @@ static void external_snapshot_prepare(BlkActionState *common,
}
}
- if (!bdrv_is_first_non_filter(state->old_bs)) {
- error_setg(errp, QERR_FEATURE_DISABLED, "snapshot");
- goto out;
- }
-
if (action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC) {
BlockdevSnapshotSync *s = action->u.blockdev_snapshot_sync.data;
const char *format = s->has_format ? s->format : "qcow2";
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 02/19] blockdev: Allow resizing everywhere
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
2020-02-18 10:34 ` [PATCH v4 01/19] blockdev: Allow external snapshots everywhere Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 03/19] block: Drop bdrv_is_first_non_filter() Max Reitz
` (17 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Block nodes that do not allow resizing should not share BLK_PERM_RESIZE.
It does not matter whether they are the first non-filter in their chain
or not.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
blockdev.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 1745d740ec..011dcfec27 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3331,11 +3331,6 @@ void qmp_block_resize(bool has_device, const char *device,
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
- if (!bdrv_is_first_non_filter(bs)) {
- error_setg(errp, QERR_FEATURE_DISABLED, "resize");
- goto out;
- }
-
if (size < 0) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "size", "a >0 size");
goto out;
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 03/19] block: Drop bdrv_is_first_non_filter()
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
2020-02-18 10:34 ` [PATCH v4 01/19] blockdev: Allow external snapshots everywhere Max Reitz
2020-02-18 10:34 ` [PATCH v4 02/19] blockdev: Allow resizing everywhere Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 04/19] iotests: Let 041 use -blockdev for quorum children Max Reitz
` (16 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
It is unused now. (And it was ugly because it needed to explore all BDS
chains from the top.)
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block.c | 26 --------------------------
include/block/block.h | 1 -
2 files changed, 27 deletions(-)
diff --git a/block.c b/block.c
index 998bb41a5b..f6731fa9be 100644
--- a/block.c
+++ b/block.c
@@ -6235,32 +6235,6 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
return false;
}
-/* This function checks if the candidate is the first non filter bs down it's
- * bs chain. Since we don't have pointers to parents it explore all bs chains
- * from the top. Some filters can choose not to pass down the recursion.
- */
-bool bdrv_is_first_non_filter(BlockDriverState *candidate)
-{
- BlockDriverState *bs;
- BdrvNextIterator it;
-
- /* walk down the bs forest recursively */
- for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
- bool perm;
-
- /* try to recurse in this top level bs */
- perm = bdrv_recurse_is_first_non_filter(bs, candidate);
-
- /* candidate is the first non filter */
- if (perm) {
- bdrv_next_cleanup(&it);
- return true;
- }
- }
-
- return false;
-}
-
BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
const char *node_name, Error **errp)
{
diff --git a/include/block/block.h b/include/block/block.h
index 78026679ab..a6c0f6668e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -394,7 +394,6 @@ int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
/* external snapshots */
bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
BlockDriverState *candidate);
-bool bdrv_is_first_non_filter(BlockDriverState *candidate);
/* check if a named node can be replaced when doing drive-mirror */
BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 04/19] iotests: Let 041 use -blockdev for quorum children
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (2 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 03/19] block: Drop bdrv_is_first_non_filter() Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 05/19] quorum: Fix child permissions Max Reitz
` (15 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Using -drive with default options means that a virtio-blk drive will be
created that has write access to the to-be quorum children. Quorum
should have exclusive write access to them, so we should use -blockdev
instead.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/041 | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 43556b9727..aa7d54d968 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -885,7 +885,10 @@ class TestRepairQuorum(iotests.QMPTestCase):
# Assign a node name to each quorum image in order to manipulate
# them
opts = "node-name=img%i" % self.IMAGES.index(i)
- self.vm = self.vm.add_drive(i, opts)
+ opts += ',driver=%s' % iotests.imgfmt
+ opts += ',file.driver=file'
+ opts += ',file.filename=%s' % i
+ self.vm = self.vm.add_blockdev(opts)
self.vm.launch()
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 05/19] quorum: Fix child permissions
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (3 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 04/19] iotests: Let 041 use -blockdev for quorum children Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 06/19] block: Add bdrv_recurse_can_replace() Max Reitz
` (14 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Quorum cannot share WRITE or RESIZE on its children. Presumably, it
only does so because as a filter, it seemed intuitively correct to point
its .bdrv_child_perm to bdrv_filter_default_perm().
However, it is not really a filter, and bdrv_filter_default_perm() does
not work for it, so we have to provide a custom .bdrv_child_perm
implementation.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/quorum.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/block/quorum.c b/block/quorum.c
index df68adcfaa..17b439056f 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1114,6 +1114,23 @@ static char *quorum_dirname(BlockDriverState *bs, Error **errp)
return NULL;
}
+static void quorum_child_perm(BlockDriverState *bs, BdrvChild *c,
+ const BdrvChildRole *role,
+ BlockReopenQueue *reopen_queue,
+ uint64_t perm, uint64_t shared,
+ uint64_t *nperm, uint64_t *nshared)
+{
+ *nperm = perm & DEFAULT_PERM_PASSTHROUGH;
+
+ /*
+ * We cannot share RESIZE or WRITE, as this would make the
+ * children differ from each other.
+ */
+ *nshared = (shared & (BLK_PERM_CONSISTENT_READ |
+ BLK_PERM_WRITE_UNCHANGED))
+ | DEFAULT_PERM_UNCHANGED;
+}
+
static const char *const quorum_strong_runtime_opts[] = {
QUORUM_OPT_VOTE_THRESHOLD,
QUORUM_OPT_BLKVERIFY,
@@ -1143,7 +1160,7 @@ static BlockDriver bdrv_quorum = {
.bdrv_add_child = quorum_add_child,
.bdrv_del_child = quorum_del_child,
- .bdrv_child_perm = bdrv_filter_default_perms,
+ .bdrv_child_perm = quorum_child_perm,
.is_filter = true,
.bdrv_recurse_is_first_non_filter = quorum_recurse_is_first_non_filter,
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 06/19] block: Add bdrv_recurse_can_replace()
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (4 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 05/19] quorum: Fix child permissions Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 07/19] blkverify: Implement .bdrv_recurse_can_replace() Max Reitz
` (13 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
After a couple of follow-up patches, this function will replace
bdrv_recurse_is_first_non_filter() in check_to_replace_node().
bdrv_recurse_is_first_non_filter() is both not sufficiently specific for
check_to_replace_node() (it allows cases that should not be allowed,
like replacing child nodes of quorum with dissenting data that have more
parents than just quorum), and it is too restrictive (it is perfectly
fine to replace filters).
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block.c | 38 ++++++++++++++++++++++++++++++++++++++
include/block/block_int.h | 10 ++++++++++
2 files changed, 48 insertions(+)
diff --git a/block.c b/block.c
index f6731fa9be..553ccff216 100644
--- a/block.c
+++ b/block.c
@@ -6235,6 +6235,44 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
return false;
}
+/*
+ * This function checks whether the given @to_replace is allowed to be
+ * replaced by a node that always shows the same data as @bs. This is
+ * used for example to verify whether the mirror job can replace
+ * @to_replace by the target mirrored from @bs.
+ * To be replaceable, @bs and @to_replace may either be guaranteed to
+ * always show the same data (because they are only connected through
+ * filters), or some driver may allow replacing one of its children
+ * because it can guarantee that this child's data is not visible at
+ * all (for example, for dissenting quorum children that have no other
+ * parents).
+ */
+bool bdrv_recurse_can_replace(BlockDriverState *bs,
+ BlockDriverState *to_replace)
+{
+ if (!bs || !bs->drv) {
+ return false;
+ }
+
+ if (bs == to_replace) {
+ return true;
+ }
+
+ /* See what the driver can do */
+ if (bs->drv->bdrv_recurse_can_replace) {
+ return bs->drv->bdrv_recurse_can_replace(bs, to_replace);
+ }
+
+ /* For filters without an own implementation, we can recurse on our own */
+ if (bs->drv->is_filter) {
+ BdrvChild *child = bs->file ?: bs->backing;
+ return bdrv_recurse_can_replace(child->bs, to_replace);
+ }
+
+ /* Safe default */
+ return false;
+}
+
BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
const char *node_name, Error **errp)
{
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 640fb82c78..eaefac210e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -102,6 +102,13 @@ struct BlockDriver {
*/
bool (*bdrv_recurse_is_first_non_filter)(BlockDriverState *bs,
BlockDriverState *candidate);
+ /*
+ * Return true if @to_replace can be replaced by a BDS with the
+ * same data as @bs without it affecting @bs's behavior (that is,
+ * without it being visible to @bs's parents).
+ */
+ bool (*bdrv_recurse_can_replace)(BlockDriverState *bs,
+ BlockDriverState *to_replace);
int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
int (*bdrv_probe_device)(const char *filename);
@@ -1263,6 +1270,9 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
uint64_t perm, uint64_t shared,
uint64_t *nperm, uint64_t *nshared);
+bool bdrv_recurse_can_replace(BlockDriverState *bs,
+ BlockDriverState *to_replace);
+
/*
* Default implementation for drivers to pass bdrv_co_block_status() to
* their file.
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 07/19] blkverify: Implement .bdrv_recurse_can_replace()
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (5 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 06/19] block: Add bdrv_recurse_can_replace() Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 08/19] quorum: " Max Reitz
` (12 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/blkverify.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/block/blkverify.c b/block/blkverify.c
index 304b0a1368..0add3ab483 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -282,6 +282,20 @@ static bool blkverify_recurse_is_first_non_filter(BlockDriverState *bs,
return bdrv_recurse_is_first_non_filter(s->test_file->bs, candidate);
}
+static bool blkverify_recurse_can_replace(BlockDriverState *bs,
+ BlockDriverState *to_replace)
+{
+ BDRVBlkverifyState *s = bs->opaque;
+
+ /*
+ * blkverify quits the whole qemu process if there is a mismatch
+ * between bs->file->bs and s->test_file->bs. Therefore, we know
+ * know that both must match bs and we can recurse down to either.
+ */
+ return bdrv_recurse_can_replace(bs->file->bs, to_replace) ||
+ bdrv_recurse_can_replace(s->test_file->bs, to_replace);
+}
+
static void blkverify_refresh_filename(BlockDriverState *bs)
{
BDRVBlkverifyState *s = bs->opaque;
@@ -328,6 +342,7 @@ static BlockDriver bdrv_blkverify = {
.is_filter = true,
.bdrv_recurse_is_first_non_filter = blkverify_recurse_is_first_non_filter,
+ .bdrv_recurse_can_replace = blkverify_recurse_can_replace,
};
static void bdrv_blkverify_init(void)
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 08/19] quorum: Implement .bdrv_recurse_can_replace()
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (6 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 07/19] blkverify: Implement .bdrv_recurse_can_replace() Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 09/19] block: Use bdrv_recurse_can_replace() Max Reitz
` (11 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/quorum.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/block/quorum.c b/block/quorum.c
index 17b439056f..3ece6e4382 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -813,6 +813,59 @@ static bool quorum_recurse_is_first_non_filter(BlockDriverState *bs,
return false;
}
+static bool quorum_recurse_can_replace(BlockDriverState *bs,
+ BlockDriverState *to_replace)
+{
+ BDRVQuorumState *s = bs->opaque;
+ int i;
+
+ for (i = 0; i < s->num_children; i++) {
+ /*
+ * We have no idea whether our children show the same data as
+ * this node (@bs). It is actually highly likely that
+ * @to_replace does not, because replacing a broken child is
+ * one of the main use cases here.
+ *
+ * We do know that the new BDS will match @bs, so replacing
+ * any of our children by it will be safe. It cannot change
+ * the data this quorum node presents to its parents.
+ *
+ * However, replacing @to_replace by @bs in any of our
+ * children's chains may change visible data somewhere in
+ * there. We therefore cannot recurse down those chains with
+ * bdrv_recurse_can_replace().
+ * (More formally, bdrv_recurse_can_replace() requires that
+ * @to_replace will be replaced by something matching the @bs
+ * passed to it. We cannot guarantee that.)
+ *
+ * Thus, we can only check whether any of our immediate
+ * children matches @to_replace.
+ *
+ * (In the future, we might add a function to recurse down a
+ * chain that checks that nothing there cares about a change
+ * in data from the respective child in question. For
+ * example, most filters do not care when their child's data
+ * suddenly changes, as long as their parents do not care.)
+ */
+ if (s->children[i]->bs == to_replace) {
+ /*
+ * We now have to ensure that there is no other parent
+ * that cares about replacing this child by a node with
+ * potentially different data.
+ * We do so by checking whether there are any other parents
+ * at all, which is stricter than necessary, but also very
+ * simple. (We may decide to implement something more
+ * complex and permissive when there is an actual need for
+ * it.)
+ */
+ return QLIST_FIRST(&to_replace->parents) == s->children[i] &&
+ QLIST_NEXT(s->children[i], next_parent) == NULL;
+ }
+ }
+
+ return false;
+}
+
static int quorum_valid_threshold(int threshold, int num_children, Error **errp)
{
@@ -1164,6 +1217,7 @@ static BlockDriver bdrv_quorum = {
.is_filter = true,
.bdrv_recurse_is_first_non_filter = quorum_recurse_is_first_non_filter,
+ .bdrv_recurse_can_replace = quorum_recurse_can_replace,
.strong_runtime_opts = quorum_strong_runtime_opts,
};
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 09/19] block: Use bdrv_recurse_can_replace()
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (7 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 08/19] quorum: " Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 10/19] block: Remove bdrv_recurse_is_first_non_filter() Max Reitz
` (10 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Let check_to_replace_node() use the more specialized
bdrv_recurse_can_replace() instead of
bdrv_recurse_is_first_non_filter(), which is too restrictive (or, in the
case of quorum, sometimes not restrictive enough).
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 553ccff216..7efe8f3b03 100644
--- a/block.c
+++ b/block.c
@@ -6273,6 +6273,17 @@ bool bdrv_recurse_can_replace(BlockDriverState *bs,
return false;
}
+/*
+ * Check whether the given @node_name can be replaced by a node that
+ * has the same data as @parent_bs. If so, return @node_name's BDS;
+ * NULL otherwise.
+ *
+ * @node_name must be a (recursive) *child of @parent_bs (or this
+ * function will return NULL).
+ *
+ * The result (whether the node can be replaced or not) is only valid
+ * for as long as no graph or permission changes occur.
+ */
BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
const char *node_name, Error **errp)
{
@@ -6297,8 +6308,11 @@ BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
* Another benefit is that this tests exclude backing files which are
* blocked by the backing blockers.
*/
- if (!bdrv_recurse_is_first_non_filter(parent_bs, to_replace_bs)) {
- error_setg(errp, "Only top most non filter can be replaced");
+ if (!bdrv_recurse_can_replace(parent_bs, to_replace_bs)) {
+ error_setg(errp, "Cannot replace '%s' by a node mirrored from '%s', "
+ "because it cannot be guaranteed that doing so would not "
+ "lead to an abrupt change of visible data",
+ node_name, parent_bs->node_name);
to_replace_bs = NULL;
goto out;
}
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 10/19] block: Remove bdrv_recurse_is_first_non_filter()
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (8 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 09/19] block: Use bdrv_recurse_can_replace() Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 11/19] mirror: Double-check immediately before replacing Max Reitz
` (9 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
It no longer has any users.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block.c | 33 ---------------------------------
block/blkverify.c | 15 ---------------
block/copy-on-read.c | 9 ---------
block/filter-compress.c | 9 ---------
block/quorum.c | 18 ------------------
block/replication.c | 7 -------
block/throttle.c | 8 --------
include/block/block.h | 4 ----
include/block/block_int.h | 8 --------
9 files changed, 111 deletions(-)
diff --git a/block.c b/block.c
index 7efe8f3b03..ca2cfbcb62 100644
--- a/block.c
+++ b/block.c
@@ -6202,39 +6202,6 @@ int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts,
return bs->drv->bdrv_amend_options(bs, opts, status_cb, cb_opaque, errp);
}
-/* This function will be called by the bdrv_recurse_is_first_non_filter method
- * of block filter and by bdrv_is_first_non_filter.
- * It is used to test if the given bs is the candidate or recurse more in the
- * node graph.
- */
-bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
- BlockDriverState *candidate)
-{
- /* return false if basic checks fails */
- if (!bs || !bs->drv) {
- return false;
- }
-
- /* the code reached a non block filter driver -> check if the bs is
- * the same as the candidate. It's the recursion termination condition.
- */
- if (!bs->drv->is_filter) {
- return bs == candidate;
- }
- /* Down this path the driver is a block filter driver */
-
- /* If the block filter recursion method is defined use it to recurse down
- * the node graph.
- */
- if (bs->drv->bdrv_recurse_is_first_non_filter) {
- return bs->drv->bdrv_recurse_is_first_non_filter(bs, candidate);
- }
-
- /* the driver is a block filter but don't allow to recurse -> return false
- */
- return false;
-}
-
/*
* This function checks whether the given @to_replace is allowed to be
* replaced by a node that always shows the same data as @bs. This is
diff --git a/block/blkverify.c b/block/blkverify.c
index 0add3ab483..ba6b1853ae 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -268,20 +268,6 @@ static int blkverify_co_flush(BlockDriverState *bs)
return bdrv_co_flush(s->test_file->bs);
}
-static bool blkverify_recurse_is_first_non_filter(BlockDriverState *bs,
- BlockDriverState *candidate)
-{
- BDRVBlkverifyState *s = bs->opaque;
-
- bool perm = bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
-
- if (perm) {
- return true;
- }
-
- return bdrv_recurse_is_first_non_filter(s->test_file->bs, candidate);
-}
-
static bool blkverify_recurse_can_replace(BlockDriverState *bs,
BlockDriverState *to_replace)
{
@@ -341,7 +327,6 @@ static BlockDriver bdrv_blkverify = {
.bdrv_co_flush = blkverify_co_flush,
.is_filter = true,
- .bdrv_recurse_is_first_non_filter = blkverify_recurse_is_first_non_filter,
.bdrv_recurse_can_replace = blkverify_recurse_can_replace,
};
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index e95223d3cb..242d3ff055 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -118,13 +118,6 @@ static void cor_lock_medium(BlockDriverState *bs, bool locked)
}
-static bool cor_recurse_is_first_non_filter(BlockDriverState *bs,
- BlockDriverState *candidate)
-{
- return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
-}
-
-
static BlockDriver bdrv_copy_on_read = {
.format_name = "copy-on-read",
@@ -143,8 +136,6 @@ static BlockDriver bdrv_copy_on_read = {
.bdrv_co_block_status = bdrv_co_block_status_from_file,
- .bdrv_recurse_is_first_non_filter = cor_recurse_is_first_non_filter,
-
.has_variable_length = true,
.is_filter = true,
};
diff --git a/block/filter-compress.c b/block/filter-compress.c
index 60137fb680..82c315b298 100644
--- a/block/filter-compress.c
+++ b/block/filter-compress.c
@@ -128,13 +128,6 @@ static void compress_lock_medium(BlockDriverState *bs, bool locked)
}
-static bool compress_recurse_is_first_non_filter(BlockDriverState *bs,
- BlockDriverState *candidate)
-{
- return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
-}
-
-
static BlockDriver bdrv_compress = {
.format_name = "compress",
@@ -154,8 +147,6 @@ static BlockDriver bdrv_compress = {
.bdrv_co_block_status = bdrv_co_block_status_from_file,
- .bdrv_recurse_is_first_non_filter = compress_recurse_is_first_non_filter,
-
.has_variable_length = true,
.is_filter = true,
};
diff --git a/block/quorum.c b/block/quorum.c
index 3ece6e4382..f57b0402d9 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -796,23 +796,6 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs)
return result;
}
-static bool quorum_recurse_is_first_non_filter(BlockDriverState *bs,
- BlockDriverState *candidate)
-{
- BDRVQuorumState *s = bs->opaque;
- int i;
-
- for (i = 0; i < s->num_children; i++) {
- bool perm = bdrv_recurse_is_first_non_filter(s->children[i]->bs,
- candidate);
- if (perm) {
- return true;
- }
- }
-
- return false;
-}
-
static bool quorum_recurse_can_replace(BlockDriverState *bs,
BlockDriverState *to_replace)
{
@@ -1216,7 +1199,6 @@ static BlockDriver bdrv_quorum = {
.bdrv_child_perm = quorum_child_perm,
.is_filter = true,
- .bdrv_recurse_is_first_non_filter = quorum_recurse_is_first_non_filter,
.bdrv_recurse_can_replace = quorum_recurse_can_replace,
.strong_runtime_opts = quorum_strong_runtime_opts,
diff --git a/block/replication.c b/block/replication.c
index 99532ce521..d6681b6c84 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -306,12 +306,6 @@ out:
return ret;
}
-static bool replication_recurse_is_first_non_filter(BlockDriverState *bs,
- BlockDriverState *candidate)
-{
- return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
-}
-
static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
{
Error *local_err = NULL;
@@ -699,7 +693,6 @@ static BlockDriver bdrv_replication = {
.bdrv_co_writev = replication_co_writev,
.is_filter = true,
- .bdrv_recurse_is_first_non_filter = replication_recurse_is_first_non_filter,
.has_variable_length = true,
.strong_runtime_opts = replication_strong_runtime_opts,
diff --git a/block/throttle.c b/block/throttle.c
index 0349f42257..71f4bb0ad1 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -207,12 +207,6 @@ static void throttle_reopen_abort(BDRVReopenState *reopen_state)
reopen_state->opaque = NULL;
}
-static bool throttle_recurse_is_first_non_filter(BlockDriverState *bs,
- BlockDriverState *candidate)
-{
- return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
-}
-
static void coroutine_fn throttle_co_drain_begin(BlockDriverState *bs)
{
ThrottleGroupMember *tgm = bs->opaque;
@@ -252,8 +246,6 @@ static BlockDriver bdrv_throttle = {
.bdrv_co_pwrite_zeroes = throttle_co_pwrite_zeroes,
.bdrv_co_pdiscard = throttle_co_pdiscard,
- .bdrv_recurse_is_first_non_filter = throttle_recurse_is_first_non_filter,
-
.bdrv_attach_aio_context = throttle_attach_aio_context,
.bdrv_detach_aio_context = throttle_detach_aio_context,
diff --git a/include/block/block.h b/include/block/block.h
index a6c0f6668e..cd6b5b95aa 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -391,10 +391,6 @@ int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
Error **errp);
-/* external snapshots */
-bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
- BlockDriverState *candidate);
-
/* check if a named node can be replaced when doing drive-mirror */
BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
const char *node_name, Error **errp);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index eaefac210e..6f9fd5e20e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -94,14 +94,6 @@ struct BlockDriver {
* must implement them and return -ENOTSUP.
*/
bool is_filter;
- /* for snapshots block filter like Quorum can implement the
- * following recursive callback.
- * It's purpose is to recurse on the filter children while calling
- * bdrv_recurse_is_first_non_filter on them.
- * For a sample implementation look in the future Quorum block filter.
- */
- bool (*bdrv_recurse_is_first_non_filter)(BlockDriverState *bs,
- BlockDriverState *candidate);
/*
* Return true if @to_replace can be replaced by a BDS with the
* same data as @bs without it affecting @bs's behavior (that is,
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 11/19] mirror: Double-check immediately before replacing
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (9 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 10/19] block: Remove bdrv_recurse_is_first_non_filter() Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 12/19] quorum: Stop marking it as a filter Max Reitz
` (8 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
There is no guarantee that we can still replace the node we want to
replace at the end of the mirror job. Double-check by calling
bdrv_recurse_can_replace().
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/mirror.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/block/mirror.c b/block/mirror.c
index cacbc70014..447051dbc6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -700,7 +700,19 @@ static int mirror_exit_common(Job *job)
* drain potential other users of the BDS before changing the graph. */
assert(s->in_drain);
bdrv_drained_begin(target_bs);
- bdrv_replace_node(to_replace, target_bs, &local_err);
+ /*
+ * Cannot use check_to_replace_node() here, because that would
+ * check for an op blocker on @to_replace, and we have our own
+ * there.
+ */
+ if (bdrv_recurse_can_replace(src, to_replace)) {
+ bdrv_replace_node(to_replace, target_bs, &local_err);
+ } else {
+ error_setg(&local_err, "Can no longer replace '%s' by '%s', "
+ "because it can no longer be guaranteed that doing so "
+ "would not lead to an abrupt change of visible data",
+ to_replace->node_name, target_bs->node_name);
+ }
bdrv_drained_end(target_bs);
if (local_err) {
error_report_err(local_err);
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 12/19] quorum: Stop marking it as a filter
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (10 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 11/19] mirror: Double-check immediately before replacing Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 13/19] iotests: Use complete_and_wait() in 155 Max Reitz
` (7 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Quorum is not a filter, for example because it cannot guarantee which of
its children will serve the next request. Thus, any of its children may
differ from the data visible to quorum's parents.
We have other filters with multiple children, but they differ in this
aspect:
- blkverify quits the whole qemu process if its children differ. As
such, we can always skip it when we want to skip it (as a filter node)
by going to any of its children. Both have the same data.
- replication generally serves requests from bs->file, so this is its
only actually filtered child.
- Block job filters currently only have one child, but they will
probably get more children in the future. Still, they will always
have only one actually filtered child.
Having "filters" as a dedicated node category only makes sense if you
can skip them by going to a one fixed child that always shows the same
data as the filter node. Quorum cannot fulfill this, so it is not a
filter.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/quorum.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/block/quorum.c b/block/quorum.c
index f57b0402d9..6d7a56bd93 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1198,7 +1198,6 @@ static BlockDriver bdrv_quorum = {
.bdrv_child_perm = quorum_child_perm,
- .is_filter = true,
.bdrv_recurse_can_replace = quorum_recurse_can_replace,
.strong_runtime_opts = quorum_strong_runtime_opts,
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 13/19] iotests: Use complete_and_wait() in 155
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (11 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 12/19] quorum: Stop marking it as a filter Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 14/19] iotests: Add VM.assert_block_path() Max Reitz
` (6 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
This way, we get to see errors during the completion phase.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/155 | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index e35b1d534b..f237868710 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -163,12 +163,7 @@ class MirrorBaseClass(BaseClass):
self.assert_qmp(result, 'return', {})
- self.vm.event_wait('BLOCK_JOB_READY')
-
- result = self.vm.qmp('block-job-complete', device='mirror-job')
- self.assert_qmp(result, 'return', {})
-
- self.vm.event_wait('BLOCK_JOB_COMPLETED')
+ self.complete_and_wait('mirror-job')
def testFull(self):
self.runMirror('full')
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 14/19] iotests: Add VM.assert_block_path()
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (12 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 13/19] iotests: Use complete_and_wait() in 155 Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 15/19] iotests/041: Drop superfluous shutdowns Max Reitz
` (5 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/iotests.py | 59 +++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 0473e824ed..8815052eb5 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -714,6 +714,65 @@ class VM(qtest.QEMUQtestMachine):
return fields.items() <= ret.items()
+ def assert_block_path(self, root, path, expected_node, graph=None):
+ """
+ Check whether the node under the given path in the block graph
+ is @expected_node.
+
+ @root is the node name of the node where the @path is rooted.
+
+ @path is a string that consists of child names separated by
+ slashes. It must begin with a slash.
+
+ Examples for @root + @path:
+ - root="qcow2-node", path="/backing/file"
+ - root="quorum-node", path="/children.2/file"
+
+ Hypothetically, @path could be empty, in which case it would
+ point to @root. However, in practice this case is not useful
+ and hence not allowed.
+
+ @expected_node may be None. (All elements of the path but the
+ leaf must still exist.)
+
+ @graph may be None or the result of an x-debug-query-block-graph
+ call that has already been performed.
+ """
+ if graph is None:
+ graph = self.qmp('x-debug-query-block-graph')['return']
+
+ iter_path = iter(path.split('/'))
+
+ # Must start with a /
+ assert next(iter_path) == ''
+
+ node = next((node for node in graph['nodes'] if node['name'] == root),
+ None)
+
+ # An empty @path is not allowed, so the root node must be present
+ assert node is not None, 'Root node %s not found' % root
+
+ for child_name in iter_path:
+ assert node is not None, 'Cannot follow path %s%s' % (root, path)
+
+ try:
+ node_id = next(edge['child'] for edge in graph['edges'] \
+ if edge['parent'] == node['id'] and
+ edge['name'] == child_name)
+
+ node = next(node for node in graph['nodes'] \
+ if node['id'] == node_id)
+ except StopIteration:
+ node = None
+
+ if node is None:
+ assert expected_node is None, \
+ 'No node found under %s (but expected %s)' % \
+ (path, expected_node)
+ else:
+ assert node['name'] == expected_node, \
+ 'Found node %s under %s (but expected %s)' % \
+ (node['name'], path, expected_node)
index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 15/19] iotests/041: Drop superfluous shutdowns
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (13 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 14/19] iotests: Add VM.assert_block_path() Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 16/19] iotests: Resolve TODOs in 041 Max Reitz
` (4 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
All tearDowns in 041 shutdown the VM. Thus, test cases do not need to
do it themselves (unless they need the VM to be down for some
post-operation check).
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/041 | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index aa7d54d968..7b2cf5c2f8 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -80,7 +80,6 @@ class TestSingleDrive(iotests.QMPTestCase):
self.cancel_and_wait(force=True)
result = self.vm.qmp('query-block')
self.assert_qmp(result, 'return[0]/inserted/file', test_img)
- self.vm.shutdown()
def test_cancel_after_ready(self):
self.assert_no_active_block_jobs()
@@ -201,8 +200,6 @@ class TestSingleDrive(iotests.QMPTestCase):
self.assert_qmp(result, 'return[0]/node-name', 'top')
self.assert_qmp(result, 'return[0]/backing/node-name', 'base')
- self.vm.shutdown()
-
def test_medium_not_found(self):
if iotests.qemu_default_machine != 'pc':
return
@@ -455,7 +452,6 @@ new_state = "1"
self.assert_qmp(event, 'data/id', 'drive0')
self.assert_no_active_block_jobs()
- self.vm.shutdown()
def test_ignore_read(self):
self.assert_no_active_block_jobs()
@@ -475,7 +471,6 @@ new_state = "1"
result = self.vm.qmp('query-block-jobs')
self.assert_qmp(result, 'return[0]/paused', False)
self.complete_and_wait()
- self.vm.shutdown()
def test_large_cluster(self):
self.assert_no_active_block_jobs()
@@ -540,7 +535,6 @@ new_state = "1"
self.complete_and_wait(wait_ready=False)
self.assert_no_active_block_jobs()
- self.vm.shutdown()
class TestWriteErrors(iotests.QMPTestCase):
image_len = 2 * 1024 * 1024 # MB
@@ -614,7 +608,6 @@ new_state = "1"
completed = True
self.assert_no_active_block_jobs()
- self.vm.shutdown()
def test_ignore_write(self):
self.assert_no_active_block_jobs()
@@ -631,7 +624,6 @@ new_state = "1"
result = self.vm.qmp('query-block-jobs')
self.assert_qmp(result, 'return[0]/paused', False)
self.complete_and_wait()
- self.vm.shutdown()
def test_stop_write(self):
self.assert_no_active_block_jobs()
@@ -667,7 +659,6 @@ new_state = "1"
self.complete_and_wait(wait_ready=False)
self.assert_no_active_block_jobs()
- self.vm.shutdown()
class TestSetSpeed(iotests.QMPTestCase):
image_len = 80 * 1024 * 1024 # MB
@@ -936,7 +927,6 @@ class TestRepairQuorum(iotests.QMPTestCase):
# here we check that the last registered quorum file has not been
# swapped out and unref
self.assert_has_block_node(None, quorum_img3)
- self.vm.shutdown()
def test_cancel_after_ready(self):
self.assert_no_active_block_jobs()
@@ -1043,7 +1033,6 @@ class TestRepairQuorum(iotests.QMPTestCase):
self.assert_has_block_node("repair0", quorum_repair_img)
# TODO: a better test requiring some QEMU infrastructure will be added
# to check that this file is really driven by quorum
- self.vm.shutdown()
# Test mirroring with a source that does not have any parents (not even a
# BlockBackend)
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 16/19] iotests: Resolve TODOs in 041
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (14 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 15/19] iotests/041: Drop superfluous shutdowns Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 17/19] iotests: Use self.image_len in TestRepairQuorum Max Reitz
` (3 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/041 | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 7b2cf5c2f8..084da6baf3 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -909,8 +909,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
self.complete_and_wait(drive="job0")
self.assert_has_block_node("repair0", quorum_repair_img)
- # TODO: a better test requiring some QEMU infrastructure will be added
- # to check that this file is really driven by quorum
+ self.vm.assert_block_path('quorum0', '/children.1', 'repair0')
self.vm.shutdown()
self.assertTrue(iotests.compare_images(quorum_img2, quorum_repair_img),
'target image does not match source after mirroring')
@@ -1031,8 +1030,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
self.complete_and_wait('job0')
self.assert_has_block_node("repair0", quorum_repair_img)
- # TODO: a better test requiring some QEMU infrastructure will be added
- # to check that this file is really driven by quorum
+ self.vm.assert_block_path('quorum0', '/children.1', 'repair0')
# Test mirroring with a source that does not have any parents (not even a
# BlockBackend)
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 17/19] iotests: Use self.image_len in TestRepairQuorum
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (15 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 16/19] iotests: Resolve TODOs in 041 Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 10:34 ` [PATCH v4 18/19] iotests: Add tests for invalid Quorum @replaces Max Reitz
` (2 subsequent siblings)
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
041's TestRepairQuorum has its own image_len, no need to refer to
TestSingleDrive. (This patch allows commenting out TestSingleDrive to
speed up 041 during test testing.)
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/041 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 084da6baf3..1d9e64ff6d 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -872,7 +872,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
# Add each individual quorum images
for i in self.IMAGES:
qemu_img('create', '-f', iotests.imgfmt, i,
- str(TestSingleDrive.image_len))
+ str(self.image_len))
# Assign a node name to each quorum image in order to manipulate
# them
opts = "node-name=img%i" % self.IMAGES.index(i)
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 18/19] iotests: Add tests for invalid Quorum @replaces
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (16 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 17/19] iotests: Use self.image_len in TestRepairQuorum Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 12:38 ` Kevin Wolf
2020-02-18 10:34 ` [PATCH v4 19/19] iotests: Check that @replaces can replace filters Max Reitz
2020-02-18 13:53 ` [PATCH v4 00/19] block: Fix check_to_replace_node() Kevin Wolf
19 siblings, 1 reply; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Add two tests to see that you cannot replace a Quorum child with the
mirror job while the child is in use by a different parent.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/041 | 70 +++++++++++++++++++++++++++++++++++++-
tests/qemu-iotests/041.out | 4 +--
2 files changed, 71 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 1d9e64ff6d..53c8671969 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -20,6 +20,7 @@
import time
import os
+import re
import iotests
from iotests import qemu_img, qemu_io
@@ -34,6 +35,8 @@ quorum_img3 = os.path.join(iotests.test_dir, 'quorum3.img')
quorum_repair_img = os.path.join(iotests.test_dir, 'quorum_repair.img')
quorum_snapshot_file = os.path.join(iotests.test_dir, 'quorum_snapshot.img')
+nbd_sock_path = os.path.join(iotests.test_dir, 'nbd.sock')
+
class TestSingleDrive(iotests.QMPTestCase):
image_len = 1 * 1024 * 1024 # MB
qmp_cmd = 'drive-mirror'
@@ -892,7 +895,8 @@ class TestRepairQuorum(iotests.QMPTestCase):
def tearDown(self):
self.vm.shutdown()
- for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file ]:
+ for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file,
+ nbd_sock_path ]:
# Do a try/except because the test may have deleted some images
try:
os.remove(i)
@@ -1032,6 +1036,70 @@ class TestRepairQuorum(iotests.QMPTestCase):
self.assert_has_block_node("repair0", quorum_repair_img)
self.vm.assert_block_path('quorum0', '/children.1', 'repair0')
+ def test_with_other_parent(self):
+ """
+ Check that we cannot replace a Quorum child when it has other
+ parents.
+ """
+ result = self.vm.qmp('nbd-server-start',
+ addr={
+ 'type': 'unix',
+ 'data': {'path': nbd_sock_path}
+ })
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm.qmp('nbd-server-add', device='img1')
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm.qmp('drive-mirror', job_id='mirror', device='quorum0',
+ sync='full', node_name='repair0', replaces='img1',
+ target=quorum_repair_img, format=iotests.imgfmt)
+ self.assert_qmp(result, 'error/desc',
+ "Cannot replace 'img1' by a node mirrored from "
+ "'quorum0', because it cannot be guaranteed that doing "
+ "so would not lead to an abrupt change of visible data")
+
+ def test_with_other_parents_after_mirror_start(self):
+ """
+ The same as test_with_other_parent(), but add the NBD server
+ only when the mirror job is already running.
+ """
+ result = self.vm.qmp('nbd-server-start',
+ addr={
+ 'type': 'unix',
+ 'data': {'path': nbd_sock_path}
+ })
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm.qmp('drive-mirror', job_id='mirror', device='quorum0',
+ sync='full', node_name='repair0', replaces='img1',
+ target=quorum_repair_img, format=iotests.imgfmt)
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm.qmp('nbd-server-add', device='img1')
+ self.assert_qmp(result, 'return', {})
+
+ # The full error message goes to stderr, we will check it later
+ self.complete_and_wait('mirror',
+ completion_error='Operation not permitted')
+
+ # Should not have been replaced
+ self.vm.assert_block_path('quorum0', '/children.1', 'img1')
+
+ # Check the full error message now
+ self.vm.shutdown()
+ log = self.vm.get_log()
+ log = re.sub(r'^\[I \d+\.\d+\] OPENED\n', '', log)
+ log = re.sub(r'^Formatting.*\n', '', log)
+ log = re.sub(r'\n\[I \+\d+\.\d+\] CLOSED\n?$', '', log)
+ log = re.sub(r'^qemu-system-[^:]*: ', '', log)
+
+ self.assertEqual(log,
+ "Can no longer replace 'img1' by 'repair0', because " +
+ "it can no longer be guaranteed that doing so would " +
+ "not lead to an abrupt change of visible data")
+
+
# Test mirroring with a source that does not have any parents (not even a
# BlockBackend)
class TestOrphanedSource(iotests.QMPTestCase):
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index f496be9197..ffc779b4d1 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-...........................................................................................
+.............................................................................................
----------------------------------------------------------------------
-Ran 91 tests
+Ran 93 tests
OK
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v4 18/19] iotests: Add tests for invalid Quorum @replaces
2020-02-18 10:34 ` [PATCH v4 18/19] iotests: Add tests for invalid Quorum @replaces Max Reitz
@ 2020-02-18 12:38 ` Kevin Wolf
2020-02-18 12:49 ` Max Reitz
0 siblings, 1 reply; 24+ messages in thread
From: Kevin Wolf @ 2020-02-18 12:38 UTC (permalink / raw)
To: Max Reitz; +Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Am 18.02.2020 um 11:34 hat Max Reitz geschrieben:
> Add two tests to see that you cannot replace a Quorum child with the
> mirror job while the child is in use by a different parent.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> tests/qemu-iotests/041 | 70 +++++++++++++++++++++++++++++++++++++-
> tests/qemu-iotests/041.out | 4 +--
> 2 files changed, 71 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> index 1d9e64ff6d..53c8671969 100755
> --- a/tests/qemu-iotests/041
> +++ b/tests/qemu-iotests/041
> @@ -20,6 +20,7 @@
>
> import time
> import os
> +import re
> import iotests
> from iotests import qemu_img, qemu_io
>
> @@ -34,6 +35,8 @@ quorum_img3 = os.path.join(iotests.test_dir, 'quorum3.img')
> quorum_repair_img = os.path.join(iotests.test_dir, 'quorum_repair.img')
> quorum_snapshot_file = os.path.join(iotests.test_dir, 'quorum_snapshot.img')
>
> +nbd_sock_path = os.path.join(iotests.test_dir, 'nbd.sock')
> +
> class TestSingleDrive(iotests.QMPTestCase):
> image_len = 1 * 1024 * 1024 # MB
> qmp_cmd = 'drive-mirror'
> @@ -892,7 +895,8 @@ class TestRepairQuorum(iotests.QMPTestCase):
>
> def tearDown(self):
> self.vm.shutdown()
> - for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file ]:
> + for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file,
> + nbd_sock_path ]:
> # Do a try/except because the test may have deleted some images
> try:
> os.remove(i)
> @@ -1032,6 +1036,70 @@ class TestRepairQuorum(iotests.QMPTestCase):
> self.assert_has_block_node("repair0", quorum_repair_img)
> self.vm.assert_block_path('quorum0', '/children.1', 'repair0')
>
> + def test_with_other_parent(self):
> + """
> + Check that we cannot replace a Quorum child when it has other
> + parents.
> + """
> + result = self.vm.qmp('nbd-server-start',
> + addr={
> + 'type': 'unix',
> + 'data': {'path': nbd_sock_path}
> + })
> + self.assert_qmp(result, 'return', {})
> +
> + result = self.vm.qmp('nbd-server-add', device='img1')
> + self.assert_qmp(result, 'return', {})
> +
> + result = self.vm.qmp('drive-mirror', job_id='mirror', device='quorum0',
> + sync='full', node_name='repair0', replaces='img1',
> + target=quorum_repair_img, format=iotests.imgfmt)
> + self.assert_qmp(result, 'error/desc',
> + "Cannot replace 'img1' by a node mirrored from "
> + "'quorum0', because it cannot be guaranteed that doing "
> + "so would not lead to an abrupt change of visible data")
> +
> + def test_with_other_parents_after_mirror_start(self):
> + """
> + The same as test_with_other_parent(), but add the NBD server
> + only when the mirror job is already running.
> + """
> + result = self.vm.qmp('nbd-server-start',
> + addr={
> + 'type': 'unix',
> + 'data': {'path': nbd_sock_path}
> + })
> + self.assert_qmp(result, 'return', {})
> +
> + result = self.vm.qmp('drive-mirror', job_id='mirror', device='quorum0',
> + sync='full', node_name='repair0', replaces='img1',
> + target=quorum_repair_img, format=iotests.imgfmt)
> + self.assert_qmp(result, 'return', {})
> +
> + result = self.vm.qmp('nbd-server-add', device='img1')
> + self.assert_qmp(result, 'return', {})
> +
> + # The full error message goes to stderr, we will check it later
> + self.complete_and_wait('mirror',
> + completion_error='Operation not permitted')
> +
> + # Should not have been replaced
> + self.vm.assert_block_path('quorum0', '/children.1', 'img1')
> +
> + # Check the full error message now
> + self.vm.shutdown()
> + log = self.vm.get_log()
> + log = re.sub(r'^\[I \d+\.\d+\] OPENED\n', '', log)
> + log = re.sub(r'^Formatting.*\n', '', log)
> + log = re.sub(r'\n\[I \+\d+\.\d+\] CLOSED\n?$', '', log)
> + log = re.sub(r'^qemu-system-[^:]*: ', '', log)
I would have applied the series, but:
+.........................F....................................................................
+======================================================================
+FAIL: test_with_other_parents_after_mirror_start (__main__.TestRepairQuorum)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+ File "041", line 1099, in test_with_other_parents_after_mirror_start
+ "it can no longer be guaranteed that doing so would " +
+AssertionError: "qemu: Can no longer replace 'img1' by 're[107 chars]data" != "Can no longer replace 'img1' by 'repair0'[101 chars]data"
+- qemu: Can no longer replace 'img1' by 'repair0', because it can no longer be guaranteed that doing so would not lead to an abrupt change of visible data
+? ------
++ Can no longer replace 'img1' by 'repair0', because it can no longer be guaranteed that doing so would not lead to an abrupt change of visible data
+
+
If you agree, I can just change this line while applying into:
log = re.sub(r'^qemu[^:]*: ', '', log)
Kevin
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 18/19] iotests: Add tests for invalid Quorum @replaces
2020-02-18 12:38 ` Kevin Wolf
@ 2020-02-18 12:49 ` Max Reitz
2020-02-18 13:29 ` Kevin Wolf
0 siblings, 1 reply; 24+ messages in thread
From: Max Reitz @ 2020-02-18 12:49 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 6147 bytes --]
On 18.02.20 13:38, Kevin Wolf wrote:
> Am 18.02.2020 um 11:34 hat Max Reitz geschrieben:
>> Add two tests to see that you cannot replace a Quorum child with the
>> mirror job while the child is in use by a different parent.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> tests/qemu-iotests/041 | 70 +++++++++++++++++++++++++++++++++++++-
>> tests/qemu-iotests/041.out | 4 +--
>> 2 files changed, 71 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
>> index 1d9e64ff6d..53c8671969 100755
>> --- a/tests/qemu-iotests/041
>> +++ b/tests/qemu-iotests/041
>> @@ -20,6 +20,7 @@
>>
>> import time
>> import os
>> +import re
>> import iotests
>> from iotests import qemu_img, qemu_io
>>
>> @@ -34,6 +35,8 @@ quorum_img3 = os.path.join(iotests.test_dir, 'quorum3.img')
>> quorum_repair_img = os.path.join(iotests.test_dir, 'quorum_repair.img')
>> quorum_snapshot_file = os.path.join(iotests.test_dir, 'quorum_snapshot.img')
>>
>> +nbd_sock_path = os.path.join(iotests.test_dir, 'nbd.sock')
>> +
>> class TestSingleDrive(iotests.QMPTestCase):
>> image_len = 1 * 1024 * 1024 # MB
>> qmp_cmd = 'drive-mirror'
>> @@ -892,7 +895,8 @@ class TestRepairQuorum(iotests.QMPTestCase):
>>
>> def tearDown(self):
>> self.vm.shutdown()
>> - for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file ]:
>> + for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file,
>> + nbd_sock_path ]:
>> # Do a try/except because the test may have deleted some images
>> try:
>> os.remove(i)
>> @@ -1032,6 +1036,70 @@ class TestRepairQuorum(iotests.QMPTestCase):
>> self.assert_has_block_node("repair0", quorum_repair_img)
>> self.vm.assert_block_path('quorum0', '/children.1', 'repair0')
>>
>> + def test_with_other_parent(self):
>> + """
>> + Check that we cannot replace a Quorum child when it has other
>> + parents.
>> + """
>> + result = self.vm.qmp('nbd-server-start',
>> + addr={
>> + 'type': 'unix',
>> + 'data': {'path': nbd_sock_path}
>> + })
>> + self.assert_qmp(result, 'return', {})
>> +
>> + result = self.vm.qmp('nbd-server-add', device='img1')
>> + self.assert_qmp(result, 'return', {})
>> +
>> + result = self.vm.qmp('drive-mirror', job_id='mirror', device='quorum0',
>> + sync='full', node_name='repair0', replaces='img1',
>> + target=quorum_repair_img, format=iotests.imgfmt)
>> + self.assert_qmp(result, 'error/desc',
>> + "Cannot replace 'img1' by a node mirrored from "
>> + "'quorum0', because it cannot be guaranteed that doing "
>> + "so would not lead to an abrupt change of visible data")
>> +
>> + def test_with_other_parents_after_mirror_start(self):
>> + """
>> + The same as test_with_other_parent(), but add the NBD server
>> + only when the mirror job is already running.
>> + """
>> + result = self.vm.qmp('nbd-server-start',
>> + addr={
>> + 'type': 'unix',
>> + 'data': {'path': nbd_sock_path}
>> + })
>> + self.assert_qmp(result, 'return', {})
>> +
>> + result = self.vm.qmp('drive-mirror', job_id='mirror', device='quorum0',
>> + sync='full', node_name='repair0', replaces='img1',
>> + target=quorum_repair_img, format=iotests.imgfmt)
>> + self.assert_qmp(result, 'return', {})
>> +
>> + result = self.vm.qmp('nbd-server-add', device='img1')
>> + self.assert_qmp(result, 'return', {})
>> +
>> + # The full error message goes to stderr, we will check it later
>> + self.complete_and_wait('mirror',
>> + completion_error='Operation not permitted')
>> +
>> + # Should not have been replaced
>> + self.vm.assert_block_path('quorum0', '/children.1', 'img1')
>> +
>> + # Check the full error message now
>> + self.vm.shutdown()
>> + log = self.vm.get_log()
>> + log = re.sub(r'^\[I \d+\.\d+\] OPENED\n', '', log)
>> + log = re.sub(r'^Formatting.*\n', '', log)
>> + log = re.sub(r'\n\[I \+\d+\.\d+\] CLOSED\n?$', '', log)
>> + log = re.sub(r'^qemu-system-[^:]*: ', '', log)
>
> I would have applied the series, but:
>
> +.........................F....................................................................
> +======================================================================
> +FAIL: test_with_other_parents_after_mirror_start (__main__.TestRepairQuorum)
> +----------------------------------------------------------------------
> +Traceback (most recent call last):
> + File "041", line 1099, in test_with_other_parents_after_mirror_start
> + "it can no longer be guaranteed that doing so would " +
> +AssertionError: "qemu: Can no longer replace 'img1' by 're[107 chars]data" != "Can no longer replace 'img1' by 'repair0'[101 chars]data"
> +- qemu: Can no longer replace 'img1' by 'repair0', because it can no longer be guaranteed that doing so would not lead to an abrupt change of visible data
> +? ------
> ++ Can no longer replace 'img1' by 'repair0', because it can no longer be guaranteed that doing so would not lead to an abrupt change of visible data
> +
> +
>
> If you agree, I can just change this line while applying into:
>
> log = re.sub(r'^qemu[^:]*: ', '', log)
Sure, thanks! I just hope noone uses the QEMU environment variable to
point to something that isn’t prefixed by “qemu”...
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 18/19] iotests: Add tests for invalid Quorum @replaces
2020-02-18 12:49 ` Max Reitz
@ 2020-02-18 13:29 ` Kevin Wolf
0 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2020-02-18 13:29 UTC (permalink / raw)
To: Max Reitz; +Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 6592 bytes --]
Am 18.02.2020 um 13:49 hat Max Reitz geschrieben:
> On 18.02.20 13:38, Kevin Wolf wrote:
> > Am 18.02.2020 um 11:34 hat Max Reitz geschrieben:
> >> Add two tests to see that you cannot replace a Quorum child with the
> >> mirror job while the child is in use by a different parent.
> >>
> >> Signed-off-by: Max Reitz <mreitz@redhat.com>
> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >> ---
> >> tests/qemu-iotests/041 | 70 +++++++++++++++++++++++++++++++++++++-
> >> tests/qemu-iotests/041.out | 4 +--
> >> 2 files changed, 71 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> >> index 1d9e64ff6d..53c8671969 100755
> >> --- a/tests/qemu-iotests/041
> >> +++ b/tests/qemu-iotests/041
> >> @@ -20,6 +20,7 @@
> >>
> >> import time
> >> import os
> >> +import re
> >> import iotests
> >> from iotests import qemu_img, qemu_io
> >>
> >> @@ -34,6 +35,8 @@ quorum_img3 = os.path.join(iotests.test_dir, 'quorum3.img')
> >> quorum_repair_img = os.path.join(iotests.test_dir, 'quorum_repair.img')
> >> quorum_snapshot_file = os.path.join(iotests.test_dir, 'quorum_snapshot.img')
> >>
> >> +nbd_sock_path = os.path.join(iotests.test_dir, 'nbd.sock')
> >> +
> >> class TestSingleDrive(iotests.QMPTestCase):
> >> image_len = 1 * 1024 * 1024 # MB
> >> qmp_cmd = 'drive-mirror'
> >> @@ -892,7 +895,8 @@ class TestRepairQuorum(iotests.QMPTestCase):
> >>
> >> def tearDown(self):
> >> self.vm.shutdown()
> >> - for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file ]:
> >> + for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file,
> >> + nbd_sock_path ]:
> >> # Do a try/except because the test may have deleted some images
> >> try:
> >> os.remove(i)
> >> @@ -1032,6 +1036,70 @@ class TestRepairQuorum(iotests.QMPTestCase):
> >> self.assert_has_block_node("repair0", quorum_repair_img)
> >> self.vm.assert_block_path('quorum0', '/children.1', 'repair0')
> >>
> >> + def test_with_other_parent(self):
> >> + """
> >> + Check that we cannot replace a Quorum child when it has other
> >> + parents.
> >> + """
> >> + result = self.vm.qmp('nbd-server-start',
> >> + addr={
> >> + 'type': 'unix',
> >> + 'data': {'path': nbd_sock_path}
> >> + })
> >> + self.assert_qmp(result, 'return', {})
> >> +
> >> + result = self.vm.qmp('nbd-server-add', device='img1')
> >> + self.assert_qmp(result, 'return', {})
> >> +
> >> + result = self.vm.qmp('drive-mirror', job_id='mirror', device='quorum0',
> >> + sync='full', node_name='repair0', replaces='img1',
> >> + target=quorum_repair_img, format=iotests.imgfmt)
> >> + self.assert_qmp(result, 'error/desc',
> >> + "Cannot replace 'img1' by a node mirrored from "
> >> + "'quorum0', because it cannot be guaranteed that doing "
> >> + "so would not lead to an abrupt change of visible data")
> >> +
> >> + def test_with_other_parents_after_mirror_start(self):
> >> + """
> >> + The same as test_with_other_parent(), but add the NBD server
> >> + only when the mirror job is already running.
> >> + """
> >> + result = self.vm.qmp('nbd-server-start',
> >> + addr={
> >> + 'type': 'unix',
> >> + 'data': {'path': nbd_sock_path}
> >> + })
> >> + self.assert_qmp(result, 'return', {})
> >> +
> >> + result = self.vm.qmp('drive-mirror', job_id='mirror', device='quorum0',
> >> + sync='full', node_name='repair0', replaces='img1',
> >> + target=quorum_repair_img, format=iotests.imgfmt)
> >> + self.assert_qmp(result, 'return', {})
> >> +
> >> + result = self.vm.qmp('nbd-server-add', device='img1')
> >> + self.assert_qmp(result, 'return', {})
> >> +
> >> + # The full error message goes to stderr, we will check it later
> >> + self.complete_and_wait('mirror',
> >> + completion_error='Operation not permitted')
> >> +
> >> + # Should not have been replaced
> >> + self.vm.assert_block_path('quorum0', '/children.1', 'img1')
> >> +
> >> + # Check the full error message now
> >> + self.vm.shutdown()
> >> + log = self.vm.get_log()
> >> + log = re.sub(r'^\[I \d+\.\d+\] OPENED\n', '', log)
> >> + log = re.sub(r'^Formatting.*\n', '', log)
> >> + log = re.sub(r'\n\[I \+\d+\.\d+\] CLOSED\n?$', '', log)
> >> + log = re.sub(r'^qemu-system-[^:]*: ', '', log)
> >
> > I would have applied the series, but:
> >
> > +.........................F....................................................................
> > +======================================================================
> > +FAIL: test_with_other_parents_after_mirror_start (__main__.TestRepairQuorum)
> > +----------------------------------------------------------------------
> > +Traceback (most recent call last):
> > + File "041", line 1099, in test_with_other_parents_after_mirror_start
> > + "it can no longer be guaranteed that doing so would " +
> > +AssertionError: "qemu: Can no longer replace 'img1' by 're[107 chars]data" != "Can no longer replace 'img1' by 'repair0'[101 chars]data"
> > +- qemu: Can no longer replace 'img1' by 'repair0', because it can no longer be guaranteed that doing so would not lead to an abrupt change of visible data
> > +? ------
> > ++ Can no longer replace 'img1' by 'repair0', because it can no longer be guaranteed that doing so would not lead to an abrupt change of visible data
> > +
> > +
> >
> > If you agree, I can just change this line while applying into:
> >
> > log = re.sub(r'^qemu[^:]*: ', '', log)
>
> Sure, thanks! I just hope noone uses the QEMU environment variable to
> point to something that isn’t prefixed by “qemu”...
Ok, I'll use this instead:
+ log = re.sub(r'^%s[^:]*: ' % os.path.basename(iotests.qemu_prog),
+ '', log)
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 19/19] iotests: Check that @replaces can replace filters
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (17 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 18/19] iotests: Add tests for invalid Quorum @replaces Max Reitz
@ 2020-02-18 10:34 ` Max Reitz
2020-02-18 13:53 ` [PATCH v4 00/19] block: Fix check_to_replace_node() Kevin Wolf
19 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-02-18 10:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/041 | 46 ++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/041.out | 4 ++--
2 files changed, 48 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 53c8671969..532a3827e7 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -1190,6 +1190,52 @@ class TestOrphanedSource(iotests.QMPTestCase):
self.assertFalse('mirror-filter' in nodes,
'Mirror filter node did not disappear')
+# Test cases for @replaces that do not necessarily involve Quorum
+class TestReplaces(iotests.QMPTestCase):
+ # Each of these test cases needs their own block graph, so do not
+ # create any nodes here
+ def setUp(self):
+ self.vm = iotests.VM()
+ self.vm.launch()
+
+ def tearDown(self):
+ self.vm.shutdown()
+ for img in (test_img, target_img):
+ try:
+ os.remove(img)
+ except OSError:
+ pass
+
+ @iotests.skip_if_unsupported(['copy-on-read'])
+ def test_replace_filter(self):
+ """
+ Check that we can replace filter nodes.
+ """
+ result = self.vm.qmp('blockdev-add', **{
+ 'driver': 'copy-on-read',
+ 'node-name': 'filter0',
+ 'file': {
+ 'driver': 'copy-on-read',
+ 'node-name': 'filter1',
+ 'file': {
+ 'driver': 'null-co'
+ }
+ }
+ })
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm.qmp('blockdev-add',
+ node_name='target', driver='null-co')
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm.qmp('blockdev-mirror', job_id='mirror', device='filter0',
+ target='target', sync='full', replaces='filter1')
+ self.assert_qmp(result, 'return', {})
+
+ self.complete_and_wait('mirror')
+
+ self.vm.assert_block_path('filter0', '/file', 'target')
+
if __name__ == '__main__':
iotests.main(supported_fmts=['qcow2', 'qed'],
supported_protocols=['file'],
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index ffc779b4d1..877b76fd31 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-.............................................................................................
+..............................................................................................
----------------------------------------------------------------------
-Ran 93 tests
+Ran 94 tests
OK
--
2.24.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v4 00/19] block: Fix check_to_replace_node()
2020-02-18 10:34 [PATCH v4 00/19] block: Fix check_to_replace_node() Max Reitz
` (18 preceding siblings ...)
2020-02-18 10:34 ` [PATCH v4 19/19] iotests: Check that @replaces can replace filters Max Reitz
@ 2020-02-18 13:53 ` Kevin Wolf
19 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2020-02-18 13:53 UTC (permalink / raw)
To: Max Reitz; +Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Am 18.02.2020 um 11:34 hat Max Reitz geschrieben:
> Branch: https://github.com/XanClic/qemu.git fix-can-replace-v4
> Branch: https://git.xanclic.moe/XanClic/qemu.git fix-can-replace-v4
>
> v1: https://lists.nongnu.org/archive/html/qemu-block/2019-09/msg01027.html
> v2: https://lists.nongnu.org/archive/html/qemu-block/2019-11/msg00370.html
> v3: https://lists.nongnu.org/archive/html/qemu-block/2020-01/msg00922.html
>
>
> Hi,
>
> For what this series does, see the cover letter of v1 (linked above).
>
>
> Changes in v4 compared to v3:
> - Following the discussion with Kevin, I changed Quorum’s
> .bdrv_recurse_can_replace() implementation from unsharing the
> CONSISTENT_READ permission and taking the WRITE permission to just
> checking whether there are any other parents
> - This made the old patches 8 and 9 unnecessary, so they’ve been
> dropped
> - And of course it requires some changes to patch 10 (now 8)
> - (Patch 10 (was: 12) gets a rebase conflict that’s obvious to
> resolve, so I kept Vladimir’s R-b)
>
> - As suggested by Vladimir, I added the root node name to the
> cannot-follow-path error message in patch 14 (was: 16), and added an
> assertion that the root node exists in the first place
Thanks, applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 24+ messages in thread