linux-kernel - Re: [PATCH v2] ext4: check folio uptodate state in ext4_page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CADhLXY6ZPwXU8EQuLKF99mv9p5t85=jA0aKG=5+8TF=sVZjJyg@mail.gmail.com>
Date: Mon, 5 Jan 2026 07:38:00 +0530
From: Deepanshu Kartikey <kartikey406@...il.com>
To: Theodore Tso <tytso@....edu>
Cc: Matthew Wilcox <willy@...radead.org>, Zhang Yi <yi.zhang@...weicloud.com>, 
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org, 
	syzbot+b0a0670332b6b3230a0a@...kaller.appspotmail.com, 
	adilger.kernel@...ger.ca, djwong@...nel.org
Subject: Re: [PATCH v2] ext4: check folio uptodate state in ext4_page_mkwrite()

On Fri, Dec 5, 2025 at 7:58 PM Deepanshu Kartikey <kartikey406@...il.com> wrote:
>
> On Fri, Dec 5, 2025 at 7:08 PM Theodore Tso <tytso@....edu> 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@...nel.org>
> >     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

Hi Ted and Matthew,

I wanted to follow up on my previous email about the fix approach.

Just to confirm: should I remove the folio invalidation from
mpage_release_unused_pages() entirely, following Darrick's commit
e9c3a8e820ed for iomap?

The change would be:

  if (invalidate) {
      if (folio_mapped(folio))
          folio_clear_dirty_for_io(folio);
-     block_invalidate_folio(folio, 0, folio_size(folio));
-     folio_clear_uptodate(folio);
  }

This keeps the folio in memory with data intact, prevents the WARNING,
and matches modern filesystem behavior.

If this approach is correct, I'll test and send v4. Otherwise, please
let me know what adjustments are needed.

Thank you,
Deepanshu