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
