Re: [PATCH 01/13] mm: Update ptep_get_lockless()s comment

[John Hubbard <jhubbard@nvidia.com>]

On 10/29/22 11:36, Linus Torvalds wrote:
>> In such a case, shrink_page_list() would consider the page clean, and would
>> indeed keep the page (since __remove_mapping() would find elevated page
>> refcount), which appears to give us a chance to mark the page as dirty
>> later.
> 
> Right. That is not different to any other function (like "write()"
> having looked up the page.
> 
>> However, IIUC, in this case shrink_page_list() might still call
>> filemap_release_folio() and release the buffers, so calling set_page_dirty()
>> afterwards - after the actual TLB invalidation took place - would fail.
> 
> I'm not seeing why.
> 
> That would imply that any "look up page, do set_page_dirty()" is
> broken. They don't have rmap either. And we have a number of them all
> over (eg think "GUP users" etc).

Yes, we do have a bunch of "look up page, do set_page_dirty()" cases.
And I think that many (most?) of them are in fact broken!

Because: the dirtiness of a page is something that the filesystem
believes that it is managing, and so filesystem coordination is, in
general, required in order to mark a page as dirty.

Jan Kara's 2018 analysis [1] (which launched the pin_user_pages()
effort) shows a nice clear example. And since then, I've come to believe
that most of the gup/pup call sites have it wrong:

    a) pin_user_pages() b) /* access page contents */ c)
    set_page_dirty() or set_page_dirty_lock() // PROBLEM HERE d)
    unpin_user_page()

ext4 has since papered over the problem, by soldiering on if it finds a
page without writeback buffers when it expected to be able to writeback
a dirty page. But you get the idea.

And I think that applies beyond the gup/pup situation.


[1] https://lore.kernel.org/linux-mm/20180103100430.GE4911@quack2.suse.cz/T/#u


thanks,
-- 
John Hubbard
NVIDIA