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

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.

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