On Wed, Mar 16, 2022 at 04:35:10PM -0700, Linus Torvalds wrote:
> On Wed, Mar 16, 2022 at 1:59 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> 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.
>
Right.. In case you missed it in my too long of a description (sorry), I
pointed out that this problem seems to manifest most recently as of that
commit. In fact looking through the past discussion for that patch, it
wouldn't surprise me a ton of this problem is some pathological
manifestation of the perf issue that you described here [1].
Indeed most of the waiters in this case come from fsync() -> ... ->
__filemap_fdatawait_range(), and your test patch in that email performs
a similar sort of trick to skip out of the wake up side (I'm curious if
that was ever determined to help?) to things that I was playing with to
try and narrow this down.
> Because I do think that "while" in the writeback waiting is a problem.
> Maybe _the_ problem.
>
FWIW, Matthew's patch does seem to address this problem. My current test
of that patch is past the point where I usually expect to see the soft
lockup warning, but I'm going to let it continue to run (and then run it
through some xfs regression if the approach is agreeable).
Getting back to the loop thing (and seeing Matthew's latest reply wrt to
wait_and_set())...
If we wanted to go back to non-looping in folio_wait_writeback() to
avoid the unserialized wait queue build up or whatever, would it make
any sense to lift the looping writeback check to write_cache_pages()? We
hold the page lock and have checked PageDirty() by that point, so ISTM
that would address the BUG_ON(PageWriteback()) race caused by the
delayed/unserialized wakeup without producing the excess wait queue
buildup caused by waiters in the __filemap_fdatawait_range() path.
Then presumably that "wait for writeback to clear" loop in
write_cache_pages() is eventually replaced by the "wait and set
writeback" thing when the rest of the fs code is fixed up appropriately.
Hm? Of course I haven't tested that so it could be completely bogus, but
I can if it makes any sort of sense as an incremental step..
Brian
[1] https://lore.kernel.org/linux-mm/CAHk-=wgD9GK5CeHopYmRHoYS9cNuCmDMsc=+MbM_KgJ0KB+=ng@xxxxxxxxxxxxxx/
> Linus
>
