On Wed, Jul 2, 2025 at 2:36 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote: > > 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()). Or actually I guess the one deviation is that with ->launder_folio() it can return back a custom error code (eg -ENOMEM) to folio_unmap_invalidate() whereas release_folio() errors will default to -EBUSY, but the error code propagated to folio->mapping will be the same. > > > > --D
