Re: [PATCH] btrfs: fix a regression where PAGECACHE_TAG_DIRTY is never cleared

On Wed, Jul 08, 2026 at 09:00:39AM +0930, Qu Wenruo wrote:
> 
> 
> 在 2026/7/8 08:46, Boris Burkov 写道:
> > On Wed, Jul 08, 2026 at 08:06:26AM +0930, Qu Wenruo wrote:
> > > 
> > > 
> > > 在 2026/7/8 04:42, Boris Burkov 写道:
> > > > On Tue, Jul 07, 2026 at 03:10:20PM +0930, Qu Wenruo wrote:
> > > [...]
> > > > > 
> > > > > > 
> > > > > > Second, and worse, it results in a deadlock which should show on btrfs/062
> > > > > > and btrfs/070, I believe. The problem is that clearing dirty at this point
> > > > > > results in a truncate being allowed to clean pages past the end of the
> > > > > > new file size without calling btrfs_mark_ordered_io_finished() so we
> > > > > > leak that OE unfinished and get stuck.
> > > > > 
> > > > > Mind to explain exactly where the missing btrfs_mark_ordered_io_finished()
> > > > > is?
> > > > > 
> > > > > The point here is, we always have the folio locked already, the difference
> > > > > is just in the timing where the folio dirty flag is cleared.
> > > > > 
> > > > > I didn't see a problem that the changed timing can cause problem related to
> > > > > btrfs_mark_ordered_io_finished().
> > > > 
> > > > Sorry, my explanation last night was rushed and clumsy, let me try to be
> > > > much more clear and specific!
> > > > 
> > > > The key change is your recent one, that went together with the removal
> > > > of folio_clear_dirty_for_io():
> > > > 
> > > > 334509ce9d07 ("btrfs: use dirty flag to check if an ordered extent needs to be truncated")
> > > > 
> > > > and specifically the lines:
> > > > 
> > > >     /*
> > > >     * If the range is not dirty, the range has been submitted and
> > > >     * since we have waited for the writeback, endio has been
> > > >     * executed, thus we must skip the range to avoid double
> > > >     * accounting for the ordered extent.
> > > >     */
> > > >     if (!btrfs_folio_test_dirty(fs_info, folio, cur, range_len))
> > > >       goto next;
> > > > 
> > > > 
> > > > and quoting from the changelog:
> > > > 
> > > >     If the OE range is dirty, it means we have allocated an ordered extent but
> > > >     have not yet submitted the range. And that's exactly the case where we need
> > > >     to truncate the ordered extent.
> > > > 
> > > > But with this new fix, which returns folio_clear_dirty_for_io before
> > > > submission, the comment, skip, and changelog argument are no longer
> > > > true.
> > > > 
> > > > Suppose we dirty folios [F,F+K] at the end of a file and start writeback
> > > > on them, creating a single OE for [F, F+K]. extent_writepage will keep
> > > > looping through the folios submitting them.
> > > > 
> > > > Now suppose a truncate races in and shrinks i_size (taking i_rwsem but
> > > > no folio locks or blocking on OEs) via:
> > > > btrfs_setsize
> > > >     truncate_setsize
> > > >       i_size_write // shrinks i_size to M <= F
> > > >         truncate_pagecache
> > > >           folio_wait_writeback (not yet marked writeback)
> > > >           folio_invalidate
> > > >             btrfs_invalidate_folio (folio clean, skipped, this is the bug)
> > > 
> > > Just a small nitpick, the problem is not from the truncate_setsize() call
> > > site, as truncate_pagecache() is grabbing the folio with FGP_LOCK, so it can
> > > not grab the folio locked by writeback.
> > > 
> > 
> > Good point, but I think the issue is as soon as you do i_size_write(),
> > so the rest is just my explanation being a little misleading, I think.
> > 
> > > > 
> > > > extent_writepage hits the code
> > > > 
> > > >     if (folio->index > end_index ||
> > > >       (folio->index == end_index && !pg_offset)) {
> > > >         folio_invalidate(folio, 0, folio_size(folio));
> > > >         folio_unlock(folio);
> > > >         return 0;
> > > >     }
> > > > 
> > > > which calls btrfs_folio_invalidate() which needs to mark the OE past
> > > > isize finished but skips it because the folio is clean (same issue as in
> > > > truncate above)
> > > 
> > > Thanks a lot! Now I understand how things are going wrong now.
> > > 
> > > But I still have a question, I thought we should have some kind of inode
> > > lock or something else to prevent i_size_read() from getting stale results.
> > > 
> > > And digging deeper, for the btrfs_setsize(), the inode is already locked.
> > > E.g. inside notify_change() there is a WARN_ON_ONCE() if the inode is not
> > > locked.
> > > 
> > > Would the root cause of the problem is that we did not lock the inode for
> > > writeback?
> > 
> > yes, I think this path to the hang definitely requires the fact that
> > writeback does not hold i_rwsem while reading i_size.
> > 
> > > 
> > > And if we do not have proper inode lock for writeback, then I think write
> > > back should never bother the isize check, and always do the writeback no
> > > matter isize.
> > > 
> > 
> > Don't we risk weird truncate vs writeback races where they undo each
> > other, then?
> 
> Writeback still has the folio locked, so truncation should wait for the
> writeback first then invalidate it.
> 
> The other direction would also be true, if the invalidation invalidated the
> page first, then writeback should not got a dirty folio anymore.
> 
> But I can be totally wrong considering how complex things are.
> 
> I can explore if this idea won't break the existing runs first.
> > > 
> > > Mind to explain why folio_mkclean() is required? Is it for the same OE hang
> > > or something else?
> > > 
> > 
> > No it's a csum corruption with mmap being allowed to write to folios
> > that are under writeback (and thus computing csums)
> > 
> > Since we got rid of folio_clear_dirty_for_io() (and thus
> > folio_mkclean()), then if we mmap a large folio, then start writeback on
> > it, we never write protect the folios while they are under writeback so
> > the mmap user can trigger write faults and mess up the csums. We have
> > observed this in practice in our early testing of btrfs large folios in
> > production and we also have a racy reproducer that enables large folios,
> > mmaps a file and has a bunch of threads racing between touching each
> > sector with some new random data and fsyncing the file. We do that a bunch
> > of times, then drop caches and read everything. Do that whole thing in a
> > loop a few times and eventually you hit it. I can share the exact LLM
> > reproducer slop code if you would like, but that is the rough shape, to
> > explain the point.
> > 
> > I can send my holistic fix proposal with your v2 included in a series?
> 
> Sounds great. Feel free to include the v2 fix:
> https://lore.kernel.org/linux-btrfs/d47af8cae38e32462d61cb62139c2732e4e8fdbe.1783407237.git.wqu@suse.com/T/
> 
> Although considering this is already breaking cachestat() and will cause
> extra overhead for writeback, I'd prefer the v2 as a hot fix first.

I gave your v2 my r-b, and will send the patch to fix the corruption
next, separately. It shouldn't affect your patch, as it only conflicted
if we went back to folio_clear_dirty_for_io().

> 
> Thanks,
> Qu
> 
> 
> > 
> > > Thanks,
> > > Qu
> > > 
> > > > 
> > > > Thanks,
> > > > Boris
> > > > 
> > > > > 
> > > > > Thanks,
> > > > > Qu
> > > > > 
>