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

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(a)jp.fujitsu.com&gt; >> >> 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(a)jp.fujitsu.com&gt; >> --- >> 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); --