[libvirt] [PATCH 0/2] cgroup: unavailable controller prevents controller disabling
The problem description is covered in patch one. I added patch two as an optional enhancement removing some complexity and improving readability. If this finds common agreement it should simply be squashed into the first patch of this series. Boris Fiuczynski (2): cgroup: unavailable controller prevents controller disabling cgroup: reduce complexity of controller disabling src/util/vircgroup.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) -- 2.5.5
The cgroup controller filtering in virCgroupDetect does not work properly if the following conditions are met: 1) the host system does not have a cgroup controller which libvirt requests (unavailable controller) and 2) libvirt is configured to disable a controller (disabled controller) and 3) the disabled controller is located before the unavailable controller in virCgroupController. As an example: The memory controller is unavailable and the cpuset controller is configured to be disabled. In this scenario trying to start a domain results in the error error: Controller 'cpuset' is not wanted, but 'memory' is co-mounted: Invalid argument This error occurs when virCgroupDetect is called with a valid parent group. The resulting group created by virCgroupCopyMounts holds for cpuset and memory controller empty mount points. The filtering of disabled controllers checks for co-mounts by comparing the mount points. The cpuset controller causes the filtering to occur before the memory controller is marked as to be ignored by modifying the controller mask since it is unavailable. Therefore the co-mount detection logic compares the cpuset and memory controller mount points and since both are empty the memory controller is regarded erroneously as being co-mounted. Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> --- src/util/vircgroup.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index b6affe3..47691e2 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -669,6 +669,8 @@ virCgroupDetect(virCgroupPtr group, controllers &= ~(1 << i); } } else { + if (!group->controllers[i].mountPoint) + continue; /* without controller co-mounting is impossible */ /* Check whether a request to disable a controller * clashes with co-mounting of controllers */ for (j = 0; j < VIR_CGROUP_CONTROLLER_LAST; j++) { -- 2.5.5
On 08.12.2016 14:24, Boris Fiuczynski wrote:
The cgroup controller filtering in virCgroupDetect does not work properly if the following conditions are met: 1) the host system does not have a cgroup controller which libvirt requests (unavailable controller) and 2) libvirt is configured to disable a controller (disabled controller) and 3) the disabled controller is located before the unavailable controller in virCgroupController.
As an example: The memory controller is unavailable and the cpuset controller is configured to be disabled. In this scenario trying to start a domain results in the error error: Controller 'cpuset' is not wanted, but 'memory' is co-mounted: Invalid argument
This error occurs when virCgroupDetect is called with a valid parent group. The resulting group created by virCgroupCopyMounts holds for cpuset and memory controller empty mount points. The filtering of disabled controllers checks for co-mounts by comparing the mount points. The cpuset controller causes the filtering to occur before the memory controller is marked as to be ignored by modifying the controller mask since it is unavailable. Therefore the co-mount detection logic compares the cpuset and memory controller mount points and since both are empty the memory controller is regarded erroneously as being co-mounted.
Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> --- src/util/vircgroup.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index b6affe3..47691e2 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -669,6 +669,8 @@ virCgroupDetect(virCgroupPtr group, controllers &= ~(1 << i); } } else { + if (!group->controllers[i].mountPoint) + continue; /* without controller co-mounting is impossible */ /* Check whether a request to disable a controller * clashes with co-mounting of controllers */ for (j = 0; j < VIR_CGROUP_CONTROLLER_LAST; j++) {
The only small nit here: I'd prefer the comment to be before the continue;. Michal
This patch reduces the complexity of the filtering algorithm in virCgroupDetect by first correcting the controller mask and then checking for potential co-mounts without any correlating controller mask modifications. If you agree that this patch removes complexity and improves readability it could simply be squashed into the first patch of this series. Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> --- src/util/vircgroup.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 47691e2..80ce43c 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -656,11 +656,8 @@ virCgroupDetect(virCgroupPtr group, if (controllers >= 0) { VIR_DEBUG("Filtering controllers %d", controllers); + /* First mark requested but non-existing controllers to be ignored */ for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { - VIR_DEBUG("Controller '%s' wanted=%s, mount='%s'", - virCgroupControllerTypeToString(i), - (1 << i) & controllers ? "yes" : "no", - NULLSTR(group->controllers[i].mountPoint)); if (((1 << i) & controllers)) { /* Remove non-existent controllers */ if (!group->controllers[i].mountPoint) { @@ -668,9 +665,15 @@ virCgroupDetect(virCgroupPtr group, virCgroupControllerTypeToString(i)); controllers &= ~(1 << i); } - } else { - if (!group->controllers[i].mountPoint) - continue; /* without controller co-mounting is impossible */ + } + } + for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { + VIR_DEBUG("Controller '%s' wanted=%s, mount='%s'", + virCgroupControllerTypeToString(i), + (1 << i) & controllers ? "yes" : "no", + NULLSTR(group->controllers[i].mountPoint)); + if (!((1 << i) & controllers) && + group->controllers[i].mountPoint) { /* Check whether a request to disable a controller * clashes with co-mounting of controllers */ for (j = 0; j < VIR_CGROUP_CONTROLLER_LAST; j++) { -- 2.5.5
Seasonal ping... :-) On 12/08/2016 02:24 PM, Boris Fiuczynski wrote:
The problem description is covered in patch one. I added patch two as an optional enhancement removing some complexity and improving readability. If this finds common agreement it should simply be squashed into the first patch of this series.
Boris Fiuczynski (2): cgroup: unavailable controller prevents controller disabling cgroup: reduce complexity of controller disabling
src/util/vircgroup.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 08.12.2016 14:24, Boris Fiuczynski wrote:
The problem description is covered in patch one. I added patch two as an optional enhancement removing some complexity and improving readability. If this finds common agreement it should simply be squashed into the first patch of this series.
Boris Fiuczynski (2): cgroup: unavailable controller prevents controller disabling cgroup: reduce complexity of controller disabling
src/util/vircgroup.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
ACKed and pushed. Michal
On 12/20/2016 11:21 AM, Michal Privoznik wrote:
On 08.12.2016 14:24, Boris Fiuczynski wrote:
The problem description is covered in patch one. I added patch two as an optional enhancement removing some complexity and improving readability. If this finds common agreement it should simply be squashed into the first patch of this series.
Boris Fiuczynski (2): cgroup: unavailable controller prevents controller disabling cgroup: reduce complexity of controller disabling
src/util/vircgroup.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
ACKed and pushed.
Michal
Thanks, Michal! -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (2)
-
Boris Fiuczynski -
Michal Privoznik