[Devel] [PATCH] spfs: Main process wakes and kills its children on exit

[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 <ktkhai at 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