On Mon, Jun 30, 2025 at 10:41 PM Darrick J. Wong <djwong@xxxxxxxxxx> wrote: > > On Wed, Jun 25, 2025 at 09:44:31AM -0700, Joanne Koong wrote: > > On Tue, Jun 24, 2025 at 11:26 PM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote: > > > > > > On Tue, Jun 24, 2025 at 10:26:01PM -0700, Joanne Koong wrote: > > > > > The question is whether this is acceptable for all the filesystem > > > > > which implement ->launder_folio today. Because we could just move the > > > > > folio_test_dirty() to after the folio_lock() and remove all the testing > > > > > of folio dirtiness from individual filesystems. > > > > > > > > Or could the filesystems that implement ->launder_folio (from what I > > > > see, there's only 4: fuse, nfs, btrfs, and orangefs) just move that > > > > logic into their .release_folio implementation? I don't see why not. > > > > In folio_unmap_invalidate(), we call: > > > > > > Without even looking into the details from the iomap POV that basically > > > doesn't matter. You'd still need the write back a single locked folio > > > interface, which adds API surface, and because it only writes a single > > > folio at a time is rather inefficient. Not a deal breaker because > > > the current version look ok, but it would still be preferable to not > > > have an extra magic interface for it. > > > > > > > Yes but as I understand it, the focus right now is on getting rid of > > ->launder_folio as an API. The iomap pov imo is a separate issue with > > determining whether fuse in particular needs to write back the dirty > > page before releasing or should just fail. > > This might not help for Joanne's case, but so far the lack of a > launder_folio in my fuse+iomap prototype hasn't hindered it at all. > From what I can tell it's ok to bounce EBUSY back to dio callers... > > > btrfs uses ->launder_folio() to free some previously allocated > > reservation (added in commit 872617a "btrfs: implement launder_folio > > for clearing dirty page reserve") so at the very least, that logic > > would need to be moved to .release_folio() (if that suffices? Adding > > the btrfs group to cc). It's still vague to me whether > > fuse/nfs/orangefs need to write back the dirty page, but it seems fine > > ...but only because a retry will initiate another writeback so > eventually we can make some forward progress. But it helps a lot that > fuse+iomap is handing the entire IO stack over to iomap. > > > to me not to - as I understand it, the worst that can happen (and > > please correct me if I'm wrong here, Matthew) from just failing it > > with -EBUSY is that the folio lingers longer in the page cache until > > it eventually gets written back and cleared out, and that only happens > > if the file is mapped and written to in that window between > > filemap_write_and_wait_range() and unmap_mapping_folio(). afaics, if > > fuse/nfs/orangefs do need to write back the dirty folio instead of > > failing w/ -EBUSY, they could just do that logic in .release_folio. > > What do you do in ->release_folio if writeback fails? Redirty it and > return false? Yeah, I was thinking we just redirty it and return false. I don't think that leads to any deviation from existing behavior (eg in folio_unmap_invalidate(), a failed writeback will return -EBUSY regardless of whether the writeback attempt happens from ->launder_folio() or ->release_folio()). > > --D
