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@xxxxxxxxxx>
>>>> ---
>>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
