Re: [PATCH v1 4/8] iomap: add writepages support for IOMAP_IN_MEM iomaps

[Christoph Hellwig <hch@infradead.org>]

On Fri, Jun 06, 2025 at 04:37:59PM -0700, Joanne Koong wrote:
> This allows IOMAP_IN_MEM iomaps to use iomap_writepages() for handling
> writeback. This lets IOMAP_IN_MEM iomaps use some of the internal
> features in iomaps such as granular dirty tracking for large folios.
> 
> This introduces a new iomap_writeback_ops callback, writeback_folio(),
> callers may pass in which hands off folio writeback logic to the caller
> for writing back dirty pages instead of relying on mapping blocks.
> 
> This exposes two apis, iomap_start_folio_write() and
> iomap_finish_folio_write(), which callers may find useful in their
> writeback_folio() callback implementation.

It might also be worth stating what you don't use.  One big thing
that springs to mind is ioends.  Which are really useful if you
need more than one request to handle a folio, something that is
pretty common in network file systems.  I guess you don't need
that for fuse?

> +	if (wpc->iomap.type == IOMAP_IN_MEM) {
> +		if (wpc->ops->submit_ioend)
> +			error = wpc->ops->submit_ioend(wpc, error);
> +		return error;
> +	}

Given that the patch that moved things around already wrapped the
error propagation to the bio into a helpr, how does this differ
from the main path in the function now?

> +	/*
> +	 * If error is non-zero, it means that we have a situation where some part of
> +	 * the submission process has failed after we've marked pages for writeback.
> +	 * We cannot cancel ioend directly in that case, so call the bio end I/O handler
> +	 * with the error status here to run the normal I/O completion handler to clear
> +	 * the writeback bit and let the file system process the errors.
> +	 */

Please add the comment in a separate preparation patch.

> +		if (wpc->ops->writeback_folio) {
> +			WARN_ON_ONCE(wpc->ops->map_blocks);
> +			error = wpc->ops->writeback_folio(wpc, folio, inode,
> +							  offset_in_folio(folio, pos),
> +							  rlen);
> +		} else {
> +			WARN_ON_ONCE(wpc->iomap.type == IOMAP_IN_MEM);
> +			error = iomap_writepage_map_blocks(wpc, wbc, folio,
> +							   inode, pos, end_pos,
> +							   rlen, &count);
> +		}

So instead of having two entirely different methods, can we
refactor the block based code to also use
->writeback_folio?

Basically move all of the code inside the do { } while loop after
the call into ->map_blocks into a helper, and then let the caller
loop and also directly discard the folio if needed.  I can give that
a spin if you want.

Note that writeback_folio is misnamed, as it doesn't write back an
entire folio, but just a dirty range.

>  	} else {
> -		if (!count)
> +		/*
> +		 * If wpc->ops->writeback_folio is set, then it is responsible
> +		 * for ending the writeback itself.
> +		 */
> +		if (!count && !wpc->ops->writeback_folio)
>  			folio_end_writeback(folio);

This fails to explain why writeback_folio does the unlocking itself.
I also don't see how that would work in case of multiple dirty ranges.

>  	}
>  	mapping_set_error(inode->i_mapping, error);
> @@ -1693,3 +1713,25 @@ iomap_writepages(struct address_space *mapping, struct writeback_control *wbc,
>  	return iomap_submit_ioend(wpc, error);
>  }
>  EXPORT_SYMBOL_GPL(iomap_writepages);
> +
> +void iomap_start_folio_write(struct inode *inode, struct folio *folio, size_t len)
> +{
> +	struct iomap_folio_state *ifs = folio->private;
> +
> +	WARN_ON_ONCE(i_blocks_per_folio(inode, folio) > 1 && !ifs);
> +	if (ifs)
> +		atomic_add(len, &ifs->write_bytes_pending);
> +}
> +EXPORT_SYMBOL_GPL(iomap_start_folio_write);
> +
> +void iomap_finish_folio_write(struct inode *inode, struct folio *folio, size_t len)
> +{
> +	struct iomap_folio_state *ifs = folio->private;
> +
> +	WARN_ON_ONCE(i_blocks_per_folio(inode, folio) > 1 && !ifs);
> +	WARN_ON_ONCE(ifs && atomic_read(&ifs->write_bytes_pending) <= 0);
> +
> +	if (!ifs || atomic_sub_and_test(len, &ifs->write_bytes_pending))
> +		folio_end_writeback(folio);

Please also use these helpers in the block based code.