[libvirt PATCHv2 0/6] kill whole virtiofsd process group (virtio-fs epopee)
v2 of: https://listman.redhat.com/archives/libvir-list/2021-June/msg00570.html Ján Tomko (6): virProcessKillPainfullyDelay: use 'rc' variable util: Introduce virProcessGroupKill util: introduce virProcessGroupGet util: virPidFileForceCleanupPath: add group argument qemu: virtiofs: kill the whole process group util: fix typo src/libvirt_private.syms | 1 + src/qemu/qemu_process.c | 3 ++- src/qemu/qemu_virtiofs.c | 2 +- src/util/virpidfile.c | 15 ++++++++++-- src/util/virpidfile.h | 2 ++ src/util/virprocess.c | 53 +++++++++++++++++++++++++++++++++++----- src/util/virprocess.h | 5 +++- 7 files changed, 70 insertions(+), 11 deletions(-) -- 2.31.1
Use 'rc' to store the return value of virProcessKill, to separate the value check from the function call. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virprocess.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 67dd599b3e..d58e983b56 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -395,6 +395,8 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay) */ for (i = 0; i < polldelay; i++) { int signum; + int rc; + if (i == 0) { signum = SIGTERM; /* kindly suggest it should exit */ } else if (i == 50 && force) { @@ -413,7 +415,9 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay) signum = 0; /* Just check for existence */ } - if (virProcessKill(pid, signum) < 0) { + rc = virProcessKill(pid, signum); + + if (rc < 0) { if (errno != ESRCH) { virReportSystemError(errno, _("Failed to terminate process %lld with SIG%s"), -- 2.31.1
On 8/3/21 5:13 PM, Ján Tomko wrote:
Use 'rc' to store the return value of virProcessKill, to separate the value check from the function call.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virprocess.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 67dd599b3e..d58e983b56 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -395,6 +395,8 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay) */ for (i = 0; i < polldelay; i++) { int signum; + int rc; + if (i == 0) { signum = SIGTERM; /* kindly suggest it should exit */ } else if (i == 50 && force) { @@ -413,7 +415,9 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay) signum = 0; /* Just check for existence */ }
- if (virProcessKill(pid, signum) < 0) { + rc = virProcessKill(pid, signum); + + if (rc < 0) { if (errno != ESRCH) { virReportSystemError(errno, _("Failed to terminate process %lld with SIG%s"),
This patch alone makes no sense. Squash it to the one where you need to differentiate whether to kill just a single PID or whole group (patch 5/6 which will effectively be 4/6). Michal
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virprocess.c | 17 +++++++++++++++++ src/util/virprocess.h | 1 + 2 files changed, 18 insertions(+) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index d58e983b56..c01c8e267b 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -361,6 +361,23 @@ int virProcessKill(pid_t pid, int sig) } +/* send signal to a process group */ +int virProcessGroupKill(pid_t pid, int sig) +{ + if (pid <= 1) { + errno = ESRCH; + return -1; + } + +#ifdef WIN32 + errno = ENOSYS; + return -1; +#else + return killpg(pid, sig); +#endif +} + + /* * Try to kill the process and verify it has exited * diff --git a/src/util/virprocess.h b/src/util/virprocess.h index d16e8b23bb..0359d7fd9f 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -52,6 +52,7 @@ virProcessWait(pid_t pid, int *exitstatus, bool raw) G_GNUC_WARN_UNUSED_RESULT; int virProcessKill(pid_t pid, int sig); +int virProcessGroupKill(pid_t pid, int sig); int virProcessKillPainfully(pid_t pid, bool force); int virProcessKillPainfullyDelay(pid_t pid, -- 2.31.1
On 8/3/21 5:13 PM, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virprocess.c | 17 +++++++++++++++++ src/util/virprocess.h | 1 + 2 files changed, 18 insertions(+)
Don't forget to add it to src/libvirt_private.syms. Michal
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virprocess.c | 17 +++++++++++++++++ src/util/virprocess.h | 1 + 2 files changed, 18 insertions(+) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index c01c8e267b..bbfa4dbdf0 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -378,6 +378,23 @@ int virProcessGroupKill(pid_t pid, int sig) } +/* get process group from a pid */ +pid_t virProcessGroupGet(pid_t pid) +{ + if (pid <= 1) { + errno = ESRCH; + return -1; + } + +#ifdef WIN32 + errno = ENOSYS; + return -1; +#else + return getpgid(pid); +#endif +} + + /* * Try to kill the process and verify it has exited * diff --git a/src/util/virprocess.h b/src/util/virprocess.h index 0359d7fd9f..9d7c0f479a 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -53,6 +53,7 @@ virProcessWait(pid_t pid, int *exitstatus, bool raw) int virProcessKill(pid_t pid, int sig); int virProcessGroupKill(pid_t pid, int sig); +pid_t virProcessGroupGet(pid_t pid); int virProcessKillPainfully(pid_t pid, bool force); int virProcessKillPainfullyDelay(pid_t pid, -- 2.31.1
On 8/3/21 5:13 PM, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virprocess.c | 17 +++++++++++++++++ src/util/virprocess.h | 1 + 2 files changed, 18 insertions(+)
Again, libvirt_private.syms ;-) Michal
Add a version of virPidFileForceCleanupPath that takes a 'group' bool argument and propagate it all the way down to virProcessKillPainfullyDelay. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_process.c | 3 ++- src/util/virpidfile.c | 8 +++++++- src/util/virpidfile.h | 2 ++ src/util/virprocess.c | 8 ++++---- src/util/virprocess.h | 3 ++- 6 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6961cdb137..e2e4c9482d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3038,6 +3038,7 @@ virPidFileConstructPath; virPidFileDelete; virPidFileDeletePath; virPidFileForceCleanupPath; +virPidFileForceCleanupPathFull; virPidFileRead; virPidFileReadIfAlive; virPidFileReadPath; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 521fda57da..6ef8ebd83e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7700,7 +7700,8 @@ qemuProcessKill(virDomainObj *vm, unsigned int flags) * to be safe against stalls by the kernel freeing up the resources */ return virProcessKillPainfullyDelay(vm->pid, !!(flags & VIR_QEMU_PROCESS_KILL_FORCE), - vm->def->nhostdevs * 2); + vm->def->nhostdevs * 2, + false); } diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index c6389c1869..b75f441008 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -514,7 +514,7 @@ virPidFileConstructPath(bool privileged, * Returns 0 if the pidfile was successfully cleaned up, -1 otherwise. */ int -virPidFileForceCleanupPath(const char *path) +virPidFileForceCleanupPathFull(const char *path, bool group) { pid_t pid = 0; int fd = -1; @@ -544,3 +544,9 @@ virPidFileForceCleanupPath(const char *path) return 0; } + +int +virPidFileForceCleanupPath(const char *path) +{ + return virPidFileForceCleanupPathFull(path, false); +} diff --git a/src/util/virpidfile.h b/src/util/virpidfile.h index 370a59892e..fd8013c41e 100644 --- a/src/util/virpidfile.h +++ b/src/util/virpidfile.h @@ -73,4 +73,6 @@ int virPidFileConstructPath(bool privileged, const char *progname, char **pidfile); +int virPidFileForceCleanupPathFull(const char *path, + bool group) ATTRIBUTE_NONNULL(1); int virPidFileForceCleanupPath(const char *path) ATTRIBUTE_NONNULL(1); diff --git a/src/util/virprocess.c b/src/util/virprocess.c index bbfa4dbdf0..a92aa8aac9 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -406,15 +406,15 @@ pid_t virProcessGroupGet(pid_t pid) * wait longer than the default. */ int -virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay) +virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay, bool group) { size_t i; /* This is in 1/5th seconds since polling is on a 0.2s interval */ unsigned int polldelay = (force ? 200 : 75) + (extradelay*5); const char *signame = "TERM"; - VIR_DEBUG("vpid=%lld force=%d extradelay=%u", - (long long)pid, force, extradelay); + VIR_DEBUG("vpid=%lld force=%d extradelay=%u group=%d", + (long long)pid, force, extradelay, group); /* This loop sends SIGTERM, then waits a few iterations (10 seconds) * to see if it dies. If the process still hasn't exited, and @@ -474,7 +474,7 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay) int virProcessKillPainfully(pid_t pid, bool force) { - return virProcessKillPainfullyDelay(pid, force, 0); + return virProcessKillPainfullyDelay(pid, force, 0, false); } #if WITH_SCHED_GETAFFINITY diff --git a/src/util/virprocess.h b/src/util/virprocess.h index 9d7c0f479a..9910331a0c 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -58,7 +58,8 @@ pid_t virProcessGroupGet(pid_t pid); int virProcessKillPainfully(pid_t pid, bool force); int virProcessKillPainfullyDelay(pid_t pid, bool force, - unsigned int extradelay); + unsigned int extradelay, + bool group); int virProcessSetAffinity(pid_t pid, virBitmap *map, bool quiet); -- 2.31.1
On 8/3/21 5:13 PM, Ján Tomko wrote:
Add a version of virPidFileForceCleanupPath that takes a 'group' bool argument and propagate it all the way down to virProcessKillPainfullyDelay.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_process.c | 3 ++- src/util/virpidfile.c | 8 +++++++- src/util/virpidfile.h | 2 ++ src/util/virprocess.c | 8 ++++---- src/util/virprocess.h | 3 ++- 6 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6961cdb137..e2e4c9482d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3038,6 +3038,7 @@ virPidFileConstructPath; virPidFileDelete; virPidFileDeletePath; virPidFileForceCleanupPath; +virPidFileForceCleanupPathFull; virPidFileRead; virPidFileReadIfAlive; virPidFileReadPath; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 521fda57da..6ef8ebd83e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7700,7 +7700,8 @@ qemuProcessKill(virDomainObj *vm, unsigned int flags) * to be safe against stalls by the kernel freeing up the resources */ return virProcessKillPainfullyDelay(vm->pid, !!(flags & VIR_QEMU_PROCESS_KILL_FORCE), - vm->def->nhostdevs * 2); + vm->def->nhostdevs * 2, + false); }
diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index c6389c1869..b75f441008 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -514,7 +514,7 @@ virPidFileConstructPath(bool privileged, * Returns 0 if the pidfile was successfully cleaned up, -1 otherwise. */ int -virPidFileForceCleanupPath(const char *path) +virPidFileForceCleanupPathFull(const char *path, bool group)
This 'group' is unused argument.
{ pid_t pid = 0; int fd = -1;
Also, I don't think anybody will want to backport this patch separately and NOT the part that's actually interesting (last two hunks from the next patch). Michal
Send TERM/KILL to virtiofsd and its child processes too and do not exit until they are all dead. https://bugzilla.redhat.com/show_bug.cgi?id=1940276 Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_virtiofs.c | 2 +- src/util/virpidfile.c | 7 ++++++- src/util/virprocess.c | 5 ++++- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c index edaedf0304..15c05479c8 100644 --- a/src/qemu/qemu_virtiofs.c +++ b/src/qemu/qemu_virtiofs.c @@ -281,7 +281,7 @@ qemuVirtioFSStop(virQEMUDriver *driver G_GNUC_UNUSED, if (!(pidfile = qemuVirtioFSCreatePidFilename(vm, fs->info.alias))) goto cleanup; - if (virPidFileForceCleanupPath(pidfile) < 0) { + if (virPidFileForceCleanupPathFull(pidfile, true) < 0) { VIR_WARN("Unable to kill virtiofsd process"); } else { if (QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock) diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index b75f441008..7069f8343d 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -529,10 +529,15 @@ virPidFileForceCleanupPathFull(const char *path, bool group) if (fd < 0) { virResetLastError(); + if (pid > 1 && group) + pid = virProcessGroupGet(pid); + /* Only kill the process if the pid is valid one. 0 means * there is somebody else doing the same pidfile cleanup * machinery. */ - if (pid) + if (group) + virProcessKillPainfullyDelay(pid, true, 0, true); + else if (pid) virProcessKillPainfully(pid, true); if (virPidFileDeletePath(path) < 0) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index a92aa8aac9..d946ac37f4 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -449,7 +449,10 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay, boo signum = 0; /* Just check for existence */ } - rc = virProcessKill(pid, signum); + if (group) + rc = virProcessGroupKill(pid, signum); + else + rc = virProcessKill(pid, signum); if (rc < 0) { if (errno != ESRCH) { -- 2.31.1
On 8/3/21 5:13 PM, Ján Tomko wrote:
Send TERM/KILL to virtiofsd and its child processes too and do not exit until they are all dead.
https://bugzilla.redhat.com/show_bug.cgi?id=1940276
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_virtiofs.c | 2 +- src/util/virpidfile.c | 7 ++++++- src/util/virprocess.c | 5 ++++- 3 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c index edaedf0304..15c05479c8 100644 --- a/src/qemu/qemu_virtiofs.c +++ b/src/qemu/qemu_virtiofs.c @@ -281,7 +281,7 @@ qemuVirtioFSStop(virQEMUDriver *driver G_GNUC_UNUSED, if (!(pidfile = qemuVirtioFSCreatePidFilename(vm, fs->info.alias))) goto cleanup;
- if (virPidFileForceCleanupPath(pidfile) < 0) { + if (virPidFileForceCleanupPathFull(pidfile, true) < 0) { VIR_WARN("Unable to kill virtiofsd process"); } else { if (QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock)
This is the only hunk that should be in this commit. The rest should have been squashed to the previous one.
diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index b75f441008..7069f8343d 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -529,10 +529,15 @@ virPidFileForceCleanupPathFull(const char *path, bool group) if (fd < 0) { virResetLastError();
+ if (pid > 1 && group) + pid = virProcessGroupGet(pid); + /* Only kill the process if the pid is valid one. 0 means * there is somebody else doing the same pidfile cleanup * machinery. */ - if (pid) + if (group) + virProcessKillPainfullyDelay(pid, true, 0, true); + else if (pid) virProcessKillPainfully(pid, true);
if (virPidFileDeletePath(path) < 0) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index a92aa8aac9..d946ac37f4 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -449,7 +449,10 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay, boo signum = 0; /* Just check for existence */ }
- rc = virProcessKill(pid, signum); + if (group) + rc = virProcessGroupKill(pid, signum); + else + rc = virProcessKill(pid, signum);
if (rc < 0) { if (errno != ESRCH) {
On Tue, Aug 03, 2021 at 05:13:44PM +0200, Ján Tomko wrote:
Send TERM/KILL to virtiofsd and its child processes too and do not exit until they are all dead.
https://bugzilla.redhat.com/show_bug.cgi?id=1940276
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Thank you for your patches! They work well. Please feel free to add: Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Thanks! Masa
--- src/qemu/qemu_virtiofs.c | 2 +- src/util/virpidfile.c | 7 ++++++- src/util/virprocess.c | 5 ++++- 3 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c index edaedf0304..15c05479c8 100644 --- a/src/qemu/qemu_virtiofs.c +++ b/src/qemu/qemu_virtiofs.c @@ -281,7 +281,7 @@ qemuVirtioFSStop(virQEMUDriver *driver G_GNUC_UNUSED, if (!(pidfile = qemuVirtioFSCreatePidFilename(vm, fs->info.alias))) goto cleanup;
- if (virPidFileForceCleanupPath(pidfile) < 0) { + if (virPidFileForceCleanupPathFull(pidfile, true) < 0) { VIR_WARN("Unable to kill virtiofsd process"); } else { if (QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock) diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index b75f441008..7069f8343d 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -529,10 +529,15 @@ virPidFileForceCleanupPathFull(const char *path, bool group) if (fd < 0) { virResetLastError();
+ if (pid > 1 && group) + pid = virProcessGroupGet(pid); + /* Only kill the process if the pid is valid one. 0 means * there is somebody else doing the same pidfile cleanup * machinery. */ - if (pid) + if (group) + virProcessKillPainfullyDelay(pid, true, 0, true); + else if (pid) virProcessKillPainfully(pid, true);
if (virPidFileDeletePath(path) < 0) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index a92aa8aac9..d946ac37f4 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -449,7 +449,10 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay, boo signum = 0; /* Just check for existence */ }
- rc = virProcessKill(pid, signum); + if (group) + rc = virProcessGroupKill(pid, signum); + else + rc = virProcessKill(pid, signum);
if (rc < 0) { if (errno != ESRCH) { -- 2.31.1
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virprocess.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index d946ac37f4..9b428bc623 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -402,7 +402,7 @@ pid_t virProcessGroupGet(pid_t pid) * was killed forcibly, -1 if it is still alive, * or another error occurred. * - * Callers can proide an extra delay in seconds to + * Callers can provide an extra delay in seconds to * wait longer than the default. */ int -- 2.31.1
On 8/3/21 5:13 PM, Ján Tomko wrote:
v2 of: https://listman.redhat.com/archives/libvir-list/2021-June/msg00570.html
Ján Tomko (6): virProcessKillPainfullyDelay: use 'rc' variable util: Introduce virProcessGroupKill util: introduce virProcessGroupGet util: virPidFileForceCleanupPath: add group argument qemu: virtiofs: kill the whole process group util: fix typo
src/libvirt_private.syms | 1 + src/qemu/qemu_process.c | 3 ++- src/qemu/qemu_virtiofs.c | 2 +- src/util/virpidfile.c | 15 ++++++++++-- src/util/virpidfile.h | 2 ++ src/util/virprocess.c | 53 +++++++++++++++++++++++++++++++++++----- src/util/virprocess.h | 5 +++- 7 files changed, 70 insertions(+), 11 deletions(-)
I think you split code into too many patches. If you squash some together you have my: Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Ján Tomko -
Masayoshi Mizuma -
Michal Prívozník