[PATCH] qemu: virtiofs: Stop virtiofsd when the qemu guest fails to start

[Ján Tomko]

On a Tuesday in 2020, 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().
>

Oops. Thanks for tracking that down!

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20201112/82f994b4/attachment-0001.sig>