On Mon, Jun 9, 2025 at 10:14 AM Darrick J. Wong <djwong@xxxxxxxxxx> wrote:
>
> 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?
>
This is something I've been confused about too, as to why there even
is a launder_folio() and why some filesystems (eg orangefs, nfs,
btrfs, fuse) call it but others don't.
It was added in commit e3db7691 ("[PATCH] NFS: Fix race in
nfs_release_page()") but I fail to see how that commit fixes the race
condition described in the commit message.
My theory is that it's only needed by remote/network filesystems
because maybe they need stronger data consistency guarantees? Out of
the ones that implement launder_folio(), btrfs is the anomaly but when
I checked with the person who added it to btrfs, they said it was a
hack for something else because it got called at the right time.
Looking forward to being enlightened on this as well.
> (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).
> >
> >
