1. Patchew
  2. Libvirt
  3. View series

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

Masayoshi Mizuma posted 1 patch 3 years, 5 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20201110050400.29742-1-msys.mizuma@gmail.com
src/qemu/qemu_domain.h   | 1 +
src/qemu/qemu_virtiofs.c | 5 +++++
2 files changed, 6 insertions(+)
[PATCH] qemu: virtiofs: Stop virtiofsd when the qemu guest fails to start
Posted by Masayoshi Mizuma 3 years, 5 months ago 
From: Masayoshi Mizuma <m.mizuma@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
and virtlogd keeps to opening the log file.

Stop virtiofsd when the qemu guest fails to start.

Signed-off-by: Masayoshi Mizuma <m.mizuma@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);
+
  cleanup:
     virErrorRestore(&orig_err);
 }
-- 
2.27.0
Re: [PATCH] qemu: virtiofs: Stop virtiofsd when the qemu guest fails to start
Posted by Daniel Henrique Barboza 3 years, 5 months ago 

On 11/10/20 2:04 AM, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma <m.mizuma@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@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.



Thanks,

DHB


>    cleanup:
>       virErrorRestore(&orig_err);
>   }
>
Re: [PATCH] qemu: virtiofs: Stop virtiofsd when the qemu guest fails to start
Posted by Masayoshi Mizuma 3 years, 5 months ago 
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@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@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);
--

Thanks,
Masa
Re: [PATCH] qemu: virtiofs: Stop virtiofsd when the qemu guest fails to start
Posted by Michal Privoznik 3 years, 5 months ago 
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@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@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
Re: [PATCH] qemu: virtiofs: Stop virtiofsd when the qemu guest fails to start
Posted by Masayoshi Mizuma 3 years, 5 months ago 
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@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@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.

Got it, thanks!
I'll send the patch as v2.

Thanks!
Masa
Re: [PATCH] qemu: virtiofs: Stop virtiofsd when the qemu guest fails to start
Posted by Ján Tomko 3 years, 5 months ago 
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@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@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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.