On Wed, Dec 16, 2020 at 9:07 AM Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> wrote:
>
> If this looks fine, I'll submit a proper patch.
That patch looks good to me.
It would be good if somebody else looked it through - maybe I like it
just because I got to pee in the snow and make my mark. But i think
that filemap_map_pages() now looks a lot more understandable, and
having that pte_offset_map_lock() outside the loop should be good.
In particular, it will make it much easier to then make arguments
about things like
- increase the page mapping count
- check that the page is not locked
- check that the page still has a mapping
we can say "we know we hold the page table lock, and we increased the
page mapping count, and we saw that the page still had a mapping and
was not locked after that".
And that means that with some trivial memory ordering rules, we can
know that any concurrent "truncate()" that got the page lock after we
checked it, will get held up at
if (page_mapped(page)) {
unsigned int nr = thp_nr_pages(page);
unmap_mapping_pages(mapping, page->index, nr, false);
}
in truncate_cleanup_page(), and that means that we can safely map the
page _without_ ever actually even doing a "trylock_page()" at all.
Before this patch of yours, that logic would have been broken, because
the page table lock wasn't actually held for the whole first iteration
of that loop in filemap_map_pages().
And getting rid of the trylock_page() in that path gets rid of _the_
major page lock case for at least one class of loads.
So I like the patch. But I would _really_ like for somebody else to
look at it, and look at my thinking above.
Because maybe I'm missing something.
Linus
