Re: [PATCH 04/11] iomap: hide ioends from the generic writeback code

[Christoph Hellwig <hch@lst.de>]

On Tue, Jun 17, 2025 at 12:22:46PM -0700, Joanne Koong wrote:
> On Tue, Jun 17, 2025 at 3:55 AM Christoph Hellwig <hch@lst.de> wrote:
> >
> > Replace the ioend pointer in iomap_writeback_ctx with a void *wb_ctx
> > one to facilitate non-block, non-ioend writeback for use.  Rename
> > the submit_ioend method to writeback_submit and make it mandatory so
> 
> I'm confused as to whether this is mandatory or not - afaict from the
> code, it's only needed if wpc->wb_ctx is also set. It seems like it's
> ok if a filesystem doesn't define ops->writeback_submit so long as
> they don't also set wpc->wb_ctx, but if they do set
> ops->writeback_submit but don't set wpc->wb_ctx then they shouldn't
> expect ->writeback_submit() to be called.

In a way yes.  But I don't really understand how a file system could
work without either, unless the folio size and the block size are always
equal.

> It seems like there's a
> tight interdependency between the two, it might be worth mentioning
> that in the documentation to make that more clear. Or alternatively,
> just always calling wpc->ops->writeback_submit() in iomap_writepages()
> and having the caller check that wpc->wb_ctx is valid.

Do you mean the callee here?  Otherwise I'm a bit confused about this
sentence.

> > -  - ``submit_ioend``: Allows the file systems to hook into writeback bio
> > -    submission.
> > -    This might include pre-write space accounting updates, or installing
> > -    a custom ``->bi_end_io`` function for internal purposes, such as
> > -    deferring the ioend completion to a workqueue to run metadata update
> > -    transactions from process context before submitting the bio.
> > -    This function is optional.
> > +  - ``writeback_submit``: Submit the previous built writeback context.
> 
> It might be helpful here to add "This function must be supplied by the
> filesystem", especially since the paragraph above has that line for
> writeback_range()

Ok.

> >  struct iomap_writeback_ops {
> >         /*
> > -        * Required, performs writeback on the passed in range
> > +        * Performs writeback on the passed in range
> 
> Is the reasoning behind removing "Required" that it's understood that
> the default is it's required, so there's no need to explicitly state
> that?

Yes.