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

On Mon, Jul 06, 2026 at 09:14:51PM -0700, Boris Burkov wrote:
> On Tue, Jul 07, 2026 at 12:10:25PM +0930, Qu Wenruo wrote:
> > [BUG]
> > The following script (already submitted as generic/798) will report
> > incorrect dirty page numbers, with 64K page size systems and 4K fs block
> > size:
> > 
> >  # mkfs.btrfs -s 4k -f $dev
> >  # mount $dev $mnt
> >  # xfs_io -f -c "pwrite 0 64K" -c fsync -c "cachestat 0 64K" $mnt/foobar
> >  Cached: 1, Dirty: 1, Writeback: 0, Evicted: 0, Recently Evicted: 0
> > 
> > Note that the dirtied page number is still 1.
> > 
> > [CAUSE]
> > The cachestat() go through the XArray of the page cache, but
> > instead of checking each folio's flag, it uses the
> > PAGECACHE_TAG_DIRTY tag to report dirty pages
> > 
> > Since commit 095be159f3eb ("btrfs: unify folio dirty flag clearing"),
> > btrfs replaced a folio_clear_dirty_for_io() call inside
> > extent_write_cache_pages() with folio_test_dirty().
> 
> This is an interesting coincidence. I have been debugging this exact
> change from a different, scarier perspective as well. Because we stopped
> calling folio_clear_dirty_for_io() we stopped calling folio_mkclean()
> which meant the folio remained writeable via mmap during writeback. We
> observed a significant uptick of csum errors with large folios, as a
> result. However, just this fix I think is not sufficient, and has two
> serious bugs of its own.
> 
> First of all, now that your patch does clear dirty here, the keep_write
> check in btrfs_subpage_start_writeback() is not correct anymore, which will
> result in clearing TOWRITE on a non-sync writeback and losing it for
> other pages in the folio on a sync writeback. So if we do clear dirty,
> we need to also add a check for the subpage dirty bits for keep_write
> like we did for extent_buffers before moving to the eb xarray.
> Incidentally since that means not using __folio_start_writeback, it does
> fix this exact bug too.
> 
> 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. This is caused by the other
> patches which removed the Ordered bitmap tracking to reduce the bitmap
> size, I believe, as we now use dirty to track the existence of OEs.
> 
> Can you confirm whether btrfs/062 and btrfs/070 finish for you every
> time under this patch? If not, something about my test setup and yours
> is different, or something is different on the branch I was working on.
> I will try to re-test with your patches and my patches on for-next as
> well.
> 

I re-tested by applying your patch to my for-next and I do believe b/062
hangs indefinitely, while it passes in ~45s for me normally.

