Re: [PATCH 1/7] kernel: don't call do_exit() for PF_IO_WORKER threads

Jens Axboe
2021-03-26T22:12:12Z

On 3/26/21 2:43 PM, Eric W. Biederman wrote:
> Jens Axboe <axboe@kernel.dk> writes:
>
>> Right now we're never calling get_signal() from PF_IO_WORKER threads, but
>> in preparation for doing so, don't handle a fatal signal for them. The
>> workers have state they need to cleanup when exiting, and they don't do
>> coredumps, so just return instead of performing either a dump or calling
>> do_exit() on their behalf. The threads themselves will detect a fatal
>> signal and do proper shutdown.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>> kernel/signal.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/kernel/signal.c b/kernel/signal.c
>> index f2a1b898da29..e3e1b8fbfe8a 100644
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -2756,6 +2756,15 @@ bool get_signal(struct ksignal *ksig)
>> */
>> current->flags |= PF_SIGNALED;
>>
>> + /*
>> + * PF_IO_WORKER threads will catch and exit on fatal signals
>> + * themselves. They have cleanup that must be performed, so
>> + * we cannot call do_exit() on their behalf. coredumps also
>> + * do not apply to them.
>> + */
>> + if (current->flags & PF_IO_WORKER)
>> + return false;
>> +
>
> Returning false when get_signal needs the caller to handle a signal
> adds a very weird and awkward special case to how get_signal returns
> arguments.
>
> Instead you should simply break and let get_signal return SIGKILL like
> any other signal that has a handler that the caller of get_signal needs
> to handle.
>
> Something like:
>> + /*
>> + * PF_IO_WORKER have cleanup that must be performed,
>> + * before calling do_exit().
>> + */
>> + if (current->flags & PF_IO_WORKER)
>> + break;
>
>
> As do_coredump does not call do_exit there is no reason to skip calling into
> the coredump handling either. And allowing it will remove yet another
> special case from the io worker code.

Thanks, I'll turn it into a break, that does seem like a better idea in
general. Actually it wants to be a goto or similar, as a break will
assume that we have the sighand lock held. With the coredump being
irrelevant, I'll just it before the do_exit() call.

--
Jens Axboe