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

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

[Christoph Hellwig]

On Fri, Apr 04, 2025 at 09:59:40AM -0700, John Hubbard wrote:
> > gup_put_folio() seems to only rely on per-folio information (esp.
> > node_stat_mod_folio).
> > 
> > So there should not be such a context requirement.
> 
> That is correct. The essence of gup/pup is that it operates on
> struct pages, and doesn't have any "moral" connection to higher
> layers or additional process context.

Hmm, indeed.  I misremembered why the block based direct I/O code is
doing the process context offload, which is to call set_page_dirty to
redirty pages where the dirty bit was cleared during direct I/O.

Which I think now that all block based file systems use FOLL_PIN
shouldn't be needed anymore, because no one can clear the dirty bit
while the folios are pinned?

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

[John Hubbard]

On 4/6/25 11:39 PM, Christoph Hellwig wrote:
> On Fri, Apr 04, 2025 at 09:59:40AM -0700, John Hubbard wrote:
>>> gup_put_folio() seems to only rely on per-folio information (esp.
>>> node_stat_mod_folio).
>>>
>>> So there should not be such a context requirement.
>>
>> That is correct. The essence of gup/pup is that it operates on
>> struct pages, and doesn't have any "moral" connection to higher
>> layers or additional process context.
> 
> Hmm, indeed.  I misremembered why the block based direct I/O code is
> doing the process context offload, which is to call set_page_dirty to
> redirty pages where the dirty bit was cleared during direct I/O.
> 
> Which I think now that all block based file systems use FOLL_PIN
> shouldn't be needed anymore, because no one can clear the dirty bit
> while the folios are pinned?
> 

No one should clear the dirty bit while the pages are pinned, agreed.

This topic always worries me, because the original problem with
dirty pages is still unfixed: setting pages dirty upon unpinning
is both widely done (last time I checked), and yet broken, because
it doesn't do a mkdirty() call to set up writeback buffers.

The solution always seemed to point toward "get a file lease on that
range, before pinning", but it's a contentious design area to say
the least.


thanks,
-- 
John Hubbard

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

[Christoph Hellwig]

On Wed, Apr 09, 2025 at 07:56:07PM -0700, John Hubbard wrote:
> This topic always worries me, because the original problem with
> dirty pages is still unfixed: setting pages dirty upon unpinning
> is both widely done (last time I checked), and yet broken, because
> it doesn't do a mkdirty() call to set up writeback buffers.
> 
> The solution always seemed to point toward "get a file lease on that
> range, before pinning", but it's a contentious design area to say
> the least.

For the bio based direct I/O implementations we do set the pages
dirty before starting I/O using bio_set_pages_dirty, which uses
folio_mark_dirty and thus calls into the file systems using
->dirty_folio.  But we also do a second pass on I/O completion
before the buffers are unpinned.  Which I think now that we pin
the folios is superfluous.

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

[John Hubbard]

On 4/10/25 12:28 AM, Christoph Hellwig wrote:
> On Wed, Apr 09, 2025 at 07:56:07PM -0700, John Hubbard wrote:
>> This topic always worries me, because the original problem with
>> dirty pages is still unfixed: setting pages dirty upon unpinning
>> is both widely done (last time I checked), and yet broken, because
>> it doesn't do a mkdirty() call to set up writeback buffers.
>>
>> The solution always seemed to point toward "get a file lease on that
>> range, before pinning", but it's a contentious design area to say
>> the least.
> 
> For the bio based direct I/O implementations we do set the pages
> dirty before starting I/O using bio_set_pages_dirty, which uses
> folio_mark_dirty and thus calls into the file systems using
> ->dirty_folio.  But we also do a second pass on I/O completion
> before the buffers are unpinned.  Which I think now that we pin
> the folios is superfluous.
> 

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

thanks,
-- 
John Hubbard

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

[Matthew Wilcox]

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:
> >> This topic always worries me, because the original problem with
> >> dirty pages is still unfixed: setting pages dirty upon unpinning
> >> is both widely done (last time I checked), and yet broken, because
> >> it doesn't do a mkdirty() call to set up writeback buffers.
> >>
> >> The solution always seemed to point toward "get a file lease on that
> >> range, before pinning", but it's a contentious design area to say
> >> the least.
> > 
> > For the bio based direct I/O implementations we do set the pages
> > dirty before starting I/O using bio_set_pages_dirty, which uses
> > folio_mark_dirty and thus calls into the file systems using
> > ->dirty_folio.  But we also do a second pass on I/O completion
> > before the buffers are unpinned.  Which I think now that we pin
> > the folios is superfluous.
> > 
> 
> 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
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.

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

[John Hubbard]

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

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

[Christoph Hellwig]

On Thu, Apr 10, 2025 at 12:11:42PM -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).

I've looked around a bit.  We do skip pinned pags in shrink_folio_list
(btw, can someone please split that thing up, it's so huge that it
is completely unreadable) but that's not really relevant for clearing
the dirty bit for filemap folios these days despite comments talking
about just that.

So I guess, yes - we'd need to skip folio_maybe_dma_pinned() in
writeback, or wait for the bit to be cleared for data integrity
writeback.  Which doesn't sound too hard, but there might be
pitfalls.