Re: [PATCH 2/7] io_uring: handle signals for IO threads like a normal thread

[Jens Axboe]

On 3/26/21 4:23 PM, Eric W. Biederman wrote:
> Jens Axboe <axboe@xxxxxxxxx> writes:
> 
>> On 3/26/21 2:29 PM, Eric W. Biederman wrote:
>>> Jens Axboe <axboe@xxxxxxxxx> writes:
>>>
>>>> We go through various hoops to disallow signals for the IO threads, but
>>>> there's really no reason why we cannot just allow them. The IO threads
>>>> never return to userspace like a normal thread, and hence don't go through
>>>> normal signal processing. Instead, just check for a pending signal as part
>>>> of the work loop, and call get_signal() to handle it for us if anything
>>>> is pending.
>>>>
>>>> With that, we can support receiving signals, including special ones like
>>>> SIGSTOP.
>>>>
>>>> Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>
>>>> ---
>>>>  fs/io-wq.c    | 24 +++++++++++++++++-------
>>>>  fs/io_uring.c | 12 ++++++++----
>>>>  2 files changed, 25 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>>>> index b7c1fa932cb3..3e2f059a1737 100644
>>>> --- a/fs/io-wq.c
>>>> +++ b/fs/io-wq.c
>>>> @@ -16,7 +16,6 @@
>>>>  #include <linux/rculist_nulls.h>
>>>>  #include <linux/cpu.h>
>>>>  #include <linux/tracehook.h>
>>>> -#include <linux/freezer.h>
>>>>  
>>>>  #include "../kernel/sched/sched.h"
>>>>  #include "io-wq.h"
>>>> @@ -503,10 +502,16 @@ static int io_wqe_worker(void *data)
>>>>  		if (io_flush_signals())
>>>>  			continue;
>>>>  		ret = schedule_timeout(WORKER_IDLE_TIMEOUT);
>>>> -		if (try_to_freeze() || ret)
>>>> +		if (signal_pending(current)) {
>>>> +			struct ksignal ksig;
>>>> +
>>>> +			if (fatal_signal_pending(current))
>>>> +				break;
>>>> +			if (get_signal(&ksig))
>>>> +				continue;
>>>                         ^^^^^^^^^^^^^^^^^^^^^^
>>>
>>> That is wrong.  You are promising to deliver a signal to signal
>>> handler and them simply discarding it.  Perhaps:
>>>
>>> 			if (!get_signal(&ksig))
>>>                         	continue;
>>> 			WARN_ON(!sig_kernel_stop(ksig->sig));
>>>                         break;
>>
>> Thanks, updated.
> 
> Gah.  Kill the WARN_ON.
> 
> I was thinking "WARN_ON(!sig_kernel_fatal(ksig->sig));"
> The function sig_kernel_fatal does not exist.
> 
> Fatal is the state that is left when a signal is neither
> ignored nor a stop signal, and does not have a handler.
> 
> The rest of the logic still works.

I've just come to the same conclusion myself after testing it.
Of the 3 cases, most of them can do the continue, but doesn't
really matter with the way the loop is structured. Anyway, looks
like this now:


commit 769186e30cd437f5e1a000e7cf00286948779da4
Author: Jens Axboe <axboe@xxxxxxxxx>
Date:   Thu Mar 25 18:16:06 2021 -0600

    io_uring: handle signals for IO threads like a normal thread
    
    We go through various hoops to disallow signals for the IO threads, but
    there's really no reason why we cannot just allow them. The IO threads
    never return to userspace like a normal thread, and hence don't go through
    normal signal processing. Instead, just check for a pending signal as part
    of the work loop, and call get_signal() to handle it for us if anything
    is pending.
    
    With that, we can support receiving signals, including special ones like
    SIGSTOP.
    
    Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>

diff --git a/fs/io-wq.c b/fs/io-wq.c
index b7c1fa932cb3..7e5970c8b0be 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -16,7 +16,6 @@
 #include <linux/rculist_nulls.h>
 #include <linux/cpu.h>
 #include <linux/tracehook.h>
-#include <linux/freezer.h>
 
 #include "../kernel/sched/sched.h"
 #include "io-wq.h"
@@ -503,10 +502,16 @@ static int io_wqe_worker(void *data)
 		if (io_flush_signals())
 			continue;
 		ret = schedule_timeout(WORKER_IDLE_TIMEOUT);
-		if (try_to_freeze() || ret)
+		if (signal_pending(current)) {
+			struct ksignal ksig;
+
+			if (fatal_signal_pending(current))
+				break;
+			if (!get_signal(&ksig))
+				continue;
+		}
+		if (ret)
 			continue;
-		if (fatal_signal_pending(current))
-			break;
 		/* timed out, exit unless we're the fixed worker */
 		if (test_bit(IO_WQ_BIT_EXIT, &wq->state) ||
 		    !(worker->flags & IO_WORKER_F_FIXED))
@@ -714,9 +719,15 @@ static int io_wq_manager(void *data)
 		set_current_state(TASK_INTERRUPTIBLE);
 		io_wq_check_workers(wq);
 		schedule_timeout(HZ);
-		try_to_freeze();
-		if (fatal_signal_pending(current))
-			set_bit(IO_WQ_BIT_EXIT, &wq->state);
+		if (signal_pending(current)) {
+			struct ksignal ksig;
+
+			if (fatal_signal_pending(current)) {
+				set_bit(IO_WQ_BIT_EXIT, &wq->state);
+				continue;
+			}
+			get_signal(&ksig);
+		}
 	} while (!test_bit(IO_WQ_BIT_EXIT, &wq->state));
 
 	io_wq_check_workers(wq);
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 54ea561db4a5..1c64e3f9b7a2 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -78,7 +78,6 @@
 #include <linux/task_work.h>
 #include <linux/pagemap.h>
 #include <linux/io_uring.h>
-#include <linux/freezer.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/io_uring.h>
@@ -6765,8 +6764,14 @@ static int io_sq_thread(void *data)
 			timeout = jiffies + sqd->sq_thread_idle;
 			continue;
 		}
-		if (fatal_signal_pending(current))
-			break;
+		if (signal_pending(current)) {
+			struct ksignal ksig;
+
+			if (fatal_signal_pending(current))
+				break;
+			if (!get_signal(&ksig))
+				continue;
+		}
 		sqt_spin = false;
 		cap_entries = !list_is_singular(&sqd->ctx_list);
 		list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) {
@@ -6809,7 +6814,6 @@ static int io_sq_thread(void *data)
 
 			mutex_unlock(&sqd->lock);
 			schedule();
-			try_to_freeze();
 			mutex_lock(&sqd->lock);
 			list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
 				io_ring_clear_wakeup_flag(ctx);

-- 
Jens Axboe