[PATCH] qemu: virtiofs: Stop virtiofsd when the qemu guest fails to start
Michal Privoznik
mprivozn at redhat.com
Tue Nov 10 17:45:34 UTC 2020
On 11/10/20 6:25 PM, Masayoshi Mizuma wrote:
> On Tue, Nov 10, 2020 at 10:10:16AM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 11/10/20 2:04 AM, Masayoshi Mizuma wrote:
>>> From: Masayoshi Mizuma <m.mizuma at jp.fujitsu.com>
>>>
>>> A qemu guest which has virtiofs config fails to start if the previous
>>> starting failed because of invalid option or something.
>>> For example of the reproduction:
>>>
>>> # virsh start guest
>>> error: Failed to start domain guest
>>> error: internal error: process exited while connecting to monitor: qemu-system-x86_64: -foo: invalid option
>>>
>>> ... fix the option ...
>>>
>>> # virsh start guest
>>> error: Failed to start domain guest
>>> error: Cannot open log file: '/var/log/libvirt/qemu/guest-fs0-virtiofsd.log': Device or resource busy
>>> #
>>>
>>> That's because the virtiofsd which was executed on the former staring remains
>>
>>
>> s/staring/start ?
>>
>>
>>> and virtlogd keeps to opening the log file.
>>>
>>> Stop virtiofsd when the qemu guest fails to start.
>>>
>>> Signed-off-by: Masayoshi Mizuma <m.mizuma at jp.fujitsu.com>
>>> ---
>>> src/qemu/qemu_domain.h | 1 +
>>> src/qemu/qemu_virtiofs.c | 5 +++++
>>> 2 files changed, 6 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>>> index ca041e207b..7d47c030bd 100644
>>> --- a/src/qemu/qemu_domain.h
>>> +++ b/src/qemu/qemu_domain.h
>>> @@ -426,6 +426,7 @@ struct _qemuDomainFSPrivate {
>>> virObject parent;
>>> char *vhostuser_fs_sock;
>>> + pid_t virtiofsd_pid;
>>> };
>>> diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
>>> index 2e239cad66..8684219915 100644
>>> --- a/src/qemu/qemu_virtiofs.c
>>> +++ b/src/qemu/qemu_virtiofs.c
>>> @@ -250,6 +250,7 @@ qemuVirtioFSStart(virLogManagerPtr logManager,
>>> }
>>> QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock = g_steal_pointer(&socket_path);
>>> + QEMU_DOMAIN_FS_PRIVATE(fs)->virtiofsd_pid = pid;
>>> ret = 0;
>>> cleanup:
>>> @@ -273,6 +274,7 @@ qemuVirtioFSStop(virQEMUDriverPtr driver G_GNUC_UNUSED,
>>> {
>>> g_autofree char *pidfile = NULL;
>>> virErrorPtr orig_err;
>>> + pid_t pid = QEMU_DOMAIN_FS_PRIVATE(fs)->virtiofsd_pid;
>>> virErrorPreserveLast(&orig_err);
>>> @@ -286,6 +288,9 @@ qemuVirtioFSStop(virQEMUDriverPtr driver G_GNUC_UNUSED,
>>> unlink(QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock);
>>> }
>>> + if (virProcessKill(pid, 0) == 0)
>>> + virProcessKillPainfully(pid, true);
>>> +
>>
>>
>> If you're adamant into killing 'pid' I suggest to just verify "if (pid)" and
>> then execute virProcessKillPainfully(), like is being done in virPidFileForceCleanupPath()
>> that is called right before.
>>
>> Speaking of which, isn't virPidFileForceCleanupPath() responsible of killing
>> virtiofsd? If that's the case, the function is failing to do so in the scenario
>> you described, but it will succeed in all other cases. The result is that you'll
>> end up trying to kill the process twice.
>>
>> Making a guess about what might be happening, if 'pidfile' does not exist in your error
>> scenario then virPidFileForceCleanupPath() is returning 0 but is doing nothing. This is
>> the case where your code might come in and kill the pid manually.
>
> Thank you pointing it out! Your guess is correct. The pid file was removed by
> virFileDeleteTree(priv->libDir) in qemuProcessStop(), so virtiofsd isn't killed
> by virPidFileForceCleanupPath().
>
> I think we can fix the error to move virFileDeleteTree(priv->libDir) after
> calling qemuExtDevicesStop(driver, vm).
> Does the following make sense?
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 0a36b49c85..fc6eb5ad13 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -7638,7 +7638,6 @@ void qemuProcessStop(virQEMUDriverPtr driver,
> /* Do this before we delete the tree and remove pidfile. */
> qemuProcessKillManagedPRDaemon(vm);
>
> - virFileDeleteTree(priv->libDir);
> virFileDeleteTree(priv->channelTargetDir);
>
> ignore_value(virDomainChrDefForeach(vm->def,
> @@ -7655,6 +7654,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
> qemuDomainCleanupRun(driver, vm);
>
> qemuExtDevicesStop(driver, vm);
> + virFileDeleteTree(priv->libDir);
>
> qemuDBusStop(driver, vm);
> --
>
This looks better, but how about moving qemuExtDevicesStop() right below
qemuProcessKillManagedPRDaemon()? Both, pr helper and external devices
are supplementary processes and thus alike.
Michal
More information about the libvir-list
mailing list