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

[John Hubbard <jhubbard@nvidia.com>]

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!

Anyway...so maybe most or even all of the unpin_user_pages_dirty_lock()
callers do not really need a _lock variant, after all.

Or, maybe it really is required and we're overlooking some subtle
filesystem-related point. It would be nice to get a second look from
Christoph Hellwig and Jan Kara (+CC).

> 
> 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
> 

I think truncate vs. gup/pup() is no more and and no less broken in this
regard than it's ever been. We need another LSF/MM conference with some
more shouting, and/or file leases, for that. :)


thanks,
-- 
John Hubbard
NVIDIA