Re: [PATCH 1/4] filemap: return pos of first dirty folio from range_has_writeback

[Brian Foster <bfoster@redhat.com>]

On Thu, Jul 18, 2024 at 04:09:03PM +0100, Matthew Wilcox wrote:
> On Thu, Jul 18, 2024 at 09:02:09AM -0400, Brian Foster wrote:
> > @@ -655,6 +655,8 @@ bool filemap_range_has_writeback(struct address_space *mapping,
> >  				folio_test_writeback(folio))
> >  			break;
> >  	}
> > +	if (folio)
> > +		*start_byte = folio_pos(folio);
> >  	rcu_read_unlock();
> >  	return folio != NULL;
> >  }
> 
> Distressingly, this is unsafe.
> 
> We have no reference on the folio at this point (not one that matters,
> anyway).  We have the rcu read lock, yes, but that doesn't protect enough
> to make folio_pos() safe.
> 
> Since we do't have folio_get() here, the folio can be freed, sent back to
> the page allocator, and then reallocated to literally any purpose.  As I'm
> reviewing patch 1/4, I have no idea if this is just a hint and you can
> survive it being completely wrong, or if this is going to cause problems.
> 

Ah, thanks. I was unsure about this when I hacked it up but then got
more focused on patch 3. I think for this implementation I'd want it to
be an accurate pos of the first dirty/wb folio. I think this could
possibly use filemap_range_has_writeback() (without patch 1) as more of
a hint/optimization, but that might involve doing the FGP_NOCREAT thing
from the previous variant of this prototype has and I was trying to
avoid that.

Do you think it would be reasonable to create a variant of this function
that did the relevant bits from __filemap_get_folio():

        if (!folio_try_get(folio))
                goto repeat;

        if (unlikely(folio != xas_reload(&xas))) {
                folio_put(folio);
                goto repeat;
        }
	/* check dirty/wb etc. */

... in order to either return a correct pos or maybe even a reference to
the folio itself? iomap_zero_iter() wants the locked folio anyways, but
that might be too ugly to pass through the iomap_folio_ops thing in
current form.

If that doesn't work, then I might chalk this up as another reason to
just do the flush thing I was rambling about in the cover letter...

Brian