Re: [PATCH v2 00/10] qemu: add option to process offloaded legacy blockjob event ealier

On Fri, Dec 04, 2020 at 19:28:55 +0300, Nikolay Shirokovskiy wrote: > > > On 04.12.2020 19:24, Peter Krempa wrote: >> On Fri, Dec 04, 2020 at 19:00:57 +0300, Nikolay Shirokovskiy wrote: >>> >>> >>> On 04.12.2020 18:52, Peter Krempa wrote: >>>> On Fri, Dec 04, 2020 at 18:44:00 +0300, Nikolay Shirokovskiy wrote: >>>>> On 04.12.2020 18:11, Peter Krempa wrote: >>>>>> On Fri, Nov 13, 2020 at 09:53:28 +0300, Nikolay Shirokovskiy wrote: >>>> >>>> [...] >>>> >>>>> Ok. By the way what about the issue described in >>>>> >>>>> [PATCH v2 09/10] qemu: fix race on legacy block completion and quering stats >>>>> >>>>> Are modern blockjobs have similar? >>>> >>>> Modern blockjobs are started with '"auto-dismiss": false' so if you get >>>> to the monitor before the thread completing the blockjob finishes it, >>>> qemu still reports the status for the job. >>> >>> But from qemu docs I read that graph changes occur on finalize step and modern >>> blockjobs use auto-finalize = true. So at the moment of querying stats >>> graph in libvirt and qemu can be different I thought. >> >> That doesn't matter for the purposte of data returned from >> 'qemuDomainGetBlockJobInfo'. QEMU still reports the job, so you will >> still see it as running and the XML will look the same as while it was >> running. >> > > Yeah, but the 9th patch is not fixing qemuDomainGetBlockJobInfo (as 10th does). > It fixes getting stats. And different graphs in qemu and libvirt affects > stats collection. Stats are queried via 'node-name' when -blockdev is used and as said, removal of the nodes happens only explicitly after we finalize the job. Thus you will get stats per the state the backing chain looked before the blockjob modified the graph as we report the graph from our internal data. Some stats may be stale perhaps, but that's the worst case.

In case of stats for pre-blockdev, you will potentially get stats from the reconfigured chain which we attempt to map into the old chain. This indeed may fail. In such case we can plainly not report them, the stats API doesn't always guarantee results. Even for the case of the stats, the proposed fix is just too invasive for something which won't be used for long.