Re: [PATCH RFCv2 2/4] iomap: optional zero range dirty folio processing

[Christoph Hellwig <hch@infradead.org>]

Just a bit of nitpicking, otherwise this looks sane, although I'd
want to return to proper review of the squashed prep patches first.

>  	if (!mapping_large_folio_support(iter->inode->i_mapping))
>  		len = min_t(size_t, len, PAGE_SIZE - offset_in_page(pos));
>  
> +	if (iter->fbatch) {
> +		struct folio *folio = folio_batch_next(iter->fbatch);
> +		if (folio) {

Missing empty line after the variable declaration.

> +	if (!folio) {
> +		WARN_ON_ONCE(!iter->fbatch);
> +		len = 0;
> +		goto out;

Given that the put label really just updates the return by reference
folio and len arguments we might as well do that here and avoid the
goto.

> +	} else if (folio_pos(folio) > iter->pos) {

Also no need for an else after a goto (or return).

> @@ -1374,6 +1393,11 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
>  		if (iter->iomap.flags & IOMAP_F_STALE)
>  			break;
>  
> +		if (!folio) {
> +			iomap_iter_advance(iter, iomap_length(iter));
> +			break;
> +		}

Maybe throw in a comment here how the NULL folio can happen?

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

> +		if (WARN_ON_ONCE(iter.fbatch && srcmap->type != IOMAP_UNWRITTEN))

Overly long line.

>  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?