[PATCH 0/3] vircgroup: Fix virCgroupKillRecursive() wrt nested controllers
See 3/3 for explanation. Michal Prívozník (3): vircgroup: Debug print all arguments of virCgroupKillRecursiveInternal() vircgroupbackend: Extend error messages in VIR_CGROUP_BACKEND_CALL() vircgroup: Fix virCgroupKillRecursive() wrt nested controllers src/util/vircgroup.c | 29 +++++++++++++++++++++++++---- src/util/vircgroupbackend.c | 1 - src/util/vircgroupbackend.h | 8 ++++++-- src/util/vircgrouppriv.h | 1 - src/util/vircgroupv1.c | 7 +------ src/util/vircgroupv2.c | 7 +------ 6 files changed, 33 insertions(+), 20 deletions(-) -- 2.26.3
Currently, only a subset of virCgroupKillRecursiveInternal() arguments is printed into debug logs. Print all of them. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircgroup.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index e7a94c47e4..96280a0a4e 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -2725,8 +2725,8 @@ virCgroupKillRecursiveInternal(virCgroup *group, g_autoptr(DIR) dp = NULL; struct dirent *ent; int direrr; - VIR_DEBUG("group=%p signum=%d pids=%p", - group, signum, pids); + VIR_DEBUG("group=%p signum=%d pids=%p taskFile=%s dormdir=%d", + group, signum, pids, taskFile, dormdir); if (virCgroupPathOfController(group, controller, "", &keypath) < 0) return -1; -- 2.26.3
The VIR_CGROUP_BACKEND_CALL() macro gets a backend for controller and calls corresponding callback in it. If either is NULL then an error message is printed out. However, the error message contains only the intended callback func and not controller or backend found. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircgroupbackend.c | 1 - src/util/vircgroupbackend.h | 8 ++++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/util/vircgroupbackend.c b/src/util/vircgroupbackend.c index 53184cd060..6d6f82cbb1 100644 --- a/src/util/vircgroupbackend.c +++ b/src/util/vircgroupbackend.c @@ -29,7 +29,6 @@ #define VIR_FROM_THIS VIR_FROM_CGROUP -VIR_ENUM_DECL(virCgroupBackend); VIR_ENUM_IMPL(virCgroupBackend, VIR_CGROUP_BACKEND_TYPE_LAST, "cgroup V2", diff --git a/src/util/vircgroupbackend.h b/src/util/vircgroupbackend.h index c7f5708b86..c4d56896b0 100644 --- a/src/util/vircgroupbackend.h +++ b/src/util/vircgroupbackend.h @@ -58,6 +58,8 @@ typedef enum { VIR_CGROUP_BACKEND_TYPE_LAST, } virCgroupBackendType; +VIR_ENUM_DECL(virCgroupBackend); + typedef bool (*virCgroupAvailableCB)(void); @@ -464,12 +466,14 @@ virCgroupBackendForController(virCgroup *group, virCgroupBackend *backend = virCgroupBackendForController(group, controller); \ if (!backend) { \ virReportError(VIR_ERR_INTERNAL_ERROR, \ - _("failed to get cgroup backend for '%s'"), #func); \ + _("failed to get cgroup backend for '%s' controller '%u'"), \ + #func, controller); \ return ret; \ } \ if (!backend->func) { \ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, \ - _("operation '%s' not supported"), #func); \ + _("operation '%s' not supported for backend '%s'"), \ + #func, virCgroupBackendTypeToString(backend->type)); \ return ret; \ } \ return backend->func(group, ##__VA_ARGS__); \ -- 2.26.3
I've encountered the following bug, but only on Gentoo with systemd and CGroupsV2. I've started an LXC container successfully but destroying it reported the following error: error: Failed to destroy domain 'amd64' error: internal error: failed to get cgroup backend for 'pathOfController' Debugging showed, that CGroup hierarchy is full of surprises: /sys/fs/cgroup/machine.slice/machine-lxc\x2d861\x2damd64.scope/ └── libvirt ├── dev-hugepages.mount ├── dev-mqueue.mount ├── init.scope ├── sys-fs-fuse-connections.mount ├── sys-kernel-config.mount ├── sys-kernel-debug.mount ├── sys-kernel-tracing.mount ├── system.slice │ ├── console-getty.service │ ├── dbus.service │ ├── system-getty.slice │ ├── system-modprobe.slice │ ├── systemd-journald.service │ ├── systemd-logind.service │ └── tmp.mount └── user.slice For comparison, here's the same container on recent Rawhide: /sys/fs/cgroup/machine.slice/machine-lxc\x2d13550\x2damd64.scope/ └── libvirt Anyway, those nested directories should not be a problem, because virCgroupKillRecursiveInternal() removes them recursively, right? Sort of. The function really does remove nested directories, but it assumes that every directory has the same controller as the rest. Just take a look at virCgroupV2KillRecursive() - it gets 'Any' controller (the first one it found in ".scope") and then passes it to virCgroupKillRecursiveInternal(). This assumption is not true though. The controllers found in ".scope" are the following: cpuset cpu io memory pids while "libvirt" has fewer: cpuset cpu io memory Up until now it's not problem, because of how we order controllers internally - "cpu" is the first and thus picking "Any" controller returns just that. But the rest of directories has no controllers, their "cgroup.controllers" is just empty. What fixes the bug is dropping @controller argument from virCgroupKillRecursiveInternal() and letting each iteration work pick its own controller. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircgroup.c | 25 +++++++++++++++++++++++-- src/util/vircgrouppriv.h | 1 - src/util/vircgroupv1.c | 7 +------ src/util/vircgroupv2.c | 7 +------ 4 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 96280a0a4e..37dde2a5ed 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1477,6 +1477,24 @@ virCgroupHasController(virCgroup *cgroup, int controller) } +static int +virCgroupGetAnyController(virCgroup *cgroup) +{ + size_t i; + + for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { + if (!cgroup->backends[i]) + continue; + + return cgroup->backends[i]->getAnyController(cgroup); + } + + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get any controller")); + return -1; +} + + int virCgroupPathOfController(virCgroup *group, unsigned int controller, @@ -2715,11 +2733,11 @@ int virCgroupKillRecursiveInternal(virCgroup *group, int signum, GHashTable *pids, - int controller, const char *taskFile, bool dormdir) { int rc; + int controller; bool killedAny = false; g_autofree char *keypath = NULL; g_autoptr(DIR) dp = NULL; @@ -2728,6 +2746,9 @@ virCgroupKillRecursiveInternal(virCgroup *group, VIR_DEBUG("group=%p signum=%d pids=%p taskFile=%s dormdir=%d", group, signum, pids, taskFile, dormdir); + if ((controller = virCgroupGetAnyController(group)) < 0) + return -1; + if (virCgroupPathOfController(group, controller, "", &keypath) < 0) return -1; @@ -2760,7 +2781,7 @@ virCgroupKillRecursiveInternal(virCgroup *group, return -1; if ((rc = virCgroupKillRecursiveInternal(subgroup, signum, pids, - controller, taskFile, true)) < 0) + taskFile, true)) < 0) return -1; if (rc == 1) killedAny = true; diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h index 00193fb101..caf7ed84db 100644 --- a/src/util/vircgrouppriv.h +++ b/src/util/vircgrouppriv.h @@ -135,6 +135,5 @@ int virCgroupRemoveRecursively(char *grppath); int virCgroupKillRecursiveInternal(virCgroup *group, int signum, GHashTable *pids, - int controller, const char *taskFile, bool dormdir); diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index 2cc7dd386a..8a04bb2e4a 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -812,12 +812,7 @@ virCgroupV1KillRecursive(virCgroup *group, int signum, GHashTable *pids) { - int controller = virCgroupV1GetAnyController(group); - - if (controller < 0) - return -1; - - return virCgroupKillRecursiveInternal(group, signum, pids, controller, + return virCgroupKillRecursiveInternal(group, signum, pids, "tasks", false); } diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index e555217355..8881d3a88a 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -577,12 +577,7 @@ virCgroupV2KillRecursive(virCgroup *group, int signum, GHashTable *pids) { - int controller = virCgroupV2GetAnyController(group); - - if (controller < 0) - return -1; - - return virCgroupKillRecursiveInternal(group, signum, pids, controller, + return virCgroupKillRecursiveInternal(group, signum, pids, "cgroup.threads", false); } -- 2.26.3
On Mon, Apr 19, 2021 at 09:20:15AM +0200, Michal Privoznik wrote:
See 3/3 for explanation.
Michal Prívozník (3): vircgroup: Debug print all arguments of virCgroupKillRecursiveInternal() vircgroupbackend: Extend error messages in VIR_CGROUP_BACKEND_CALL() vircgroup: Fix virCgroupKillRecursive() wrt nested controllers
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (2)
-
Michal Privoznik -
Pavel Hrdina