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

[Jens Axboe <axboe@kernel.dk>]

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