Kevin Wolf <kw...@redhat.com> writes:
> Commits 0db832f and 6cdbceb introduced the automatic insertion of filter
> nodes above the top layer of mirror and commit block jobs. The
> assumption made there was that since libvirt doesn't do node-level
> management of the block layer yet, it shouldn't be affected by added
> nodes.
>
> This is true as far as commands issued by libvirt are concerned. It only
> uses BlockBackend names to address nodes, so any operations it performs
> still operate on the root of the tree as intended.
>
> However, the assumption breaks down when you consider query commands,
> which return data for the wrong node now. These commands also return
> information on some child nodes (bs->file and/or bs->backing), which
> libvirt does make use of, and which refer to the wrong nodes, too.
>
> One of the consequences is that oVirt gets wrong information about the
> image size and stops the VM in response as long as a mirror or commit
> job is running:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1470634
>
> This patch fixes the problem by hiding the implict nodes created
> automatically by the mirror and commit block jobs in the output of
> query-block and BlockBackend-based query-blockstats as long as the user
> doesn't indicate that they are aware of those nodes by providing a node
> name for them in the QMP command to start the block job.
>
> The node-based commands query-named-block-nodes and query-blockstats
> with query-nodes=true still show all nodes, including implicit ones.
The only other query-FOO in block*.json is query-block-jobs. Good.
> This ensures that users that are capable of node-level management can
> still access the full information; users that only know BlockBackends
> won't use these commands.
I think I can follow your reasoning, but I could use a concrete example.
A small reproducer with output before and after the patch, and an
explanation what exactly makes the output before the patch problematic.
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
> block/commit.c | 3 +++
> block/mirror.c | 3 +++
> block/qapi.c | 13 ++++++++++++-
> include/block/block_int.h | 1 +
> qapi/block-core.json | 6 ++++--
> tests/qemu-iotests/041 | 23 +++++++++++++++++++++++
> tests/qemu-iotests/041.out | 4 ++--
> 7 files changed, 48 insertions(+), 5 deletions(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index 5cc910f..c7857c3 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -346,6 +346,9 @@ void commit_start(const char *job_id, BlockDriverState
> *bs,
> if (commit_top_bs == NULL) {
> goto fail;
> }
> + if (!filter_node_name) {
> + commit_top_bs->implicit = true;
> + }
> commit_top_bs->total_sectors = top->total_sectors;
> bdrv_set_aio_context(commit_top_bs, bdrv_get_aio_context(top));
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 8583b76..c9a6a3c 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1168,6 +1168,9 @@ static void mirror_start_job(const char *job_id,
> BlockDriverState *bs,
> if (mirror_top_bs == NULL) {
> return;
> }
> + if (!filter_node_name) {
> + mirror_top_bs->implicit = true;
> + }
> mirror_top_bs->total_sectors = bs->total_sectors;
> bdrv_set_aio_context(mirror_top_bs, bdrv_get_aio_context(bs));
>
> diff --git a/block/qapi.c b/block/qapi.c
> index 95b2e2d..0ed23b8 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -324,6 +324,11 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo
> **p_info,
> BlockDriverState *bs = blk_bs(blk);
> char *qdev;
>
> + /* Skip automatically inserted nodes that the user isn't aware of */
> + while (bs && bs->drv && bs->implicit) {
> + bs = backing_bs(bs);
> + }
> +
> info->device = g_strdup(blk_name(blk));
> info->type = g_strdup("unknown");
> info->locked = blk_dev_is_medium_locked(blk);
> @@ -518,12 +523,18 @@ BlockStatsList *qmp_query_blockstats(bool
> has_query_nodes,
> }
> } else {
> for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
> + BlockDriverState *bs = blk_bs(blk);
> BlockStatsList *info = g_malloc0(sizeof(*info));
> AioContext *ctx = blk_get_aio_context(blk);
> BlockStats *s;
>
> + /* Skip automatically inserted nodes that the user isn't aware
> of */
> + while (bs && bs->drv && bs->implicit) {
> + bs = backing_bs(bs);
> + }
> +
> aio_context_acquire(ctx);
> - s = bdrv_query_bds_stats(blk_bs(blk), true);
> + s = bdrv_query_bds_stats(bs, true);
> s->has_device = true;
> s->device = g_strdup(blk_name(blk));
> bdrv_query_blk_stats(s->stats, blk);
The result types of query-block and query-blockstats are recursive. How
can I convince myself that we're skipping everywhere we need to?
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 5c6b761..d4f4ea7 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -549,6 +549,7 @@ struct BlockDriverState {
> bool sg; /* if true, the device is a /dev/sg* */
> bool probed; /* if true, format was probed rather than specified */
> bool force_share; /* if true, always allow all shared permissions */
> + bool implicit; /* if true, this filter node was automatically inserted
> */
Makes sense. How can I convince myself that your patch marks them all?
>
> BlockDriver *drv; /* NULL means no media */
> void *opaque;
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ff8e2ba..006e048 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -520,7 +520,8 @@
> #
> # Get a list of BlockInfo for all virtual block devices.
> #
> -# Returns: a list of @BlockInfo describing each virtual block device
> +# Returns: a list of @BlockInfo describing each virtual block device. Filter
> +# nodes that were created implicitly are skipped over.
> #
> # Since: 0.14.0
> #
> @@ -780,7 +781,8 @@
> # information, but not "backing".
> # If false or omitted, the behavior is as before - query all
> the
> # device backends, recursively including their "parent" and
> -# "backing". (Since 2.3)
> +# "backing". Filter nodes that were created implicitly are
> +# skipped over in this mode. (Since 2.3)
> #
> # Returns: A list of @BlockStats for each virtual block devices.
> #
> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> index 2f54986..b798cca 100755
> --- a/tests/qemu-iotests/041
> +++ b/tests/qemu-iotests/041
> @@ -169,6 +169,29 @@ class TestSingleDrive(iotests.QMPTestCase):
> self.assertTrue(iotests.compare_images(test_img, target_img),
> 'target image does not match source after mirroring')
>
> + # Tests that the insertion of the mirror_top filter node doesn't make a
> + # difference to query-block
> + def test_implicit_node(self):
> + self.assert_no_active_block_jobs()
> +
> + result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
> + target=self.qmp_target)
> + self.assert_qmp(result, 'return', {})
> +
> + result = self.vm.qmp('query-block')
> + self.assert_qmp(result, 'return[0]/inserted/file', test_img)
> + self.assert_qmp(result, 'return[0]/inserted/drv', iotests.imgfmt)
> + self.assert_qmp(result, 'return[0]/inserted/backing_file',
> backing_img)
> + self.assert_qmp(result, 'return[0]/inserted/backing_file_depth', 1)
> +
> + self.cancel_and_wait(force=True)
> + result = self.vm.qmp('query-block')
> + self.assert_qmp(result, 'return[0]/inserted/file', test_img)
> + self.assert_qmp(result, 'return[0]/inserted/drv', iotests.imgfmt)
> + self.assert_qmp(result, 'return[0]/inserted/backing_file',
> backing_img)
> + self.assert_qmp(result, 'return[0]/inserted/backing_file_depth', 1)
> + self.vm.shutdown()
> +
> def test_medium_not_found(self):
> if iotests.qemu_default_machine != 'pc':
> return
> diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
> index e30fd3b..c28b392 100644
> --- a/tests/qemu-iotests/041.out
> +++ b/tests/qemu-iotests/041.out
> @@ -1,5 +1,5 @@
> -...............................................................................
> +.....................................................................................
> ----------------------------------------------------------------------
> -Ran 79 tests
> +Ran 85 tests
>
> OK
The patch makes sense to me. The hard part is judging whether it's
complete. I could use a bit of help with that.
