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

[Matthew Wilcox <willy@infradead.org>]

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.