Re: [PATCH v2] ext4: check folio uptodate state in ext4_page_mkwrite()

[Deepanshu Kartikey]

On Fri, Dec 5, 2025 at 7:08 PM Theodore Tso <tytso@xxxxxxx> wrote:
>
> On Fri, Dec 05, 2025 at 03:33:22AM +0000, Matthew Wilcox wrote:
> > It sounds like I was confused -- I thought the folios being
> > invalidated in mpage_release_unused_pages() belonged to the block
> > device, but from what you're saying, they belong to a user-visible
> > file?
>
> Yes, correct.  I'm guessing that we were marking the page !uptodate
> back when that was the only way to indicate that there had been any
> kind of I/O error (either on the read or write side).  Obviously we
> have much better ways of doing it in the 21st century.  :-)
>
> > Now, is the folio necessarily dirty at this point?  I guess so if
> > we're in the writeback path.  Darrick got rid of similar code in
> > iomap a few years ago; see commit e9c3a8e820ed.  So it'd probably be
> > good to have ext4 behave the same way.
>
> Hmm, yes.   Agreed.
>
>     commit e9c3a8e820ed0eeb2be05072f29f80d1b79f053b
>     Author: Darrick J. Wong <djwong@xxxxxxxxxx>
>     Date:   Mon May 16 15:27:38 2022 -0700
>
>     iomap: don't invalidate folios after writeback errors
>
>     XFS has the unique behavior (as compared to the other Linux
>     filesystems) that on writeback errors it will completely
>     invalidate the affected folio and force the page cache to reread
>     the contents from disk.  All other filesystems leave the page
>     mapped and up to date.
>
>     This is a rude awakening for user programs, since (in the case
>     where write fails but reread doesn't) file contents will appear to
>     revert old disk contents with no notification other than an EIO on
>     fsync.  This might have been annoying back in the days when iomap
>     dealt with one page at a time, but with multipage folios, we can
>     now throw away *megabytes* worth of data for a single write error...
>
> As Darrick pointed out we could potentially append a *single* byte to
> a file, and if there was some kind of writeback error, we could
> potentially throw away *vast* amounts of data for no good reason.
>
>                                      - Ted


Hi Ted and Matthew,

Thank you for pointing out the iomap commit. I now understand that
invalidating folios on writeback errors is the wrong approach.

Looking at Darrick's commit e9c3a8e820ed, iomap removed both
folio_clear_uptodate() and the invalidation call, keeping folios in
memory with their data intact even after writeback errors.

For ext4, should I apply the same approach to mpage_release_unused_pages()?
Specifically, remove the invalidation entirely:

  if (invalidate) {
      /*
       * On writeback errors, do not invalidate the folio or
       * clear the uptodate flag. This follows the behavior
       * established by iomap (commit e9c3a8e820ed "iomap:
       * don't invalidate folios after writeback errors").
       */
      if (folio_mapped(folio))
          folio_clear_dirty_for_io(folio);
-     block_invalidate_folio(folio, 0, folio_size(folio));
-     folio_clear_uptodate(folio);
  }

This would:
- Keep user data in memory instead of discarding it
- Prevent the WARNING since folio remains uptodate
- Match the behavior of modern filesystems
- Prevent data loss from discarding potentially megabytes of data

Is this the correct approach? If so, I'll send v4 with this fix.

Best regards,
Deepanshu