Re: [PATCH 14/17] gup: Convert for_each_compound_head() to gup_for_each_folio()

[Matthew Wilcox <willy@infradead.org>]

On Wed, Jan 05, 2022 at 12:17:46AM -0800, John Hubbard wrote:
> > +		if (!folio_test_dirty(folio)) {
> > +			folio_lock(folio);
> > +			folio_mark_dirty(folio);
> > +			folio_unlock(folio);
> 
> At some point, maybe even here, I suspect that creating the folio
> version of set_page_dirty_lock() would help. I'm sure you have
> a better feel for whether it helps, after doing all of this conversion
> work, but it just sort of jumped out at me as surprising to see it
> in this form.

I really hate set_page_dirty_lock().  It smacks of "there is a locking
rule here which we're violating, so we'll just take the lock to fix it"
without understanding why there's a locking problem here.

As far as I can tell, originally, the intent was that you would lock
the page before modifying any of the data in the page.  ie you would
do:

	gup()
	lock_page()
	addr = kmap_page()
	*addr = 1;
	kunmap_page()
	set_page_dirty()
	unlock_page()
	put_page()

and that would prevent races between modifying the page and (starting)
writeback, not to mention truncate() and various other operations.

Clearly we can't do that for DMA-pinned pages.  There's only one lock
bit.  But do we even need to take the lock if we have the page pinned?
What are we protecting against?

If it's truncate(), I think we must already have protection against
that as we could easily have:

	pup()
				truncate()
	lock_page()
	set_page_dirty()
	unlock_page()
	unpin