On Thu, Feb 20, 2020 at 15:32:49 +0100, Ján Tomko wrote:
Wire up the code to put virtiofsd in the emulator cgroup on domain startup.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_extdevice.c | 15 +++++++++++++++ src/qemu/qemu_virtiofs.c | 28 ++++++++++++++++++++++++++++ src/qemu/qemu_virtiofs.h | 6 ++++++ 3 files changed, 49 insertions(+)
[...]
@@ -271,5 +278,13 @@ qemuExtDevicesSetupCgroup(virQEMUDriverPtr driver, qemuExtTPMSetupCgroup(driver, def, cgroup) < 0) return -1;
+ for (i = 0; i < def->nfss; i++) { + virDomainFSDefPtr fs = def->fss[i]; + + if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS && + qemuVirtioFSSetupCgroup(driver, def, fs, cgroup) < 0)
While this is consistent with what we do with other cases of 'externa' devices. I'm worried though that that stashing everything into a single cgroup will not scale.
+ return -1; + } + return 0; } diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c index 600a3644bd..9e354a30c6 100644 --- a/src/qemu/qemu_virtiofs.c +++ b/src/qemu/qemu_virtiofs.c @@ -298,3 +298,31 @@ qemuVirtioFSStop(virQEMUDriverPtr driver, cleanup: virErrorRestore(&orig_err); } + + +int +qemuVirtioFSSetupCgroup(virQEMUDriverPtr driver, + virDomainDefPtr def, + virDomainFSDefPtr fs, + virCgroupPtr cgroup) +{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + g_autofree char *pidfile = NULL; + pid_t pid = -1; + int rc; + + if (!(pidfile = qemuVirtioFSCreatePidFilename(cfg, def, fs->info.alias))) + return -1; + + rc = virPidFileReadPathIfAlive(pidfile, &pid, NULL); + if (rc < 0 || pid == (pid_t) -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("virtiofsd died unexpectedly")); + return -1; + }
Why don't we store the pid in the private data? Seems to be a waste of cycles to read it here.
+ + if (virCgroupAddProcess(cgroup, pid) < 0) + return -1; + + return 0; +}
Please consider storing the pid somewhere rather than looking it up. With that: Reviewed-by: Peter Krempa <pkrempa@redhat.com> We can worry about the cgroup congestion later since that's pre-existing.