Re: Does GUP page unpinning have to be done in the pinning context?

[John Hubbard <jhubbard@nvidia.com>]

On 4/10/25 12:14 PM, Matthew Wilcox wrote:
> On Thu, Apr 10, 2025 at 12:11:42PM -0700, John Hubbard wrote:
>> On 4/10/25 12:28 AM, Christoph Hellwig wrote:
>>> On Wed, Apr 09, 2025 at 07:56:07PM -0700, John Hubbard wrote:
...
>> Oh actually I think I was wrong in my earlier reply about clearing
>> the dirty bit. Because in Jan Kara's original bug report, what
>> happened was that periodic writeback came in while the pages
>> were pinned, and cleared the dirty bit--and also deleted the
>> page buffers (file system specific behavior) that are required
>> for writeback.
>>
>> So then later when the pages are unpinned and marked dirty,
>> that causes the next writeback to fail in an unexpected way
>> (it used to cause ext4 BUG checks, in fact).
>>
>> So the problem here is that these pinned pages can get cleaned
>> while they are pinned, and then dirtied again by DMA (invisible
>> to the filesystem).
> 
> Did we fix that already?  Because it's relatively easy to writeback

Maybe not?

On a closely related note, I do still see folio_clear_dirty() being 
called from ext4's mpage_prepare_extent_to_map(), along with the ext4
bandaid [1] that was applied to mitigate the problem:

	/*
	* Should never happen but for buggy code in
	* other subsystems that call
	* set_page_dirty() without properly warning
	* the file system first.  See [1] for more
	* information.
	*
	* [1] https://lore.kernel.org/linux-mm/20180103100430.GE4911@quack2.suse.cz
	*/
if (!folio_buffers(folio)) {
	ext4_warning_inode(mpd->inode, "page %lu does not have buffers attached", folio->index);
	folio_clear_dirty(folio);
	folio_unlock(folio);
	continue;
}

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cc5095747edf


> pinned pages and _not_ clear the dirty flag.  That handles the two
> problems which are falsely thinking that a heavily-mapped order-0 page 
> is pinned (we write it back anyway, so don't lose data on crash),
> and doesn't strip the bufferheads.

That sounds like a viable solution!

thanks,
-- 
John Hubbard