[libvirt RFC] virFile: new VIR_FILE_WRAPPER_BIG_PIPE to improve performance
Claudio Fontana
cfontana at suse.de
Wed Mar 16 12:17:26 UTC 2022
On 3/14/22 6:48 PM, Daniel P. Berrangé wrote:
> On Mon, Mar 14, 2022 at 06:38:31PM +0100, Claudio Fontana wrote:
>> On 3/14/22 6:17 PM, Daniel P. Berrangé wrote:
>>> On Sat, Mar 12, 2022 at 05:30:01PM +0100, Claudio Fontana wrote:
>>>> the first user is the qemu driver,
>>>>
>>>> virsh save/resume would slow to a crawl with a default pipe size (64k).
>>>>
>>>> This improves the situation by 400%.
>>>>
>>>> Going through io_helper still seems to incur in some penalty (~15%-ish)
>>>> compared with direct qemu migration to a nc socket to a file.
>>>>
>>>> Signed-off-by: Claudio Fontana <cfontana at suse.de>
>>>> ---
>>>> src/qemu/qemu_driver.c | 6 +++---
>>>> src/qemu/qemu_saveimage.c | 11 ++++++-----
>>>> src/util/virfile.c | 12 ++++++++++++
>>>> src/util/virfile.h | 1 +
>>>> 4 files changed, 22 insertions(+), 8 deletions(-)
>>>>
>>>> Hello, I initially thought this to be a qemu performance issue,
>>>> so you can find the discussion about this in qemu-devel:
>>>>
>>>> "Re: bad virsh save /dev/null performance (600 MiB/s max)"
>>>>
>>>> https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg03142.html
>>>>
>>>> RFC since need to validate idea, and it is only lightly tested:
>>>>
>>>> save - about 400% benefit in throughput, getting around 20 Gbps to /dev/null,
>>>> and around 13 Gbps to a ramdisk.
>>>> By comparison, direct qemu migration to a nc socket is around 24Gbps.
>>>>
>>>> restore - not tested, _should_ also benefit in the "bypass_cache" case
>>>> coredump - not tested, _should_ also benefit like for save
>>>>
>>>> Thanks for your comments and review,
>>>>
>>>> Claudio
>>>>
>>>>
>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>> index c1b3bd8536..be248c1e92 100644
>>>> --- a/src/qemu/qemu_driver.c
>>>> +++ b/src/qemu/qemu_driver.c
>>>> @@ -3044,7 +3044,7 @@ doCoreDump(virQEMUDriver *driver,
>>>> virFileWrapperFd *wrapperFd = NULL;
>>>> int directFlag = 0;
>>>> bool needUnlink = false;
>>>> - unsigned int flags = VIR_FILE_WRAPPER_NON_BLOCKING;
>>>> + unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING | VIR_FILE_WRAPPER_BIG_PIPE;
>>>> const char *memory_dump_format = NULL;
>>>> g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>>>> g_autoptr(virCommand) compressor = NULL;
>>>> @@ -3059,7 +3059,7 @@ doCoreDump(virQEMUDriver *driver,
>>>>
>>>> /* Create an empty file with appropriate ownership. */
>>>> if (dump_flags & VIR_DUMP_BYPASS_CACHE) {
>>>> - flags |= VIR_FILE_WRAPPER_BYPASS_CACHE;
>>>> + wrapperFlags |= VIR_FILE_WRAPPER_BYPASS_CACHE;
>>>> directFlag = virFileDirectFdFlag();
>>>> if (directFlag < 0) {
>>>> virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>>>> @@ -3072,7 +3072,7 @@ doCoreDump(virQEMUDriver *driver,
>>>> &needUnlink)) < 0)
>>>> goto cleanup;
>>>>
>>>> - if (!(wrapperFd = virFileWrapperFdNew(&fd, path, flags)))
>>>> + if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
>>>> goto cleanup;
>>>>
>>>> if (dump_flags & VIR_DUMP_MEMORY_ONLY) {
>>>> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
>>>> index c0139041eb..1b522a1542 100644
>>>> --- a/src/qemu/qemu_saveimage.c
>>>> +++ b/src/qemu/qemu_saveimage.c
>>>> @@ -267,7 +267,7 @@ qemuSaveImageCreate(virQEMUDriver *driver,
>>>> int fd = -1;
>>>> int directFlag = 0;
>>>> virFileWrapperFd *wrapperFd = NULL;
>>>> - unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING;
>>>> + unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING | VIR_FILE_WRAPPER_BIG_PIPE;
>>>>
>>>> /* Obtain the file handle. */
>>>> if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) {
>>>> @@ -463,10 +463,11 @@ qemuSaveImageOpen(virQEMUDriver *driver,
>>>> if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0)
>>>> return -1;
>>>>
>>>> - if (bypass_cache &&
>>>> - !(*wrapperFd = virFileWrapperFdNew(&fd, path,
>>>> - VIR_FILE_WRAPPER_BYPASS_CACHE)))
>>>> - return -1;
>>>> + if (bypass_cache) {
>>>> + unsigned int wrapperFlags = VIR_FILE_WRAPPER_BYPASS_CACHE | VIR_FILE_WRAPPER_BIG_PIPE;
>>>> + if (!(*wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
>>>> + return -1;
>>>> + }
>>>>
>>>> data = g_new0(virQEMUSaveData, 1);
>>>>
>>>> diff --git a/src/util/virfile.c b/src/util/virfile.c
>>>> index a04f888e06..fdacd17890 100644
>>>> --- a/src/util/virfile.c
>>>> +++ b/src/util/virfile.c
>>>> @@ -282,6 +282,18 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags)
>>>>
>>>> ret->cmd = virCommandNewArgList(iohelper_path, name, NULL);
>>>>
>>>> + if (flags & VIR_FILE_WRAPPER_BIG_PIPE) {
>>>> + /*
>>>> + * virsh save/resume would slow to a crawl with a default pipe size (usually 64k).
>>>> + * This improves the situation by 400%, although going through io_helper still incurs
>>>> + * in a performance penalty compared with a direct qemu migration to a socket.
>>>> + */
>>>> + int pipe_sz, rv = virFileReadValueInt(&pipe_sz, "/proc/sys/fs/pipe-max-size");
>>>
>>> This is fine as an experiment but I don't think it is that safe
>>> to use in the real world. There could be a variety of reasons why
>>> an admin can enlarge this value, and we shouldn't assume the max
>>> size is sensible for libvirt/QEMU to use.
>>>
>>> I very much suspect there are diminishing returns here in terms
>>> of buffer sizes.
>>>
>>> 64k is obvious too small, but 1 MB, may be sufficiently large
>>> that the bottleneck is then elsewhere in our code. IOW, If the
>>> pipe max size is 100 MB, we shouldn't blindly use it. Can you
>>> do a few tests with varying sizes to see where a sensible
>>> tradeoff falls ?
>>
>>
>> Hi Daniel,
>>
>> this is a very good point. Actually I see very diminishing returns after the default pipe-max-size (1MB).
>>
>> The idea was that beyond allowing larger size, the admin could have set a _smaller_ pipe-max-size,
>> so we want to use that in that case, otherwise an attempt to use 1MB would result in EPERM, if the process does not have CAP_SYS_RESOURCE or CAP_SYS_ADMIN.
>> I am not sure if used with Kubevirt, for example, CAP_SYS_RESOURCE or CAP_SYS_ADMIN would be available...?
>>
>> So maybe one idea could be to use the minimum between /proc/sys/fs/pipe-max-size and for example 1MB, but will do more testing to see where the actual break point is.
>
> That's reasonable.
>
Just as an update: still running tests with various combinations, and larger VMs (to RAM, to slow disk, and now to nvme).
For now no clear winner yet. There seems to be a significant benefit already going from 1MB (my previous default) to 2MB,
but anything more than 16MB seems to not improve anything at all.
But I just need to do more testing, more runs.
Thanks,
Claudio
More information about the libvir-list
mailing list