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: qemu-sta...@nongnu.org > Signed-off-by: Kevin Wolf <kw...@redhat.com> > --- > > 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
