Re: [libvirt] [PATCH v2 2/4] qemu: caps: add QEMU_CAPS_QCOW2_L2_CACHE_SIZE

On 12/11/18 6:04 AM, Nikolay Shirokovskiy wrote: > > > On 10.12.2018 19:58, John Ferlan wrote: >> >> >> On 11/8/18 8:02 AM, Nikolay Shirokovskiy wrote: >>> For qemu capable of setting l2-cache-size for qcow2 images >>> to INT64_MAX and semantics of upper limit on l2 cache >>> size. We can only check this by qemu version (3.1.0) now. >>> >>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; >>> --- >>> src/qemu/qemu_capabilities.c | 5 +++++ >>> src/qemu/qemu_capabilities.h | 1 + >>> 2 files changed, 6 insertions(+) >>> >> >> This patch needs to be updated to top of tree. >> >>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c >>> index 2ca5af3..49a3b60 100644 >>> --- a/src/qemu/qemu_capabilities.c >>> +++ b/src/qemu/qemu_capabilities.c >>> @@ -509,6 +509,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, >>> "vfio-pci.display", >>> "blockdev", >>> "vfio-ap", >>> + "qcow2.l2-cache-size", >> >> When you do update, you will need to fix the comma-less entry for >> "egl-headless.rendernode" as well, unless someone else gets to it first. >> >>> ); >>> >>> >>> @@ -4117,6 +4118,10 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, >>> virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_MAX_CPU_COMPAT); >>> } >>> >>> + /* l2-cache-size before 3001000 does not accept INT64_MAX */ >>> + if (qemuCaps->version >= 3001000) >>> + virQEMUCapsSet(qemuCaps, QEMU_CAPS_QCOW2_L2_CACHE_SIZE); >>> + >> >> Not a fan of this. >> >> The 'l2-cache-size' was supported since QEMU 2.2. A "newer" algorithm >> for cache adjustment has been supported since QEMU 2.12 (and there's >> l2-cache-entry-size that "could be used" to know that). Then there's >> some unreferenced commit indicating usage of INT64_MAX. Tracking that >> down, I find qemu.git commit 6c1c8d5d from Max which enforces MAX_INT. > > This is a failure rather than enforcement. And AFAIU code that limit cache > to appropriate maximum when INT64_MAX is given in read_cache_sizes: > > *l2_cache_size = MIN(max_l2_cache, l2_cache_max_setting); > > only appeared after release of 3.0 in > > commit b749562d9822d14ef69c9eaa5f85903010b86c30 > Author: Leonid Bloch <lbloch(a)janustech.com&gt; > Date: Wed Sep 26 19:04:43 2018 +0300 > > qcow2: Assign the L2 cache relatively to the image size > > > ie setting cache size to INT64_MAX before 3.1 will fail. In > other words in earlier versions there were no value to specify that > cache size need to be set to maximum. You can calculate this value > yourself knowing disk size and disk cluster size and set it but > this is not convinient. > So prior to this patch the max could be 32 MB? And now it's calculate-able?

Do you think it would be "worthwhile" to add logic that knows QEMU 2.12 and 3.0 could still support using l2_cache_size, but with a lower value say 32MB? Then if we find 3.1 installed then we can supply INT_MAX for the value? Oh, and if the "maximum" attribute isn't set, then the default of 0 is used. Thus being able to use "Z" instead of "I" for patch 3?

BTW: One issue with providing a numerical QEMU version for something is that someone could backport that patch (and it's dependencies) into some maintenance branch or even downstream repo that won't report as 3.1, but would have the patch. But since libvirt would only accept 3.1 or later, it wouldn't allow usage.

>> >> Still does that really matter? Let QEMU pick their own max and don't >> have libvirt be the arbiter of that (like I noted in my 1/4 review). My >> reasoning is that there's been quite a few "adjustments" to the >> algorithms along the way. Those adjustments are one of the concerns >> voiced in the past why making any "semi-permanent" change to the value >> in some libvirt XML format has been NACKed historically. THus by placing >> boundaries that are true today we limit ourselves for the future. > > IIUC setting INT64_MAX is ok in this respect. It is not real value for cache > but rather order for QEMU to pick up one. > Right providing some unrealistically large value would appear to work. What that value "could be" for 2.12 and 3.0.0 which do support some level of alteration and would be 'nice' to include in the mix, but not required I suppose. And yes, I do have some downstream representation in mind by thinking about this - it's in beta now ;-). John >> >> BTW: If 3.1 was the "base" from which you want to work, then adjustments >> to the tests/qemucapabilitiesdata/*3_1*{.replies|.xml } files would be >> required as evidenced by your patch 4. The *.xml file would need to have >> the correct <version>3001000</version> setting which should allow patch4 >> to be merged into patch3. > > Yeah, but 3.1 is not yet released and I need blockdev also as > patch only makes difference in latter case. > > Nikolay > >> >> John >> >>> if (virQEMUCapsProbeQMPCommands(qemuCaps, mon) < 0) >>> goto cleanup; >>> >>> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h >>> index 6bb9a2c..bb2ac17 100644 >>> --- a/src/qemu/qemu_capabilities.h >>> +++ b/src/qemu/qemu_capabilities.h >>> @@ -493,6 +493,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ >>> QEMU_CAPS_VFIO_PCI_DISPLAY, /* -device vfio-pci.display */ >>> QEMU_CAPS_BLOCKDEV, /* -blockdev and blockdev-add are supported */ >>> QEMU_CAPS_DEVICE_VFIO_AP, /* -device vfio-ap */ >>> + QEMU_CAPS_QCOW2_L2_CACHE_SIZE, /* -blockdev supports l2-cache-size with INT64_MAX value */ >>> >>> QEMU_CAPS_LAST /* this must always be the last item */ >>> } virQEMUCapsFlags; >>>