[libvirt PATCHv4 12/15] qemu: put virtiofsd in the emulator cgroup

[Peter Krempa]

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 at 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 at redhat.com>

We can worry about the cgroup congestion later since that's
pre-existing.