Re: [Qemu-devel] [PATCH for-2.10] block: Skip implicit nodes in query-block/blockstats

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5013 bytes --]

Am 19.07.2017 um 13:15 hat Peter Krempa geschrieben:
> On Wed, Jul 19, 2017 at 10:44:47 +0200, Kevin Wolf wrote:
> > 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.
> > 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.
> > 
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Kevin Wolf <kwolf@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(-)
> 
> Fails to apply to master, thus I didn't do a functional check, but from
> reading the code ...

My last pull request was just merged into master, so now it should apply
cleanly.

> > 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);
> > +    }
> 
> So bdrv_query_info then calls bdrv_block_device_info, which also
> populates the backing chain for that bs. That function does not do this
> trick of skipping the implicit nodes.
> 
> From what I've understood from the 'commit' job, it's possible to have
> the implicit node inserted even at the @top bs of the commit job, so it
> looks to me as if bdrv_block_device_info will need such a tweak too.

Ah, yes, I missed the recursive calls. I will add another loop as you
suggest.

> > +
> >      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);
> 
> Same here. The recursive call in bdrv_query_bds_stats does not skip the
> implicitly added layers IIUC.

Indeed.

> >              s->has_device = true;
> >              s->device = g_strdup(blk_name(blk));
> >              bdrv_query_blk_stats(s->stats, blk);
> 
> Feel free to add a 'Reviewed-by' if I did not understand what's
> happening in the commit job properly, or if you add the skipping into
> the gathering functions as well.

I'll send a v2 for this, it doesn't seem trivial enough to just add
Reviewed-by.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]