Re: [PATCH RFC 0/9] qemu: Support mapped-ram migration capability

On 8/7/24 12:39, Daniel P. Berrangé wrote: > On Wed, Aug 07, 2024 at 12:04:18PM -0600, Jim Fehlig wrote: >> On 8/7/24 09:45, Daniel P. Berrangé wrote: >>> On Thu, Jun 13, 2024 at 04:43:14PM -0600, Jim Fehlig via Devel wrote: >>>> The QEMU mapped-ram capability currently does not support directio. >>>> Fabino is working on that now [3]. This complicates merging support >>>> in libvirt. I don't think it's reasonable to enable mapped-ram by >>>> default when BYPASS_CACHE cannot be supported. Should we wait until >>>> the mapped-ram directio support is merged in QEMU before supporting >>>> mapped-ram in libvirt? >>>> >>>> For the moment, compression is ignored in the new save version. >>>> Currently, libvirt connects the output of QEMU's save stream to the >>>> specified compression program via a pipe. This approach is incompatible >>>> with mapped-ram since the fd provided to QEMU must be seekable. One >>>> option is to reopen and compress the saved image after the actual save >>>> operation has completed. This has the downside of requiring the iohelper >>>> to handle BYPASS_CACHE, which would preclude us from removing it >>>> sometime in the future. Other suggestions much welcomed. >>> >>> Going back to the original motivation for mapped-ram. The first key >>> factor was that it will make it more viable to use multi-fd for >>> parallelized saving and restoring, as it lets threads write concurrently >>> without needing synchronization. The predictable worst case file size >>> when the VM is live & dirtying memory, was an added benefit. >> >> Correct. The main motivation for mapped-ram is parallelizing save/restore. >> To that end, we could base the whole feature on whether parallel is >> requested. If so, we save with mapped-ram (failing if not supported by >> underlying qemu) and don't support compression. If parallel not requested, >> use existing save/restore implementation. > > We had discussed that as a strategy previously and thought it > was a good idea. > > Your other reply to me though makes me re-consider. You're > showing that /without/ O_DIRECT and multifd, then mapped-ram > is already a big win over the legacy method. The scale of that > improvement is somewhat surprising to me and if reliably so, > that is compelling to want all the time. How do we have it all of the time without issues of spareness you describe? Seems it will always be opt-in, unless using the new parallel option. > >>> Overall I'm wondering if we need to give a direct choice to mgmt >>> apps. >>> >>> We added a the save/restore variants that accept virTypedParameters, >>> so we could define a VIR_DOMAIN_SAVE_PARAM_FORMAT, which accepts >>> 'stream' and 'mapped' as options. This choice would then influence >>> whether we save in v2 or v3 format.  On restore we don't need a >>> parameter as we just probe the on disk format. >> >> I worry these options will confuse users. IMO we'd need a basic description >> of the stream formats, along with caveats of file size, lack of compression >> support, etc. It seems like quite a bit for the average user to absorb. >> >>> >>> As a documentation task we can then save that compression is >>> incompatible with 'mapped'. >>> >>> Annoyingly we already have a 'save_image_formt' in qemu.conf though >>> taking  'raw', 'zstd', 'lzop', etc to choose the compression type. >>> So we have a terminology clash. >>> >>> We probably should have exposed this compression choice in the API >>> too, via a VIR_DOMAIN_SAVE_PARAM_COMPRESSION typed parameter, taking >>> values 'default', 'raw', 'zstd', 'lzop', etc, where 'default' (or >>> omitted) means honour the qemu.conf setting and all others override >>> qemu.conf >> >> Agree with that suggestion. And VIR_DOMAIN_SAVE_PARAM_COMPRESSION would be >> easy to describe and clear to average user :-). >> >>> >>> Might suggest we name the new API parameter VR_DOMAIN_SAVE_PARAM_LAYOUT >>> for stream vs mapped choice ? >> >> These stream formats seem like an implementation detail we should attempt to >> hide from the user. > > Yeah it is rather an impl detail, but perhaps it is an impl detail we > cannot avoid exposing users to. After all they need to be aware of > use of sparseness in order to handle the files without ballooning up > their size. As for a name for the new parameter, I can't think of anything better than your suggestion of VIR_DOMAIN_SAVE_PARAM_LAYOUT. I much prefer _FORMAT, but yeah, that ship has sailed. Before working on the API parameter, I'll summarize my understanding of the options for supporting mapped-ram: 1. No user control. If qemu supports mapped-ram, use it 2. Control at driver level