Re: writeback completion soft lockup BUG in folio_wake_bit()

[Linus Torvalds <torvalds@linux-foundation.org>]

On Wed, Mar 16, 2022 at 1:59 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> As I recall, the bookmark hack was introduced in order to handle
> lock_page() problems.  It wasn't really supposed to handle writeback,
> but nobody thought it would cause any harm (and indeed, it didn't at the
> time).  So how about we only use bookmarks for lock_page(), since
> lock_page() usually doesn't have the multiple-waker semantics that
> writeback has?

I was hoping that some of the page lock problems are gone and we could
maybe try to get rid of the bookmarks entirely.

But the page lock issues only ever showed up on some private
proprietary load and machine, so we never really got confirmation that
they are fixed. There were lots of strong signs to them being related
to the migration page locking, and it may be that the bookmark code is
only hurting these days.

See for example commit 9a1ea439b16b ("mm:
put_and_wait_on_page_locked() while page is migrated") which doesn't
actually change the *locking* side, but drops the page reference when
waiting for the locked page to be unlocked, which in turn removes a
"loop and try again when migration". And that may have been the real
_fix_ for the problem.

Because while the bookmark thing avoids the NMI lockup detector firing
due to excessive hold times, the bookmarking also _causes_ that "we
now will see the same page multiple times because we dropped the lock
and somebody re-added it at the end of the queue" issue. Which seems
to be the problem here.

Ugh. I wish we had some way to test "could we just remove the bookmark
code entirely again".

Of course, the PG_lock case also works fairly hard to not actually
remove and re-add the lock waiter to the queue, but having an actual
"wait for and get the lock" operation. The writeback bit isn't done
that way.

I do hate how we had to make folio_wait_writeback{_killable}() use
"while" rather than an "if". It *almost* works with just a "wait for
current writeback", but not quite. See commit c2407cf7d22d ("mm: make
wait_on_page_writeback() wait for multiple pending writebacks") for
why we have to loop. Ugly, ugly.

Because I do think that "while" in the writeback waiting is a problem.
Maybe _the_ problem.

                        Linus