[Devel] [PATCH] spfs: Main process wakes and kills its children on exit
Stanislav Kinsburskiy says:
"SPFS manager has a special "--exit-with-spfs" options, which is used by CRIU.
The idea of the option is simple: force SPFS manager to exit, when it has some
SPFS processes among its children (i.e. spfs was mounted at least once),
but all these processes have exited for whatever reason (which usually happens
when restore has failed and spfs mounts where unmounted).
Although it works in overall (main SPFS manager process exits), its children
(responsible to SPFS replacement) may wait on FUTEX for "release" command
for corresponding SPFS mount and thus never stop until they are killed".
The patch makes the thing and kills all process group on exit.
This does not act on normal exit, as spfs main process is the
only process than, and it does not kill itself, because as kill() is
called from signal handler right before exit().
https://jira.sw.ru/browse/PSBM-80055
Signed-off-by: Kirill Tkhai
---
manager/context.c |8
manager/context.h |1 +
manager/main.c|5 +
3 files changed, 14 insertions(+)
diff --git a/manager/context.c b/manager/context.c
index 1eb37c9..7097ff0 100644
--- a/manager/context.c
+++ b/manager/context.c
@@ -64,6 +64,13 @@ static void cleanup_spfs_mount(struct spfs_manager_context_s
*ctx,
close_namespaces(info->ns_fds);
}
+static void kill_spfs_pgrp(struct spfs_manager_context_s *ctx)
+{
+ pid_t pgid = ctx->spfs_pgid;
+ if (kill(-pgid, SIGKILL))
+ pr_perror("Can't force process group to exit");
+}
+
static void sigchld_handler(int signal, siginfo_t *siginfo, void *data)
{
struct spfs_manager_context_s *ctx = &spfs_manager_context;
@@ -95,6 +102,7 @@ static void sigchld_handler(int signal, siginfo_t *siginfo,
void *data)
cleanup_spfs_mount(ctx, info, status);
if (list_empty(&ctx->spfs_mounts->list) &&
ctx->exit_with_spfs) {
pr_info("spfs list is empty.
Exiting.\n");
+ kill_spfs_pgrp(ctx);
exit(0);
}
}
diff --git a/manager/context.h b/manager/context.h
index a307803..0d994c9 100644
--- a/manager/context.h
+++ b/manager/context.h
@@ -13,6 +13,7 @@ struct spfs_manager_context_s {
char*log_file;
char*socket_path;
int verbosity;
+ pid_t spfs_pgid;
booldaemonize;
boolexit_with_spfs;
char*ovz_id;
diff --git a/manager/main.c b/manager/main.c
index d3c7aaf..e490842 100644
--- a/manager/main.c
+++ b/manager/main.c
@@ -34,6 +34,11 @@ int main(int argc, char *argv[])
return -errno;
}
}
+ ctx->spfs_pgid = getpid();
+ if (getpgrp() != ctx->spfs_pgid && setpgrp() != 0) {
+ pr_perror("Can't set pgid");
+ return -errno;
+ }
return unreliable_socket_loop(ctx->sock, ctx, false,
spfs_manager_packet_handler);
}
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
[Devel] [PATCH] spfs: Main process wakes and kills its children on exit
Stanislav Kinsburskiy says:
"SPFS manager has a special "--exit-with-spfs" options, which is used by CRIU.
The idea of the option is simple: force SPFS manager to exit, when it has some
SPFS processes among its children (i.e. spfs was mounted at least once),
but all these processes have exited for whatever reason (which usually happens
when restore has failed and spfs mounts where unmounted).
Although it works in overall (main SPFS manager process exits), its children
(responsible to SPFS replacement) may wait on FUTEX for "release" command
for corresponding SPFS mount and thus never stop until they are killed".
1 spfs-manager
2 \_ spfs
3 \_ spfs-manager
4 \_ spfs
5 \_ spfs-manager
2 and 3 are pair of a mount, and 4 and 5 are pair of another mount.
The patch makes spfs-manager 1 kill 3 in case of 2 exited.
https://jira.sw.ru/browse/PSBM-80055
Signed-off-by: Kirill Tkhai
---
manager/context.c |4
manager/spfs.c|1 +
2 files changed, 5 insertions(+)
diff --git a/manager/context.c b/manager/context.c
index 1eb37c9..4464a23 100644
--- a/manager/context.c
+++ b/manager/context.c
@@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct spfs_manager_context_s
*ctx,
/* SPFS master was killed. We need to release the reference */
spfs_release_mnt(info);
+ if (killed || WEXITSTATUS(status))
+ kill(info->replacer, SIGKILL);
+
info->dead = true;
del_spfs_info(ctx->spfs_mounts, info);
@@ -88,6 +91,7 @@ static void sigchld_handler(int signal, siginfo_t *siginfo,
void *data)
* corresponding fd.
*/
spfs_release_mnt(info);
+ info->replacer = -1;
} else {
info = find_spfs_by_pid(ctx->spfs_mounts, pid);
if (info) {
diff --git a/manager/spfs.c b/manager/spfs.c
index 3e0f667..0b94587 100644
--- a/manager/spfs.c
+++ b/manager/spfs.c
@@ -39,6 +39,7 @@ int create_spfs_info(const char *id,
pr_err("failed to allocate\n");
return -ENOMEM;
}
+ info->replacer = -1;
err = init_mount_info(&info->mnt, id, mountpoint, ns_mountpoint);
if (err)
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] spfs: Main process wakes and kills its children on exit
Missed header. Actually it's v2
On 23.01.2018 12:41, Kirill Tkhai wrote:
> Stanislav Kinsburskiy says:
>
> "SPFS manager has a special "--exit-with-spfs" options, which is used by CRIU.
> The idea of the option is simple: force SPFS manager to exit, when it has
> some
> SPFS processes among its children (i.e. spfs was mounted at least once),
> but all these processes have exited for whatever reason (which usually
> happens
> when restore has failed and spfs mounts where unmounted).
> Although it works in overall (main SPFS manager process exits), its children
> (responsible to SPFS replacement) may wait on FUTEX for "release" command
> for corresponding SPFS mount and thus never stop until they are killed".
>
> 1 spfs-manager
> 2 \_ spfs
> 3 \_ spfs-manager
> 4 \_ spfs
> 5 \_ spfs-manager
>
> 2 and 3 are pair of a mount, and 4 and 5 are pair of another mount.
> The patch makes spfs-manager 1 kill 3 in case of 2 exited.
>
> https://jira.sw.ru/browse/PSBM-80055
>
> Signed-off-by: Kirill Tkhai
> ---
> manager/context.c |4
> manager/spfs.c|1 +
> 2 files changed, 5 insertions(+)
>
> diff --git a/manager/context.c b/manager/context.c
> index 1eb37c9..4464a23 100644
> --- a/manager/context.c
> +++ b/manager/context.c
> @@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct
> spfs_manager_context_s *ctx,
> /* SPFS master was killed. We need to release the reference */
> spfs_release_mnt(info);
>
> + if (killed || WEXITSTATUS(status))
> + kill(info->replacer, SIGKILL);
> +
> info->dead = true;
> del_spfs_info(ctx->spfs_mounts, info);
>
> @@ -88,6 +91,7 @@ static void sigchld_handler(int signal, siginfo_t *siginfo,
> void *data)
>* corresponding fd.
>*/
> spfs_release_mnt(info);
> + info->replacer = -1;
> } else {
> info = find_spfs_by_pid(ctx->spfs_mounts, pid);
> if (info) {
> diff --git a/manager/spfs.c b/manager/spfs.c
> index 3e0f667..0b94587 100644
> --- a/manager/spfs.c
> +++ b/manager/spfs.c
> @@ -39,6 +39,7 @@ int create_spfs_info(const char *id,
> pr_err("failed to allocate\n");
> return -ENOMEM;
> }
> + info->replacer = -1;
>
> err = init_mount_info(&info->mnt, id, mountpoint, ns_mountpoint);
> if (err)
>
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] spfs: Main process wakes and kills its children on exit
Please, see a couple of nits below
23.01.2018 10:41, Kirill Tkhai пишет:
> Stanislav Kinsburskiy says:
>
> "SPFS manager has a special "--exit-with-spfs" options, which is used by CRIU.
> The idea of the option is simple: force SPFS manager to exit, when it has
> some
> SPFS processes among its children (i.e. spfs was mounted at least once),
> but all these processes have exited for whatever reason (which usually
> happens
> when restore has failed and spfs mounts where unmounted).
> Although it works in overall (main SPFS manager process exits), its children
> (responsible to SPFS replacement) may wait on FUTEX for "release" command
> for corresponding SPFS mount and thus never stop until they are killed".
>
> 1 spfs-manager
> 2 \_ spfs
> 3 \_ spfs-manager
> 4 \_ spfs
> 5 \_ spfs-manager
>
> 2 and 3 are pair of a mount, and 4 and 5 are pair of another mount.
> The patch makes spfs-manager 1 kill 3 in case of 2 exited.
>
> https://jira.sw.ru/browse/PSBM-80055
>
> Signed-off-by: Kirill Tkhai
> ---
> manager/context.c |4
> manager/spfs.c|1 +
> 2 files changed, 5 insertions(+)
>
> diff --git a/manager/context.c b/manager/context.c
> index 1eb37c9..4464a23 100644
> --- a/manager/context.c
> +++ b/manager/context.c
> @@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct
> spfs_manager_context_s *ctx,
> /* SPFS master was killed. We need to release the reference */
> spfs_release_mnt(info);
>
> + if (killed || WEXITSTATUS(status))
> + kill(info->replacer, SIGKILL);
> +
There is "if (killed)" check above.
Could you please move this hunk there?
> info->dead = true;
> del_spfs_info(ctx->spfs_mounts, info);
>
> @@ -88,6 +91,7 @@ static void sigchld_handler(int signal, siginfo_t *siginfo,
> void *data)
>* corresponding fd.
>*/
> spfs_release_mnt(info);
> + info->replacer = -1;
> } else {
> info = find_spfs_by_pid(ctx->spfs_mounts, pid);
> if (info) {
> diff --git a/manager/spfs.c b/manager/spfs.c
> index 3e0f667..0b94587 100644
> --- a/manager/spfs.c
> +++ b/manager/spfs.c
> @@ -39,6 +39,7 @@ int create_spfs_info(const char *id,
> pr_err("failed to allocate\n");
> return -ENOMEM;
> }
> + info->replacer = -1;
>
Could you, please, move it down to the end of the function where unconditional
actions take place?
> err = init_mount_info(&info->mnt, id, mountpoint, ns_mountpoint);
> if (err)
>
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] spfs: Main process wakes and kills its children on exit
On 23.01.2018 12:48, Stanislav Kinsburskiy wrote:
> Please, see a couple of nits below
>
> 23.01.2018 10:41, Kirill Tkhai пишет:
>> Stanislav Kinsburskiy says:
>>
>> "SPFS manager has a special "--exit-with-spfs" options, which is used by
>> CRIU.
>> The idea of the option is simple: force SPFS manager to exit, when it has
>> some
>> SPFS processes among its children (i.e. spfs was mounted at least once),
>> but all these processes have exited for whatever reason (which usually
>> happens
>> when restore has failed and spfs mounts where unmounted).
>> Although it works in overall (main SPFS manager process exits), its children
>> (responsible to SPFS replacement) may wait on FUTEX for "release" command
>> for corresponding SPFS mount and thus never stop until they are killed".
>>
>> 1 spfs-manager
>> 2 \_ spfs
>> 3 \_ spfs-manager
>> 4 \_ spfs
>> 5 \_ spfs-manager
>>
>> 2 and 3 are pair of a mount, and 4 and 5 are pair of another mount.
>> The patch makes spfs-manager 1 kill 3 in case of 2 exited.
>>
>> https://jira.sw.ru/browse/PSBM-80055
>>
>> Signed-off-by: Kirill Tkhai
>> ---
>> manager/context.c |4
>> manager/spfs.c|1 +
>> 2 files changed, 5 insertions(+)
>>
>> diff --git a/manager/context.c b/manager/context.c
>> index 1eb37c9..4464a23 100644
>> --- a/manager/context.c
>> +++ b/manager/context.c
>> @@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct
>> spfs_manager_context_s *ctx,
>> /* SPFS master was killed. We need to release the reference */
>> spfs_release_mnt(info);
>>
>> +if (killed || WEXITSTATUS(status))
>> +kill(info->replacer, SIGKILL);
>> +
>
> There is "if (killed)" check above.
> Could you please move this hunk there?
There is logical OR (||) in the hunk. How should I move it to unconditional
check?
>> info->dead = true;
>> del_spfs_info(ctx->spfs_mounts, info);
>>
>> @@ -88,6 +91,7 @@ static void sigchld_handler(int signal, siginfo_t
>> *siginfo, void *data)
>> * corresponding fd.
>> */
>> spfs_release_mnt(info);
>> +info->replacer = -1;
>> } else {
>> info = find_spfs_by_pid(ctx->spfs_mounts, pid);
>> if (info) {
>> diff --git a/manager/spfs.c b/manager/spfs.c
>> index 3e0f667..0b94587 100644
>> --- a/manager/spfs.c
>> +++ b/manager/spfs.c
>> @@ -39,6 +39,7 @@ int create_spfs_info(const char *id,
>> pr_err("failed to allocate\n");
>> return -ENOMEM;
>> }
>> +info->replacer = -1;
>>
>
> Could you, please, move it down to the end of the function where
> unconditional actions take place?
No, problem
>> err = init_mount_info(&info->mnt, id, mountpoint, ns_mountpoint);
>> if (err)
>>
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] spfs: Main process wakes and kills its children on exit
On 23.01.2018 13:07, Stanislav Kinsburskiy wrote: > > > 23.01.2018 10:51, Kirill Tkhai пишет: >> On 23.01.2018 12:48, Stanislav Kinsburskiy wrote: >>> Please, see a couple of nits below >>> >>> 23.01.2018 10:41, Kirill Tkhai пишет: Stanislav Kinsburskiy says: "SPFS manager has a special "--exit-with-spfs" options, which is used by CRIU. The idea of the option is simple: force SPFS manager to exit, when it has some SPFS processes among its children (i.e. spfs was mounted at least once), but all these processes have exited for whatever reason (which usually happens when restore has failed and spfs mounts where unmounted). Although it works in overall (main SPFS manager process exits), its children (responsible to SPFS replacement) may wait on FUTEX for "release" command for corresponding SPFS mount and thus never stop until they are killed". 1 spfs-manager 2 \_ spfs 3 \_ spfs-manager 4 \_ spfs 5 \_ spfs-manager 2 and 3 are pair of a mount, and 4 and 5 are pair of another mount. The patch makes spfs-manager 1 kill 3 in case of 2 exited. https://jira.sw.ru/browse/PSBM-80055 Signed-off-by: Kirill Tkhai --- manager/context.c |4 manager/spfs.c|1 + 2 files changed, 5 insertions(+) diff --git a/manager/context.c b/manager/context.c index 1eb37c9..4464a23 100644 --- a/manager/context.c +++ b/manager/context.c @@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct spfs_manager_context_s *ctx, /* SPFS master was killed. We need to release the reference */ spfs_release_mnt(info); + if (killed || WEXITSTATUS(status)) + kill(info->replacer, SIGKILL); + >>> >>> There is "if (killed)" check above. >>> Could you please move this hunk there? >> >> There is logical OR (||) in the hunk. How should I move it to unconditional >> check? >> > > Ah, ok. Then let the check be as it is. > Could you please add warning message for this kill with the process pid being > killed and the reason why (spfs was killed of exited with error)? Ok ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] spfs: Main process wakes and kills its children on exit
23.01.2018 10:51, Kirill Tkhai пишет: > On 23.01.2018 12:48, Stanislav Kinsburskiy wrote: >> Please, see a couple of nits below >> >> 23.01.2018 10:41, Kirill Tkhai пишет: >>> Stanislav Kinsburskiy says: >>> >>> "SPFS manager has a special "--exit-with-spfs" options, which is used by >>> CRIU. >>> The idea of the option is simple: force SPFS manager to exit, when it has >>> some >>> SPFS processes among its children (i.e. spfs was mounted at least once), >>> but all these processes have exited for whatever reason (which usually >>> happens >>> when restore has failed and spfs mounts where unmounted). >>> Although it works in overall (main SPFS manager process exits), its >>> children >>> (responsible to SPFS replacement) may wait on FUTEX for "release" command >>> for corresponding SPFS mount and thus never stop until they are killed". >>> >>> 1 spfs-manager >>> 2 \_ spfs >>> 3 \_ spfs-manager >>> 4 \_ spfs >>> 5 \_ spfs-manager >>> >>> 2 and 3 are pair of a mount, and 4 and 5 are pair of another mount. >>> The patch makes spfs-manager 1 kill 3 in case of 2 exited. >>> >>> https://jira.sw.ru/browse/PSBM-80055 >>> >>> Signed-off-by: Kirill Tkhai >>> --- >>> manager/context.c |4 >>> manager/spfs.c|1 + >>> 2 files changed, 5 insertions(+) >>> >>> diff --git a/manager/context.c b/manager/context.c >>> index 1eb37c9..4464a23 100644 >>> --- a/manager/context.c >>> +++ b/manager/context.c >>> @@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct >>> spfs_manager_context_s *ctx, >>> /* SPFS master was killed. We need to release the reference */ >>> spfs_release_mnt(info); >>> >>> + if (killed || WEXITSTATUS(status)) >>> + kill(info->replacer, SIGKILL); >>> + >> >> There is "if (killed)" check above. >> Could you please move this hunk there? > > There is logical OR (||) in the hunk. How should I move it to unconditional > check? > Ah, ok. Then let the check be as it is. Could you please add warning message for this kill with the process pid being killed and the reason why (spfs was killed of exited with error)? ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] spfs: Main process wakes and kills its children on exit
On 23.01.2018 13:07, Stanislav Kinsburskiy wrote: > > > 23.01.2018 10:51, Kirill Tkhai пишет: >> On 23.01.2018 12:48, Stanislav Kinsburskiy wrote: >>> Please, see a couple of nits below >>> >>> 23.01.2018 10:41, Kirill Tkhai пишет: Stanislav Kinsburskiy says: "SPFS manager has a special "--exit-with-spfs" options, which is used by CRIU. The idea of the option is simple: force SPFS manager to exit, when it has some SPFS processes among its children (i.e. spfs was mounted at least once), but all these processes have exited for whatever reason (which usually happens when restore has failed and spfs mounts where unmounted). Although it works in overall (main SPFS manager process exits), its children (responsible to SPFS replacement) may wait on FUTEX for "release" command for corresponding SPFS mount and thus never stop until they are killed". 1 spfs-manager 2 \_ spfs 3 \_ spfs-manager 4 \_ spfs 5 \_ spfs-manager 2 and 3 are pair of a mount, and 4 and 5 are pair of another mount. The patch makes spfs-manager 1 kill 3 in case of 2 exited. https://jira.sw.ru/browse/PSBM-80055 Signed-off-by: Kirill Tkhai --- manager/context.c |4 manager/spfs.c|1 + 2 files changed, 5 insertions(+) diff --git a/manager/context.c b/manager/context.c index 1eb37c9..4464a23 100644 --- a/manager/context.c +++ b/manager/context.c @@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct spfs_manager_context_s *ctx, /* SPFS master was killed. We need to release the reference */ spfs_release_mnt(info); + if (killed || WEXITSTATUS(status)) + kill(info->replacer, SIGKILL); + >>> >>> There is "if (killed)" check above. >>> Could you please move this hunk there? >> >> There is logical OR (||) in the hunk. How should I move it to unconditional >> check? >> > > Ah, ok. Then let the check be as it is. > Could you please add warning message for this kill with the process pid being > killed and the reason why (spfs was killed of exited with error)? Maybe we should call spfs_release_mnt() in case of exit status != 0 too? ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] spfs: Main process wakes and kills its children on exit
23.01.2018 11:09, Kirill Tkhai пишет: > On 23.01.2018 13:07, Stanislav Kinsburskiy wrote: >> >> >> 23.01.2018 10:51, Kirill Tkhai пишет: >>> On 23.01.2018 12:48, Stanislav Kinsburskiy wrote: Please, see a couple of nits below 23.01.2018 10:41, Kirill Tkhai пишет: > Stanislav Kinsburskiy says: > > "SPFS manager has a special "--exit-with-spfs" options, which is used by > CRIU. > The idea of the option is simple: force SPFS manager to exit, when it > has some > SPFS processes among its children (i.e. spfs was mounted at least once), > but all these processes have exited for whatever reason (which usually > happens > when restore has failed and spfs mounts where unmounted). > Although it works in overall (main SPFS manager process exits), its > children > (responsible to SPFS replacement) may wait on FUTEX for "release" command > for corresponding SPFS mount and thus never stop until they are killed". > > 1 spfs-manager > 2 \_ spfs > 3 \_ spfs-manager > 4 \_ spfs > 5 \_ spfs-manager > > 2 and 3 are pair of a mount, and 4 and 5 are pair of another mount. > The patch makes spfs-manager 1 kill 3 in case of 2 exited. > > https://jira.sw.ru/browse/PSBM-80055 > > Signed-off-by: Kirill Tkhai > --- > manager/context.c |4 > manager/spfs.c|1 + > 2 files changed, 5 insertions(+) > > diff --git a/manager/context.c b/manager/context.c > index 1eb37c9..4464a23 100644 > --- a/manager/context.c > +++ b/manager/context.c > @@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct > spfs_manager_context_s *ctx, > /* SPFS master was killed. We need to release the reference */ > spfs_release_mnt(info); > > + if (killed || WEXITSTATUS(status)) > + kill(info->replacer, SIGKILL); > + There is "if (killed)" check above. Could you please move this hunk there? >>> >>> There is logical OR (||) in the hunk. How should I move it to unconditional >>> check? >>> >> >> Ah, ok. Then let the check be as it is. >> Could you please add warning message for this kill with the process pid >> being killed and the reason why (spfs was killed of exited with error)? > > Maybe we should call spfs_release_mnt() in case of exit status != 0 too? > Well, this makes sense. If spfs exited with non-zero result (either it was killed or exited due to some error), then there is no need in replacer. And there is no need in the reference. So, probably this check you proposed should be used for both spfs_release_mnt(info) and kill(info->replacer, SIGKILL). ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] spfs: Main process wakes and kills its children on exit
On 23.01.2018 13:14, Stanislav Kinsburskiy wrote: > > > 23.01.2018 11:09, Kirill Tkhai пишет: >> On 23.01.2018 13:07, Stanislav Kinsburskiy wrote: >>> >>> >>> 23.01.2018 10:51, Kirill Tkhai пишет: On 23.01.2018 12:48, Stanislav Kinsburskiy wrote: > Please, see a couple of nits below > > 23.01.2018 10:41, Kirill Tkhai пишет: >> Stanislav Kinsburskiy says: >> >> "SPFS manager has a special "--exit-with-spfs" options, which is used by >> CRIU. >> The idea of the option is simple: force SPFS manager to exit, when it >> has some >> SPFS processes among its children (i.e. spfs was mounted at least once), >> but all these processes have exited for whatever reason (which usually >> happens >> when restore has failed and spfs mounts where unmounted). >> Although it works in overall (main SPFS manager process exits), its >> children >> (responsible to SPFS replacement) may wait on FUTEX for "release" >> command >> for corresponding SPFS mount and thus never stop until they are killed". >> >> 1 spfs-manager >> 2 \_ spfs >> 3 \_ spfs-manager >> 4 \_ spfs >> 5 \_ spfs-manager >> >> 2 and 3 are pair of a mount, and 4 and 5 are pair of another mount. >> The patch makes spfs-manager 1 kill 3 in case of 2 exited. >> >> https://jira.sw.ru/browse/PSBM-80055 >> >> Signed-off-by: Kirill Tkhai >> --- >> manager/context.c |4 >> manager/spfs.c|1 + >> 2 files changed, 5 insertions(+) >> >> diff --git a/manager/context.c b/manager/context.c >> index 1eb37c9..4464a23 100644 >> --- a/manager/context.c >> +++ b/manager/context.c >> @@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct >> spfs_manager_context_s *ctx, >> /* SPFS master was killed. We need to release the >> reference */ >> spfs_release_mnt(info); >> >> +if (killed || WEXITSTATUS(status)) >> +kill(info->replacer, SIGKILL); >> + > > There is "if (killed)" check above. > Could you please move this hunk there? There is logical OR (||) in the hunk. How should I move it to unconditional check? >>> >>> Ah, ok. Then let the check be as it is. >>> Could you please add warning message for this kill with the process pid >>> being killed and the reason why (spfs was killed of exited with error)? >> >> Maybe we should call spfs_release_mnt() in case of exit status != 0 too? >> > > Well, this makes sense. > If spfs exited with non-zero result (either it was killed or exited due to > some error), then there is no need in replacer. And there is no need in the > reference. > So, probably this check you proposed should be used for both > spfs_release_mnt(info) and kill(info->replacer, SIGKILL). Are you OK with this change sent in the only patch, or should we use one more patch? ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] spfs: Main process wakes and kills its children on exit
23.01.2018 11:17, Kirill Tkhai пишет: > On 23.01.2018 13:14, Stanislav Kinsburskiy wrote: >> >> >> 23.01.2018 11:09, Kirill Tkhai пишет: >>> On 23.01.2018 13:07, Stanislav Kinsburskiy wrote: 23.01.2018 10:51, Kirill Tkhai пишет: > On 23.01.2018 12:48, Stanislav Kinsburskiy wrote: >> Please, see a couple of nits below >> >> 23.01.2018 10:41, Kirill Tkhai пишет: >>> Stanislav Kinsburskiy says: >>> >>> "SPFS manager has a special "--exit-with-spfs" options, which is used >>> by CRIU. >>> The idea of the option is simple: force SPFS manager to exit, when it >>> has some >>> SPFS processes among its children (i.e. spfs was mounted at least >>> once), >>> but all these processes have exited for whatever reason (which usually >>> happens >>> when restore has failed and spfs mounts where unmounted). >>> Although it works in overall (main SPFS manager process exits), its >>> children >>> (responsible to SPFS replacement) may wait on FUTEX for "release" >>> command >>> for corresponding SPFS mount and thus never stop until they are >>> killed". >>> >>> 1 spfs-manager >>> 2 \_ spfs >>> 3 \_ spfs-manager >>> 4 \_ spfs >>> 5 \_ spfs-manager >>> >>> 2 and 3 are pair of a mount, and 4 and 5 are pair of another mount. >>> The patch makes spfs-manager 1 kill 3 in case of 2 exited. >>> >>> https://jira.sw.ru/browse/PSBM-80055 >>> >>> Signed-off-by: Kirill Tkhai >>> --- >>> manager/context.c |4 >>> manager/spfs.c|1 + >>> 2 files changed, 5 insertions(+) >>> >>> diff --git a/manager/context.c b/manager/context.c >>> index 1eb37c9..4464a23 100644 >>> --- a/manager/context.c >>> +++ b/manager/context.c >>> @@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct >>> spfs_manager_context_s *ctx, >>> /* SPFS master was killed. We need to release the >>> reference */ >>> spfs_release_mnt(info); >>> >>> + if (killed || WEXITSTATUS(status)) >>> + kill(info->replacer, SIGKILL); >>> + >> >> There is "if (killed)" check above. >> Could you please move this hunk there? > > There is logical OR (||) in the hunk. How should I move it to > unconditional check? > Ah, ok. Then let the check be as it is. Could you please add warning message for this kill with the process pid being killed and the reason why (spfs was killed of exited with error)? >>> >>> Maybe we should call spfs_release_mnt() in case of exit status != 0 too? >>> >> >> Well, this makes sense. >> If spfs exited with non-zero result (either it was killed or exited due to >> some error), then there is no need in replacer. And there is no need in the >> reference. >> So, probably this check you proposed should be used for both >> spfs_release_mnt(info) and kill(info->replacer, SIGKILL). > > Are you OK with this change sent in the only patch, or should we use one more > patch? > I think one patch is OK. Only mention the change explicitly in the patch description, please. ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] spfs: Main process wakes and kills its children on exit
On 23.01.2018 13:18, Stanislav Kinsburskiy wrote: > > > 23.01.2018 11:17, Kirill Tkhai пишет: >> On 23.01.2018 13:14, Stanislav Kinsburskiy wrote: >>> >>> >>> 23.01.2018 11:09, Kirill Tkhai пишет: On 23.01.2018 13:07, Stanislav Kinsburskiy wrote: > > > 23.01.2018 10:51, Kirill Tkhai пишет: >> On 23.01.2018 12:48, Stanislav Kinsburskiy wrote: >>> Please, see a couple of nits below >>> >>> 23.01.2018 10:41, Kirill Tkhai пишет: Stanislav Kinsburskiy says: "SPFS manager has a special "--exit-with-spfs" options, which is used by CRIU. The idea of the option is simple: force SPFS manager to exit, when it has some SPFS processes among its children (i.e. spfs was mounted at least once), but all these processes have exited for whatever reason (which usually happens when restore has failed and spfs mounts where unmounted). Although it works in overall (main SPFS manager process exits), its children (responsible to SPFS replacement) may wait on FUTEX for "release" command for corresponding SPFS mount and thus never stop until they are killed". 1 spfs-manager 2 \_ spfs 3 \_ spfs-manager 4 \_ spfs 5 \_ spfs-manager 2 and 3 are pair of a mount, and 4 and 5 are pair of another mount. The patch makes spfs-manager 1 kill 3 in case of 2 exited. https://jira.sw.ru/browse/PSBM-80055 Signed-off-by: Kirill Tkhai --- manager/context.c |4 manager/spfs.c|1 + 2 files changed, 5 insertions(+) diff --git a/manager/context.c b/manager/context.c index 1eb37c9..4464a23 100644 --- a/manager/context.c +++ b/manager/context.c @@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct spfs_manager_context_s *ctx, /* SPFS master was killed. We need to release the reference */ spfs_release_mnt(info); + if (killed || WEXITSTATUS(status)) + kill(info->replacer, SIGKILL); + >>> >>> There is "if (killed)" check above. >>> Could you please move this hunk there? >> >> There is logical OR (||) in the hunk. How should I move it to >> unconditional check? >> > > Ah, ok. Then let the check be as it is. > Could you please add warning message for this kill with the process pid > being killed and the reason why (spfs was killed of exited with error)? Maybe we should call spfs_release_mnt() in case of exit status != 0 too? >>> >>> Well, this makes sense. >>> If spfs exited with non-zero result (either it was killed or exited due to >>> some error), then there is no need in replacer. And there is no need in the >>> reference. >>> So, probably this check you proposed should be used for both >>> spfs_release_mnt(info) and kill(info->replacer, SIGKILL). >> >> Are you OK with this change sent in the only patch, or should we use one >> more patch? >> > > I think one patch is OK. Only mention the change explicitly in the patch > description, please. Ok, then what about spfs_cleanup_env(info, killed)? It also has killed argument ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] spfs: Main process wakes and kills its children on exit
23.01.2018 11:19, Kirill Tkhai пишет:
> On 23.01.2018 13:18, Stanislav Kinsburskiy wrote:
>>
>>
>> 23.01.2018 11:17, Kirill Tkhai пишет:
>>> On 23.01.2018 13:14, Stanislav Kinsburskiy wrote:
23.01.2018 11:09, Kirill Tkhai пишет:
> On 23.01.2018 13:07, Stanislav Kinsburskiy wrote:
>>
>>
>> 23.01.2018 10:51, Kirill Tkhai пишет:
>>> On 23.01.2018 12:48, Stanislav Kinsburskiy wrote:
Please, see a couple of nits below
23.01.2018 10:41, Kirill Tkhai пишет:
> Stanislav Kinsburskiy says:
>
> "SPFS manager has a special "--exit-with-spfs" options, which is used
> by CRIU.
> The idea of the option is simple: force SPFS manager to exit, when
> it has some
> SPFS processes among its children (i.e. spfs was mounted at least
> once),
> but all these processes have exited for whatever reason (which
> usually happens
> when restore has failed and spfs mounts where unmounted).
> Although it works in overall (main SPFS manager process exits), its
> children
> (responsible to SPFS replacement) may wait on FUTEX for "release"
> command
> for corresponding SPFS mount and thus never stop until they are
> killed".
>
> 1 spfs-manager
> 2 \_ spfs
> 3 \_ spfs-manager
> 4 \_ spfs
> 5 \_ spfs-manager
>
> 2 and 3 are pair of a mount, and 4 and 5 are pair of another mount.
> The patch makes spfs-manager 1 kill 3 in case of 2 exited.
>
> https://jira.sw.ru/browse/PSBM-80055
>
> Signed-off-by: Kirill Tkhai
> ---
> manager/context.c |4
> manager/spfs.c|1 +
> 2 files changed, 5 insertions(+)
>
> diff --git a/manager/context.c b/manager/context.c
> index 1eb37c9..4464a23 100644
> --- a/manager/context.c
> +++ b/manager/context.c
> @@ -53,6 +53,9 @@ static void cleanup_spfs_mount(struct
> spfs_manager_context_s *ctx,
> /* SPFS master was killed. We need to release the
> reference */
> spfs_release_mnt(info);
>
> + if (killed || WEXITSTATUS(status))
> + kill(info->replacer, SIGKILL);
> +
There is "if (killed)" check above.
Could you please move this hunk there?
>>>
>>> There is logical OR (||) in the hunk. How should I move it to
>>> unconditional check?
>>>
>>
>> Ah, ok. Then let the check be as it is.
>> Could you please add warning message for this kill with the process pid
>> being killed and the reason why (spfs was killed of exited with error)?
>
> Maybe we should call spfs_release_mnt() in case of exit status != 0 too?
>
Well, this makes sense.
If spfs exited with non-zero result (either it was killed or exited due to
some error), then there is no need in replacer. And there is no need in
the reference.
So, probably this check you proposed should be used for both
spfs_release_mnt(info) and kill(info->replacer, SIGKILL).
>>>
>>> Are you OK with this change sent in the only patch, or should we use one
>>> more patch?
>>>
>>
>> I think one patch is OK. Only mention the change explicitly in the patch
>> description, please.
>
> Ok, then what about spfs_cleanup_env(info, killed)? It also has killed
> argument
>
Well, this is also true...
Maybe the right thing here would be to have 2 patches, and the first one should
be something like this (also rename variable to reflect the change):
diff --git a/manager/context.c b/manager/context.c
index 1eb37c9..756971b 100644
--- a/manager/context.c
+++ b/manager/context.c
@@ -45,12 +45,12 @@ const char *mgr_ovz_id(void)
static void cleanup_spfs_mount(struct spfs_manager_context_s *ctx,
struct spfs_info_s *info, int status)
{
- bool killed = WIFSIGNALED(status);
+ bool failed = WIFSIGNALED(status) || WEXITSTATUS(status);
pr_debug("removing info %s from the list\n", info->mnt.id);
- if (killed)
- /* SPFS master was killed. We need to release the reference */
+ if (failed)
+ /* SPFS master has failed. We need to release the reference */
spfs_release_mnt(info);
info->dead = true;
@@ -59,7 +59,7 @@ static void cleanup_spfs_mount(struct spfs_manager_context_s
*ctx,
if (unlink(info->socket_path))
pr_perror("failed to unlink %s", info->socket_path);
- spfs_cleanup_env(info, killed);
+ spfs_cleanup_env(info, failed);
close_namespaces(info->ns_fds);
}
diff --git a/manager/spfs.c b/manager/spfs.c
index 3e0f667..99845b1 100644
--- a/manager/spfs.c
+++ b/manager/spfs.c
@@ -409,9 +409,9 @@ int spfs_p
