On Wed, Jul 19, 2017 at 4:05 PM Markus Armbruster <arm...@redhat.com> wrote:
> 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. > You can find a reproducer and example output here: https://bugzilla.redhat.com/show_bug.cgi?id=1470634#c28 > > > 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. >