March 2022 - Devel - libvirt List Archives

[PATCH v2 00/19] Refactoring conf to use XPath
by Kristina Hanicova 26 Mar '22

26 Mar '22

This is v2 from https://listman.redhat.com/archives/libvir-list/2021-April/msg00617.html Changes since v1: - rebase to the lastest git master This series reworks the outdated way of parsing XML to parsing by XPath, which is more obvious and saves a few lines of code. Kristina Hanicova (19): conf: Propagate xmlXPathContextPtr into virDomainHostdevSubsysUSBDefParseXML() Refactoring virDomainHostdevSubsysUSBDefParseXML() to use XPath conf: Propagate xmlXPathContextPtr into virDomainBlkioDeviceParseXML() Refactoring virDomainBlkioDeviceParseXML() to use XPath conf: Propagate xmlXPathContextPtr into virDomainHostdevSubsysPCIDefParseXML() Refactoring virDomainHostdevSubsysPCIDefParseXML() to use XPath conf: Propagate xmlXPathContextPtr into virDomainLeaseDefParseXML() Refactoring virDomainLeaseDefParseXML() to use XPath Refactoring virDomainFSDefParseXML() to use XPath Refactoring virDomainNetDefParseXML() to use XPath conf: Propagate xmlXPathContextPtr into virDomainChrDefParseTargetXML() Refactoring virDomainChrDefParseTargetXML() to use XPath Refactoring virDomainChrSourceDefParseXML() to use XPath Refactoring virDomainChrDefParseXML() to use XPath Refactoring virDomainSmartcardDefParseXML() to use XPath Refactoring virDomainGraphicsDefParseXMLSpice() to use XPath conf: Propagate xmlXPathContextPtr into virDomainVideoDriverDefParseXML() Refactoring virDomainVideoDriverDefParseXML() to use XPath Refactoring virDomainVideoDefParseXML() to use XPath src/conf/domain_conf.c | 1641 +++++++++++++++++----------------------- 1 file changed, 707 insertions(+), 934 deletions(-) -- 2.30.2

3 26

[PATCH v2] meson: do not look for libparted if not necessary
by Paolo Bonzini 26 Mar '22

26 Mar '22

libparted_dep is not used if -Dstorage_disk=disabled. Do not bother looking for it if the disk storage backend was not requested. Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com> --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meson.build b/meson.build index eb13c7efa4..27c21d1d67 100644 --- a/meson.build +++ b/meson.build @@ -1013,7 +1013,7 @@ else endif libparted_version = '1.8.0' -libparted_dep = dependency('libparted', version: '>=' + libparted_version, required: false) +libparted_dep = dependency('libparted', version: '>=' + libparted_version, required: get_option('storage_disk')) libpcap_version = '1.5.0' if not get_option('libpcap').disabled() -- 2.35.1

2 1

[PATCH] meson: do not look for librbd/librados if not requested
by Paolo Bonzini 26 Mar '22

26 Mar '22

