On Tue, Nov 10, 2020 at 06:45:34PM +0100, Michal Privoznik wrote:
> 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@xxxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxxx>
> > > > ---
> > > > 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.
Got it, thanks!
I'll send the patch as v2.
Thanks!
Masa
