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 <ktk...@virtuozzo.com> >>>>>>> --- >>>>>>> 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
