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