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

[Jens Axboe <axboe@kernel.dk>]

On 9/27/21 9:52 AM, Eric W. Biederman wrote:
> Jens Axboe <axboe@kernel.dk> writes:
> 
>> 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...
> 
> I suspect rather we should update this comment in get_signal
> instead.
> 
> 		/*
> 		 * 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.
> 		 */
> 		if (current->flags & PF_IO_WORKER)
> 			goto out;
> 
> 
> Although I would not mind updating io-wq.c and io_uring.c where
> they call get_signal as well. 

Probably best to leave the explanation to the source, in get_signal(). If
you don't mind, I'll leave updating that one to you.

-- 
Jens Axboe