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

[Jan Kara <jack@suse.cz>]

On Sun 09-01-22 00:01:49, John Hubbard wrote:
> On 1/8/22 20:39, Matthew Wilcox wrote:
> > 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?
> 
> This is a fun question, because you're asking it at a point when the
> overall problem remains unsolved. That is, the interaction between
> file-backed pages and gup/pup is still completely broken.
> 
> And I don't have an answer for you: it does seem like lock_page() is
> completely pointless here. Looking back, there are some 25 callers of
> unpin_user_pages_dirty_lock(), and during all those patch reviews, no
> one noticed this point!

I'd say it is underdocumented but not obviously pointless :) AFAIR (and
Christoph or Andrew may well correct me) the page lock in
set_page_dirty_lock() is there to protect metadata associated with the page
through page->private. Otherwise truncate could free these (e.g.
block_invalidatepage()) while ->set_page_dirty() callback (e.g.
__set_page_dirty_buffers()) works on this metadata.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR