Re: [libvirt] [PATCH v4 10/14] qemu: Start PR daemon on domain startup
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
>>> ---
>>> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 10/14] qemu: Start PR daemon on domain startup
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
>> ---
>> 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().
>
>> +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.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 10/14] qemu: Start PR daemon on domain startup
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
> ---
> 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.
> +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
> +qemuProcessStartPRDaemonHook(void *opaque)
> +{
> +virDomainObjPtr vm = opaque;
> +size_t i, nfds = 0;
> +int *fds = NULL;
> +int ret = -1;
> +
> +if (virProcessGetNamespaces(vm->pid, &nfds, &fds) < 0)
> +return ret;
> +
> +if (nfds > 0 &&
> +virProcessSetNamespaces(nfds, fds) < 0)
> +goto cleanup;
> +
> +ret = 0;
> + cleanup:
> +for (i = 0; i < nfds; i++)
> +VIR_FORCE_CLOSE(fds[i]);
> +VIR_FREE(fds);
> +return ret;
> +}
> +
> +
> +static int
> +qemuProcessStartPRDaemon(virDomainObjPtr vm,
> + qemuDomainDiskPRDPtr prd)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +virQEMUDriverPtr driver = priv->driver;
> +virQEMUDriverConfigPtr cfg;
> +int errfd = -1;
> +char *pidfile = NULL;
> +int pidfd = -1;
> +pid_t cpid = -1;
> +virCommandPtr cmd = NULL;
> +virTimeBackOffVar timebackoff;
> +const unsigned long long timeout = 50; /* ms */
> +int ret = -1;
> +
> +cfg = virQEMUDriverGetConfig(driver);
> +
> +if (!virFileIsExecutable(cfg->prHelperName)) {
> +virReportSystemError(errno, _("'%s' is not a suitable pr helper"),
> + cfg->prHelperName);
> +goto cleanup;
> +}
> +
> +if (!(pidfile = qemuProcessBuildPRHelperPidfilePath(vm, prd->alias)))
> +goto cleanup;
> +
> +/* Just try to acquire. Dummy pid will be replaced later */
> +if ((pidfd = virPidFileAcquirePath(pidfile, false, -1)) < 0)
> +goto cleanup;
> +
> +/* Remove stale socket */
> +if (unlink(prd->path) < 0 &&
> +errno != ENOENT) {
> +virReportSystemError(errno,
> + _("Unable to remove stale socket path: %s"),
> + prd->path);
> +goto cleanup;
> +}
> +
> +if (!(cmd = virCommandNewArgList(cfg->prHelperName,
> + "-k", prd->path,
> + "-f", pidfile,
> + NULL)))
> +goto cleanup;
> +
> +virCommandDaemonize(cmd);
> +/* We want our virCommand to write child PID into the pidfile
> + * so that we can read it even before exec(). */
> +virCommandSetPidFile(cm
