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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
We can worry about the cgroup congestion later since that's
pre-existing.
