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

[Michal Privoznik]

On 04/17/2018 02:25 PM, John Ferlan wrote:
> 
> 
> On 04/16/2018 10:56 AM, Michal Privoznik wrote:
>> On 04/14/2018 02:55 PM, John Ferlan wrote:
>>>
>>>
>>> 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.
>>
>> Yeah, I'd like to keep them separated. Even though they allow access to
>> the same path they do that in different areas of the code.
>>
> 
> Separate is fine...
> 
>>>
>>>>
>>>> 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.
>>
>> Yeah :(
>>
>>>
>>>>  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...
>>
>> It shouldn't be expensive. At the very first call of
>> virDevMapperGetTargetsImpl() we get ENXIO (this the change below) and
>> thus there only very small time penalty added.
>>
> 
> Suffice to say it wasn't obvious to me WTF virDevMapperGetTargets[Impl]
> causes the ENXIO.  That logic is not entirely self documenting.
> 
>>>
>>>>      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...
>>>
>>
>> How come? Say there are 3 disks for a domain and the first one is
>> managed: disks = {A, B, C} (where A.managed = yes}.
>>
> 
> So it made sense when I wrote the comment... let's see.
> 
> 
>> So the loop goes like this:
>>
>>   qemuTeardownImageCgroup(A):
>>     i = 0;
>>     if (A.src == disks[0].src) //true
>>       continue;
>>
>>     i = 1;
>>     if (A.src == disks[1].src) //false
>>       continue;
>>
>>     if (isManaged(disks[1]) //false
>>       break;
>>
>>     i = 2;
>>     if (A.src == disks[2].src) //false
>>       continue;
>>
>>     if (isManaged(disks[2]) //false
>>       break;
>>
>>     // Here, i == ndisks == 3;
>>
>> Or am I missing something?
>>
> 
> OK - with the example I see...  /me wonders at this point whether it'd
> be clearer to keep a list of pr-helper managed LUN's and "Add" and
> "Remove" where the 1st add and the last remove trigger the PR start/stop
> rather than looping through ndisks.

Well, that would possibly allocate much more than 11 bytes ;-)

What we can do, however, when qemu introduces the even support is to
track if pr-helper is running (say, we'd have a bool under
vm->privateData) and doing the following:

if (vm->privateData->prRunning == false && virStoragePRDefIsManaged())
  startPRHelper();

Hm.. in fact we can do something like that now even though it will not
reflect reality 100%. But it's not going to be any worse that what we
have now. Nor better though.

If I will have to send yet another version, I might rework it. But as I
say, it doesn't make any difference now.

Michal