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

["Darrick J. Wong" <djwong@kernel.org>]

On Sun, Jun 08, 2025 at 10:32:20PM -0700, Christoph Hellwig wrote:
> 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?

My initial thought was "I wonder if Joanne would be better off with a
totally separate iomap_writepage_map_blocks implementation" since I
*think* fuse just needs a callback from iomap to initiate FUSE_WRITE
calls on the dirty range(s) of a folio, and then fuse can call
mapping_set_error and iomap_finish_folio_write when those FUSE_WRITE
calls complete.  There are no bios, so I don't see much point in using
the ioend machinery.

--D

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