[libvirt] [PATCH v4 10/14] qemu: Start PR daemon on domain startup

[John Ferlan]

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 at redhat.com>
>>> ---
>>>  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