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

[Joanne Koong <joannelkoong@gmail.com>]

On Mon, Jun 9, 2025 at 8:58 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Jun 09, 2025 at 04:15:27PM -0700, Joanne Koong wrote:
> > ioends are used in fuse for cleaning up state. fuse implements a
> > ->submit_ioend() callback in fuse_iomap_submit_ioend() (in patch 7/8).
>
> But do you use struct iomap_ioend at all?  (Sorry, still don't have a
> whole tree with the patches applied in front of me).

I don't use struct iomap_ioend at all and I'm realizing now that I
should just have fuse manually do the end-of-io submission after it
calls iomap_writepages() / iomap_writeback_dirty_folio() instead of
defining a ->submit_ioend(). Then, we can get rid of this

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

that was added and leave the iomap_submit_ioend() logic untouched.

>
> > > 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 we don't add this special casing for IOMAP_IN_MEM here, then in
> > this function it'll hit the "if (!wpc->ioend)" case right in the
> > beginning and return error without calling the ->submit_ioend()
>
> So this suggests you don't use struct iomap_ioend at all.  Given that
> you add a private field to iomap_writepage_ctx I guess that is where
> you chain the fuse requests?
>
> Either way I think we should clean this up one way or another.  If the
> non-block iomap writeback code doesn't use ioends we should probably move
> the ioend chain into the private field, and hide everything using it (or
> even the ioend name) in proper abstraction.  In this case this means
> finding another way to check for a non-empty wpc.  One way would be to
> check nr_folios as any non-empty wbc must have a number of folios
> attached to it, the other would be to move the check into the
> ->submit_ioend method including the block fallback.  For a proper split
> the method should probably be renamed, and we'd probably want to require
> every use to provide the submit method, even if the trivial block
> users set it to the default one provided.
>
> > > > -             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.
> >
> > writeback_folio needs to do the unlocking itself because the writeback
> > may be done asynchronously (as in the case for fuse). I'll add that as
> > a comment in v2.
>
> So how do you end up with a zero count here and still want
> the fuse code to unlock?
>

count is unused by ->writeback_folio(), so it's always just zero. But
I see what you're saying now. I should just increment count after
doing the ->writeback_folio() call and then we could just leave the
"if (!count)" check untouched. I like this suggestion a lot, i'll make
this change in v2.