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

[Eric Blake]

On 07/19/2017 08:58 AM, 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.

I'm a bit worried about this statement.  Libvirt controls the
BLOCK_WRITE_THRESHOLD event via block-set-write-threshold, which
requires the use of a node name (for a qcow2-backed-by-block-device, you
want the threshold to be tied to the block-device protocol BDS, not the
qcow2 format BDS).  We need to test that this patch does not break
write-threshold computation (ie. that libvirt is still able to use the
query commands to learn WHICH node name to pass to
block-set-write-threshold).

Or put another way, there are two types of implicit nodes names: the
implicit node names for protocol BDS (libvirt cares about those), and
the implicit node names for block job temporary BDS (libvirt does not
care about those).  If this patch is touching ONLY the block job
implicit node names, we may be okay - but that's something we want to
test with libvirt before accepting this patch.  Peter, are you in a
position to test it faster than me?

> 
> 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 implicit 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.

Current libvirt is using both of those commands, mainly in support of
computing the generated node name to pass to block-set-write-threshold.
So the fact that these are not filtered may still be enough for libvirt
to get everything it needs.

> 
> Cc: address@hidden
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
> 
> v2:
> - Skip implicit nodes not only on the top level, but also during the recursive
>   calls [Peter]
> - Spelling fix in the commit message [Manos]
> 
At any rate, the code looks okay to me, so now it's just a matter of
testing that the patch doesn't break libvirt's other uses, while fixing
the incorrect allocation reporting while a job is active.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature