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@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