rbd_dep is not used if -Dstorage_rbd=disabled. Do not bother looking for the libraries that compose it if the rbd storage backend was not requested. Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com> --- meson.build | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/meson.build b/meson.build index 2c1d3bc64d..eb13c7efa4 100644 --- a/meson.build +++ b/meson.build @@ -1127,8 +1127,8 @@ parallels_sdk_dep = dependency('parallels-sdk', version: '>=' + parallels_sdk_ve pciaccess_version = '0.10.0' pciaccess_dep = dependency('pciaccess', version: '>=' + pciaccess_version, required: get_option('pciaccess')) -rbd_dep = cc.find_library('rbd', required: false) -rados_dep = cc.find_library('rados', required: false) +rbd_dep = cc.find_library('rbd', required: get_option('storage_rbd')) +rados_dep = cc.find_library('rados', required: get_option('storage_rbd')) if rbd_dep.found() and not cc.has_function('rbd_get_features', dependencies: rbd_dep) rbd_dep = dependency('', required: false) endif -- 2.35.1

2 1

[PATCH] meson: do not look for libdevmapper/libparted if not requested
by Paolo Bonzini 26 Mar '22

26 Mar '22

devmapper_dep and libparted_dep are not used if -Dstorage_disk=disabled. Do not bother looking for these libraries if the disk storage backend was not requested. Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com> --- meson.build | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/meson.build b/meson.build index eb13c7efa4..9d1f90fc80 100644 --- a/meson.build +++ b/meson.build @@ -933,7 +933,7 @@ if curl_dep.found() endif devmapper_version = '1.0.0' -devmapper_dep = dependency('devmapper', version: '>=' + devmapper_version, required: false) +devmapper_dep = dependency('devmapper', version: '>=' + devmapper_version, required: get_option('storage_disk')) if devmapper_dep.found() conf.set('WITH_DEVMAPPER', 1) endif @@ -1013,7 +1013,7 @@ else endif libparted_version = '1.8.0' -libparted_dep = dependency('libparted', version: '>=' + libparted_version, required: false) +libparted_dep = dependency('libparted', version: '>=' + libparted_version, required: get_option('storage_disk')) libpcap_version = '1.5.0' if not get_option('libpcap').disabled() -- 2.35.1

2 1

[PATCH] tests: add dependencies to meson declaration
by Paolo Bonzini 26 Mar '22

26 Mar '22

Make sure that all tests are run after the helpers and mocks are (re)built. This enables for example using "meson test" as the command line passed to "git bisect run". Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com> --- tests/meson.build | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/meson.build b/tests/meson.build index 8f92f2033f..fd78d8a2fd 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -191,8 +191,9 @@ test_file_wrapper_lib = static_library( dependencies: [ tests_dep ], ) +tests_deps = [] foreach mock : mock_libs - shared_library( + tests_deps += shared_library( mock['name'], mock.get('sources', [ '@0@.c'.format(mock['name']) ]), override_options: [ @@ -215,7 +216,7 @@ endforeach # Must not link to any libvirt modules - libc only otherwise external # libraries might unexpectedly leak file descriptors into commandhelper # invalidating the test logic assumptions. -executable( +tests_deps += executable( 'commandhelper', [ 'commandhelper.c' ], dependencies: [ @@ -227,7 +228,7 @@ executable( ) # This is a fake SSH we use from virnetsockettest -executable( +tests_deps += executable( 'ssh', [ 'ssh.c' ], dependencies: [ @@ -592,7 +593,7 @@ foreach data : tests # default meson timeout timeout = 30 endif - test(data['name'], test_bin, env: tests_env, timeout: timeout) + test(data['name'], test_bin, env: tests_env, timeout: timeout, depends: tests_deps) endforeach -- 2.35.1

2 1

[libvirt RFCv3] virfile: set pipe size in virFileWrapperFdNew to improve throughput
by Claudio Fontana 25 Mar '22

25 Mar '22

currently the only user of virFileWrapperFdNew is the qemu driver; virsh save is very slow with a default pipe size. This change improves throughput by ~400% on fast nvme or ramdisk. Best value currently measured is 1MB, which happens to be also the kernel default for the pipe-max-size. Signed-off-by: Claudio Fontana <cfontana(a)suse.de> --- see v2 at https://listman.redhat.com/archives/libvir-list/2022-March/229423.html Changes v2 -> v3: * removed reading of max-pipe-size from procfs, instead make multiple attempts on EPERM with smaller sizes. In the regular case, this should succeed on the first try. (Daniel) Changes v1 -> v2: * removed VIR_FILE_WRAPPER_BIG_PIPE, made the new pipe resizing unconditional (Michal) * moved code to separate functions (Michal) * removed ternary op, disliked in libvirt (Michal) * added #ifdef __linux__ (Ani Sinha) * try smallest value between currently best measured value (1MB) and the pipe-max-size setting. If pipe-max-size cannot be read, try kernel default max (1MB). (Daniel) src/util/virfile.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/util/virfile.c b/src/util/virfile.c index a04f888e06..876b865974 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -201,6 +201,51 @@ struct _virFileWrapperFd { }; #ifndef WIN32 + +#ifdef __linux__ + +/** + * virFileWrapperSetPipeSize: + * @fd: the fd of the pipe + * + * Set best pipe size on the passed file descriptor for bulk transfers of data. + * + * default pipe size (usually 64K) is generally not suited for large transfers + * to fast devices. A value of 1MB has been measured to improve virsh save + * by 400% in ideal conditions. We retry multiple times with smaller sizes + * on EPERM to account for possible small values of /proc/sys/fs/pipe-max-size. + * + * Return value is 0 on success, -1 and errno set on error. + * OS note: only for linux, on other OS this is a no-op. + */ +static int +virFileWrapperSetPipeSize(int fd) +{ + int sz; + + for (sz = 1024 * 1024; sz >= 64 * 1024; sz /= 2) { + int rv = fcntl(fd, F_SETPIPE_SZ, sz); + if (rv < 0 && errno == EPERM) { + continue; /* retry with half the size */ + } + if (rv < 0) { + break; + } + VIR_INFO("fd %d pipe size adjusted to %d", fd, sz); + return 0; + } + VIR_WARN("failed to set pipe size to %d (errno=%d)", sz, errno); + return -1; +} + +#else /* !__linux__ */ +static int virFileWrapperSetPipeSize(int fd) +{ + return 0; +} +#endif /* !__linux__ */ + + /** * virFileWrapperFdNew: * @fd: pointer to fd to wrap @@ -282,6 +327,10 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags) ret->cmd = virCommandNewArgList(iohelper_path, name, NULL); + if (virFileWrapperSetPipeSize(pipefd[!output]) < 0) { + virReportError(VIR_ERR_SYSTEM_ERROR, "%s", _("unable to set pipe size, data transfer might be slow")); + } + if (output) { virCommandSetInputFD(ret->cmd, pipefd[0]); virCommandSetOutputFD(ret->cmd, fd); -- 2.35.1

2 5

[PATCH 0/5] Generalize job enums across all hypervisors
by Kristina Hanicova 25 Mar '22

25 Mar '22

2 7

[libvirt RFC v2] virfile: set pipe size in virFileWrapperFdNew to improve throughput
by Claudio Fontana 25 Mar '22

25 Mar '22

virsh save is very slow with a default pipe size, so set a larger one. This change improves throughput by ~400% on fast nvme or ramdisk, for the current only user of virFileWrapperFdNew: the qemu driver. Best value currently measured is 1MB, which happens to be also the kernel default for the pipe-max-size. We do not try to use a pipe buffer larger than what the setting of /proc/sys/fs/pipe-max-size currently allows. Signed-off-by: Claudio Fontana <cfontana(a)suse.de> --- src/util/virfile.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) see v1 at https://listman.redhat.com/archives/libvir-list/2022-March/229252.html Changes v1 -> v2: * removed VIR_FILE_WRAPPER_BIG_PIPE, made the new pipe resizing unconditional (Michal) * moved code to separate functions (Michal) * removed ternary op, disliked in libvirt (Michal) * added #ifdef __linux__ (Ani Sinha) * try smallest value between currently best measured value (1MB) and the pipe-max-size setting. If pipe-max-size cannot be read, try kernel default max (1MB). (Daniel) diff --git a/src/util/virfile.c b/src/util/virfile.c index a04f888e06..13bdd42c68 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -201,6 +201,71 @@ struct _virFileWrapperFd { }; #ifndef WIN32 + +#ifdef __linux__ +/** + * virFileWrapperGetBestPipeSize: + * + * get the best pipe size to use with virFileWrapper. + * + * We first check the maximum we are allowed by the system pipe-max-size, + * and then use the minimum between that and our tested best value. + * This is because a request beyond pipe-max-size may fail with EPERM. + * If we are unable to read pipe-max-size, use the kernel default (1MB). + * + * Return value is the pipe size to use. + */ + +static int virFileWrapperGetBestPipeSize(void) +{ + const char path[] = "/proc/sys/fs/pipe-max-size"; + int best_sz = 1024 * 1024; /* good virsh save results with this size */ + int max_sz; + + if (virFileReadValueInt(&max_sz, path) < 0) { + max_sz = 1024 * 1024; /* this is the kernel default pipe-max-size */ + VIR_WARN("failed to read %s, trying default %d", path, max_sz); + } else if (max_sz > best_sz) { + max_sz = best_sz; + } + return max_sz; +} + +/** + * virFileWrapperSetPipeSize: + * @fd: the fd of the pipe + * + * Set best pipe size on the passed file descriptor for bulk transfers of data. + * + * default pipe size (usually 64K) is generally not suited for large transfers + * to fast devices. This has been measured to improve virsh save by 400% + * in ideal conditions. + * + * Return value is 0 on success, -1 and errno set on error. + * OS note: only for linux, on other OS this is a no-op. + */ +static int +virFileWrapperSetPipeSize(int fd) +{ + int pipe_sz = virFileWrapperGetBestPipeSize(); + int rv = fcntl(fd, F_SETPIPE_SZ, pipe_sz); + + if (rv < 0) { + VIR_ERROR(_("failed to set pipe size to %d (errno=%d)"), pipe_sz, errno); + return -1; + } + VIR_INFO("fd %d pipe size adjusted to %d", fd, rv); + return 0; +} + +#else /* !__linux__ */ +static int virFileWrapperSetPipeSize(int fd) +{ + return 0; +} +#endif /* !__linux__ */ + + /** * virFileWrapperFdNew: * @fd: pointer to fd to wrap @@ -282,6 +347,10 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags) ret->cmd = virCommandNewArgList(iohelper_path, name, NULL); + if (virFileWrapperSetPipeSize(pipefd[!output]) < 0) { + virReportError(VIR_ERR_SYSTEM_ERROR, "%s", _("unable to set pipe size, data transfer might be slow")); + } + if (output) { virCommandSetInputFD(ret->cmd, pipefd[0]); virCommandSetOutputFD(ret->cmd, fd); -- 2.35.1

2 3

[PATCH] error message in virDomainDiskDefIotuneParse shifted to virDomainDefPostParseCheck
by Jamm02 25 Mar '22

25 Mar '22

From: Moteen Shah <moteenshah.02(a)gmail.com> domain_conf.c: all the option collision total and... error messages in virDomainDiskDefIotuneParse shifted to new function virDomainDefPostParseCheck --- src/conf/domain_conf.c | 86 ++++++++++++++++++++++-------------------- 1 file changed, 45 insertions(+), 41 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 153954a0b0..f2480f37f6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6229,6 +6229,50 @@ virDomainDefPostParseCheckFailure(virDomainDef *def, return 0; } +static int +virDomainDefPostParseCheck(virDomainDiskDef *def) +{ + if ((def->blkdeviotune.total_bytes_sec && + def->blkdeviotune.read_bytes_sec) || + (def->blkdeviotune.total_bytes_sec && + def->blkdeviotune.write_bytes_sec)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("total and read/write bytes_sec " + "cannot be set at the same time")); + return -1; + } + + if ((def->blkdeviotune.total_iops_sec && + def->blkdeviotune.read_iops_sec) || + (def->blkdeviotune.total_iops_sec && + def->blkdeviotune.write_iops_sec)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("total and read/write iops_sec " + "cannot be set at the same time")); + return -1; + } + + if ((def->blkdeviotune.total_bytes_sec_max && + def->blkdeviotune.read_bytes_sec_max) || + (def->blkdeviotune.total_bytes_sec_max && + def->blkdeviotune.write_bytes_sec_max)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("total and read/write bytes_sec_max " + "cannot be set at the same time")); + return -1; + } + + if ((def->blkdeviotune.total_iops_sec_max && + def->blkdeviotune.read_iops_sec_max) || + (def->blkdeviotune.total_iops_sec_max && + def->blkdeviotune.write_iops_sec_max)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("total and read/write iops_sec_max " + "cannot be set at the same time")); + return -1; + } + return 0; +} int virDomainDefPostParse(virDomainDef *def, @@ -8784,47 +8828,7 @@ virDomainDiskDefIotuneParse(virDomainDiskDef *def, def->blkdeviotune.group_name = virXPathString("string(./iotune/group_name)", ctxt); - if ((def->blkdeviotune.total_bytes_sec && - def->blkdeviotune.read_bytes_sec) || - (def->blkdeviotune.total_bytes_sec && - def->blkdeviotune.write_bytes_sec)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("total and read/write bytes_sec " - "cannot be set at the same time")); - return -1; - } - - if ((def->blkdeviotune.total_iops_sec && - def->blkdeviotune.read_iops_sec) || - (def->blkdeviotune.total_iops_sec && - def->blkdeviotune.write_iops_sec)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("total and read/write iops_sec " - "cannot be set at the same time")); - return -1; - } - - if ((def->blkdeviotune.total_bytes_sec_max && - def->blkdeviotune.read_bytes_sec_max) || - (def->blkdeviotune.total_bytes_sec_max && - def->blkdeviotune.write_bytes_sec_max)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("total and read/write bytes_sec_max " - "cannot be set at the same time")); - return -1; - } - - if ((def->blkdeviotune.total_iops_sec_max && - def->blkdeviotune.read_iops_sec_max) || - (def->blkdeviotune.total_iops_sec_max && - def->blkdeviotune.write_iops_sec_max)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("total and read/write iops_sec_max " - "cannot be set at the same time")); - return -1; - } - - return 0; + return virDomainDefPostParseCheck(def); } #undef PARSE_IOTUNE -- 2.35.1

2 1

[PATCH 0/2] qemu: remove else after return / goto
by Kristina Hanicova 25 Mar '22

25 Mar '22

Kristina Hanicova (2): qemu: remove unnecessary else branches after return / goto qemu: remove else branches after return in qemuMigrationSrcPerform() src/qemu/qemu_domainjob.c | 3 +- src/qemu/qemu_migration.c | 59 +++++++++++++++++++-------------------- src/qemu/qemu_process.c | 4 +-- src/qemu/qemu_snapshot.c | 13 ++++----- 4 files changed, 38 insertions(+), 41 deletions(-) -- 2.35.1

2 4