On Sun, Jun 08, 2025 at 09:51:54PM -0700, Christoph Hellwig 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?
What does fuse use folio laundering for, anyway? It looks to me like
the primary users are invalidate_inode_pages*. Either the caller cares
about flushing dirty data and has called filemap_write_and_wait_range;
or it doesn't and wants to tear down the pagecache ahead of some other
operation that's going to change the file contents and doesn't care.
I suppose it could be useful as a last-chance operation on a dirty folio
that was dirtied after a filemap_write_and_wait_range but before
invalidate_inode_pages*? Though for xfs we just return EBUSY and let
the caller try again (or not). Is there a subtlety to fuse here that I
don't know about?
(Both of those questions are directed at hch or joanne or anyone else
who knows ;))
--D
> > @@ -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.
>
> > +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).
>
>
