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

[Boris Burkov <boris@bur.io>]

On Tue, Jul 07, 2026 at 04:24:30PM +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() goes 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 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.
> 
> There is a deeper problem, dirty and writeback folio flags are updated
> at very different timing.
> The dirty flag is only cleared when the last sub-folio block has dirty
> flag cleared.
> But the writeback flag is set when the first block starts writeback, and
> later blocks that go through writeback will not call
> folio_start_writeback() again.
> 
> If we rely on folio_start_writeback() to update the
> PAGECACHE_TAG_DIRTY and PAGECACHE_TAG_TOWRITE, it will always be
> incorrect in one way or another.
> 
> [FIX]
> Do not let folio_start_writeback() do any PAGECACHE_TAG_TOWRITE
> handling.
> 
> Instead, manually clear both PAGECACHE_TAG_TOWRITE and
> PAGECACHE_TAG_DIRTY flags when the folio is no longer dirty during
> btrfs_subpage_set_writeback().
> 
> However this is only a hot-fix, for the long term solution we will
> follow iomap, by calling folio_start_writeback() immediately for the
> whole folio, and folio_end_writeback() after all writeback finished
> for the folio.
> 

Since I was planning to include exactly this as part of fixing the other
issue I found and this doesn't regress anything on its own, I agree we
should just get it in as is.

Reviewed-by: Boris Burkov <boris@bur.io>

> Fixes: 095be159f3eb ("btrfs: unify folio dirty flag clearing")
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> Changelog:
> v2:
> - Completely decouple the PAGECACHE_TAG_* handling from
>   folio_start_writeback()
>   Since folio dirty and writeback flags are only updated once, but at
>   very different timings (the last dirty block cleared vs the first
>   writeback block started), we have to manually handle the
>   PAGECACHE_TAG_* update by ourselves.
> 
> - Add a mention about the proper fix using the same solution
>   as iomap
> ---
>  fs/btrfs/subpage.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
> index 56060acac2e9..2a9397be8116 100644
> --- a/fs/btrfs/subpage.c
> +++ b/fs/btrfs/subpage.c
> @@ -359,6 +359,23 @@ void btrfs_subpage_set_dirty(const struct btrfs_fs_info *fs_info,
>  	folio_mark_dirty(folio);
>  }
>  
> +static void folio_clear_tags(struct folio *folio)
> +{
> +	struct address_space *mapping = folio_mapping(folio);
> +	XA_STATE(xas, &mapping->i_pages, folio->index);
> +	unsigned long flags;
> +
> +	ASSERT(folio_test_locked(folio));
> +	ASSERT(mapping);
> +	ASSERT(mapping_use_writeback_tags(mapping));
> +
> +	xas_lock_irqsave(&xas, flags);
> +	xas_load(&xas);
> +	xas_clear_mark(&xas, PAGECACHE_TAG_DIRTY);
> +	xas_clear_mark(&xas, PAGECACHE_TAG_TOWRITE);
> +	xas_unlock_irqrestore(&xas, flags);
> +}
> +
>  /*
>   * Extra clear_and_test function for subpage dirty bitmap.
>   *
> @@ -403,7 +420,6 @@ void btrfs_subpage_set_writeback(const struct btrfs_fs_info *fs_info,
>  	unsigned int start_bit = subpage_calc_start_bit(fs_info, folio,
>  							writeback, start, len);
>  	unsigned long flags;
> -	bool keep_write;
>  
>  	spin_lock_irqsave(&bfs->lock, flags);
>  	bitmap_set(bfs->bitmaps, start_bit, len >> fs_info->sectorsize_bits);
> @@ -413,10 +429,14 @@ void btrfs_subpage_set_writeback(const struct btrfs_fs_info *fs_info,
>  	 * folio. Doing so can cause WB_SYNC_ALL writepages() to overlook it,
>  	 * assume writeback is complete, and exit too early — violating sync
>  	 * ordering guarantees.
> +	 *
> +	 * Instead we manually clear the DIRTY and TOWRITE tags after the folio
> +	 * is no longer dirty.
>  	 */
> -	keep_write = folio_test_dirty(folio);
>  	if (!folio_test_writeback(folio))
> -		__folio_start_writeback(folio, keep_write);
> +		__folio_start_writeback(folio, true);
> +	if (!folio_test_dirty(folio))
> +		folio_clear_tags(folio);
>  	spin_unlock_irqrestore(&bfs->lock, flags);
>  }
>  
> -- 
> 2.54.0
>