[libvirt] [PATCH v4 07/14] qemu_cgroup: Allow /dev/mapper/control for PR

[John Ferlan]

On 04/10/2018 10:58 AM, Michal Privoznik wrote:
> Just like in previous commit, qemu-pr-helper might want to open
> /dev/mapper/control under certain circumstances. Therefore we
> have to allow it in cgroups.

Perhaps the two patches should be combined then...  yes, I know cgroups
is different than namespace, so I understand the separation.

> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_cgroup.c  | 33 ++++++++++++++++++++++++++++++---
>  src/util/virdevmapper.c |  8 +++++++-
>  2 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index d88eb7881f..546a4c8e63 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -114,6 +114,8 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
>  }
>  
>  
> +#define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control"
> +

Almost too bad we didn't have a common place for this in some existing
included .h file.

>  static int
>  qemuSetupImageCgroupInternal(virDomainObjPtr vm,
>                               virStorageSourcePtr src,
> @@ -125,6 +127,10 @@ qemuSetupImageCgroupInternal(virDomainObjPtr vm,
>          return 0;
>      }
>  
> +    if (virStoragePRDefIsManaged(src->pr) &&
> +        qemuSetupImagePathCgroup(vm, DEVICE_MAPPER_CONTROL_PATH, false) < 0)
> +        return -1;
> +

Could the above be done potentially many times?  Could be expensive, no?
 Considering what virDevMapperGetTargets[Impl] does...

>      return qemuSetupImagePathCgroup(vm, src->path, src->readonly || forceReadonly);
>  }
>  
> @@ -142,9 +148,8 @@ qemuTeardownImageCgroup(virDomainObjPtr vm,
>                          virStorageSourcePtr src)
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> -    int perms = VIR_CGROUP_DEVICE_READ |
> -                VIR_CGROUP_DEVICE_WRITE |
> -                VIR_CGROUP_DEVICE_MKNOD;
> +    int perms = VIR_CGROUP_DEVICE_RWM;
> +    size_t i;
>      int ret;
>  
>      if (!virCgroupHasController(priv->cgroup,
> @@ -157,6 +162,28 @@ qemuTeardownImageCgroup(virDomainObjPtr vm,
>          return 0;
>      }
>  
> +    for (i = 0; i < vm->def->ndisks; i++) {
> +        virStorageSourcePtr diskSrc = vm->def->disks[i]->src;
> +
> +        if (src == diskSrc)
> +            continue;
> +
> +        if (virStoragePRDefIsManaged(diskSrc->pr))
> +            break;
> +    }
> +
> +    if (i == vm->def->ndisks) {

If there was only one that's managed and it's @src, then we don't get
here...

> +        VIR_DEBUG("Disabling device mapper control");
> +        ret = virCgroupDenyDevicePath(priv->cgroup,
> +                                      DEVICE_MAPPER_CONTROL_PATH, perms, true);

You go through great lengths to ensure this is only done once converse
to the qemuSetupImageCgroupInternal change...

I guess I would think that if we can determine at Prepare time that NS
and cgroups would need to add /dev/mapper/control and we only want to do
that config once, then we should be able

Yes, of course we then we potentially miss adding for hotplug. Honestly
it seems more of a "global" to qemu_domain rather than "local" to a
particular disk source even though it's needed by the local disk source,
is it really something the disk source itself (or it's private data
structure) should be managing?

> +        virDomainAuditCgroupPath(vm, priv->cgroup, "deny",
> +                                 DEVICE_MAPPER_CONTROL_PATH,
> +                                 virCgroupGetDevicePermsString(perms), ret);
> +        if (ret < 0)
> +            return ret;
> +    }
> +
> +
>      VIR_DEBUG("Deny path %s", src->path);
>  
>      ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true);
> diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
> index d2c25af003..ef4b1e480a 100644
> --- a/src/util/virdevmapper.c
> +++ b/src/util/virdevmapper.c
> @@ -101,8 +101,14 @@ virDevMapperGetTargetsImpl(const char *path,
>  
>      dm_task_no_open_count(dmt);
>  
> -    if (!dm_task_run(dmt))
> +    if (!dm_task_run(dmt)) {
> +        if (errno == ENXIO) {
> +            /* In some cases devmapper realizes this late device
> +             * is not managed by it. */
> +            ret = 0;
> +        }

Should this be separated? Or is it related to /dev/mapper/control?

John

>          goto cleanup;
> +    }
>  
>      if (!dm_task_get_info(dmt, &info))
>          goto cleanup;
>