wait_on_page_bit_common(TASK_KILLABLE, EXCLUSIVE) can miss wakeup?

From: Oleg Nesterov
Date: Wed Jun 24 2020 - 12:11:57 EST


Suppose that 2 threads T1 and T2 call __lock_page_killable() and sleep in
wait_on_page_bit_common() -> io_schedule().

T1 is killed, it does test_and_set_bit_lock() but the page is still locked.

unlock_page() calls __wake_up_common(nr_exclusive = 1), this wakes T1 up.
T2 is not woken.

T1 checks signal_pending_state() and returns EINTR.

T2 will sleep until another thread does lock/unlock ?

----------------------------------------------------------------------------
I noticed this by accident, I am hunting for another / unrelated bug. I did
git-blame and iiuc the commit a8b169afbf06a ("Avoid page waitqueue race leaving
possible page locker waiting") tried to fix the problem but see above, I don't
understand how can it help.

Don't we need something like below or I am totally confused?

Oleg.

--- x/mm/filemap.c
+++ x/mm/filemap.c
@@ -1131,14 +1131,23 @@ static inline int wait_on_page_bit_commo
wait_page.bit_nr = bit_nr;

for (;;) {
+ int intr = 0;
+
spin_lock_irq(&q->lock);

- if (likely(list_empty(&wait->entry))) {
- __add_wait_queue_entry_tail(q, wait);
- SetPageWaiters(page);
- }
+ // see the comment prepare_to_wait_event()
+ if (signal_pending_state(state, current)) {
+ list_del_init(&wait->entry);
+ intr = 1;
+ } else {
+ if (likely(list_empty(&wait->entry))) {
+ // HMM. head/tail depending on EXCLUSIVE ???
+ __add_wait_queue_entry_tail(q, wait);
+ SetPageWaiters(page);
+ }

- set_current_state(state);
+ set_current_state(state);
+ }

spin_unlock_irq(&q->lock);

@@ -1146,7 +1155,7 @@ static inline int wait_on_page_bit_commo
if (behavior == DROP)
put_page(page);

- if (likely(bit_is_set))
+ if (!intr && likely(bit_is_set))
io_schedule();

if (behavior == EXCLUSIVE) {
@@ -1157,7 +1166,7 @@ static inline int wait_on_page_bit_commo
break;
}

- if (signal_pending_state(state, current)) {
+ if (intr) {
ret = -EINTR;
break;
}