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