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

[Joanne Koong <joannelkoong@gmail.com>]

On Tue, Jun 10, 2025 at 11:23 AM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> 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.

Actually, nvm. You're right, it's cleaner to just have ioend stuff be totally
abstracted away like you suggested below. I'll make that change for
v2.

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