Re: [GIT PULL] io_uring fixes for 5.15-rc3

[Jens Axboe <axboe@kernel.dk>]

On 9/27/21 9:13 AM, Eric W. Biederman wrote:
> Jens Axboe <axboe@kernel.dk> writes:
> 
>> On 9/27/21 8:29 AM, Jens Axboe wrote:
>>> On 9/27/21 7:51 AM, Eric W. Biederman wrote:
>>>> Jens Axboe <axboe@kernel.dk> writes:
>>>>
>>>>> On 9/25/21 5:05 PM, Linus Torvalds wrote:
>>>>>> On Sat, Sep 25, 2021 at 1:32 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>>
>>>>>>> - io-wq core dump exit fix (me)
>>>>>>
>>>>>> Hmm.
>>>>>>
>>>>>> That one strikes me as odd.
>>>>>>
>>>>>> I get the feeling that if the io_uring thread needs to have that
>>>>>> signal_group_exit() test, something is wrong in signal-land.
>>>>>>
>>>>>> It's basically a "fatal signal has been sent to another thread", and I
>>>>>> really get the feeling that "fatal_signal_pending()" should just be
>>>>>> modified to handle that case too.
>>>>>
>>>>> It did surprise me as well, which is why that previous change ended up
>>>>> being broken for the coredump case... You could argue that the io-wq
>>>>> thread should just exit on signal_pending(), which is what we did
>>>>> before, but that really ends up sucking for workloads that do use
>>>>> signals for communication purposes. postgres was the reporter here.
>>>>
>>>> The primary function get_signal is to make signals not pending.  So I
>>>> don't understand any use of testing signal_pending after a call to
>>>> get_signal.
>>>>
>>>> My confusion doubles when I consider the fact io_uring threads should
>>>> only be dequeuing SIGSTOP and SIGKILL.
>>>>
>>>> I am concerned that an io_uring thread that dequeues SIGKILL won't call
>>>> signal_group_exit and thus kill the other threads in the thread group.
>>>>
>>>> What motivated removing the break and adding the fatal_signal_pending
>>>> test?
>>>
>>> I played with this a bit this morning, and I agree it doesn't seem to be
>>> needed at all. The original issue was with postgres, I'll give that a
>>> whirl as well and see if we run into any unwarranted exits. My simpler
>>> test case did not.
>>
>> Ran the postgres test, and we get tons of io-wq exiting on get_signal()
>> returning true. Took a closer look, and it actually looks very much
>> expected, as it's a SIGKILL to the original task.
>>
>> So it looks like I was indeed wrong, and this probably masked the
>> original issue that was fixed in that series. I've been running with
>> this:
>>
>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>> index c2360cdc403d..afd1db8e000d 100644
>> --- a/fs/io-wq.c
>> +++ b/fs/io-wq.c
>> @@ -584,10 +584,9 @@ static int io_wqe_worker(void *data)
>>  
>>  			if (!get_signal(&ksig))
>>  				continue;
>> -			if (fatal_signal_pending(current) ||
>> -			    signal_group_exit(current->signal))
>> -				break;
>> -			continue;
>> +			if (ksig.sig != SIGKILL)
>> +				printk("exit on sig! fatal? %d, sig=%d\n", fatal_signal_pending(current), ksig.sig);
>> +			break;
>>  		}
>>  		last_timeout = !ret;
>>  	}
>>
>> and it's running fine and, as expected, we don't generate any printk
>> activity as these are all fatal deliveries to the parent.
> 
> Good.  So just a break should be fine.

Indeed, I'll send out a patch for that.

> A little bit of me is concerned about not calling do_group_exit in this
> case.  Fortunately it is not a problem as complete_signal kills all of
> the threads in a signal_group when SIGKILL is delivered.
> 
> So at least until something else is refactored and io_uring threads
> unblock another fatal signal all is well.

Should we put a comment in io-wq to that effect? I don't see why we'd
ever unblock other signals there, but...

-- 
Jens Axboe