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

[Christoph Hellwig <hch@infradead.org>]

On Tue, Nov 19, 2024 at 10:46:54AM -0500, Brian Foster wrote:
> +loff_t
> +iomap_fill_dirty_folios(
> +	struct inode		*inode,
> +	struct iomap		*iomap,

If you pass in the batch directly instead of the iomap this is
completely generic and could go into filemap.c.  Adding willy
and linux-mm for these kinds of things also tends to help to
get good review feedback and often improvements.

> +	loff_t			offset,
> +	loff_t			length)
> +{
> +	struct address_space	*mapping = inode->i_mapping;
> +	struct folio_batch	fbatch;
> +	pgoff_t			start, end;
> +	loff_t			end_pos;
> +
> +	folio_batch_init(&fbatch);
> +	folio_batch_init(&iomap->fbatch);
> +
> +	end_pos = offset + length;
> +	start = offset >> PAGE_SHIFT;
> +	end = (end_pos - 1) >> PAGE_SHIFT;

Nit: initializing these at declaration time make the code easier to
read (at least for me :)).

> +
> +	while (filemap_get_folios(mapping, &start, end, &fbatch) &&
> +	       folio_batch_space(&iomap->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;

What does the lock protect here given that it can become stale as soon
as we unlock?

Note that there also is a filemap_get_folios_tag that only looks up
folios with the right tag (dirty or writeback).  Currently it only
supports a single tag, which wuld not be helpful here, but this might
be worth talking to the pagecache and xarray maintainer.