> > 
> > This will cause the following call sequence for the folio at file offset
> > 0:
> > 
> >  extent_write_cache_pages()
> >  |- folio_test_dirty()
> >  |  The folio is still dirty, continue to writeback.
> >  |
> >  |- extent_writepage()
> >     |- extent_writepage_io()
> >        |- submit_one_sector() for range [0, 4K)
> >        |  |- btrfs_folio_clear_dirty()
> >        |  |- btrfs_folio_set_writeback()
> >        |     |- folio_start_writeback()
> >        |        It's the first writeback block, we set the writeback
> >        |	flag for the folio.
> >        |	But the folio is still dirty, PAGECACHE_TAG_DIRTY is
> >        |	kept
> >        |
> >        |- submit_one_sector() for range [4K, 8K)
> >        |  |- btrfs_folio_clear_dirty()
> >        |  |- btrfs_folio_set_writeback()
> >        |     The folio already has writeback flag, no need to call
> >        |     folio_start_writeback()
> >        |
> >        | ...
> >        |- submit_one_sector() for range [60K, 64K)
> > 	  |- btrfs_folio_clear_dirty()
> > 	  |- btrfs_folio_set_writeback()
> >              The folio already has writeback flag, no need to call
> >              folio_start_writeback()
> > 
> > So the PAGECACHE_TAG_DIRTY is never cleared.
> > 
> > Meanwhile for the old code, before that commit, the sequence looks
> > like:
> > 
> >  extent_write_cache_pages()
> >  |- folio_clear_dirty_for_io()
> >  |  The folio is still dirty, so continue to writeback.
> >  |  But the folio dirty flag is cleared now.
> >  |
> >  |- extent_writepage()
> >     |- extent_writepage_io()
> >        |- submit_one_sector() for range [0, 4K)
> >        |  |- btrfs_folio_clear_dirty()
> >        |  |- btrfs_folio_set_writeback()
> >        |     |- folio_start_writeback()
> >        |        |- xas_clear(PAGECACHE_TAG)
> >        |
> >        |        It's the first writeback block, we set the writeback
> >        |	flag for the folio.
> >        |	And the folio is not dirty, PAGECACHE_TAG_DIRTY is
> >        |        cleared
> >        |
> >        |- submit_one_sector() for range [4K, 8K)
> >        |  |- btrfs_folio_clear_dirty()
> >        |  |- btrfs_folio_set_writeback()
> >        |     The folio already has writeback flag, no need to call
> >        |     folio_start_writeback()
> >        |
> >        | ...
> >        |- submit_one_sector() for range [60K, 64K)
> > 	  |- btrfs_folio_clear_dirty()
> > 	  |- btrfs_folio_set_writeback()
> >              The folio already has writeback flag, no need to call
> >              folio_start_writeback()
> > 
> > Unlike the new code, old code will clear PAGECACHE_TAG_DIRTY for the
> > first writeback block.
> > 
> > [FIX]
> > Revert that folio_test_dirty() back to folio_clear_dirty_for_io(), and
> > add a comment explaining why we need to use folio_clear_dirty_for_io().
> > 
> > Fixes: 095be159f3eb ("btrfs: unify folio dirty flag clearing")
> > Signed-off-by: Qu Wenruo <[email protected]>
> > ---
> >  fs/btrfs/extent_io.c | 25 ++++++++++++++++++++++++-
> >  1 file changed, 24 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 8fbb798767ca..78bd4ec541f8 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -2584,8 +2584,31 @@ static int extent_write_cache_pages(struct address_space *mapping,
> >  				folio_wait_writeback(folio);
> >  			}
> >  
> > +			/*
> > +			 * For folios that are under writeback or no longer dirty,
> > +			 * we can skip the folio.
> > +			 *
> > +			 * Here we have to call folio_clear_dirty_for_io() to clear
> > +			 * the folio dirty flag.
> > +			 *
> > +			 * Without that call, btrfs_folio_set_writeback() will call
> > +			 * folio_start_writeback() for the first block to be written back.
> > +			 * But since the folio can still be dirty, e.g. other
> > +			 * blocks inside the folio are still dirty,
> > +			 * PAGECACHE_TAG_DIRTY will not be cleared.
> > +			 *
> > +			 * On the other hand, for btrfs_folio_set_writeback() of the
> > +			 * last block, although the folio is no longer dirty,
> > +			 * btrfs_folio_set_writeback() won't call folio_start_writeback()
> > +			 * as the folio already has that flag.
> > +			 *
> > +			 * This means, if we only rely one btrfs_folio_*()
> > +			 * helpers for both dirty and writeback flags,
> > +			 * PAGECACHE_TAG_DIRTY will never be cleared for subpage cases.
> > +			 * The only way to break out is to clear folio dirty here.
> > +			 */
> >  			if (folio_test_writeback(folio) ||
> > -			    !folio_test_dirty(folio)) {
> > +			    !folio_clear_dirty_for_io(folio)) {
> 
> 
> So far, the best solution I have come up with is to call a raw
> "folio_mkclean" here, as opposed to folio_clear_dirty_for_io().
> 
> a second place it is needed is in extent_range_clear_dirty_for_io()
> which also used to call folio_clear_dirty_for_io() and needs stable
> pages during the compression, for which we need to call folio_mkclean().
> 
> Sorry to derail your fix with a whole different problem.. But unless I
> am missing something I think we will have to fix all of it to fix any of
> it.
> Either by fixing forward with folio_mkclean() or by reverting and/or
> rethinking all of the patches in the series:
> https://lore.kernel.org/linux-btrfs/[email protected]/
> 
> specifically:
> 095be159f3eb ("btrfs: unify folio dirty flag clearing")
> and
> 7d97bdca4bcb ("btrfs: use dirty flag to check if an ordered extent needs to be truncated")
> 
> Thanks,
> Boris
> 
> >  				folio_unlock(folio);
> >  				continue;
> >  			}
> > -- 
> > 2.54.0
> >