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 <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.
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