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

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

[Joanne Koong]

On Tue, Jun 24, 2025 at 11:26 PM Christoph Hellwig <hch@infradead.org> 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.

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
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.

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

[Darrick J. Wong]

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@infradead.org> 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?

--D

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

[Joanne Koong]

On Mon, Jun 30, 2025 at 10:41 PM Darrick J. Wong <djwong@kernel.org> 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@infradead.org> 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

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

[Joanne Koong]

On Wed, Jul 2, 2025 at 2:36 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Mon, Jun 30, 2025 at 10:41 PM Darrick J. Wong <djwong@kernel.org> 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@infradead.org> 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

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

[Miklos Szeredi]

On Wed, 25 Jun 2025 at 18:44, Joanne Koong <joannelkoong@gmail.com> wrote:

> 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.

Fuse calls invalidate_inode_pages2() not just for direct I/O:

 - open without FOPEN_KEEP_CACHE
 - FUSE_NOTIFY_INVAL_INODE
 - mtime/size change with FUSE_AUTO_INVAL_DATA turned
on/FUSE_EXPLICIT_INVAL_DATA turned off
 - truncate

In most of these cases dirty pages d need to be written back.

Thanks,
Miklos