Re: [PATCH v1 5/8] iomap: add iomap_writeback_dirty_folio()

[Joanne Koong <joannelkoong@gmail.com>]

On Sun, Jun 8, 2025 at 9:52 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Jun 06, 2025 at 04:38:00PM -0700, Joanne Koong wrote:
> > Add iomap_writeback_dirty_folio() for writing back a dirty folio.
> > One use case of this is for folio laundering.
>
> Where "folio laundering" means calling ->launder_folio, right?

Yes. I'll change the wording to "laundering folios" to make this more clear.
>
> > @@ -1675,7 +1677,8 @@ static int iomap_writepage_map(struct iomap_writepage_ctx *wpc,
> >        * already at this point.  In that case we need to clear the writeback
> >        * bit ourselves right after unlocking the page.
> >        */
> > -     folio_unlock(folio);
> > +     if (unlock_folio)
> > +             folio_unlock(folio);
> >       if (ifs) {
> >               if (atomic_dec_and_test(&ifs->write_bytes_pending))
> >                       folio_end_writeback(folio);
>
> When writing this code I was under the impression that
> folio_end_writeback needs to be called after unlocking the page.
>
> If that is not actually the case we can just move the unlocking into the
> caller and make things a lot cleaner than the conditional locking
> argument.

I don't think we can move this into the caller of ->writeback_folio()
/ ->map_blocks(). iomap does some preliminary optimization checking
(eg skipping writing back truncated ranges) in
iomap_writepage_handle_eof() which will unlock the folio if succesful
and is called separately from those callbacks. As well it's not clear
for the caller to know when the folio can be unlocked (eg if the range
being written back / mapped is the last range in that folio).
>
> > +int iomap_writeback_dirty_folio(struct folio *folio, struct writeback_control *wbc,
> > +                             struct iomap_writepage_ctx *wpc,
> > +                             const struct iomap_writeback_ops *ops)
>
> Please stick to the usual iomap coding style:  80 character lines,
> two-tab indent for multiline function declarations.  (Also in a few
> other places).

Will do.
>
>