On Wed, Jan 08, 2025 at 11:20:39PM -0800, Christoph Hellwig wrote:
> Just a bit of nitpicking, otherwise this looks sane, although I'd
> want to return to proper review of the squashed prep patches first.
>
Yeah.. I mainly wanted to send this just to show the use case for the
iter advance changes. I'll look into tweaks for the various
nits/comments.
...
> > + while (filemap_get_folios(mapping, &start, end, &fbatch) &&
> > + folio_batch_space(iter->fbatch)) {
> > + struct folio *folio;
> > + while ((folio = folio_batch_next(&fbatch))) {
> > + if (folio_trylock(folio)) {
> > + bool clean = !folio_test_dirty(folio) &&
> > + !folio_test_writeback(folio);
> > + folio_unlock(folio);
> > + if (clean)
> > + continue;
> > + }
> > +
> > + folio_get(folio);
> > + if (!folio_batch_add(iter->fbatch, folio)) {
> > + end_pos = folio_pos(folio) + folio_size(folio);
> > + break;
> > + }
> > + }
> > + folio_batch_release(&fbatch);
>
> I think I mentioned this last time, but I'd much prefer to do away
> with the locla fbatch used for processing and rewrite this using a
> find_get_entry() loop. That probably means this helper needs to move
> to filemap.c, which should be easy if we pass in the mapping and outer
> fbatch.
>
I recall we discussed making this more generic. That is still on my
radar, I just hadn't got to it yet.
I don't recall the find_get_entry() loop suggestion, but that seems
reasonable at a quick glance. I've been away from this for a few weeks
but I think my main concern with this trajectory was if/how to deal with
iomap_folio_state if we wanted fully granular dirty folio && dirty block
processing.
For example, if we have a largish dirty folio backed by an unwritten
extent with maybe a single block that is actually dirty, would we be
alright to just zero the requested portion of the folio as long as some
part of the folio is dirty? Given the historical ad hoc nature of XFS
speculative prealloc zeroing, personally I don't see that as much of an
issue in practice as long as subsequent reads return zeroes, but I could
be missing something.
...
> > static inline void iomap_iter_reset_iomap(struct iomap_iter *iter)
> > {
> > + if (iter->fbatch) {
> > + folio_batch_release(iter->fbatch);
> > + kfree(iter->fbatch);
> > + iter->fbatch = NULL;
> > + }
>
> Does it make sense to free the fbatch allocation on every iteration,
> or should we keep the memory allocation around and only free it after
> the last iteration?
>
In the current implementation the existence of the fbatch is what
controls the folio lookup path, so we'd only want it for unwritten
mappings. That said, this could be done differently with a flag or
something that indicates whether to use the batch. Given that we release
the folios anyways and zero range isn't the most frequent thing, I
figured this keeps things simple for now. I don't really have a strong
preference for either approach, however.
Brian
