On 04/16/2018 10:57 AM, Michal Privoznik wrote:
> On 04/14/2018 04:56 PM, John Ferlan wrote:
>>
>>
>> On 04/10/2018 10:58 AM, Michal Privoznik wrote:
>>> Before we exec() qemu we have to spawn pr-helper processes for
>>> all managed reservations (well, technically there can only one).
>>
>> Don't mince words - what have to spawn the qemu-pr-helper process for
>> all managed persistent reservations.
>>
>>> The only caveat there is that we should place the process into
>>> the same namespace and cgroup as qemu (so that it shares the same
>>> view of the system). But we can do that only after we've forked.
>>> That means calling the setup function between fork() and exec().
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>>> ---
>>> src/qemu/qemu_process.c | 224 ++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 224 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>>> index f02114c693..982c989a0a 100644
>>> --- a/src/qemu/qemu_process.c
>>> +++ b/src/qemu/qemu_process.c
>>> @@ -2556,6 +2556,223 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver,
>>> ret = 0;
>>> cleanup:
>>> virObjectUnref(caps);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +
>>> +static char *
>>> +qemuProcessBuildPRHelperPidfilePath(virDomainObjPtr vm,
>>> + const char *prdAlias)
>>> +{
>>> + qemuDomainObjPrivatePtr priv = vm->privateData;
>>> +
>>> + return virPidFileBuildPath(priv->libDir, prdAlias);
>>> +}
>>> +
>>> +
>>> +static void
>>> +qemuProcessKillPRDaemon(virDomainObjPtr vm)
>>> +{
>>> + virErrorPtr orig_err;
>>> + char *prAlias;
>>> + char *prPidfile;
>>
>> I would hope that we'd be able to figure out in some way that we
>> actually started this for the the domain.
>>
>>> +
>>> + if (!(prAlias = qemuDomainGetManagedPRAlias())) {
>>> + VIR_WARN("Unable to get default PR manager alias");
>>> + return;
>>> + }
>>> +
>>> + if (!(prPidfile = qemuProcessBuildPRHelperPidfilePath(vm, prAlias))) {
>>
>> Since we know that we're using the managed one, then we should be able
>> to pass/formulate the "static" prAlias here rather than possibly failing
>> because we don't have enough memory above and then need to free it
>> immediately afterwards.
>
> Yeah, well if there's not enough memory to allocate 11 bytes there's
> probably not enough memory to do any stuff done below, i.e.
> qemuProcessBuildPRHelperPidfilePath(), or any syscall done in
> virPidFileForceCleanupPath().
>
True, if we're lacking memory not much is going to happen.
>>
>>> + VIR_WARN("Unable to construct pr-helper pidfile path");
>>> + VIR_FREE(prAlias);
>>> + return;
>>> + }
>>> + VIR_FREE(prAlias);
>>> +
>>> + virErrorPreserveLast(&orig_err);
>>> + if (virPidFileForceCleanupPath(prPidfile) < 0) {
>>> + VIR_WARN("Unable to kill pr-helper process");
>>> + } else if (unlink(prPidfile) < 0 &&
>>> + errno != ENOENT) {
>>> + virReportSystemError(errno,
>>> + _("Unable to remove stale pidfile %s"),
>>> + prPidfile);
>>> + }
>>> + virErrorRestore(&orig_err);
>>> +
>>> + VIR_FREE(prPidfile);
>>> +}
>>> +
>
>>> +static int
>>> +qemuProcessMaybeStartPRDaemon(virDomainObjPtr vm)
>>> +{
>>> + size_t i;
>>> + int ret = -1;
>>> + qemuDomainDiskPRDPtr prd = NULL;
>>> +
>>> + for (i = 0; i < vm->def->ndisks; i++) {
>>> + const virDomainDiskDef *disk = vm->def->disks[i];
>>> + qemuDomainStorageSourcePrivatePtr srcPriv;
>>> +
>>> + if (!virStoragePRDefIsManaged(disk->src->pr))
>>> + continue;
>>> +
>>> + srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
>>> + prd = srcPriv->prd;
>>> + break;
>>> + }
>>
>> Now that we know we needed to start one, then once we're successful we
>> should be able to store that we did start it... and that could be added
>> to some sort of private structure. This is where I could see a private
>> data being used.
>
> Well, we can have that when qemu finally implements events. Then we can
> keep this internal state in sync with reality.
>
The managed pr-helper is domain level based on some storage sources
needing it... That's more what I was referring to. If the domain private
kept track of it being started (no matter how it did so), then as we
found/added/hotplugged a storage source that needed it, then we wouldn't
have to keep running the entire ndisks and checking whether something
may have already added it or not. I dunno, it's I guess it's just
implementation details and a different way of thinking about how to
relate this bolted on piece of software.
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
